client: buffer the truncate if we have the Fx caps #43286

lxbsz · 2021-09-23T11:37:03Z

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

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

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

src/client/Client.cc

ukernel · 2021-09-26T01:42:25Z

you can't buffer "truncating file to smaller size"

lxbsz · 2021-09-30T09:23:46Z

you can't buffer "truncating file to smaller size"

This won't buffer in that case, only buffer the truncate when the new size is larger.

vshankar · 2021-09-30T13:17:40Z

you can't buffer "truncating file to smaller size"

Is this due to the reason that the mds has special handling for shrinking truncates? AFAICS, shrinking truncate additionally notifies clients (with updated caps) and initiates file truncate (via objecter). For extending file truncates, the cap update to clients is probably deferred?

vshankar · 2021-09-30T13:19:23Z

src/client/Client.cc

+      in->cap_dirtier_gid = perms.gid();
+      in->mark_caps_dirty(CEPH_CAP_FILE_EXCL);
+      mask &= ~(CEPH_SETATTR_SIZE);
+      mask |= CEPH_SETATTR_MTIME;


We could avoid some code duplication when updating inode (citme, uid,..) with what's done in the block below.

Makes sense, I can fix this.

lxbsz · 2021-10-08T07:13:49Z

you can't buffer "truncating file to smaller size"

Is this due to the reason that the mds has special handling for shrinking truncates? AFAICS, shrinking truncate additionally notifies clients (with updated caps) and initiates file truncate (via objecter). For extending file truncates, the cap update to clients is probably deferred?

For this, I think Zheng means that for the current code only the handle_client_setattr() will do the object truncating stuff. If we buffer the truncate with smaller size, then the client will defer do the truncate in the cap updating related code and in the MDS side the cap updating related code won't do the object truncating work.

IMO if we will buffer the truncate with smaller size we must adapt the MDS side code at the same time in this PR, which will make the code more complicated, seems doesn't make much sense AFAIK.

jtlayton · 2021-10-08T10:47:17Z

IMO if we will buffer the truncate with smaller size we must adapt the MDS side code at the same time in this PR, which will make the code more complicated, seems doesn't make much sense AFAIK.

Agreed. Let's just stick with not buffering truncates to smaller sizes. It wouldn't give us much benefit anyway.

vshankar · 2021-10-08T11:32:44Z

For this, I think Zheng means that for the current code only the handle_client_setattr() will do the object truncating stuff. If we buffer the truncate with smaller size, then the client will defer do the truncate in the cap updating related code and in the MDS side the cap updating related code won't do the object truncating work.

IMO if we will buffer the truncate with smaller size we must adapt the MDS side code at the same time in this PR, which will make the code more complicated, seems doesn't make much sense AFAIK.

Sure. I wasn't proposing that we start buffering shrinking truncates, I was interested in the reason as to why its handled differently. The client should adhere to the MDS handling of an operation.

jtlayton · 2021-10-08T11:42:16Z

Just to add, truncates in cephfs are really complex. It would be nice to have all of the cases carefully documented for posterity.

jtlayton · 2021-10-08T12:48:18Z

src/client/Client.cc

