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

[Xiubo Li]

On Mon, Nov 9, 2026 zhangdaolong <dlzhangak@xxxxxxxxxxxxxx> wrote:
> - Note: Xiubo, please feel free to change Suggested-by to Co-developed-by
>   (with your Signed-off-by) if you prefer.

Hi Daolong,

Thanks for v2. The code is the same as the diff I tested.

Reviewed-by: Xiubo Li <xiubo.li@xxxxxxxxx>

Thanks,
- Xiubo

zhangdaolong <dlzhangak@xxxxxxxxxxxxxx> 于2026年11月9日周一 08:30写道：
>
> When a snapshot is deleted on one client node, its corresponding
> '.snap/<snapname>' dentry cached in the VFS dcache on other client nodes
> never gets invalidated. As a result, on another client node:
> - recreating a snapshot with the same name fails with -EEXIST;
> - removing the already deleted snapshot fails with -ENOENT while leaving
>   the stale positive dentry in the dcache;
> - if the deleted snapshot is recreated with the same name on the remote
>   node, the local client still reads the file contents of the old
>   snapshot;
> - a removed ancestor snapshot stays visible as '.snap/_<name>_<ino>' in
>   child directories.
>
> Since commit 2817b000b02c ("ceph: directory operations"),
> ceph_d_revalidate() has unconditionally trusted all dentries whose
> parent directory has ceph_snap(dir) != CEPH_NOSNAP (later written as
> ceph_in_snap(dir)). Because that condition matches both immutable
> snapped directories (ceph_snap(dir) <= CEPH_MAXSNAP) and the mutable
> virtual '.snap' directory itself (ceph_snap(dir) == CEPH_SNAPDIR), it
> unconditionally sets valid = 1 when dir is CEPH_SNAPDIR, rendering the
> CEPH_MDS_OP_LOOKUPSNAP revalidation path added in commit 200fd27c8fa2
> ("ceph: use lookup request to revalidate dentry") unreachable.
>
> However, simply falling back to CEPH_MDS_OP_LOOKUPSNAP for every dentry
> under CEPH_SNAPDIR would cause every path walk through '.snap/<name>' to
> send a synchronous LOOKUPSNAP request to the MDS, because the MDS issues
> no lease for dentries under '.snap', the snapdir only has CEPH_CAP_PIN,
> and a snapped inode has no real caps.
>
> Instead, introduce snapdir_dentry_is_valid() to validate cached dentries
> under CEPH_SNAPDIR against the snapped head directory's snap realm
> cached_context. Because the MDS sends a snap update to every client
> holding caps in a realm whenever a snapshot in it is created or removed,
> we can keep trusting a cached dentry under '.snap' as long as we hold
> real caps on the head directory and the snapshot's snapid
> (ceph_snap(inode)) is still present in its realm's cached_context, and
> only fall back to CEPH_MDS_OP_LOOKUPSNAP when it is not or when no caps
> are held on that directory.
>
> Fixes: 2817b000b02c ("ceph: directory operations")
> Link: https://tracker.ceph.com/issues/81579
> Suggested-by: Xiubo Li <xiubli@xxxxxxxxxx>
> Signed-off-by: zhangdaolong <dlzhangak@xxxxxxxxxxxxxx>
> ---
> Changes in v2:
> - Add snapdir_dentry_is_valid() to validate dentries under .snap against
>   the head directory's snap realm cached_context when holding caps,
>   avoiding unnecessary LOOKUPSNAP round trips on cache hits (Xiubo Li).
> - Drop the ceph_unlink() -ENOENT hunk (to be sent separately).
> - Update comments in ceph_d_revalidate() and point Fixes: to 2817b000b02c.
> - Note: Xiubo, please feel free to change Suggested-by to Co-developed-by
>   (with your Signed-off-by) if you prefer.
>
>  fs/ceph/dir.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 62 insertions(+), 2 deletions(-)
>
> 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);
> --
> 2.48.1
>