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

[Xiubo Li]

On Fri, Nov 6, 2026 zhangdaolong <dlzhangak@xxxxxxxxxxxxxx> wrote:
> 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, recreating a snapshot with the same
> name on another node fails with -EEXIST, and removing it fails with
> -ENOENT while still leaving the stale positive dentry in the dcache.

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.

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

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.

Thanks,
- Xiubo

zhangdaolong <dlzhangak@xxxxxxxxxxxxxx> 于2026年11月6日周五 03:13写道：
>
> 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, recreating a snapshot with the same
> name on another node fails with -EEXIST, and removing it fails with
> -ENOENT while still leaving the stale positive dentry in the dcache.
>
> In ceph_d_revalidate(), the condition 'if (ceph_in_snap(dir))' matches
> both immutable snapped directories (ceph_snap(dir) <= CEPH_MAXSNAP) and
> the mutable virtual '.snap' directory itself (ceph_snap(dir) ==
> CEPH_SNAPDIR). Because it unconditionally sets valid = 1 when dir is
> CEPH_SNAPDIR, the CEPH_MDS_OP_LOOKUPSNAP revalidation path below inside
> 'if (!valid)' is rendered unreachable dead code.
>
> Fix this by excluding CEPH_SNAPDIR from the unconditional trust check in
> ceph_d_revalidate() so that cached dentries directly under '.snap' are
> revalidated via CEPH_MDS_OP_LOOKUPSNAP. Additionally, drop the dentry in
> ceph_unlink() when the MDS returns -ENOENT.
>
> Fixes: 200fd27c8fa2 ("ceph: use lookup request to revalidate dentry")
> Link: https://tracker.ceph.com/issues/81579
> Signed-off-by: zhangdaolong <dlzhangak@xxxxxxxxxxxxxx>
> ---
>  fs/ceph/dir.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 6eb5b1cb039b..9f84beac189e 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -1475,6 +1475,8 @@ static int ceph_unlink(struct inode *dir, struct dentry *dentry)
>                 err = ceph_mdsc_do_request(mdsc, dir, req);
>                 if (!err && !req->r_reply_info.head->is_dentry)
>                         d_delete(dentry);
> +               else if (err == -ENOENT)
> +                       d_drop(dentry);
>         }
>
>         ceph_mdsc_put_request(req);
> @@ -2003,7 +2005,7 @@ 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_in_snap(dir) && ceph_snap(dir) != CEPH_SNAPDIR) {
>                 doutc(cl, "%p '%pd' inode %p is SNAPPED\n", dentry,
>                       dentry, inode);
>                 valid = 1;
> --
> 2.48.1
>
>