Re: [PATCH 09/13] cachefiles: change cachefiles_bury_object to use start_renaming_dentry()

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> Rather then using lock_rename() and lookup_one() etc we can use
> the new start_renaming_dentry().  This is part of centralising dir
> locking and lookup so that locking rules can be changed.
> 
> Some error check are removed as not necessary.  Checks for rep being a
> non-dir or IS_DEADDIR and the check that ->graveyard is still a
> directory only provide slightly more informative errors and have been
> dropped.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/cachefiles/namei.c | 76 ++++++++-----------------------------------
>  1 file changed, 14 insertions(+), 62 deletions(-)
> 
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index e5ec90dccc27..3af42ec78411 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -270,7 +270,8 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
>  			   struct dentry *rep,
>  			   enum fscache_why_object_killed why)
>  {
> -	struct dentry *grave, *trap;
> +	struct dentry *grave;
> +	struct renamedata rd = {};
>  	struct path path, path_to_graveyard;
>  	char nbuffer[8 + 8 + 1];
>  	int ret;
> @@ -302,77 +303,36 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
>  		(uint32_t) ktime_get_real_seconds(),
>  		(uint32_t) atomic_inc_return(&cache->gravecounter));
>  
> -	/* do the multiway lock magic */
> -	trap = lock_rename(cache->graveyard, dir);
> -	if (IS_ERR(trap))
> -		return PTR_ERR(trap);
> -
> -	/* do some checks before getting the grave dentry */
> -	if (rep->d_parent != dir || IS_DEADDIR(d_inode(rep))) {
> -		/* the entry was probably culled when we dropped the parent dir
> -		 * lock */
> -		unlock_rename(cache->graveyard, dir);
> -		_leave(" = 0 [culled?]");
> -		return 0;
> -	}
> -
> -	if (!d_can_lookup(cache->graveyard)) {
> -		unlock_rename(cache->graveyard, dir);
> -		cachefiles_io_error(cache, "Graveyard no longer a directory");
> -		return -EIO;
> -	}
> -
> -	if (trap == rep) {
> -		unlock_rename(cache->graveyard, dir);
> -		cachefiles_io_error(cache, "May not make directory loop");
> +	rd.mnt_idmap = &nop_mnt_idmap;
> +	rd.old_parent = dir;
> +	rd.new_parent = cache->graveyard;
> +	rd.flags = 0;
> +	ret = start_renaming_dentry(&rd, 0, rep, &QSTR(nbuffer));
> +	if (ret) {
> +		cachefiles_io_error(cache, "Cannot lock/lookup in graveyard");
>  		return -EIO;
>  	}
>  
>  	if (d_mountpoint(rep)) {
> -		unlock_rename(cache->graveyard, dir);
> +		end_renaming(&rd);
>  		cachefiles_io_error(cache, "Mountpoint in cache");
>  		return -EIO;
>  	}
>  
> -	grave = lookup_one(&nop_mnt_idmap, &QSTR(nbuffer), cache->graveyard);
> -	if (IS_ERR(grave)) {
> -		unlock_rename(cache->graveyard, dir);
> -		trace_cachefiles_vfs_error(object, d_inode(cache->graveyard),
> -					   PTR_ERR(grave),
> -					   cachefiles_trace_lookup_error);
> -
> -		if (PTR_ERR(grave) == -ENOMEM) {
> -			_leave(" = -ENOMEM");
> -			return -ENOMEM;
> -		}
> -
> -		cachefiles_io_error(cache, "Lookup error %ld", PTR_ERR(grave));
> -		return -EIO;
> -	}
> -
> +	grave = rd.new_dentry;
>  	if (d_is_positive(grave)) {
> -		unlock_rename(cache->graveyard, dir);
> -		dput(grave);
> +		end_renaming(&rd);
>  		grave = NULL;
>  		cond_resched();
>  		goto try_again;
>  	}
>  
>  	if (d_mountpoint(grave)) {
> -		unlock_rename(cache->graveyard, dir);
> -		dput(grave);
> +		end_renaming(&rd);
>  		cachefiles_io_error(cache, "Mountpoint in graveyard");
>  		return -EIO;
>  	}
>  
> -	/* target should not be an ancestor of source */
> -	if (trap == grave) {
> -		unlock_rename(cache->graveyard, dir);
> -		dput(grave);
> -		cachefiles_io_error(cache, "May not make directory loop");
> -		return -EIO;
> -	}
> -
>  	/* attempt the rename */
>  	path.mnt = cache->mnt;
>  	path.dentry = dir;
> @@ -382,13 +342,6 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
>  	if (ret < 0) {
>  		cachefiles_io_error(cache, "Rename security error %d", ret);
>  	} else {
> -		struct renamedata rd = {
> -			.mnt_idmap	= &nop_mnt_idmap,
> -			.old_parent	= dir,
> -			.old_dentry	= rep,
> -			.new_parent	= cache->graveyard,
> -			.new_dentry	= grave,
> -		};
>  		trace_cachefiles_rename(object, d_inode(rep)->i_ino, why);
>  		ret = cachefiles_inject_read_error();
>  		if (ret == 0)
> @@ -402,8 +355,7 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
>  	}
>  
>  	__cachefiles_unmark_inode_in_use(object, d_inode(rep));
> -	unlock_rename(cache->graveyard, dir);
> -	dput(grave);
> +	end_renaming(&rd);
>  	_leave(" = 0");
>  	return 0;
>  }

Reviewed-by: Jeff Layton <jlayton@kernel.org>