Skip to content

mds: don't stall the asok thread for flush commands#57274

Merged
batrick merged 2 commits intomainfrom
wip-lusov-quiescer-relax
May 16, 2024
Merged

mds: don't stall the asok thread for flush commands#57274
batrick merged 2 commits intomainfrom
wip-lusov-quiescer-relax

Conversation

@leonid-s-usov
Copy link
Contributor

@leonid-s-usov leonid-s-usov commented May 5, 2024 •
edited
Loading

Also, relax some timing requirements in the quiescer

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

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

@leonid-s-usov leonid-s-usov requested a review from batrick May 5, 2024 21:22
@github-actions github-actions bot added the tests label May 5, 2024
@leonid-s-usov
Copy link
Contributor Author

leonid-s-usov commented May 5, 2024

This is used as the suite branch for https://pulpito.ceph.com/leonidus-2024-05-05_21:28:36-fs-wip-pdonnell-testing-20240503.163550-debug-distro-default-smithi/

@leonid-s-usov leonid-s-usov requested a review from a team May 6, 2024 06:20
vshankar
vshankar reviewed May 6, 2024
View reviewed changes
Copy link
Contributor

@vshankar vshankar left a comment

Choose a reason for hiding this comment

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

Looks fine!

Conditional approve post successful test run.

batrick
batrick requested changes May 6, 2024
View reviewed changes
Copy link
Member

@batrick batrick left a comment

Choose a reason for hiding this comment

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

https://tracker.ceph.com/issues/65803:#note-2

I think the simple fix is to make flush journal asynchronous. The quiesce command already is so please just do some similar logic there.

@leonid-s-usov
Copy link
Contributor Author

leonid-s-usov commented May 6, 2024

https://tracker.ceph.com/issues/65803:#note-2

I think the simple fix is to make flush journal asynchronous. The quiesce command already is so please just do some similar logic there.

I'm not sure it's that simple. We can't just change a synchronous command to asynchronous, can we? We'll have to add a switch to the command and change the invocation along with verifying that it'll know how to deal with the new approach.

In any case, it would probably have to be a separate ticket with its own PR...

@leonid-s-usov leonid-s-usov force-pushed the wip-lusov-quiescer-relax branch from 5c17ef6 to e7f8941 Compare May 6, 2024 16:53
@github-actions github-actions bot added the cephfs Ceph File System label May 6, 2024
@leonid-s-usov leonid-s-usov changed the title qa/quiescer: relax timing requirements for teuthology mds: don't stall the asok thread for flush commands May 6, 2024
@leonid-s-usov leonid-s-usov force-pushed the wip-lusov-quiescer-relax branch from e7f8941 to a4dc881 Compare May 6, 2024 16:59
@leonid-s-usov
Copy link
Contributor Author

leonid-s-usov commented May 6, 2024

https://tracker.ceph.com/issues/65803:#note-2
I think the simple fix is to make flush journal asynchronous. The quiesce command already is so please just do some similar logic there.

I'm not sure it's that simple. We can't just change a synchronous command to asynchronous, can we? We'll have to add a switch to the command and change the invocation along with verifying that it'll know how to deal with the new approach.

In any case, it would probably have to be a separate ticket with its own PR...

In a talk with @batrick he explained that it was about not holding up the asok thread, which I followed here

@leonid-s-usov leonid-s-usov requested a review from batrick May 6, 2024 17:03
@leonid-s-usov
Copy link
Contributor Author

leonid-s-usov commented May 7, 2024

jenkins test make check

lxbsz
lxbsz reviewed May 7, 2024
View reviewed changes
@leonid-s-usov
Copy link
Contributor Author

leonid-s-usov commented May 7, 2024

jenkins test make check

@batrick
Copy link
Member

batrick commented May 7, 2024

jenkins test make check arm64

vshankar
vshankar requested changes May 8, 2024
View reviewed changes
Copy link
Contributor

@vshankar vshankar left a comment

Choose a reason for hiding this comment

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

Nicely done.

vshankar
vshankar approved these changes May 8, 2024
View reviewed changes
Copy link
Contributor

@vshankar vshankar left a comment

Choose a reason for hiding this comment

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

Approved!

@batrick
Copy link
Member

batrick commented May 8, 2024

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

@leonid-s-usov leonid-s-usov mentioned this pull request May 14, 2024
Merged
@leonid-s-usov
Copy link
Contributor Author

leonid-s-usov commented May 15, 2024

@batrick please approve for merge. See #57332 (comment)

@batrick batrick merged commit 70ed382 into main May 16, 2024
@batrick batrick deleted the wip-lusov-quiescer-relax branch May 16, 2024 02:57
This was referenced May 20, 2024
Closed
Merged
@leonid-s-usov leonid-s-usov mentioned this pull request May 27, 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 vshankar approved these changes

@lxbsz lxbsz lxbsz approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@leonid-s-usov @batrick @vshankar @lxbsz