client: issue a cap release immediately if no cap exists by lxbsz · Pull Request #51500 · ceph/ceph

lxbsz · 2023-05-16T02:20:42Z

In case:

           mds                             client
                                - Releases cap and put Inode
  - Increase cap->seq and sends
    revokes req to the client
  - Receives release req and    - Receives & drops the revoke req
    skip removing the cap and
    then eval the CInode and
    issue or revoke caps again.
                                - Receives & drops the caps update
                                  or revoke req
  - Health warning for client
    isn't responding to
    mclientcaps(revoke)

All the IMPORT/REVOKE/GRANT cap ops will increase the session seq
in MDS side and then the client need to issue a cap release to
unblock MDS to remove the corresponding cap to unblock possible
waiters.

Fixes: https://tracker.ceph.com/issues/57244
Fixes: https://tracker.ceph.com/issues/61148
Signed-off-by: Xiubo Li xiubli@redhat.com

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- 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

vshankar

@lxbsz Could you explain how this change fixed the issue mentioned in https://tracker.ceph.com/issues/61148#note-7

lxbsz · 2023-05-17T10:28:44Z

@lxbsz Could you explain how this change fixed the issue mentioned in https://tracker.ceph.com/issues/61148#note-7

The 9fbde6d will remove the cap entity directly in MDS side even the seqs mismatch due to the racy between mds and client.

This is okay for the caps revoke/grant/import cases, because the clients will drop the cap metadatas directly too.
But for the case of https://tracker.ceph.com/issues/61148#note-7 the client won't drop the cap metadatas and will add it to the client instead. Then the cap_id and seq will be stale.

And if a new client request comes to MDS and when encoding the reply message it will add a new cap entity in the MDS side with a fresh new cap_id and a new seq.

This is why we can see the call traces and warning logs in dmesg.

Since only the caps revoke/grant/import have the issue we were trying to fix in 9fbde6d, then we can just issue a release immediately in client side just before dropping them.

vshankar · 2023-05-17T15:02:13Z

@lxbsz Could you explain how this change fixed the issue mentioned in https://tracker.ceph.com/issues/61148#note-7

The 9fbde6d will remove the cap entity directly in MDS side even the seqs mismatch due to the racy between mds and client.

For the record - commit 9fbde6d removes the part where a cap release from the client removes the cap from the revocation list (but does not remove the cap) if the mds had sent a revoke. Without this the "client failed to respond to cap revoke" warning would show up since the mds is tracking the cap in revokes list but the client has already dropped it.

This is okay for the caps revoke/grant/import cases, because the clients will drop the cap metadatas directly too. But for the case of https://tracker.ceph.com/issues/61148#note-7 the client won't drop the cap metadatas and will add it to the client instead. Then the cap_id and seq will be stale.

Got it. So with 9fbde6d the mds lost the cap metadata but the client keeps a note of it during open() when the open() call in the mds races with the delayed cap release (in the mds).

And if a new client request comes to MDS and when encoding the reply message it will add a new cap entity in the MDS side with a fresh new cap_id and a new seq.

Looks fine so far.

This is why we can see the call traces and warning logs in dmesg.

Since only the caps revoke/grant/import have the issue we were trying to fix in 9fbde6d, then we can just issue a release immediately in client side just before dropping them.

So, we add the logic back where the mds would remove the cap from the revokes list and not remove the cap from the inode, but the client is not tracking the relevant caps. I need to closely check the code you added to send cap releases...

lxbsz · 2023-05-18T02:03:26Z

This is why we can see the call traces and warning logs in dmesg.
Since only the caps revoke/grant/import have the issue we were trying to fix in 9fbde6d, then we can just issue a release immediately in client side just before dropping them.

So, we add the logic back where the mds would remove the cap from the revokes list and not remove the cap from the inode, but the client is not tracking the relevant caps. I need to closely check the code you added to send cap releases...

This is what the import will do if it couldn't find the corresponding cap in clients. When sending cap releases to MDS in this case it will use the increased seq and then the MDS could remove the cap since the seqs will match this time.

I just add the revoke/grant cases. Please note only the import/revoke/grant cases will increase the seq in MDS side.

vshankar · 2023-05-18T07:39:18Z

