On Thu, Feb 17, 2022 at 6:55 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > On Thu, 2022-02-17 at 11:03 +0800, Yan, Zheng wrote: > > On Tue, Feb 15, 2022 at 11:04 PM <xiubli@xxxxxxxxxx> wrote: > > > > > > From: Xiubo Li <xiubli@xxxxxxxxxx> > > > > > > No need to update snapshot context when any of the following two > > > cases happens: > > > 1: if my context seq matches realm's seq and realm has no parent. > > > 2: if my context seq equals or is larger than my parent's, this > > > works because we rebuild_snap_realms() works _downward_ in > > > hierarchy after each update. > > > > > > This fix will avoid those inodes which accidently calling > > > ceph_queue_cap_snap() and make no sense, for exmaple: > > > > > > There have 6 directories like: > > > > > > /dir_X1/dir_X2/dir_X3/ > > > /dir_Y1/dir_Y2/dir_Y3/ > > > > > > Firstly, make a snapshot under /dir_X1/dir_X2/.snap/snap_X2, then > > > make a root snapshot under /.snap/root_snap. And every time when > > > we make snapshots under /dir_Y1/..., the kclient will always try > > > to rebuild the snap context for snap_X2 realm and finally will > > > always try to queue cap snaps for dir_Y2 and dir_Y3, which makes > > > no sense. > > > > > > That's because the snap_X2's seq is 2 and root_snap's seq is 3. > > > So when creating a new snapshot under /dir_Y1/... the new seq > > > will be 4, and then the mds will send kclient a snapshot backtrace > > > in _downward_ in hierarchy: seqs 4, 3. Then in ceph_update_snap_trace() > > > it will always rebuild the from the last realm, that's the root_snap. > > > So later when rebuilding the snap context it will always rebuild > > > the snap_X2 realm and then try to queue cap snaps for all the inodes > > > related in snap_X2 realm, and we are seeing the logs like: > > > > > > "ceph: queue_cap_snap 00000000a42b796b nothing dirty|writing" > > > > > > URL: https://tracker.ceph.com/issues/44100 > > > Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx> > > > --- > > > fs/ceph/snap.c | 16 +++++++++------- > > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > > > diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c > > > index d075d3ce5f6d..1f24a5de81e7 100644 > > > --- a/fs/ceph/snap.c > > > +++ b/fs/ceph/snap.c > > > @@ -341,14 +341,16 @@ static int build_snap_context(struct ceph_snap_realm *realm, > > > num += parent->cached_context->num_snaps; > > > } > > > > > > - /* do i actually need to update? not if my context seq > > > - matches realm seq, and my parents' does to. (this works > > > - because we rebuild_snap_realms() works _downward_ in > > > - hierarchy after each update.) */ > > > + /* do i actually need to update? No need when any of the following > > > + * two cases: > > > + * #1: if my context seq matches realm's seq and realm has no parent. > > > + * #2: if my context seq equals or is larger than my parent's, this > > > + * works because we rebuild_snap_realms() works _downward_ in > > > + * hierarchy after each update. > > > + */ > > > if (realm->cached_context && > > > - realm->cached_context->seq == realm->seq && > > > - (!parent || > > > - realm->cached_context->seq >= parent->cached_context->seq)) { > > > + ((realm->cached_context->seq == realm->seq && !parent) || > > > + (parent && realm->cached_context->seq >= parent->cached_context->seq))) { > > > > With this change. When you mksnap on /dir_Y1/, its snap context keeps > > unchanged. In ceph_update_snap_trace, reset the 'invalidate' variable > > for each realm should fix this issue. > > > > This comment is terribly vague. "invalidate" is a local variable in that > function and isn't set on a per-realm basis. > > Could you suggest a patch on top of Xiubo's patch instead? > something like this (not tested) diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c index af502a8245f0..6ef41764008b 100644 --- a/fs/ceph/snap.c +++ b/fs/ceph/snap.c @@ -704,7 +704,8 @@ int ceph_update_snap_trace(struct ceph_mds_client *mdsc, __le64 *prior_parent_snaps; /* encoded */ struct ceph_snap_realm *realm = NULL; struct ceph_snap_realm *first_realm = NULL; - int invalidate = 0; + struct ceph_snap_realm *realm_to_inval = NULL; + int invalidate; int err = -ENOMEM; LIST_HEAD(dirty_realms); @@ -712,6 +713,7 @@ int ceph_update_snap_trace(struct ceph_mds_client *mdsc, dout("update_snap_trace deletion=%d\n", deletion); more: + invalidate = 0; ceph_decode_need(&p, e, sizeof(*ri), bad); ri = p; p += sizeof(*ri); @@ -774,8 +776,10 @@ int ceph_update_snap_trace(struct ceph_mds_client *mdsc, realm, invalidate, p, e); /* invalidate when we reach the _end_ (root) of the trace */ - if (invalidate && p >= e) - rebuild_snap_realms(realm, &dirty_realms); + if (invalidate) + realm_to_inval = realm; + if (realm_to_inval && p >= e) + rebuild_snap_realms(realm_to_inval, &dirty_realms); if (!first_realm) first_realm = realm; > > > > dout("build_snap_context %llx %p: %p seq %lld (%u snaps), > > > " (unchanged)\n", > > > realm->ino, realm, realm->cached_context, > > > -- > > > 2.27.0 > > > > > -- > Jeff Layton <jlayton@xxxxxxxxxx>
