Skip to content

mds: try to choose a new batch head in request_clientup()#57553

Merged
batrick merged 2 commits intoceph:mainfrom
lxbsz:wip-66124
Jun 4, 2024
Merged

mds: try to choose a new batch head in request_clientup()#57553
batrick merged 2 commits intoceph:mainfrom
lxbsz:wip-66124

Conversation

@lxbsz
Copy link
Member

@lxbsz lxbsz commented May 20, 2024 •
edited by leonid-s-usov
Loading

This will happen only for the client requests, not peer requests.
The 'mdr->killed' and 'mdr->dead' will always be set at the same
time when killing the client requests.

Fixes: https://tracker.ceph.com/issues/66124

Checklist

  • Tracker (select at least one)
    • References tracker ticket
  • Component impact
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • No doc update is appropriate
  • Tests (select at least one)
    • No tests
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows
  • jenkins test rook e2e

@lxbsz lxbsz requested review from a team, batrick, leonid-s-usov and vshankar May 20, 2024 04:24
@github-actions github-actions bot added the cephfs Ceph File System label May 20, 2024
@rishabh-d-dave
Copy link
Contributor

rishabh-d-dave commented May 20, 2024

@lxbsz Signed-off-by line is missing for commit Revert "mds: find a new head for the batch ops when the head is dead"

@lxbsz
Copy link
Member Author

lxbsz commented May 20, 2024

@lxbsz Signed-off-by line is missing for commit Revert "mds: find a new head for the batch ops when the head is dead"

@rishabh-d-dave Done. Thanks!

leonid-s-usov
leonid-s-usov reviewed May 20, 2024
View reviewed changes
leonid-s-usov
leonid-s-usov reviewed May 20, 2024
View reviewed changes
leonid-s-usov
leonid-s-usov reviewed May 20, 2024
View reviewed changes
Copy link
Contributor

@leonid-s-usov leonid-s-usov left a comment

Choose a reason for hiding this comment

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

Unlike in the old places, in the request_cleanup we should move on to the cleanup after we choose a new batch head, shouldn't we?

@lxbsz
mds: try to choose a new batch head in request_clientup()
73b266e 
This will happen only for the client requests, not peer requests.
The 'mdr->killed' and 'mdr->dead' will always be set at the same
time when killing the client requests.

Fixes: https://tracker.ceph.com/issues/66124
Signed-off-by: Xiubo Li <xiubli@redhat.com>
@lxbsz
Copy link
Member Author

lxbsz commented May 20, 2024

Unlike in the old places, in the request_cleanup we should move on to the cleanup after we choose a new batch head, shouldn't we?

BTW, what cleanup ? Could you point it out ?

@leonid-s-usov
Copy link
Contributor

leonid-s-usov commented May 20, 2024

Unlike in the old places, in the request_cleanup we should move on to the cleanup after we choose a new batch head, shouldn't we?

BTW, what cleanup ? Could you point it out ?

well, everything below your new block in the request_cleanup method, starting with the line https://github.com/ceph/ceph/pull/57553/files#diff-ea7cb1a6ba9fa08363b14dd00a86bc6b79e01673e93af84ffdfcdbd0d3f26b19R9956 and until the end

@lxbsz
Copy link
Member Author

lxbsz commented May 20, 2024

Unlike in the old places, in the request_cleanup we should move on to the cleanup after we choose a new batch head, shouldn't we?

BTW, what cleanup ? Could you point it out ?

well, everything below your new block in the request_cleanup method, starting with the line https://github.com/ceph/ceph/pull/57553/files#diff-ea7cb1a6ba9fa08363b14dd00a86bc6b79e01673e93af84ffdfcdbd0d3f26b19R9956 and until the end

Currently doesn't it what you mean ?

request_cleanup() {
   1, choose new batch head
   2, cleanup
}

Or do you mean we should :

request_cleanup() {
   1, cleanup
   2, choose new batch head
}

???

@leonid-s-usov
Copy link
Contributor

leonid-s-usov commented May 20, 2024 •
edited
Loading

No no, with your latest update, it's all good. Sorry for the confusion. I commented on the code lines but I also had to leave the general review comment because it wouldn't allow me to submit it otherwise - so I just rephrased the same thing.

@batrick
Copy link
Member

batrick commented May 28, 2024

This PR is under test in https://tracker.ceph.com/issues/66261.

@batrick
Copy link
Member

batrick commented May 28, 2024

jenkins test make check

@batrick
Copy link
Member

batrick commented May 28, 2024

jenkins test make check arm64

@batrick
Copy link
Member

batrick commented May 30, 2024

jenkins test make check

@batrick
Copy link
Member

batrick commented Jun 4, 2024

https://tracker.ceph.com/issues/66261#note-3

@batrick batrick merged commit c8f71d6 into ceph:main Jun 4, 2024
@lxbsz lxbsz mentioned this pull request Jun 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@batrick batrick batrick approved these changes

@vshankar vshankar Awaiting requested review from vshankar

+1 more reviewer

@leonid-s-usov leonid-s-usov leonid-s-usov left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cephfs Ceph File System needs-qa wip-pdonnell-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lxbsz @rishabh-d-dave @leonid-s-usov @batrick