Re: [PATCH] ceph: revalidate dentries under snapdir to drop stale snapshots

[Xiubo Li]

On Mon, Nov 9, 2026 zhangdaolong <dlzhangak@xxxxxxxxxxxxxx> wrote:
> Validating the dentry against the head directory's snap realm
> cached_context (when holding caps on the head directory) before falling
> back to LOOKUPSNAP makes total sense and avoids the unnecessary MDS round
> trips on cache hits. Could you please share the diff you tested? I would
> be glad to incorporate it and test it for v2.

Sure, below is the diff I tested. It already contains your ceph_d_revalidate()
change and the comment update, so it can replace that part of your patch in
v2. It leaves out the ceph_unlink() hunk.

I tested it with two kernel clients on one host, against a vstart
cluster, on a KASAN + lockdep kernel. It covered recreating a removed
snapshot, stale data after a same-name snapshot was recreated, a removed
ancestor snapshot (_p1_<ino>), 50 rounds of "B rmdir, A mkdir right
away", and a removed snapshot while A has its cwd inside it after an MDS
"cache drop". All of them behave the same as with your patch. Cached
lookups send no LOOKUPSNAP (0 vs 3000 for 3000 stats). There were no
KASAN reports. Lockdep stayed clean when I ran the same diff on top of
our development branch; on plain master, an unrelated VFS lockdep report
turned lockdep off before these tests ran.

> Agreed. I will drop this hunk from v2 and send it as a separate patch.

> In v2, I will update the comment and point Fixes: to 2817b000b02c
> (while also mentioning 200fd27c8fa2 in the commit message).

Sounds good, thanks.

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 6eb5b1cb039b..21983e14cb67 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1979,6 +1979,58 @@ static int dir_lease_is_valid(struct inode
*dir, struct dentry *dentry,
        return valid;
 }

+/*
+ * Check whether a cached dentry under .snap still names an existing snapshot.
+ *
+ * The MDS issues no lease for these dentries, but it sends a snap update to
+ * every client holding caps in a realm when a snapshot in it is created or
+ * removed.  So as long as we hold caps on the snapped directory, its realm's
+ * snap context tells us whether the snapshot still exists.  Without caps we
+ * cannot tell, and the caller has to ask the MDS.
+ */
+static int snapdir_dentry_is_valid(struct inode *dir, struct dentry *dentry,
+                                  struct ceph_mds_client *mdsc)
+{
+       struct inode *inode = d_inode(dentry);
+       struct ceph_vino vino = {
+               .ino = ceph_ino(dir),
+               .snap = CEPH_NOSNAP,
+       };
+       struct ceph_snap_context *snapc = NULL;
+       struct ceph_inode_info *pci;
+       struct inode *parent;
+       u32 i;
+       int valid = 0;
+
+       if (!inode || ceph_snap(inode) > CEPH_MAXSNAP)
+               return 0;
+
+       parent = ceph_find_inode(dir->i_sb, vino);
+       if (!parent)
+               return 0;
+       pci = ceph_inode(parent);
+
+       down_read(&mdsc->snap_rwsem);
+       spin_lock(&pci->i_ceph_lock);
+       if (__ceph_is_any_real_caps(pci) && pci->i_snap_realm &&
+           pci->i_snap_realm->cached_context)
+               snapc =
ceph_get_snap_context(pci->i_snap_realm->cached_context);
+       spin_unlock(&pci->i_ceph_lock);
+       up_read(&mdsc->snap_rwsem);
+       iput(parent);
+
+       if (!snapc)
+               return 0;
+       for (i = 0; i < snapc->num_snaps; i++) {
+               if (snapc->snaps[i] == ceph_snap(inode)) {
+                       valid = 1;
+                       break;
+               }
+       }
+       ceph_put_snap_context(snapc);
+       return valid;
+}
+
 /*
  * Check if cached dentry can be trusted.
  */
