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

jewel: osd: fix infinite loops in fiemap #15189

Merged
merged 3 commits into from Sep 10, 2017

Conversation

Projects
None yet
4 participants
@mslovy
Contributor

mslovy commented May 20, 2017

Ning Yao
os/filestore: fix infinit loops in fiemap()
since fiemap can get extents based on offset --> len
but we should consider last extents is retrieved when len == 0
even though it is not last fiemap extents

Signed-off-by: Ning Yao <yaoning@unitedstack.com>
(cherry picked from commit 36f6b66)

@smithfarm smithfarm added this to the jewel milestone May 22, 2017

@smithfarm smithfarm changed the title from jewel: fix infinit loops in fiemap to jewel: osd: fix infinite loops in fiemap Jun 19, 2017

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Jun 19, 2017

Contributor

Jenkins re-test this please

Contributor

smithfarm commented Jun 19, 2017

Jenkins re-test this please

@mslovy

This comment has been minimized.

Show comment
Hide comment
@mslovy

mslovy Jul 31, 2017

Contributor

@smithfarm, could we fix this in next release?

Contributor

mslovy commented Jul 31, 2017

@smithfarm, could we fix this in next release?

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Jul 31, 2017

Contributor

@mslovy Assuming it passes integration tests, yes! I'm working on kraken 11.2.1 now. Once that is in QE, I'll turn my attention to jewel 10.2.10.

Contributor

smithfarm commented Jul 31, 2017

@mslovy Assuming it passes integration tests, yes! I'm working on kraken 11.2.1 now. Once that is in QE, I'll turn my attention to jewel 10.2.10.

@smithfarm

Causes rados/objectstore/objectstore.yaml test to fail in rados suite.

@smithfarm smithfarm changed the title from jewel: osd: fix infinite loops in fiemap to [DNM] jewel: osd: fix infinite loops in fiemap Aug 22, 2017

@mslovy

This comment has been minimized.

Show comment
Hide comment
@mslovy

mslovy Aug 28, 2017

Contributor

@smithfarm , I submit a fixed commit on master, when it is ok, backport to this fix together?
#17313

Contributor

mslovy commented Aug 28, 2017

@smithfarm , I submit a fixed commit on master, when it is ok, backport to this fix together?
#17313

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Aug 28, 2017

Contributor

@mslovy Sure - just ping me when it is merged.

Contributor

smithfarm commented Aug 28, 2017

@mslovy Sure - just ping me when it is merged.

@mslovy

This comment has been minimized.

Show comment
Hide comment
@mslovy

mslovy Aug 31, 2017

Contributor

ping @smithfarm , the master branch merge this #17313 now

Contributor

mslovy commented Aug 31, 2017

ping @smithfarm , the master branch merge this #17313 now

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Aug 31, 2017

Contributor

@mslovy Cool, please cherry-pick that commit into this PR and I'll retest.

Contributor

smithfarm commented Aug 31, 2017

@mslovy Cool, please cherry-pick that commit into this PR and I'll retest.

Ning Yao
os:kstore fix unittest for FiemapHole
kstore always return [0, object_size] regardless of offset and length

Signed-off-by: Ning Yao <yaoning@unitedstack.com>
(cherry picked from commit dddae89)
@mslovy

This comment has been minimized.

Show comment
Hide comment
@mslovy

mslovy Aug 31, 2017

Contributor

okey, update it.

Contributor

mslovy commented Aug 31, 2017

okey, update it.

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Aug 31, 2017

Contributor

Jenkins re-test this please (The job died or timeout on unittest_throttle.log)

Contributor

smithfarm commented Aug 31, 2017

Jenkins re-test this please (The job died or timeout on unittest_throttle.log)

@smithfarm smithfarm changed the title from [DNM] jewel: osd: fix infinite loops in fiemap to jewel: osd: fix infinite loops in fiemap Aug 31, 2017

@mslovy

This comment has been minimized.

Show comment
Hide comment
@mslovy

mslovy Sep 6, 2017

Contributor

@smithfarm , can we consider this in 10.2.10?

Contributor

mslovy commented Sep 6, 2017

@smithfarm , can we consider this in 10.2.10?

Additional cherry-pick was added

@mslovy

This comment has been minimized.

Show comment
Hide comment
@mslovy

mslovy Sep 7, 2017

Contributor

Oh, I see. master stop the fiemap test...
//g_ceph_context->_conf->set_val("filestore_fiemap", "true");

Contributor

mslovy commented Sep 7, 2017

Oh, I see. master stop the fiemap test...
//g_ceph_context->_conf->set_val("filestore_fiemap", "true");

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Sep 7, 2017

Contributor

@mslovy Do you think the test failure will go away if you cherry-pick 60ae186 into this PR?

Contributor

smithfarm commented Sep 7, 2017

@mslovy Do you think the test failure will go away if you cherry-pick 60ae186 into this PR?

@mslovy

This comment has been minimized.

Show comment
Hide comment
@mslovy

mslovy Sep 7, 2017

Contributor

yeah, I think so. But should we disable it? or fix it.
I submit another #17550.

and I can cherry pick this 60ae186 PR first and have a try

Contributor

mslovy commented Sep 7, 2017

yeah, I think so. But should we disable it? or fix it.
I submit another #17550.

and I can cherry pick this 60ae186 PR first and have a try

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Sep 7, 2017

Contributor

Fixing sounds preferable, but since kstore is not supported, I don't see any reason to test it. On the other hand, I know little, or nothing, about this code so I'm probably not the right person to be talking to here.