-    req->inode_drop |= CEPH_CAP_AUTH_SHARED;
-    ldout(cct,10) << "changing mode to " << stx->stx_mode << dendl;
+    if (!in->caps_issued_mask(CEPH_CAP_AUTH_SHARED) ||
+        stx->stx_mode != in->mode) {


It's pretty confusing having the logic for each CEPH_SETATTR_* value split into two different stanzas -- one that occurs before "force request:" and one that occurs after. It sort of makes sense in the existing code -- we check to see if we can buffer the attribute change and if we can't we then build a request and send it off.

This patch changes that though and it's harder to follow the logic around whether the change will be buffered.

Why not do the testing/handling for As caps and the mode change in the CEPH_SETATTR_MODE section above this point (around line #7524)?

Ditto for the other CEPH_SETATTR_* changes.

It's pretty confusing having the logic for each CEPH_SETATTR_* value split into two different stanzas -- one that occurs before "force request:" and one that occurs after. It sort of makes sense in the existing code -- we check to see if we can buffer the attribute change and if we can't we then build a request and send it off.

Yeah, this sounds good.

This patch changes that though and it's harder to follow the logic around whether the change will be buffered.

Why not do the testing/handling for As caps and the mode change in the CEPH_SETATTR_MODE section above this point (around line #7524)?

Ditto for the other CEPH_SETATTR_* changes.

Sure, will improve it.

jtlayton

I'm not a fan of this patch. I think the existing organization of this function (where you decide whether to buffer any of the changes and then allocate a new req and fire it off if so), is basically correct. This seems like a step backward from that.

src/client/Client.cc

jtlayton · 2021-10-11T19:00:29Z

Nice work @lxbsz !

batrick

Otherwise looks like good changes. Well done!

batrick · 2021-10-19T00:32:12Z

src/client/Client.cc

+      in->mark_caps_dirty(CEPH_CAP_FILE_EXCL);
+      mask &= ~CEPH_SETATTR_MTIME;
+    } else if (in->caps_issued_mask(CEPH_CAP_FILE_WR) &&
+               utime_t(stx->stx_mtime) > in->mtime) {


This one concerns me. Would this not break a reader + writer where the reader is polling for mtime updates?

Yeah, this sounds a problem. Anyway in this case we must update the mtime to the MDS by either setattr request or cap update request.

To be simple I will remove this code currently. Thanks.

There is some general confusion around what caps cover these attributes. The kernel wants Fs to read the mtime in a getattr request:

if (want & (STATX_ATIME|STATX_MTIME|STATX_CTIME|STATX_SIZE| STATX_BLOCKS)) mask |= CEPH_CAP_FILE_SHARED;

...but that looks wrong, in hindsight. Can a client hold Fw caps while another has Fs? I suspect that it can. If so, then the kclient's getattr path is not working as it should.

Again -- we really need a canonical treatise on what caps cover what inode metadata (particularly for FILE caps). Maybe we should badger @liewegas, @gregsfortytwo and @ukernel for such a thing?

My guess though is that this has never been clearly defined throughout cephfs's existence. We'll probably need to write up a "spec" for this ourselves, and then fix the userland and kernel code to conform to it.

There is some general confusion around what caps cover these attributes. The kernel wants Fs to read the mtime in a getattr request:

if (want & (STATX_ATIME|STATX_MTIME|STATX_CTIME|STATX_SIZE| STATX_BLOCKS)) mask |= CEPH_CAP_FILE_SHARED;

...but that looks wrong, in hindsight. Can a client hold Fw caps while another has Fs? I suspect that it can. If so, then the kclient's getattr path is not working as it should.

You should be able to check this by grokking filelock in mds/locks.c, which...okay, when I started writing this I didn't but I think I remember how it works now:
You cannot issue both Fs and Fw to distinct clients — the CEPH_CAP_GWR can be issued only in LOCK_MIX or LOCK_EXCL(usive) states, and MIX doesn't allow CEPH_CAP_GSHARED. (EXCL does, but you can only be in that state with a single client on the file.)

Again -- we really need a canonical treatise on what caps cover what inode metadata (particularly for FILE caps). Maybe we should badger @liewegas, @gregsfortytwo and @ukernel for such a thing?

My guess though is that this has never been clearly defined throughout cephfs's existence. We'll probably need to write up a "spec" for this ourselves, and then fix the userland and kernel code to conform to it.

There are a few fuzzy bits — I seem to recall stuff like the dentry leases have timestamps that are provided on basic listings, and those don't get updated because it was wildly too expensive, but a full stat will refresh mtime properly — but I think this is pretty well defined. If you want to write out specific questions or inode metadata fields and their covering bits we can correct and help fill it in, but you've pretty well got it in ceph.git/doc/cephfs/capabilities.rst and being able to read locks.c will help you go a long way when trying to understand the conflicts and interactions you need to worry about.

There is some general confusion around what caps cover these attributes. The kernel wants Fs to read the mtime in a getattr request:

if (want & (STATX_ATIME|STATX_MTIME|STATX_CTIME|STATX_SIZE| STATX_BLOCKS)) mask |= CEPH_CAP_FILE_SHARED;

...but that looks wrong, in hindsight. Can a client hold Fw caps while another has Fs? I suspect that it can. If so, then the kclient's getattr path is not working as it should.

You should be able to check this by grokking filelock in mds/locks.c, which...okay, when I started writing this I didn't but I think I remember how it works now: You cannot issue both Fs and Fw to distinct clients — the CEPH_CAP_GWR can be issued only in LOCK_MIX or LOCK_EXCL(usive) states, and MIX doesn't allow CEPH_CAP_GSHARED. (EXCL does, but you can only be in that state with a single client on the file.)

Yeah, double checked this. The Fs and Fw are not allowed to distinct clients at the same time.

There is some general confusion around what caps cover these attributes. The kernel wants Fs to read the mtime in a getattr request:

if (want & (STATX_ATIME|STATX_MTIME|STATX_CTIME|STATX_SIZE| STATX_BLOCKS)) mask |= CEPH_CAP_FILE_SHARED;

...but that looks wrong, in hindsight. Can a client hold Fw caps while another has Fs? I suspect that it can. If so, then the kclient's getattr path is not working as it should.

Thanks @jtlayton, you just remind me that when polling for the mtime it should get the Fs caps not the Fr, and the other clients shouldn't have the Fw caps, so this should be safe. I will add this back.

@batrick @vshankar Please take a look.

As Jeff mentioned and I double checked, the Fs and Fw caps are not allowed to distinct clients at the same time.

If one reader is polling the mtime updates it should use the statx() with AT_STATX_FORCE_SYNC. From the posix doc, without the AT_STATX_FORCE_SYNC, it won't make sure that the mtime will be the newest, just return what the local cache has.

Once the AT_STATX_FORCE_SYNC was set, client will always sync the attributes from the MDS server by requiring the Fs caps. Then in MDS it will rdlock the FILE lock, that means all the Fw caps must be revoked. Then the writer client will flush it.

More detail about the Fs caps, please see my another PR #43722.

lxbsz · 2021-11-18T02:49:02Z

Rebased onto the master.

lxbsz · 2021-11-18T04:18:50Z

jenkins test make check arm64

lxbsz · 2021-11-18T04:19:57Z

jenkins retest this please

vshankar · 2021-11-23T05:02:23Z

jenkins test api

lxbsz · 2021-11-30T01:34:39Z

Rebased it.

lxbsz · 2021-12-01T00:39:40Z

Rebased it to address the check failures.

lxbsz · 2021-12-01T08:41:42Z

jenkins test make check

vshankar

LGTM.

I would have liked to avoid code duplication, but that's NBD and can be done later.

If the As caps is issued and the gid/uid/mode/btime don't change, then it's safe to skip sending out a setattr request to MDS. Signed-off-by: Xiubo Li <xiubli@redhat.com>

If the Fx caps is issued and new size is larger we can try to buffer the truncate, and just ignore in case the new size equals to current size. Signed-off-by: Xiubo Li <xiubli@redhat.com>

When the Fw caps is issued and increasing the atime/mtime, we can buffer the atime/mtime. And when the Fs caps is issued and they are not changing we can just ignore them. In case when recovering the atime/mtime maybe decreased, no matter the Fsw caps are issued, always send the request immediatelly. Signed-off-by: Xiubo Li <xiubli@redhat.com>

If the caller perms is different with the current dirtier's, we can still cache the attributes locally if possible, but will mark the ctime needs to be updated forcely and the request will implicitly flush the dirty caps for us. Signed-off-by: Xiubo Li <xiubli@redhat.com>

vshankar · 2022-01-06T04:50:18Z

@lxbsz Is this ready for review/testing?

lxbsz · 2022-01-06T06:23:26Z

@lxbsz Is this ready for review/testing?

Yeah.

vshankar · 2022-01-06T06:30:25Z

jenkins test api

vshankar · 2022-01-13T12:48:43Z

https://tracker.ceph.com/projects/cephfs/wiki/Master#2022-Jan-13

Comments addressed.

github-actions bot added the cephfs Ceph File System label Sep 23, 2021

lxbsz requested review from a team, jtlayton and batrick September 23, 2021 11:37

jtlayton approved these changes Sep 23, 2021

View reviewed changes

src/client/Client.cc Outdated Show resolved Hide resolved

vshankar requested changes Sep 30, 2021

View reviewed changes

lxbsz force-pushed the improve_setattr branch from b59c6f9 to b93cdc6 Compare October 8, 2021 02:31

lxbsz requested review from vshankar and jtlayton October 8, 2021 02:35

jtlayton suggested changes Oct 8, 2021

View reviewed changes

lxbsz force-pushed the improve_setattr branch 2 times, most recently from bca4269 to 6437ed0 Compare October 11, 2021 11:49

lxbsz requested a review from jtlayton October 11, 2021 11:50

jtlayton suggested changes Oct 11, 2021

View reviewed changes

src/client/Client.cc Show resolved Hide resolved

lxbsz force-pushed the improve_setattr branch 2 times, most recently from 43caa12 to 3a8e870 Compare October 11, 2021 14:52

jtlayton self-requested a review October 11, 2021 14:59

jtlayton approved these changes Oct 11, 2021

View reviewed changes

lxbsz added the needs-qa label Oct 14, 2021

batrick previously requested changes Oct 19, 2021

View reviewed changes

lxbsz force-pushed the improve_setattr branch from 3a8e870 to 22833db Compare October 19, 2021 05:03

lxbsz force-pushed the improve_setattr branch from 1289883 to ca57e9e Compare November 18, 2021 02:48

lxbsz force-pushed the improve_setattr branch from ca57e9e to 9831685 Compare November 30, 2021 01:34

lxbsz force-pushed the improve_setattr branch from 9831685 to 2bb030a Compare December 1, 2021 00:38

vshankar approved these changes Dec 9, 2021

View reviewed changes

vshankar added wip-vshankar-testing and removed wip-vshankar-testing labels Dec 9, 2021

lxbsz added 4 commits December 20, 2021 08:44

client: try to skip changing the gid/uid/mode/btime if As is issued

ec32f09

If the As caps is issued and the gid/uid/mode/btime don't change, then it's safe to skip sending out a setattr request to MDS. Signed-off-by: Xiubo Li <xiubli@redhat.com>

client: try to buffer the truncate if we have the Fx caps

b557d64

If the Fx caps is issued and new size is larger we can try to buffer the truncate, and just ignore in case the new size equals to current size. Signed-off-by: Xiubo Li <xiubli@redhat.com>

lxbsz force-pushed the improve_setattr branch from 2bb030a to 569770a Compare December 20, 2021 02:50

vshankar added the wip-vshankar-testing label Jan 6, 2022

vshankar approved these changes Jan 13, 2022

View reviewed changes

vshankar merged commit 6028ffb into ceph:master Jan 13, 2022

vshankar removed the wip-vshankar-testing label Jan 13, 2022

lxbsz mentioned this pull request Feb 21, 2022

client: Inode::hold_caps_until is time from monotonic clock now. #44247

Merged

14 tasks

lxbsz mentioned this pull request Apr 6, 2022

pacific: client: buffer the truncate if we have the Fx caps #45792

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: buffer the truncate if we have the Fx caps #43286

client: buffer the truncate if we have the Fx caps #43286

lxbsz commented Sep 23, 2021 •

edited

ukernel commented Sep 26, 2021

lxbsz commented Sep 30, 2021

vshankar commented Sep 30, 2021

vshankar Sep 30, 2021

lxbsz Oct 8, 2021

lxbsz commented Oct 8, 2021

jtlayton commented Oct 8, 2021

vshankar commented Oct 8, 2021

jtlayton commented Oct 8, 2021

jtlayton Oct 8, 2021

lxbsz Oct 11, 2021

jtlayton left a comment

jtlayton commented Oct 11, 2021

batrick left a comment

batrick Oct 19, 2021

lxbsz Oct 19, 2021

jtlayton Oct 19, 2021

gregsfortytwo Oct 19, 2021

lxbsz Oct 21, 2021

lxbsz Oct 21, 2021

lxbsz Nov 4, 2021 •

edited

lxbsz commented Nov 18, 2021

lxbsz commented Nov 18, 2021

lxbsz commented Nov 18, 2021

vshankar commented Nov 23, 2021

lxbsz commented Nov 30, 2021

lxbsz commented Dec 1, 2021

lxbsz commented Dec 1, 2021

vshankar left a comment

vshankar commented Jan 6, 2022

lxbsz commented Jan 6, 2022

vshankar commented Jan 6, 2022

vshankar commented Jan 13, 2022

client: buffer the truncate if we have the Fx caps #43286

client: buffer the truncate if we have the Fx caps #43286

Conversation

lxbsz commented Sep 23, 2021 • edited

Checklist

ukernel commented Sep 26, 2021

lxbsz commented Sep 30, 2021

vshankar commented Sep 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxbsz commented Oct 8, 2021

jtlayton commented Oct 8, 2021

vshankar commented Oct 8, 2021

jtlayton commented Oct 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtlayton left a comment

Choose a reason for hiding this comment

jtlayton commented Oct 11, 2021

batrick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxbsz Nov 4, 2021 • edited

Choose a reason for hiding this comment

lxbsz commented Nov 18, 2021

lxbsz commented Nov 18, 2021

lxbsz commented Nov 18, 2021

vshankar commented Nov 23, 2021

lxbsz commented Nov 30, 2021

lxbsz commented Dec 1, 2021

lxbsz commented Dec 1, 2021

vshankar left a comment

Choose a reason for hiding this comment

vshankar commented Jan 6, 2022

lxbsz commented Jan 6, 2022

vshankar commented Jan 6, 2022

vshankar commented Jan 13, 2022

lxbsz commented Sep 23, 2021 •

edited

lxbsz Nov 4, 2021 •

edited