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: mds: damage reporting by ino number is useless #14699

Merged
merged 6 commits into from Aug 23, 2017

Conversation

Projects
None yet
4 participants

@smithfarm smithfarm self-assigned this Apr 20, 2017

@smithfarm smithfarm added this to the jewel milestone Apr 20, 2017

@smithfarm smithfarm added bug fix core cephfs and removed core labels Apr 20, 2017

@smithfarm smithfarm changed the title from jewel: MDS: damage reporting by ino number is useless to jewel: mds: damage reporting by ino number is useless Apr 20, 2017

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Apr 25, 2017

Contributor

@jcsp Getting "cluster [WRN] Scrub error on inode" (warning in mds log) in the latest jewel integration runs. [1] Do you think it could be caused by this PR?

[1] http://tracker.ceph.com/issues/19538#note-45

Contributor

smithfarm commented Apr 25, 2017

@jcsp Getting "cluster [WRN] Scrub error on inode" (warning in mds log) in the latest jewel integration runs. [1] Do you think it could be caused by this PR?

[1] http://tracker.ceph.com/issues/19538#note-45

@smithfarm smithfarm changed the title from jewel: mds: damage reporting by ino number is useless to [DNM] jewel: mds: damage reporting by ino number is useless Apr 25, 2017

@jcsp

This comment has been minimized.

Show comment
Hide comment
@jcsp

jcsp May 11, 2017

Contributor

@smithfarm yes, looks like there was a followup commit that fixed the tests to whitelist the log messages:

commit 795094628b65eb9c3f5d51c6c895fe1443c5f4cf
Author: John Spray <john.spray@redhat.com>
Date:   Thu Sep 29 17:14:54 2016 +0100

    suites: update log whitelist for scrub msg
    
    Fixes: http://tracker.ceph.com/issues/16016
    Signed-off-by: John Spray <john.spray@redhat.com>
Contributor

jcsp commented May 11, 2017

@smithfarm yes, looks like there was a followup commit that fixed the tests to whitelist the log messages:

commit 795094628b65eb9c3f5d51c6c895fe1443c5f4cf
Author: John Spray <john.spray@redhat.com>
Date:   Thu Sep 29 17:14:54 2016 +0100

    suites: update log whitelist for scrub msg
    
    Fixes: http://tracker.ceph.com/issues/16016
    Signed-off-by: John Spray <john.spray@redhat.com>
@jcsp

This comment has been minimized.

Show comment
Hide comment
@jcsp

jcsp Jun 14, 2017

Contributor

@smithfarm could you update this PR to cherry pick in that log whitelist update?

Contributor

jcsp commented Jun 14, 2017

@smithfarm could you update this PR to cherry pick in that log whitelist update?

@smithfarm smithfarm changed the title from [DNM] jewel: mds: damage reporting by ino number is useless to jewel: mds: damage reporting by ino number is useless Jun 14, 2017

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Jun 14, 2017

Contributor

@jcsp Done!

Contributor

smithfarm commented Jun 14, 2017

@jcsp Done!

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Jun 14, 2017

Contributor

@jcsp Should I cherry-pick in the other commits related to http://tracker.ceph.com/issues/16016 as well? I.e. the three from #11136 ?

Contributor

smithfarm commented Jun 14, 2017

@jcsp Should I cherry-pick in the other commits related to http://tracker.ceph.com/issues/16016 as well? I.e. the three from #11136 ?

@jcsp

This comment has been minimized.

Show comment
Hide comment
@jcsp

jcsp Jun 14, 2017

Contributor

The other ones from #11136 are probably a bit too invasive for a backport -- if the log whitelists are in sync with the code now then we're good to go.

Contributor

jcsp commented Jun 14, 2017

The other ones from #11136 are probably a bit too invasive for a backport -- if the log whitelists are in sync with the code now then we're good to go.

@jcsp

This comment has been minimized.

Show comment
Hide comment
@jcsp

jcsp Jun 14, 2017

Contributor

Sorry, I'm contradicting myself -- it was me that set the backport field on the other ticket to begin with!

