New issue

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

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

Already on GitHub? Sign in to your account

rgw_file: fix double unref on rgw_fh for rename #13988

Merged
merged 1 commit into from Mar 20, 2017

Conversation

Projects
None yet
4 participants
@guihecheng
Contributor

guihecheng commented Mar 16, 2017

Skip unref after unlink to fix the problem.

Signed-off-by: Gui Hecheng guihecheng@cmss.chinamobile.com

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Mar 16, 2017

@guihecheng I think you're right about the double unref (not double free, since we are refcounting), but I think my intent (see comment in case 1 /*!LOCKED, -ref */) in the code was that the unlink would be the last valid access to rgw_fh, so rather than omitting to unlock and unref in unlink, we should goto out or return here in rename, because we are not supposed to touch rgw_fh after unlinking it. Can you buy that?

@mattbenjamin mattbenjamin self-assigned this Mar 16, 2017

@guihecheng

This comment has been minimized.

Contributor

guihecheng commented Mar 17, 2017

@mattbenjamin ok, it is reasonable that unlink should always be the one to kill an fh, thanks.

Gui Hecheng
rgw_file: fix double unref on rgw_fh for rename
Skip unref after unlink to fix the problem.

Signed-off-by: Gui Hecheng <guihecheng@cmss.chinamobile.com>

@guihecheng guihecheng changed the title from rgw_file: fix double free on rgw_fh for rename to rgw_file: fix double unref on rgw_fh for rename Mar 17, 2017

@dmick

This comment has been minimized.

Member

dmick commented Mar 17, 2017

My fault the submodule test failed, ignore

@mattbenjamin mattbenjamin self-requested a review Mar 19, 2017

@mattbenjamin

looks good logically, will retest before merging

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Mar 20, 2017

@guihecheng this appears to work perfectly, combined w/Dan's version of the nfs-ganesha invalidate-for-rename change (sorry again that was lost, and thanks for root-causing and proposing a fix!)

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Mar 20, 2017

verified f23

@mattbenjamin mattbenjamin merged commit 2beb4ee into ceph:master Mar 20, 2017

2 of 3 checks passed

Unmodifed Submodules Approval needed: modified submodules found
Details
Signed-off-by all commits in this PR are signed
Details
default Build finished.
Details

@guihecheng guihecheng deleted the guihecheng:rgw_file-fix-rename branch Apr 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment