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

[zhangdaolong]

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