So yes, let's go ahead and bring that change in too.

Contributor

jcsp commented Jun 14, 2017

Sorry, I'm contradicting myself -- it was me that set the backport field on the other ticket to begin with!

So yes, let's go ahead and bring that change in too.

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Jun 14, 2017

Contributor

@jcsp I don't care one way or the other, but did notice that the backport field had been set.

Contributor

smithfarm commented Jun 14, 2017

@jcsp I don't care one way or the other, but did notice that the backport field had been set.

@smithfarm smithfarm changed the title from jewel: mds: damage reporting by ino number is useless to [DNM] jewel: mds: damage reporting by ino number is useless Jun 14, 2017

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Jun 14, 2017

Contributor

Backport needs to be re-done (The 16016 commits need to go in first). Marking DNM and putting it on my todo list.

Contributor

smithfarm commented Jun 14, 2017

Backport needs to be re-done (The 16016 commits need to go in first). Marking DNM and putting it on my todo list.

stiopaa1 and others added some commits Oct 11, 2016

mds/DamageTable: move classes to .cc file
Signed-off-by: Michal Jarzabek <stiopa@gmail.com>
(cherry picked from commit 96018b0)
mds: remove redundant checks for null ScrubHeader
This was originally optional but now all the paths
that kick off a scrub should be going through
enqueue_scrub and thereby getting a header set.

Signed-off-by: John Spray <john.spray@redhat.com>
(cherry picked from commit 0c89028)

Conflicts:
	src/mds/CInode.cc (jewel does not have 5259683)
mds: tidy up ScrubHeader
Signed-off-by: John Spray <john.spray@redhat.com>
(cherry picked from commit 111d2cf)

Conflicts:
	src/mds/CInode.cc (jewel does not have 5259683)
mds: populate DamageTable from scrub and log more quietly
Fixes: http://tracker.ceph.com/issues/16016
Signed-off-by: John Spray <john.spray@redhat.com>
(cherry picked from commit 9c82040)
mds: include advisory `path` field in damage
This will just be whatever path we were looking
at at the point that damage was notified -- no
intention whatsoever of providing any up to date
path or resolution when there are multiple paths
to an inode.

Fixes: http://tracker.ceph.com/issues/18509
Signed-off-by: John Spray <john.spray@redhat.com>
(cherry picked from commit c0bff51)

Conflicts:
    src/mds/CDir.cc - omit dout(10) because jewel does not have cb86740
    src/mds/ScrubStack.cc - jewel does not have 7b45610 which changed
       in->make_path_string_projected() call to in->make_path_string() but
       it's moot because that line is dropped
suites: update log whitelist for scrub msg
Fixes: http://tracker.ceph.com/issues/16016
Signed-off-by: John Spray <john.spray@redhat.com>
(cherry picked from commit 7950946)

Conflicts:
	suites/fs/recovery/tasks/forward-scrub.yaml (file does not exist in jewel)

@smithfarm smithfarm changed the title from [DNM] jewel: mds: damage reporting by ino number is useless to jewel: mds: damage reporting by ino number is useless Jun 19, 2017

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Aug 22, 2017

Contributor

@batrick @ukernel @jcsp This passed an fs suite at http://tracker.ceph.com/issues/20613#note-7 with two occurrences of the following transient failure: http://tracker.ceph.com/issues/16881

I see @batrick has already approved, but I'll leave it to you guys to merge after considering the implications of http://tracker.ceph.com/issues/16881 - thanks!

Contributor

smithfarm commented Aug 22, 2017

@batrick @ukernel @jcsp This passed an fs suite at http://tracker.ceph.com/issues/20613#note-7 with two occurrences of the following transient failure: http://tracker.ceph.com/issues/16881

I see @batrick has already approved, but I'll leave it to you guys to merge after considering the implications of http://tracker.ceph.com/issues/16881 - thanks!

@smithfarm smithfarm merged commit fd197dc into ceph:jewel Aug 23, 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

@smithfarm smithfarm deleted the smithfarm:wip-19679-jewel branch Aug 23, 2017

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