Re: [PATCH v2] dcache: remove unnecessary NULL check in dget_dlock()

[Vegard Nossum]

[Fixed up a couple of bad addresses in Cc]

Hi,

I didn't get a response to this v2 of the patch below and I don't see it
in vfs.git.

Was there something wrong or is it just awaiting review? Is there
anything I can do or help with? I would be happy to try to review other
patches if there is anything outstanding.

Thanks,


Vegard

On 06/11/2023 14:44, Vegard Nossum wrote:
> dget_dlock() requires dentry->d_lock to be held when called, yet
> contains a NULL check for dentry.
>
> An audit of all calls to dget_dlock() shows that it is never called
> with a NULL pointer (as spin_lock()/spin_unlock() would crash in these
> cases):
>
>    $ git grep -W '\<dget_dlock\>'
>
>    arch/powerpc/platforms/cell/spufs/inode.c-              spin_lock(&dentry->d_lock);
>    arch/powerpc/platforms/cell/spufs/inode.c-              if (simple_positive(dentry)) {
>    arch/powerpc/platforms/cell/spufs/inode.c:                      dget_dlock(dentry);
>
>    fs/autofs/expire.c-             spin_lock_nested(&child->d_lock, DENTRY_D_LOCK_NESTED);
>    fs/autofs/expire.c-             if (simple_positive(child)) {
>    fs/autofs/expire.c:                     dget_dlock(child);
>
>    fs/autofs/root.c:                       dget_dlock(active);
>    fs/autofs/root.c-                       spin_unlock(&active->d_lock);
>
>    fs/autofs/root.c:                       dget_dlock(expiring);
>    fs/autofs/root.c-                       spin_unlock(&expiring->d_lock);
>
>    fs/ceph/dir.c-          if (!spin_trylock(&dentry->d_lock))
>    fs/ceph/dir.c-                  continue;
>    [...]
>    fs/ceph/dir.c:                          dget_dlock(dentry);
>
>    fs/ceph/mds_client.c-           spin_lock(&alias->d_lock);
>    [...]
>    fs/ceph/mds_client.c:                   dn = dget_dlock(alias);
>
>    fs/configfs/inode.c-            spin_lock(&dentry->d_lock);
>    fs/configfs/inode.c-            if (simple_positive(dentry)) {
>    fs/configfs/inode.c:                    dget_dlock(dentry);
>
>    fs/libfs.c:                             found = dget_dlock(d);
>    fs/libfs.c-                     spin_unlock(&d->d_lock);
>
>    fs/libfs.c:             found = dget_dlock(child);
>    fs/libfs.c-     spin_unlock(&child->d_lock);
>
>    fs/libfs.c:                             child = dget_dlock(d);
>    fs/libfs.c-                     spin_unlock(&d->d_lock);
>
>    fs/ocfs2/dcache.c:                      dget_dlock(dentry);
>    fs/ocfs2/dcache.c-                      spin_unlock(&dentry->d_lock);
>
>    include/linux/dcache.h:static inline struct dentry *dget_dlock(struct dentry *dentry)
>
> After taking out the NULL check, dget_dlock() becomes almost identical
> to __dget_dlock(); the only difference is that dget_dlock() returns the
> dentry that was passed in. These are static inline helpers, so we can
> rely on the compiler to discard unused return values. We can therefore
> also remove __dget_dlock() and replace calls to it by dget_dlock().
>
> Also fix up and improve the kerneldoc comments while we're at it.
>
> Al Viro pointed out that we can also clean up some of the callers to
> make use of the returned value and provided a bit more info for the
> kerneldoc.
>
> While preparing v2 I also noticed that the tabs used in the kerneldoc
> comments were causing the kerneldoc to get parsed incorrectly so I also
> fixed this up (including for d_unhashed, which is otherwise unrelated).
>
> Testing: x86 defconfig build + boot; make htmldocs for the kerneldoc
> warning. objdump shows there are code generation changes.
>
> Link: https://lore.kernel.org/all/20231022164520.915013-1-vegard.nossum@xxxxxxxxxx/
> Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: Christian Brauner <brauner@xxxxxxxxxx>
> Cc: linux-fsdevel@xxxxxxxxxxxxxxx
> Cc: Nick Piggin <npiggin@xxxxxxxxx>
> Cc: Waiman Long <Waiman.Long@xxxxxx>
> Cc: linux-doc@xxxxxxxxxxxxxxx
> Signed-off-by: Vegard Nossum <vegard.nossum@xxxxxxxxxx>
> ---
>   fs/dcache.c            | 16 ++++------------
>   include/linux/dcache.h | 29 +++++++++++++++++++----------
>   2 files changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index c82ae731df9a..4bf33ba588d8 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -942,12 +942,6 @@ void dput_to_list(struct dentry *dentry, struct list_head *list)
>   	spin_unlock(&dentry->d_lock);
>   }
>   
> -/* This must be called with d_lock held */
> -static inline void __dget_dlock(struct dentry *dentry)
> -{
> -	dentry->d_lockref.count++;
> -}
> -
>   static inline void __dget(struct dentry *dentry)
>   {
>   	lockref_get(&dentry->d_lockref);
> @@ -1034,7 +1028,7 @@ static struct dentry *__d_find_alias(struct inode *inode)
>   	hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) {
>   		spin_lock(&alias->d_lock);
>    		if (!d_unhashed(alias)) {
> -			__dget_dlock(alias);
> +			dget_dlock(alias);
>   			spin_unlock(&alias->d_lock);
>   			return alias;
>   		}
> @@ -1707,8 +1701,7 @@ static enum d_walk_ret find_submount(void *_data, struct dentry *dentry)
>   {
>   	struct dentry **victim = _data;
>   	if (d_mountpoint(dentry)) {
> -		__dget_dlock(dentry);
> -		*victim = dentry;
> +		*victim = dget_dlock(dentry);
>   		return D_WALK_QUIT;
>   	}
>   	return D_WALK_CONTINUE;
> @@ -1853,8 +1846,7 @@ struct dentry *d_alloc(struct dentry * parent, const struct qstr *name)
>   	 * don't need child lock because it is not subject
>   	 * to concurrency here
>   	 */
> -	__dget_dlock(parent);
> -	dentry->d_parent = parent;
> +	dentry->d_parent = dget_dlock(parent);
>   	list_add(&dentry->d_child, &parent->d_subdirs);
>   	spin_unlock(&parent->d_lock);
>   
> @@ -2851,7 +2843,7 @@ struct dentry *d_exact_alias(struct dentry *entry, struct inode *inode)
>   			spin_unlock(&alias->d_lock);
>   			alias = NULL;
>   		} else {
> -			__dget_dlock(alias);
> +			dget_dlock(alias);
>   			__d_rehash(alias);
>   			spin_unlock(&alias->d_lock);
>   		}
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index 3da2f0545d5d..82127cf10992 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -301,20 +301,29 @@ extern char *dentry_path(const struct dentry *, char *, int);
>   /* Allocation counts.. */
>   
>   /**
> - *	dget, dget_dlock -	get a reference to a dentry
> - *	@dentry: dentry to get a reference to
> + * dget_dlock - get a reference to a dentry
> + * @dentry: dentry to get a reference to
>    *
> - *	Given a dentry or %NULL pointer increment the reference count
> - *	if appropriate and return the dentry. A dentry will not be
> - *	destroyed when it has references.
> + * Given a live dentry, increment the reference count and return
> + * the dentry.  For a dentry to be live, it can be hashed, positive,
> + * or have a non-negative &dentry->d_lockref.count
> + *
> + * Context: @dentry->d_lock must be held.
>    */
>   static inline struct dentry *dget_dlock(struct dentry *dentry)
>   {
> -	if (dentry)
> -		dentry->d_lockref.count++;
> +	dentry->d_lockref.count++;
>   	return dentry;
>   }
>   
> +/**
> + * dget - get a reference to a dentry
> + * @dentry: dentry to get a reference to
> + *
> + * Given a dentry or %NULL pointer increment the reference count
> + * if appropriate and return the dentry.  A dentry will not be
> + * destroyed when it has references.
> + */
>   static inline struct dentry *dget(struct dentry *dentry)
>   {
>   	if (dentry)
> @@ -325,10 +334,10 @@ static inline struct dentry *dget(struct dentry *dentry)
>   extern struct dentry *dget_parent(struct dentry *dentry);
>   
>   /**
> - *	d_unhashed -	is dentry hashed
> - *	@dentry: entry to check
> + * d_unhashed - is dentry hashed
> + * @dentry: entry to check
>    *
> - *	Returns true if the dentry passed is not currently hashed.
> + * Returns true if the dentry passed is not currently hashed.
>    */
>    
>   static inline int d_unhashed(const struct dentry *dentry)