@@ -2002,12 +2054,20 @@ static int ceph_d_revalidate(struct inode
*dir, const struct qstr *name,

        mdsc = ceph_sb_to_fs_client(dir->i_sb)->mdsc;

-       /* always trust cached snapped dentries, snapdir dentry */
-       if (ceph_in_snap(dir)) {
+       if (ceph_snap(dir) == CEPH_SNAPDIR) {
+               /* a snapshot under .snap may have been removed by others */
+               if (flags & LOOKUP_RCU)
+                       return -ECHILD;
+               valid = snapdir_dentry_is_valid(dir, dentry, mdsc);
+               doutc(cl, "%p '%pd' inode %p under snapdir, valid %d\n",
+                     dentry, dentry, inode, valid);
+       } else if (ceph_in_snap(dir)) {
+               /* snapshots are immutable: trust dentries inside one */
                doutc(cl, "%p '%pd' inode %p is SNAPPED\n", dentry,
                      dentry, inode);
                valid = 1;
        } else if (inode && ceph_snap(inode) == CEPH_SNAPDIR) {
+               /* the .snap dentry itself */
                valid = 1;
        } else {
                valid = dentry_lease_is_valid(dentry, flags);

Thanks,
- Xiubo

zhangdaolong <dlzhangak@xxxxxxxxxxxxxx> 于2026年11月9日周一 02:39写道：
>
> On Sat, Nov 7, 2026 at 12:17 PM Xiubo Li <xiubli@xxxxxxxxxx> wrote:
> >
> > Hi Daolong,
> >
> > Thanks for the patch. I can reproduce this with two kernel clients, and
> > it is a bit worse than described: after B removes .snap/s3 and creates a
> > new .snap/s3, A still reads the file contents of the old snapshot. A
> > removed ancestor snapshot also stays visible as .snap/_p1_<ino> in child
> > directories. Your patch fixes all of these cases.
> >
> > [...]
> >
> > >         /* always trust cached snapped dentries, snapdir dentry */
> > > -       if (ceph_in_snap(dir)) {
> > > +       if (ceph_in_snap(dir) && ceph_snap(dir) != CEPH_SNAPDIR) {
> >
> > With this change nothing can validate a dentry under .snap anymore. The
> > MDS issues no lease for it, the snapdir only has CEPH_CAP_PIN, and a
> > snapped inode has no real caps. So every path walk through .snap/<name>
> > does a synchronous LOOKUPSNAP. With 3000 stats of .snap/s3/f on a cached
> > snapshot (in a VM, so only the order of magnitude matters):
> >
> >                  ops/s   LOOKUPSNAP   p50/p99 (us)
> >   unpatched     257834        0         3.1/6.9
> >   this patch     10065     3000        89.9/273.1
> >   realm check   234741        0         3.4/7.5
> >
> > The "realm check" row is an alternative that avoids the round trip. The
> > client already knows which snapshots exist: the directory's snap realm
> > keeps the list in cached_context, and the MDS sends a snap update to
> > every client with caps in the realm when a snapshot is created or
> > removed. So d_revalidate can keep trusting a dentry under .snap while
> > its snapid (ceph_snap(inode)) is still in the realm's snap context of
> > the head directory, and only fall back to LOOKUPSNAP when it is not, or
> > when we hold no caps on that directory.
> >
> > I tried this on 7.2.9. It fixes the same cases as your patch: recreating
> > a snapshot, reading stale data, removed ancestor snapshots, and 50/50
> > "B rmdir, A mkdir right away" rounds. It sends no LOOKUPSNAP on cache
> > hits. I can send you the diff I tested if it helps.
>
> Hi Xiubo,
>
> Thank you very much for reproducing the issue, identifying the additional
> stale read and removed ancestor snapshot cases, and benchmarking the
> LOOKUPSNAP overhead.
>
> Validating the dentry against the head directory's snap realm
> cached_context (when holding caps on the head directory) before falling
> back to LOOKUPSNAP makes total sense and avoids the unnecessary MDS round
> trips on cache hits. Could you please share the diff you tested? I would
> be glad to incorporate it and test it for v2.
>
> > >                 if (!err && !req->r_reply_info.head->is_dentry)
> > >                         d_delete(dentry);
> > > +               else if (err == -ENOENT)
> > > +                       d_drop(dentry);
> >
> > Please make this a separate patch. With the revalidate fix, rmdir of a
> > stale snapshot already fails in the lookup and never sends RMSNAP. I
> > checked the MDS counter with and without this hunk. Also, this hunk
> > affects unlink() of regular files too.
>
> Agreed. I will drop this hunk from v2 and send it as a separate patch.
>
> > Nits: the "always trust cached snapped dentries, snapdir dentry" comment
> > needs an update. Also, the trust-all check dates back to 2817b000b02c,
> > while 200fd27c8fa2 only added the unreachable LOOKUPSNAP branch, so
> > please explain the choice of Fixes: tag.
>
> I originally chose 200fd27c8fa2 ("ceph: use lookup request to revalidate
> dentry") because it explicitly added the CEPH_MDS_OP_LOOKUPSNAP branch in
> ceph_d_revalidate() intending to revalidate dentries under CEPH_SNAPDIR,
> but overlooked that the earlier check from 2817b000b02c ("ceph: directory
> operations") made it unreachable. In v2, I will update the comment and
> point Fixes: to 2817b000b02c (while also mentioning 200fd27c8fa2 in the
> commit message).
>
> Thanks,
> Daolong