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? > > 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>