@jdurgin What do you think?

Contributor

smithfarm commented Sep 7, 2017

Fixing sounds preferable, but since kstore is not supported, I don't see any reason to test it. On the other hand, I know little, or nothing, about this code so I'm probably not the right person to be talking to here.

@jdurgin What do you think?

@mslovy

This comment has been minimized.

Show comment
Hide comment
@mslovy

mslovy Sep 7, 2017

Contributor

I cherry-pick Sage's commit first. wait for @jdurgin 's reply.

Contributor

mslovy commented Sep 7, 2017

I cherry-pick Sage's commit first. wait for @jdurgin 's reply.

@mslovy

This comment has been minimized.

Show comment
Hide comment
@mslovy

mslovy Sep 7, 2017

Contributor

@smithfarm , I test
ObjectStore/StoreTest.FiemapHoles/0
ObjectStore/StoreTest.FiemapHoles/1
ObjectStore/StoreTest.FiemapHoles/3
with fiemap feature disabled and it pass those tests.

Contributor

mslovy commented Sep 7, 2017

@smithfarm , I test
ObjectStore/StoreTest.FiemapHoles/0
ObjectStore/StoreTest.FiemapHoles/1
ObjectStore/StoreTest.FiemapHoles/3
with fiemap feature disabled and it pass those tests.

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Sep 7, 2017

Contributor

@mslovy Can you please remove the commit 4c64287 "fix misc fiemap testing" (as it is not in master yet)?

Contributor

smithfarm commented Sep 7, 2017

@mslovy Can you please remove the commit 4c64287 "fix misc fiemap testing" (as it is not in master yet)?

@mslovy

This comment has been minimized.

Show comment
Hide comment
@mslovy

mslovy Sep 7, 2017

Contributor

oh, yeah. sorry, my fault

Contributor

mslovy commented Sep 7, 2017

oh, yeah. sorry, my fault

ceph_test_objectstore: disable filestore_fiemap
This very reliably triggers a test failure for
ObjectStore/StoreTest.Synthetic/1.

FIEMAP is bad!  Do not use it!

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas

This comment has been minimized.

Show comment
Hide comment
@liewegas

liewegas Sep 7, 2017

Member
Member

liewegas commented Sep 7, 2017

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Sep 7, 2017

Contributor

Jenkins re-test this please (The job died or timeout on unittest_throttle.log)

Contributor

smithfarm commented Sep 7, 2017

Jenkins re-test this please (The job died or timeout on unittest_throttle.log)

@jdurgin

This comment has been minimized.

Show comment
Hide comment
@jdurgin

jdurgin Sep 8, 2017

Member

I agree with sage - no need to test or fix kstore

Member

jdurgin commented Sep 8, 2017

I agree with sage - no need to test or fix kstore

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Sep 8, 2017

Contributor

jenkins test docs

Contributor

smithfarm commented Sep 8, 2017

jenkins test docs

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Sep 8, 2017

Contributor

jenkins test docs

Contributor

smithfarm commented Sep 8, 2017

jenkins test docs

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Sep 8, 2017

Contributor

I agree with sage - no need to test or fix kstore

What does that mean for this PR, though? Good to go? It passed a rados suite at http://tracker.ceph.com/issues/20613#note-48 (modulo the kstore failure and one other unrelated failure)

Contributor

smithfarm commented Sep 8, 2017

I agree with sage - no need to test or fix kstore

What does that mean for this PR, though? Good to go? It passed a rados suite at http://tracker.ceph.com/issues/20613#note-48 (modulo the kstore failure and one other unrelated failure)

@mslovy

This comment has been minimized.

Show comment
Hide comment
@mslovy

mslovy Sep 8, 2017

Contributor

so shoud we fix filestore? If though, should we test it and skip kstore like gtest_filter=-*/3

Contributor

mslovy commented Sep 8, 2017

so shoud we fix filestore? If though, should we test it and skip kstore like gtest_filter=-*/3

@jdurgin

jdurgin approved these changes Sep 8, 2017

@jdurgin

This comment has been minimized.

Show comment
Hide comment
@jdurgin

jdurgin Sep 8, 2017

Member

I think this PR is good to merge as-is for fixing filestore. Adding a filter to skip kstore testing like @mslovy suggests sounds like a good idea - can be a separate PR though.

Member

jdurgin commented Sep 8, 2017

I think this PR is good to merge as-is for fixing filestore. Adding a filter to skip kstore testing like @mslovy suggests sounds like a good idea - can be a separate PR though.

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Sep 10, 2017

Contributor

This PR was included in another rados suite [1] but unfortunately the ObjectStore/StoreTest.FiemapHoles/3, where GetParam() = "kstore" failure is still there. @mslovy Can you open another PR to disable the failing test?

[1] http://tracker.ceph.com/issues/20613#note-58

Contributor

smithfarm commented Sep 10, 2017

This PR was included in another rados suite [1] but unfortunately the ObjectStore/StoreTest.FiemapHoles/3, where GetParam() = "kstore" failure is still there. @mslovy Can you open another PR to disable the failing test?

[1] http://tracker.ceph.com/issues/20613#note-58

@smithfarm smithfarm merged commit ac27f23 into ceph:jewel Sep 10, 2017

4 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
@mslovy

This comment has been minimized.

Show comment
Hide comment
@mslovy

mslovy Sep 12, 2017

Contributor

#17677, hi @smithfarm , this PR is to disable the failing test

Contributor

mslovy commented Sep 12, 2017

#17677, hi @smithfarm , this PR is to disable the failing test

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