src/client/Client.cc

-        m->get_mseq(),
-        cap_epoch_barrier);
-    } else {
-      ldout(cct, 5) << __func__ << " don't have vino " << vino << ", dropping" << dendl;


The else case flushes the queued release caps. This behaviour is modified by this change - do you see any issues due to that?

No, the else case will only print this debug log and the flush just out side the else case.

Right, but the current code will always flush queued cap releases if vino isn't found.

IMO that's should be okay and the flush cap release will be trigger per tick.

Why it flushed it here is because the MDS may waiting for the cap release here in IMPORT case. And only when the MDS is waiting for it will it make sense to trigger the flush immediately here, or the MDS may will be stuck for 1 seconds at most.

ACK. Fair enough.

vshankar

LGTM

vshankar · 2023-05-22T06:31:21Z

@lxbsz In the interest if time (to get this PR tested and merged), should we revert PR #47752 for the time being?

EDIT: I already ack'd this and plan to put it to test, however, I might be on PTO for sometime. I'll try my best to get this tested asap, however, the main branch runs have the cap_id mismatch asserts.

lxbsz · 2023-05-22T07:48:14Z

@lxbsz In the interest if time (to get this PR tested and merged), should we revert PR #47752 for the time being?

EDIT: I already ack'd this and plan to put it to test, however, I might be on PTO for sometime. I'll try my best to get this tested asap, however, the main branch runs have the cap_id mismatch asserts.

Sure. Later I will generate a new PR together with #51500.

lxbsz · 2023-05-22T07:49:13Z

@lxbsz In the interest if time (to get this PR tested and merged), should we revert PR #47752 for the time being?

EDIT: I already ack'd this and plan to put it to test, however, I might be on PTO for sometime. I'll try my best to get this tested asap, however, the main branch runs have the cap_id mismatch asserts.

IMO, only revert 9fbde6d commit should be enough.

vshankar · 2023-05-22T08:06:33Z

@lxbsz In the interest if time (to get this PR tested and merged), should we revert PR #47752 for the time being?
EDIT: I already ack'd this and plan to put it to test, however, I might be on PTO for sometime. I'll try my best to get this tested asap, however, the main branch runs have the cap_id mismatch asserts.

IMO, only revert 9fbde6d commit should be enough.

True, however, I would prefer a PR revert (since it reverts the whole change) and then pick the other commits into this PR. WDYT?

lxbsz · 2023-05-22T08:19:16Z

@lxbsz In the interest if time (to get this PR tested and merged), should we revert PR #47752 for the time being?
EDIT: I already ack'd this and plan to put it to test, however, I might be on PTO for sometime. I'll try my best to get this tested asap, however, the main branch runs have the cap_id mismatch asserts.

IMO, only revert 9fbde6d commit should be enough.

True, however, I would prefer a PR revert (since it reverts the whole change) and then pick the other commits into this PR. WDYT?

Sure, np.

github-actions · 2023-05-22T17:57:10Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Will add the debug logs later in confirm_receipt(). Fixes: https://tracker.ceph.com/issues/57244 Signed-off-by: Xiubo Li <xiubli@redhat.com>

When revoking caps from clients and if the clients could release some of the caps references and the clients still could send cap update request back to MDS, while the confirm_receipt() will clear the _revokes list anyway. But this cap will still be kept in revoking_caps list. At the same time add one debug log when revocation is not totally finished. Fixes: https://tracker.ceph.com/issues/57244 Signed-off-by: Xiubo Li <xiubli@redhat.com>

In case: mds client - Releases cap and put Inode - Increase cap->seq and sends revokes req to the client - Receives release req and - Receives & drops the revoke req skip removing the cap and then eval the CInode and issue or revoke caps again. - Receives & drops the caps update or revoke req - Health warning for client isn't responding to mclientcaps(revoke) All the IMPORT/REVOKE/GRANT cap ops will increase the session seq in MDS side and then the client need to issue a cap release to unblock MDS to remove the corresponding cap to unblock possible waiters. Fixes: https://tracker.ceph.com/issues/57244 Fixes: https://tracker.ceph.com/issues/61148 Signed-off-by: Xiubo Li <xiubli@redhat.com>

When writing to a file and the max_size is approaching the client will try to trigger to call check_caps() and flush the caps to MDS. But just in case the MDS is revoking Fsxrw caps, since the client keeps writing and holding the Fw caps it may only release part of the caps but the Fw. Fixes: https://tracker.ceph.com/issues/57244 Signed-off-by: Xiubo Li <xiubli@redhat.com>

lxbsz · 2023-05-22T23:57:06Z

Pushed the reverted commits back from #47752 to this new PR and removed the problematic one.

mchangir

LGTM

vshankar · 2023-06-14T12:43:11Z

Pkgs building now -- will update with the run link.

vshankar · 2023-06-16T05:40:03Z

https://pulpito.ceph.com/vshankar-2023-06-15_04:58:28-fs-wip-vshankar-testing-20230614.124123-testing-default-smithi/

vshankar · 2023-06-19T07:41:24Z

https://pulpito.ceph.com/vshankar-2023-06-15_04:58:28-fs-wip-vshankar-testing-20230614.124123-testing-default-smithi/

Test run looks fine. I'll update the run wiki and merge this today. Nice work @lxbsz

lxbsz · 2023-06-19T10:12:38Z

https://pulpito.ceph.com/vshankar-2023-06-15_04:58:28-fs-wip-vshankar-testing-20230614.124123-testing-default-smithi/

Test run looks fine. I'll update the run wiki and merge this today. Nice work @lxbsz

Cool, Thanks @vshankar !

vshankar · 2023-06-20T07:27:43Z

https://tracker.ceph.com/projects/cephfs/wiki/Main#20-June-2023

batrick · 2024-06-26T18:37:41Z

src/client/Client.cc

+    session->enqueue_cap_release(
+      m->get_ino(),
+      m->get_cap_id(),
+      m->get_seq(),


This is problematic (change also in the kernel driver), causing:

https://tracker.ceph.com/issues/64977

It should be the last issue_seq but at this point the client does not have a valid issue_seq because the cap is already removed. Furthermore, the MDS does not share its view of the issue_seq associated with the grant/revoke; sadly only clients actually set the issue_seq in an MClientCaps message header.

For the CEPH_CAP_OP_IMPORT case, it was okay to use m->get_seq() because that's implicitly the issue_seq as well but this is not okay for the CEPH_CAP_OP_GRANT or CEPH_CAP_OP_REVOKE cases.

I'm confused by how the original issue occurs with the stated sequence in this PR's description:

https://tracker.ceph.com/issues/57244

specifically the second step on the MDS side "Receives release req and...". The MDS does not increase issue_seq when sending revokes. The client's release should be processed successfully. This is an expected sequence of events which was accounted for as long ago as b180b6c. I think there was something else going on here that should be investigated.

With that said, we're now obligated to support this new scenario where the client sends a cap release with issue_seq == (last revoke/grant) seq. This only happens if the client is basically saying: "I don't acknowledge that I have this cap; please release it.". The MDS should be able to handle this gracefully and that's basically done in #56828.

I'm in the middle of some cleanup relating to issue_seq but wanted to leave this note here for anyone looking at this in the future. Cleanup tracked in https://tracker.ceph.com/issues/66704.

cc @lxbsz @vshankar @kotreshhr

For the CEPH_CAP_OP_IMPORT case, it was okay to use m->get_seq() because that's implicitly the issue_seq as well but this is not okay for the CEPH_CAP_OP_GRANT or CEPH_CAP_OP_REVOKE cases.

Agreed. For the wider audience, the last_sent/last_issue/mseq has deep history to solve particularly this release/revoke race which unfortunately broke with this commit.

specifically the second step on the MDS side "Receives release req and...". The MDS does not increase issue_seq when sending revokes. The client's release should be processed successfully. This is an expected sequence of events which was accounted for as long ago as b180b6c. I think there was something else going on here that should be investigated.

... and this wasn't caught with fs suite runs which makes me worried a lot. Yes, it's a concurrent release/revoke case, but really we aren't testing to the extent where such cases are covered (esp. since we have now seen a couple of reports for this bug, one without much scale involved).

This is problematic (change also in the kernel driver), causing:

https://tracker.ceph.com/issues/64977

It should be the last issue_seq but at this point the client does not have a valid issue_seq because the cap is already removed.

How could this happen is what I eagerly want to figure out, after that I think we can resolve all the confusion about the revoke stuck issues.

Furthermore, the MDS does not share its view of the issue_seq associated with the grant/revoke; sadly only clients actually set the issue_seq in an MClientCaps message header.

The issue_seq in client will be updated only together with the client request reply msgs.

For the CEPH_CAP_OP_IMPORT case, it was okay to use m->get_seq() because that's implicitly the issue_seq as well but this is not okay for the CEPH_CAP_OP_GRANT or CEPH_CAP_OP_REVOKE cases.

I'm confused by how the original issue occurs with the stated sequence in this PR's description:

https://tracker.ceph.com/issues/57244

specifically the second step on the MDS side "Receives release req and...". The MDS does not increase issue_seq when sending revokes. The client's release should be processed successfully. This is an expected sequence of events which was accounted for as long ago as b180b6c. I think there was something else going on here that should be investigated.

With that said, we're now obligated to support this new scenario where the client sends a cap release with issue_seq == (last revoke/grant) seq. This only happens if the client is basically saying: "I don't acknowledge that I have this cap; please release it.". The MDS should be able to handle this gracefully and that's basically done in #56828.

I'm in the middle of some cleanup relating to issue_seq but wanted to leave this note here for anyone looking at this in the future. Cleanup tracked in https://tracker.ceph.com/issues/66704.

lxbsz requested a review from a team May 16, 2023 02:20

github-actions bot added the cephfs Ceph File System label May 16, 2023

lxbsz force-pushed the wip-61148 branch 3 times, most recently from 66b083f to 717d62b Compare May 16, 2023 03:06

vshankar reviewed May 17, 2023

View reviewed changes

vshankar reviewed May 18, 2023

View reviewed changes

vshankar approved these changes May 19, 2023

View reviewed changes

vshankar added the wip-vshankar-testing1 label May 19, 2023

lxbsz mentioned this pull request May 22, 2023

Revert "mds: fix client isn't responding to mclientcaps(revoke)" #51661

Merged

github-actions bot added the needs-rebase label May 22, 2023

lxbsz added 4 commits May 23, 2023 07:53

mds: move confirm_receipt() to Capability.cc

f9aa17c

Will add the debug logs later in confirm_receipt(). Fixes: https://tracker.ceph.com/issues/57244 Signed-off-by: Xiubo Li <xiubli@redhat.com>

lxbsz force-pushed the wip-61148 branch from 717d62b to 3c63980 Compare May 22, 2023 23:55

github-actions bot added the tests label May 22, 2023

github-actions bot removed the needs-rebase label May 23, 2023

mchangir approved these changes May 30, 2023

View reviewed changes

vshankar merged commit b02e1b4 into ceph:main Jun 20, 2023

vshankar removed the wip-vshankar-testing1 label Jun 20, 2023

This was referenced Aug 7, 2023

reef: client: issue a cap release immediately if no cap exists #52850

Merged

quincy: client: issue a cap release immediately if no cap exists #52851

Merged

pacific: client: issue a cap release immediately if no cap exists #52852

Merged

batrick reviewed Jun 26, 2024

View reviewed changes

Conversation

lxbsz commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

lxbsz commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vshankar commented May 17, 2023

Uh oh!

lxbsz commented May 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar commented May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lxbsz commented May 22, 2023

Uh oh!

lxbsz commented May 22, 2023

Uh oh!

vshankar commented May 22, 2023

Uh oh!

lxbsz commented May 22, 2023

Uh oh!

github-actions bot commented May 22, 2023

Uh oh!

lxbsz commented May 22, 2023

Uh oh!

mchangir left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar commented Jun 14, 2023

Uh oh!

vshankar commented Jun 16, 2023

Uh oh!

vshankar commented Jun 19, 2023

Uh oh!

lxbsz commented Jun 19, 2023

Uh oh!

vshankar commented Jun 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lxbsz commented May 16, 2023 •

edited

Loading

lxbsz commented May 17, 2023 •

edited

Loading

vshankar commented May 22, 2023 •

edited

Loading