Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

pmem2: remove data_mover memory leak #5681

Merged
merged 2 commits into from
May 23, 2023
Merged

Conversation

grom72
Copy link
Contributor

@grom72 grom72 commented May 23, 2023

Automatically created data_mover is deleted during the map delete process.

Fixes: #5637


This change is Reviewable

@grom72 grom72 added the libpmem2 libpmem- and libpmem2-related label May 23, 2023
@grom72 grom72 added this to the 1.13 on GHA milestone May 23, 2023
@grom72 grom72 requested review from janekmi and osalyk May 23, 2023 06:53
@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #5681 (a614348) into stable-1.13 (67ce085) will decrease coverage by 1.98%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           stable-1.13    #5681      +/-   ##
===============================================
- Coverage        74.27%   72.29%   -1.98%     
===============================================
  Files              145      145              
  Lines            22131    22635     +504     
  Branches          3704     3771      +67     
===============================================
- Hits             16437    16365      -72     
- Misses            5694     6270     +576     

Copy link
Contributor

@osalyk osalyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @grom72 and @janekmi)


src/libpmem2/map.c line 2 at r1 (raw file):

// SPDX-License-Identifier: BSD-3-Clause
/* Copyright 2019-2023, Intel Corporation */

Not needed.


src/libpmem2/map_posix.c line 624 at r1 (raw file):

				goto err_register_map;
		}

Please remove empty line

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @grom72)


src/libpmem2/map_posix.c line 629 at r1 (raw file):

	/*
	 * Always delete automatically created mover.
	 */

I know it is a discovery for this code base. Since probably till now nobody knew when and what should be released or not. But now it is obvious from the code. Hence, this comment is redundant.


src/libpmem2/mover.c line 188 at r1 (raw file):

		return ret;

	LOG(3, "map %p, vdm %p", map, dms);

It looks atypical. Is it intentional?


src/libpmem2/mover.c line 213 at r1 (raw file):
It seems we tend to print here names of arguments.

LOG(3, "dms %p", dms);

Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @janekmi and @osalyk)


src/libpmem2/map_posix.c line 624 at r1 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

Please remove empty line

Done.


src/libpmem2/map_posix.c line 629 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I know it is a discovery for this code base. Since probably till now nobody knew when and what should be released or not. But now it is obvious from the code. Hence, this comment is redundant.

Done.


src/libpmem2/mover.c line 188 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It looks atypical. Is it intentional?

Yes, to see the address of allocated dms -> see mover_delete()


src/libpmem2/mover.c line 213 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It seems we tend to print here names of arguments.

LOG(3, "dms %p", dms);

Done.


src/libpmem2/map.c line 2 at r1 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

Not needed.

Done.

Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 4 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), 5 unresolved discussions (waiting on @janekmi and @osalyk)

Copy link
Contributor

@osalyk osalyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @janekmi)

Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @janekmi)

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @grom72)

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)


src/libpmem2/mover.c line 188 at r4 (raw file):

		return ret;
src/libpmem2/mover.c:188: ERROR duplicated empty line

Automatically created data_mover is deleted during
the map delete process.

Fixes: pmem#5637

Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @grom72)

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @grom72)

Fixes: pmem#5594

Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
Copy link
Contributor Author

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r5.
Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @janekmi)

@janekmi janekmi merged commit 43933bd into pmem:stable-1.13 May 23, 2023
@grom72 grom72 deleted the fix-5637 branch May 25, 2023 04:52
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
libpmem2 libpmem- and libpmem2-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants