Skip to content
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.

Sign up for GitHub

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

Jump to bottom

hammer: osd: fix fiemap issue in xfs when #extents > 1364 #11615

Merged
merged 2 commits into from
Jan 5, 2017
Merged

hammer: osd: fix fiemap issue in xfs when #extents > 1364 #11615

merged 2 commits into from
Jan 5, 2017

Conversation

mslovy
Copy link
Contributor

@mslovy mslovy commented Oct 24, 2016 •
edited by smithfarm
Loading

@mslovy mslovy changed the title FileStore:: fix fiemap issue in xfs when #extents > 1364 hammer: fix fiemap issue in xfs when #extents > 1364 Oct 24, 2016
@smithfarm smithfarm changed the title hammer: fix fiemap issue in xfs when #extents > 1364 [DNM] hammer: osd: fix fiemap issue in xfs when #extents > 1364 Nov 20, 2016
@smithfarm smithfarm added this to the hammer milestone Nov 20, 2016
@smithfarm smithfarm self-assigned this Nov 20, 2016
@smithfarm
Copy link
Contributor

@mslovy Please rebase to get rid of the "This branch is out-of-date" warning.

@smithfarm smithfarm mentioned this pull request Nov 20, 2016
Merged
@smithfarm
Copy link
Contributor

smithfarm commented Nov 20, 2016 •
edited
Loading

@mslovy While you're rebasing, please re-do the cherry-pick of 1a1c126 using git cherry-pick -x -- the "(cherry picked from . . .)" line is needed in the commit message.

Also, please describe how the cherry-pick conflicts were resolved. For complete info, see http://tracker.ceph.com/projects/ceph-releases/wiki/HOWTO_backport_commits

FileStore:: fix fiemap issue in xfs when #extents > 1364
d1e1207 
Fixes: ceph#17610
Backport: jewel, hammer
Signed-off-by: Ning Yao <yaoning@unitedstack.com>
(cherry picked from commit 1a1c126)

Conflicts:
        src/os/FileStore.cc
            in hammer, there is no _do_seek_hole_data() function so remove it
            in hammer, the logic is in FileStore::fiemap not in _do_fiemap()
            so port the logic to the else branch in FileStore::fiemap
@mslovy
Copy link
Contributor Author

mslovy commented Nov 21, 2016

redo cherry-pick and solve conflict, please review again

@smithfarm
Copy link
Contributor

@mslovy Thanks for the backport! I will test it.

@smithfarm smithfarm changed the title [DNM] hammer: osd: fix fiemap issue in xfs when #extents > 1364 hammer: osd: fix fiemap issue in xfs when #extents > 1364 Nov 23, 2016
smithfarm added a commit that referenced this pull request Nov 23, 2016
@smithfarm
Merge pull request #11615: hammer: osd: fix fiemap issue in xfs when …
dfca222 
…#extents > 1364

Reviewed-by: Nathan Cutler <ncutler@suse.com>
smithfarm added a commit that referenced this pull request Nov 24, 2016
@smithfarm
Merge pull request #11615: hammer: osd: fix fiemap issue in xfs when …
c3fd377 
…#extents > 1364

Reviewed-by: Nathan Cutler <ncutler@suse.com>
@smithfarm
Copy link
Contributor

@liewegas This PR passed a rados run at http://tracker.ceph.com/issues/17151#note-23

Do you think it can be merged?

@tchaikov
Copy link
Contributor

might want to cherry-pick c3748fa also once #12148 is merged.

@liewegas
Copy link
Member

let's make sure this is merged along with the use-after-free fix. otherwise lgtm!

@tchaikov
Copy link
Contributor

#12148 is merged, and the commit to be cherry-picked in that PR is c3748fa

@smithfarm
Copy link
Contributor

@mslovy Can you cherry-pick c3748fa into this PR?

@mslovy
Copy link
Contributor Author

mslovy commented Nov 30, 2016 •
edited
Loading

okey, repush, please review

smithfarm
smithfarm reviewed Nov 30, 2016
View reviewed changes
src/os/FileStore.cc
@@ -2913,7 +2913,7 @@ int FileStore::fiemap(coll_t cid, const ghobject_t& oid,
} else {
uint64_t i;
struct fiemap_extent *extent = NULL;
struct fiemap_extent *last = NULL;
struct fiemap_extent *last = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

please change "nullptr" to 0 (zero) because nullptr is a C++11ism

Copy link
Contributor

Choose a reason for hiding this comment

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

or just leave the NULL

@smithfarm
Copy link
Contributor

@mslovy hammer uses C++98 so nullptr will not work, please change it to "NULL" and re-push.

@mslovy
Copy link
Contributor Author

mslovy commented Dec 2, 2016 •
edited
Loading

got it， repushed

@tchaikov
FileStore::_do_fiemap: do not reference fiemap after it is freed
7106f42 
`last` points to fiemap::fm_extends[n], and if fiemap gets freed, we can
not reference any of its fieldis. so we could remember the check result before
freeing it.

Signed-off-by: Kefu Chai <kchai@redhat.com>
(cherry picked from commit c3748fa)

Conflicts:
	src/os/FileStore.cc
	put the parameter is_last in the right place
smithfarm added a commit to SUSE/ceph that referenced this pull request Dec 13, 2016
@smithfarm
Merge pull request ceph#11615: hammer: osd: fix fiemap issue in xfs w…
a682963 
…hen #extents > 1364

Reviewed-by: Nathan Cutler <ncutler@suse.com>
smithfarm added a commit that referenced this pull request Dec 13, 2016
@smithfarm
Merge pull request #11615: hammer: osd: fix fiemap issue in xfs when …
b544459 
…#extents > 1364

Reviewed-by: Nathan Cutler <ncutler@suse.com>
smithfarm added a commit that referenced this pull request Dec 14, 2016
@smithfarm
Merge pull request #11615: hammer: osd: fix fiemap issue in xfs when …
6e04176 
…#extents > 1364

Reviewed-by: Nathan Cutler <ncutler@suse.com>
smithfarm added a commit that referenced this pull request Dec 14, 2016
@smithfarm
Merge pull request #11615: hammer: osd: fix fiemap issue in xfs when …
b07d325 
…#extents > 1364

Reviewed-by: Nathan Cutler <ncutler@suse.com>
smithfarm added a commit that referenced this pull request Jan 2, 2017
@smithfarm
Merge pull request #11615: hammer: osd: fix fiemap issue in xfs when …
58ead95 
…#extents > 1364

Reviewed-by: Nathan Cutler <ncutler@suse.com>
@smithfarm
Copy link
Contributor

smithfarm commented Jan 5, 2017 •
edited
Loading

@athanatos This just passed another rados run at http://tracker.ceph.com/issues/17151#note-38 and it includes the use-after-free fix @liewegas mentioned at #11615 (comment)

Do you think it's OK to merge?

@athanatos athanatos merged commit 61917cb into ceph:hammer Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smithfarm smithfarm smithfarm left review comments

Assignees

@smithfarm smithfarm

Labels
bug-fix core
Projects
None yet
Milestone
hammer
Development

Successfully merging this pull request may close these issues.
5 participants
@mslovy @smithfarm @tchaikov @liewegas @athanatos