Re: [PATCH 2/2] fs: remove the inode argument to ->d_real() method

[Stefan Berger <stefanb@linux.ibm.com>]

On 2/2/24 06:01, Amir Goldstein wrote:
> The only remaining user of ->d_real() method is d_real_inode(), which
> passed NULL inode argument to get the real data dentry.
> 
> There are no longer any users that call ->d_real() with a non-NULL
> inode argument for getting a detry from a specific underlying layer.
> 
> Remove the inode argument of the method and replace it with an integer
> 'type' argument, to allow callers to request the real metadata dentry
> instead of the real data dentry.
> 
> All the current users of d_real_inode() (e.g. uprobe) continue to get
> the real data inode.  Caller that need to get the real metadata inode
> (e.g. IMA/EVM) can use d_inode(d_real(dentry, D_REAL_METADATA)).
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Tested-by: Stefan Berger <stefanb@linux.ibm.com>


> ---
>   Documentation/filesystems/locking.rst |  2 +-
>   Documentation/filesystems/vfs.rst     | 16 ++++-----
>   fs/overlayfs/super.c                  | 52 ++++++++++++---------------
>   include/linux/dcache.h                | 18 ++++++----
>   4 files changed, 41 insertions(+), 47 deletions(-)
> 
> diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
> index d5bf4b6b7509..453039a2e49b 100644
> --- a/Documentation/filesystems/locking.rst
> +++ b/Documentation/filesystems/locking.rst
> @@ -29,7 +29,7 @@ prototypes::
>   	char *(*d_dname)((struct dentry *dentry, char *buffer, int buflen);
>   	struct vfsmount *(*d_automount)(struct path *path);
>   	int (*d_manage)(const struct path *, bool);
> -	struct dentry *(*d_real)(struct dentry *, const struct inode *);
> +	struct dentry *(*d_real)(struct dentry *, int type);
>   
>   locking rules:
>   
> diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
> index eebcc0f9e2bc..2a39e718fdf8 100644
> --- a/Documentation/filesystems/vfs.rst
> +++ b/Documentation/filesystems/vfs.rst
> @@ -1264,7 +1264,7 @@ defined:
>   		char *(*d_dname)(struct dentry *, char *, int);
>   		struct vfsmount *(*d_automount)(struct path *);
>   		int (*d_manage)(const struct path *, bool);
> -		struct dentry *(*d_real)(struct dentry *, const struct inode *);
> +		struct dentry *(*d_real)(struct dentry *, int type);
>   	};
>   
>   ``d_revalidate``
> @@ -1419,16 +1419,14 @@ defined:
>   	the dentry being transited from.
>   
>   ``d_real``
> -	overlay/union type filesystems implement this method to return
> -	one of the underlying dentries hidden by the overlay.  It is
> -	used in two different modes:
> +	overlay/union type filesystems implement this method to return one
> +	of the underlying dentries of a regular file hidden by the overlay.
>   
> -	Called from file_dentry() it returns the real dentry matching
> -	the inode argument.  The real dentry may be from a lower layer
> -	already copied up, but still referenced from the file.  This
> -	mode is selected with a non-NULL inode argument.
> +	The 'type' argument takes the values D_REAL_DATA or D_REAL_METADATA
> +	for returning the real underlying dentry that refers to the inode
> +	hosting the file's data or metadata respectively.
>   
> -	With NULL inode the topmost real underlying dentry is returned.
> +	For non-regular files, the 'dentry' argument is returned.
>   
>   Each dentry has a pointer to its parent dentry, as well as a hash list
>   of child dentries.  Child dentries are basically like files in a
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 2eef6c70b2ae..938852a0a92b 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -28,41 +28,38 @@ MODULE_LICENSE("GPL");
>   
>   struct ovl_dir_cache;
>   
> -static struct dentry *ovl_d_real(struct dentry *dentry,
> -				 const struct inode *inode)
> +static struct dentry *ovl_d_real(struct dentry *dentry, int type)
>   {
> -	struct dentry *real = NULL, *lower;
> +	struct dentry *upper, *lower;
>   	int err;
>   
> -	/*
> -	 * vfs is only expected to call d_real() with NULL from d_real_inode()
> -	 * and with overlay inode from file_dentry() on an overlay file.
> -	 *
> -	 * TODO: remove @inode argument from d_real() API, remove code in this
> -	 * function that deals with non-NULL @inode and remove d_real() call
> -	 * from file_dentry().
> -	 */
> -	if (inode && d_inode(dentry) == inode)
> -		return dentry;
> -	else if (inode)
> +	switch (type) {
> +	case D_REAL_DATA:
> +	case D_REAL_METADATA:
> +		break;
> +	default:
>   		goto bug;
> +	}
>   
>   	if (!d_is_reg(dentry)) {
>   		/* d_real_inode() is only relevant for regular files */
>   		return dentry;
>   	}
>   
> -	real = ovl_dentry_upper(dentry);
> -	if (real && (inode == d_inode(real)))
> -		return real;
> +	upper = ovl_dentry_upper(dentry);
> +	if (upper && (type == D_REAL_METADATA ||
> +		      ovl_has_upperdata(d_inode(dentry))))
> +		return upper;
>   
> -	if (real && !inode && ovl_has_upperdata(d_inode(dentry)))
> -		return real;
> +	if (type == D_REAL_METADATA) {
> +		lower = ovl_dentry_lower(dentry);
> +		goto real_lower;
> +	}
>   
>   	/*
> -	 * Best effort lazy lookup of lowerdata for !inode case to return
> +	 * Best effort lazy lookup of lowerdata for D_REAL_DATA case to return
>   	 * the real lowerdata dentry.  The only current caller of d_real() with
> -	 * NULL inode is d_real_inode() from trace_uprobe and this caller is
> +	 * D_REAL_DATA is d_real_inode() from trace_uprobe and this caller is
>   	 * likely going to be followed reading from the file, before placing
>   	 * uprobes on offset within the file, so lowerdata should be available
>   	 * when setting the uprobe.
> @@ -73,18 +70,13 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
>   	lower = ovl_dentry_lowerdata(dentry);
>   	if (!lower)
>   		goto bug;
> -	real = lower;
>   
> -	/* Handle recursion */
> -	real = d_real(real, inode);
> +real_lower:
> +	/* Handle recursion into stacked lower fs */
> +	return d_real(lower, type);
>   
> -	if (!inode || inode == d_inode(real))
> -		return real;
>   bug:
> -	WARN(1, "%s(%pd4, %s:%lu): real dentry (%p/%lu) not found\n",
> -	     __func__, dentry, inode ? inode->i_sb->s_id : "NULL",
> -	     inode ? inode->i_ino : 0, real,
> -	     real && d_inode(real) ? d_inode(real)->i_ino : 0);
> +	WARN(1, "%s(%pd4, %d): real dentry not found\n", __func__, dentry, type);
>   	return dentry;
>   }
>   
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index 1666c387861f..019ad02f2b7e 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -139,7 +139,7 @@ struct dentry_operations {
>   	char *(*d_dname)(struct dentry *, char *, int);
>   	struct vfsmount *(*d_automount)(struct path *);
>   	int (*d_manage)(const struct path *, bool);
> -	struct dentry *(*d_real)(struct dentry *, const struct inode *);
> +	struct dentry *(*d_real)(struct dentry *, int type);
>   } ____cacheline_aligned;
>   
>   /*
> @@ -543,27 +543,31 @@ static inline struct inode *d_backing_inode(const struct dentry *upper)
>   	return inode;
>   }
>   
> +enum d_real_type {
> +	D_REAL_DATA,
> +	D_REAL_METADATA,
> +};
> +
>   /**
>    * d_real - Return the real dentry
>    * @dentry: the dentry to query
> - * @inode: inode to select the dentry from multiple layers (can be NULL)
> + * @type: the type of real dentry (data or metadata)
>    *
>    * If dentry is on a union/overlay, then return the underlying, real dentry.
>    * Otherwise return the dentry itself.
>    *
>    * See also: Documentation/filesystems/vfs.rst
>    */
> -static inline struct dentry *d_real(struct dentry *dentry,
> -				    const struct inode *inode)
> +static inline struct dentry *d_real(struct dentry *dentry, int type)
>   {
>   	if (unlikely(dentry->d_flags & DCACHE_OP_REAL))
> -		return dentry->d_op->d_real(dentry, inode);
> +		return dentry->d_op->d_real(dentry, type);
>   	else
>   		return dentry;
>   }
>   
>   /**
> - * d_real_inode - Return the real inode
> + * d_real_inode - Return the real inode hosting the data
>    * @dentry: The dentry to query
>    *
>    * If dentry is on a union/overlay, then return the underlying, real inode.
> @@ -572,7 +576,7 @@ static inline struct dentry *d_real(struct dentry *dentry,
>   static inline struct inode *d_real_inode(const struct dentry *dentry)
>   {
>   	/* This usage of d_real() results in const dentry */
> -	return d_backing_inode(d_real((struct dentry *) dentry, NULL));
> +	return d_inode(d_real((struct dentry *) dentry, D_REAL_DATA));
>   }
>   
>   struct name_snapshot {