ceph: do not cache negative dentries for snapped directories

[PATCH] ceph: do not cache negative dentries for snapped directories

Posted by Xiubo Li via B4 Relay 22 hours ago

From: Xiubo Li <xiubo.li@clyso.com>

When a LOOKUP/LOOKUPSNAP in a snapped directory returns ENOENT
without a trace, ceph_finish_lookup() creates a negative dentry
via d_add(dentry, NULL).  For live directories this is fine — the
dentry naturally expires.  But for snapped directories,
ceph_d_revalidate() unconditionally trusts all cached dentries
(valid = 1), so a negative dentry created by a transient error
persists forever, hiding entries that genuinely exist in the
snapshot.

Only cache negative dentries for live (non-snapshotted) parent
directories.  For snapped parents, skip the negative dentry so
that VFS retries the lookup on the next access.  Since the
conditions that trigger a negative dentry (MDS transient error,
local ENOENT shortcut, or MDS null dentry lease) are all rare in
snapped directories, the performance impact of this change is
negligible.

Reported-by: Andras Pataki <apataki@flatironinstitute.org>
Closes: https://tracker.ceph.com/issues/78529
Signed-off-by: Xiubo Li <xiubo.li@clyso.com>
---
 fs/ceph/dir.c   | 10 ++++++++--
 fs/ceph/inode.c |  3 ++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index b4b541a1180c..d5d8f935fb62 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -774,8 +774,13 @@ struct dentry *ceph_finish_lookup(struct ceph_mds_request *req,
 				d_drop(dentry);
 				err = -ENOENT;
 			} else {
-				if (d_unhashed(dentry))
-					d_add(dentry, NULL);
+				if (d_unhashed(dentry)) {
+					struct inode *parent =
+						d_inode(dentry->d_parent);
+					if (!parent ||
+					    ceph_snap(parent) == CEPH_NOSNAP)
+						d_add(dentry, NULL);
+				}
 			}
 		}
 	}
@@ -840,6 +845,7 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
 			    dentry->d_name.len) &&
 		    !is_root_ceph_dentry(dir, dentry) &&
 		    ceph_test_mount_opt(fsc, DCACHE) &&
+		    ceph_snap(dir) == CEPH_NOSNAP &&
 		    __ceph_dir_is_complete(ci) &&
 		    __ceph_caps_issued_mask_metric(ci, CEPH_CAP_FILE_SHARED, 1)) {
 			__ceph_touch_fmode(ci, mdsc, CEPH_FILE_MODE_RD);
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 61d7c0b8161f..d52e2b389e0b 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1814,7 +1814,8 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
 				ceph_dir_clear_ordered(dir);
 				d_delete(dn);
 			} else if (have_lease) {
-				if (d_unhashed(dn))
+				if (d_unhashed(dn) &&
+				    ceph_snap(dir) == CEPH_NOSNAP)
 					d_add(dn, NULL);
 			}
 

---
base-commit: 602bb4525c8146865b7f6e44c81d54bf97861aed
change-id: 20260722-b4-snap-negative-dentry-bbf74719a151

Best regards,
--  
Xiubo Li <xiubo.li@clyso.com>

Re: [PATCH] ceph: do not cache negative dentries for snapped directories

Posted by Viacheslav Dubeyko 12 hours ago

On Thu, 2026-07-23 at 14:28 +0800, Xiubo Li via B4 Relay wrote:
> From: Xiubo Li <xiubo.li@clyso.com>
> 
> When a LOOKUP/LOOKUPSNAP in a snapped directory returns ENOENT
> without a trace, ceph_finish_lookup() creates a negative dentry
> via d_add(dentry, NULL).  For live directories this is fine — the
> dentry naturally expires.  But for snapped directories,
> ceph_d_revalidate() unconditionally trusts all cached dentries
> (valid = 1), so a negative dentry created by a transient error
> persists forever, hiding entries that genuinely exist in the
> snapshot.
> 
> Only cache negative dentries for live (non-snapshotted) parent
> directories.  For snapped parents, skip the negative dentry so
> that VFS retries the lookup on the next access.  Since the
> conditions that trigger a negative dentry (MDS transient error,
> local ENOENT shortcut, or MDS null dentry lease) are all rare in
> snapped directories, the performance impact of this change is
> negligible.
> 
> Reported-by: Andras Pataki <apataki@flatironinstitute.org>
> Closes: https://tracker.ceph.com/issues/78529
> Signed-off-by: Xiubo Li <xiubo.li@clyso.com>
> ---
>  fs/ceph/dir.c   | 10 ++++++++--
>  fs/ceph/inode.c |  3 ++-
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index b4b541a1180c..d5d8f935fb62 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -774,8 +774,13 @@ struct dentry *ceph_finish_lookup(struct
> ceph_mds_request *req,
>  				d_drop(dentry);
>  				err = -ENOENT;
>  			} else {
> -				if (d_unhashed(dentry))
> -					d_add(dentry, NULL);
> +				if (d_unhashed(dentry)) {
> +					struct inode *parent =
> +						d_inode(dentry-
> >d_parent);
> +					if (!parent ||
> +					    ceph_snap(parent) ==
> CEPH_NOSNAP)
> +						d_add(dentry, NULL);
> +				}
>  			}
>  		}
>  	}
> @@ -840,6 +845,7 @@ static struct dentry *ceph_lookup(struct inode
> *dir, struct dentry *dentry,
>  			    dentry->d_name.len) &&
>  		    !is_root_ceph_dentry(dir, dentry) &&
>  		    ceph_test_mount_opt(fsc, DCACHE) &&
> +		    ceph_snap(dir) == CEPH_NOSNAP &&
>  		    __ceph_dir_is_complete(ci) &&
>  		    __ceph_caps_issued_mask_metric(ci,
> CEPH_CAP_FILE_SHARED, 1)) {
>  			__ceph_touch_fmode(ci, mdsc,
> CEPH_FILE_MODE_RD);
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 61d7c0b8161f..d52e2b389e0b 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -1814,7 +1814,8 @@ int ceph_fill_trace(struct super_block *sb,
> struct ceph_mds_request *req)
>  				ceph_dir_clear_ordered(dir);
>  				d_delete(dn);
>  			} else if (have_lease) {
> -				if (d_unhashed(dn))
> +				if (d_unhashed(dn) &&
> +				    ceph_snap(dir) == CEPH_NOSNAP)
>  					d_add(dn, NULL);
>  			}
>  
> 
> ---
> base-commit: 602bb4525c8146865b7f6e44c81d54bf97861aed
> change-id: 20260722-b4-snap-negative-dentry-bbf74719a151
> 
> Best regards,
> --  
> Xiubo Li <xiubo.li@clyso.com>
> 
> 

Makes sense.

Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Slava.