client: flush the caps release in filesystem sync by lxbsz · Pull Request #58896 · ceph/ceph

lxbsz · 2024-07-29T06:30:00Z

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

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. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

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
jenkins test rook e2e

vshankar

Please explain the commit message regarding the idea behind the change.

dparmar18 · 2024-07-29T10:27:50Z

were there any problems uncovered due this @lxbsz ?

dparmar18 · 2024-07-29T10:49:05Z

In tracker you mentioned "Though the tick thread will do this but some times the tick could be stuck.", is it due to the thread entering sleep here?:

ceph/src/client/Client.cc

Lines 7119 to 7121 in 2fa0e43

    
           ldout(cct, 20) << "upkeep thread waiting interval " << interval << dendl; 
        
           if (!tick_thread_stopped) 
        
             upkeep_cond.wait_for(cl, interval);

So maybe when the thread is sleeping, we're at a situation where the cap flush is needed but can't until the thread wakes up, but this should just be a slight delay i guess, shouldn't be that critical. What are other scenarios?

lxbsz · 2024-07-29T12:44:07Z

Please explain the commit message regarding the idea behind the change.

Done.

lxbsz · 2024-07-29T12:46:55Z

In tracker you mentioned "Though the tick thread will do this but some times the tick could be stuck.", is it due to the thread entering sleep here?:

ceph/src/client/Client.cc

Lines 7119 to 7121 in 2fa0e43

ldout(cct, 20) << "upkeep thread waiting interval " << interval << dendl;

if (!tick_thread_stopped)

upkeep_cond.wait_for(cl, interval);

So maybe when the thread is sleeping, we're at a situation where the cap flush is needed but can't until the thread wakes up, but this should just be a slight delay i guess, shouldn't be that critical. What are other scenarios?

Not sure, before I hit several times the tick thread was blocked for a long time, maybe over load or something else.

Here for this we just want to explicitly expose one method to flush the cap release manually to get rid of the cap revoke stuck bugs as we hit in https://tracker.ceph.com/issues/57244 many times before.

lxbsz · 2024-07-29T12:48:16Z

were there any problems uncovered due this @lxbsz ?

We have figured out there has a race between caps revoke and the cap release in kclient. Just want to expose one way to flush the cap releases manually to get rid of the bug.

dparmar18 · 2024-08-06T13:11:58Z

were there any problems uncovered due this @lxbsz ?

We have figured out there has a race between caps revoke and the cap release in kclient. Just want to expose one way to flush the cap releases manually to get rid of the bug.

So this is a temporary fix?

vshankar · 2024-08-06T16:29:15Z

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

lxbsz · 2024-08-06T23:57:19Z

were there any problems uncovered due this @lxbsz ?

We have figured out there has a race between caps revoke and the cap release in kclient. Just want to expose one way to flush the cap releases manually to get rid of the bug.

So this is a temporary fix?

No, the caps releases also the metadata should be flushed when syncing the whole filesystem.

lxbsz · 2024-08-06T23:57:54Z

jenkins test api

lxbsz · 2024-08-06T23:58:12Z

jenkins test make check

dparmar18

LGTM, we need a mechanism to flush cap releases manually and till now we had none. Other thing is Client::sync_fs does flush the metadata logs at Client::flush_mdlog_sync but never did we flush the cap releases. This is adding the missing bit to the function.

vshankar

https://tracker.ceph.com/projects/cephfs/wiki/Main#wip-vshankar-testing-20240806162829-debug

vshankar · 2024-08-14T05:04:37Z

jenkins test make check

vshankar · 2024-08-14T05:04:42Z

jenkins test api

* refs/pull/58896/head: client: flush the caps release in filesystem sync

vshankar · 2024-08-14T15:12:30Z

jenkins retest this please

We have hit a race between cap releases and cap revoke request that will cause the check_caps() to miss sending a cap revoke ack to MDS. And the client will depend on the cap release to release that revoking caps, which could be delayed for some unknown reasons. In Kclient we have figured out the RCA about race and we need a way to explictly trigger this manually could help to get rid of the caps revoke stuck issue. Fixes: https://tracker.ceph.com/issues/67221 Signed-off-by: Xiubo Li <xiubli@redhat.com>

vshankar · 2024-08-16T06:02:46Z

rebased and pushed for getting jenkins tests to pass.

lxbsz requested a review from a team July 29, 2024 06:30

github-actions bot added the cephfs Ceph File System label Jul 29, 2024

vshankar requested changes Jul 29, 2024

View reviewed changes

lxbsz force-pushed the wip-67221 branch from 5ae44d8 to ad66f40 Compare July 29, 2024 12:35

vshankar added the wip-vshankar-testing1 label Aug 6, 2024

dparmar18 approved these changes Aug 7, 2024

View reviewed changes

vshankar approved these changes Aug 14, 2024

View reviewed changes

joscollin pushed a commit to joscollin/ceph that referenced this pull request Aug 14, 2024

Merge PR ceph#58896 into wip-vshankar-testing-20240806.162829-debug

732f0a4

* refs/pull/58896/head: client: flush the caps release in filesystem sync

vshankar force-pushed the wip-67221 branch from ad66f40 to b5bfc45 Compare August 16, 2024 02:58

vshankar merged commit 7ac44d6 into ceph:main Aug 16, 2024

vshankar removed the wip-vshankar-testing1 label Aug 16, 2024

Conversation

lxbsz commented Jul 29, 2024 • edited by dparmar18 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!

dparmar18 commented Jul 29, 2024

Uh oh!

dparmar18 commented Jul 29, 2024

Uh oh!

lxbsz commented Jul 29, 2024

Uh oh!

lxbsz commented Jul 29, 2024

Uh oh!

lxbsz commented Jul 29, 2024

Uh oh!

dparmar18 commented Aug 6, 2024

Uh oh!

vshankar commented Aug 6, 2024

Uh oh!

lxbsz commented Aug 6, 2024

Uh oh!

lxbsz commented Aug 6, 2024

Uh oh!

lxbsz commented Aug 6, 2024

Uh oh!

dparmar18 left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar commented Aug 14, 2024

Uh oh!

vshankar commented Aug 14, 2024

Uh oh!

vshankar commented Aug 14, 2024

Uh oh!

vshankar commented Aug 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lxbsz commented Jul 29, 2024 •

edited by dparmar18

Loading