diff mbox series

[02/22] switch nfsd_client_rmdir() to use of simple_recursive_removal()

Message ID 20231109062056.3181775-2-viro@zeniv.linux.org.uk (mailing list archive)
State New, archived
Headers show
Series [01/22] struct dentry: get rid of randomize_layout idiocy | expand

Commit Message

Al Viro Nov. 9, 2023, 6:20 a.m. UTC 
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Tested-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/nfsd/nfsctl.c | 70 ++++++++++--------------------------------------
 1 file changed, 14 insertions(+), 56 deletions(-)

Comments

Christian Brauner Nov. 9, 2023, 1:42 p.m. UTC | #1 
On Thu, Nov 09, 2023 at 06:20:36AM +0000, Al Viro wrote:
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Tested-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Acked-by: Christian Brauner <brauner@kernel.org>
Chuck Lever III Nov. 9, 2023, 2:01 p.m. UTC | #2 
On Thu, Nov 09, 2023 at 06:20:36AM +0000, Al Viro wrote:
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Tested-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Future me is going to be mightily confused by the lack of a patch
description. I went back to the series cover letter and found some
text that would be nice to include here:

> 02/22) nfsd_client_rmdir() and its gut open-code simple_recursive_removal();
> converting to calling that cleans the things up in there *and* reduces
> the amount of places where we touch the list of children, which simplifies
> the work later in the series.


> ---
>  fs/nfsd/nfsctl.c | 70 ++++++++++--------------------------------------
>  1 file changed, 14 insertions(+), 56 deletions(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 7ed02fb88a36..035b42c1a181 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1235,63 +1235,34 @@ static inline void _nfsd_symlink(struct dentry *parent, const char *name,
>  
>  #endif
>  
> -static void clear_ncl(struct inode *inode)
> +static void clear_ncl(struct dentry *dentry)
>  {
> +	struct inode *inode = d_inode(dentry);
>  	struct nfsdfs_client *ncl = inode->i_private;
>  
> +	spin_lock(&inode->i_lock);
>  	inode->i_private = NULL;
> +	spin_unlock(&inode->i_lock);
>  	kref_put(&ncl->cl_ref, ncl->cl_release);
>  }
>  
> -static struct nfsdfs_client *__get_nfsdfs_client(struct inode *inode)
> -{
> -	struct nfsdfs_client *nc = inode->i_private;
> -
> -	if (nc)
> -		kref_get(&nc->cl_ref);
> -	return nc;
> -}
> -
>  struct nfsdfs_client *get_nfsdfs_client(struct inode *inode)
>  {
>  	struct nfsdfs_client *nc;
>  
> -	inode_lock_shared(inode);
> -	nc = __get_nfsdfs_client(inode);
> -	inode_unlock_shared(inode);
> +	spin_lock(&inode->i_lock);
> +	nc = inode->i_private;
> +	if (nc)
> +		kref_get(&nc->cl_ref);
> +	spin_unlock(&inode->i_lock);
>  	return nc;
>  }
> -/* from __rpc_unlink */
> -static void nfsdfs_remove_file(struct inode *dir, struct dentry *dentry)
> -{
> -	int ret;
> -
> -	clear_ncl(d_inode(dentry));
> -	dget(dentry);
> -	ret = simple_unlink(dir, dentry);
> -	d_drop(dentry);
> -	fsnotify_unlink(dir, dentry);
> -	dput(dentry);
> -	WARN_ON_ONCE(ret);
> -}
> -
> -static void nfsdfs_remove_files(struct dentry *root)
> -{
> -	struct dentry *dentry, *tmp;
> -
> -	list_for_each_entry_safe(dentry, tmp, &root->d_subdirs, d_child) {
> -		if (!simple_positive(dentry)) {
> -			WARN_ON_ONCE(1); /* I think this can't happen? */
> -			continue;
> -		}
> -		nfsdfs_remove_file(d_inode(root), dentry);
> -	}
> -}
>  
>  /* XXX: cut'n'paste from simple_fill_super; figure out if we could share
>   * code instead. */
>  static  int nfsdfs_create_files(struct dentry *root,
>  				const struct tree_descr *files,
> +				struct nfsdfs_client *ncl,
>  				struct dentry **fdentries)
>  {
>  	struct inode *dir = d_inode(root);
> @@ -1310,8 +1281,9 @@ static  int nfsdfs_create_files(struct dentry *root,
>  			dput(dentry);
>  			goto out;
>  		}
> +		kref_get(&ncl->cl_ref);
>  		inode->i_fop = files->ops;
> -		inode->i_private = __get_nfsdfs_client(dir);
> +		inode->i_private = ncl;
>  		d_add(dentry, inode);
>  		fsnotify_create(dir, dentry);
>  		if (fdentries)
> @@ -1320,7 +1292,6 @@ static  int nfsdfs_create_files(struct dentry *root,
>  	inode_unlock(dir);
>  	return 0;
>  out:
> -	nfsdfs_remove_files(root);
>  	inode_unlock(dir);
>  	return -ENOMEM;
>  }
> @@ -1340,7 +1311,7 @@ struct dentry *nfsd_client_mkdir(struct nfsd_net *nn,
>  	dentry = nfsd_mkdir(nn->nfsd_client_dir, ncl, name);
>  	if (IS_ERR(dentry)) /* XXX: tossing errors? */
>  		return NULL;
> -	ret = nfsdfs_create_files(dentry, files, fdentries);
> +	ret = nfsdfs_create_files(dentry, files, ncl, fdentries);
>  	if (ret) {
>  		nfsd_client_rmdir(dentry);
>  		return NULL;
> @@ -1351,20 +1322,7 @@ struct dentry *nfsd_client_mkdir(struct nfsd_net *nn,
>  /* Taken from __rpc_rmdir: */
>  void nfsd_client_rmdir(struct dentry *dentry)
>  {
> -	struct inode *dir = d_inode(dentry->d_parent);
> -	struct inode *inode = d_inode(dentry);
> -	int ret;
> -
> -	inode_lock(dir);
> -	nfsdfs_remove_files(dentry);
> -	clear_ncl(inode);
> -	dget(dentry);
> -	ret = simple_rmdir(dir, dentry);
> -	WARN_ON_ONCE(ret);
> -	d_drop(dentry);
> -	fsnotify_rmdir(dir, dentry);
> -	dput(dentry);
> -	inode_unlock(dir);
> +	simple_recursive_removal(dentry, clear_ncl);
>  }
>  
>  static int nfsd_fill_super(struct super_block *sb, struct fs_context *fc)
> -- 
> 2.39.2
> 
>
Al Viro Nov. 9, 2023, 6:47 p.m. UTC | #3 
On Thu, Nov 09, 2023 at 09:01:19AM -0500, Chuck Lever wrote:
> On Thu, Nov 09, 2023 at 06:20:36AM +0000, Al Viro wrote:
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> > Tested-by: Jeff Layton <jlayton@kernel.org>
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> Future me is going to be mightily confused by the lack of a patch
> description. I went back to the series cover letter and found some
> text that would be nice to include here:

Does the following work for you?

switch nfsd_client_rmdir() to use of simple_recursive_removal()

nfsd_client_rmdir() open-codes a subset of simple_recursive_removal().
Conversion to calling simple_recursive_removal() allows to clean things
up quite a bit.

While we are at it, nfsdfs_create_files() doesn't need to mess with "pick    
the reference to struct nfsdfs_client from the already created parent" -
the caller already knows it (that's where the parent got it from,
after all), so we might as well just pass it as an explicit argument.
So __get_nfsdfs_client() is only needed in get_nfsdfs_client() and
can be folded in there.

Incidentally, the locking in get_nfsdfs_client() is too heavy - we don't 
need ->i_rwsem for that, ->i_lock serves just fine.
Chuck Lever III Nov. 9, 2023, 6:50 p.m. UTC | #4 
> On Nov 9, 2023, at 1:47 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> 
> On Thu, Nov 09, 2023 at 09:01:19AM -0500, Chuck Lever wrote:
>> On Thu, Nov 09, 2023 at 06:20:36AM +0000, Al Viro wrote:
>>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>>> Tested-by: Jeff Layton <jlayton@kernel.org>
>>> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>> 
>> Future me is going to be mightily confused by the lack of a patch
>> description. I went back to the series cover letter and found some
>> text that would be nice to include here:
> 
> Does the following work for you?
> 
> switch nfsd_client_rmdir() to use of simple_recursive_removal()
> 
> nfsd_client_rmdir() open-codes a subset of simple_recursive_removal().
> Conversion to calling simple_recursive_removal() allows to clean things
> up quite a bit.
> 
> While we are at it, nfsdfs_create_files() doesn't need to mess with "pick    
> the reference to struct nfsdfs_client from the already created parent" -
> the caller already knows it (that's where the parent got it from,
> after all), so we might as well just pass it as an explicit argument.
> So __get_nfsdfs_client() is only needed in get_nfsdfs_client() and
> can be folded in there.
> 
> Incidentally, the locking in get_nfsdfs_client() is too heavy - we don't 
> need ->i_rwsem for that, ->i_lock serves just fine.

Very nice, thanks.

Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>>

--
Chuck Lever
diff mbox series

Patch

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 7ed02fb88a36..035b42c1a181 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1235,63 +1235,34 @@  static inline void _nfsd_symlink(struct dentry *parent, const char *name,
 
 #endif
 
-static void clear_ncl(struct inode *inode)
+static void clear_ncl(struct dentry *dentry)
 {
+	struct inode *inode = d_inode(dentry);
 	struct nfsdfs_client *ncl = inode->i_private;
 
+	spin_lock(&inode->i_lock);
 	inode->i_private = NULL;
+	spin_unlock(&inode->i_lock);
 	kref_put(&ncl->cl_ref, ncl->cl_release);
 }
 
-static struct nfsdfs_client *__get_nfsdfs_client(struct inode *inode)
-{
-	struct nfsdfs_client *nc = inode->i_private;
-
-	if (nc)
-		kref_get(&nc->cl_ref);
-	return nc;
-}
-
 struct nfsdfs_client *get_nfsdfs_client(struct inode *inode)
 {
 	struct nfsdfs_client *nc;
 
-	inode_lock_shared(inode);
-	nc = __get_nfsdfs_client(inode);
-	inode_unlock_shared(inode);
+	spin_lock(&inode->i_lock);
+	nc = inode->i_private;
+	if (nc)
+		kref_get(&nc->cl_ref);
+	spin_unlock(&inode->i_lock);
 	return nc;
 }
-/* from __rpc_unlink */
-static void nfsdfs_remove_file(struct inode *dir, struct dentry *dentry)
-{
-	int ret;
-
-	clear_ncl(d_inode(dentry));
-	dget(dentry);
-	ret = simple_unlink(dir, dentry);
-	d_drop(dentry);
-	fsnotify_unlink(dir, dentry);
-	dput(dentry);
-	WARN_ON_ONCE(ret);
-}
-
-static void nfsdfs_remove_files(struct dentry *root)
-{
-	struct dentry *dentry, *tmp;
-
-	list_for_each_entry_safe(dentry, tmp, &root->d_subdirs, d_child) {
-		if (!simple_positive(dentry)) {
-			WARN_ON_ONCE(1); /* I think this can't happen? */
-			continue;
-		}
-		nfsdfs_remove_file(d_inode(root), dentry);
-	}
-}
 
 /* XXX: cut'n'paste from simple_fill_super; figure out if we could share
  * code instead. */
 static  int nfsdfs_create_files(struct dentry *root,
 				const struct tree_descr *files,
+				struct nfsdfs_client *ncl,
 				struct dentry **fdentries)
 {
 	struct inode *dir = d_inode(root);
@@ -1310,8 +1281,9 @@  static  int nfsdfs_create_files(struct dentry *root,
 			dput(dentry);
 			goto out;
 		}
+		kref_get(&ncl->cl_ref);
 		inode->i_fop = files->ops;
-		inode->i_private = __get_nfsdfs_client(dir);
+		inode->i_private = ncl;
 		d_add(dentry, inode);
 		fsnotify_create(dir, dentry);
 		if (fdentries)
@@ -1320,7 +1292,6 @@  static  int nfsdfs_create_files(struct dentry *root,
 	inode_unlock(dir);
 	return 0;
 out:
-	nfsdfs_remove_files(root);
 	inode_unlock(dir);
 	return -ENOMEM;
 }
@@ -1340,7 +1311,7 @@  struct dentry *nfsd_client_mkdir(struct nfsd_net *nn,
 	dentry = nfsd_mkdir(nn->nfsd_client_dir, ncl, name);
 	if (IS_ERR(dentry)) /* XXX: tossing errors? */
 		return NULL;
-	ret = nfsdfs_create_files(dentry, files, fdentries);
+	ret = nfsdfs_create_files(dentry, files, ncl, fdentries);
 	if (ret) {
 		nfsd_client_rmdir(dentry);
 		return NULL;
@@ -1351,20 +1322,7 @@  struct dentry *nfsd_client_mkdir(struct nfsd_net *nn,
 /* Taken from __rpc_rmdir: */
 void nfsd_client_rmdir(struct dentry *dentry)
 {
-	struct inode *dir = d_inode(dentry->d_parent);
-	struct inode *inode = d_inode(dentry);
-	int ret;
-
-	inode_lock(dir);
-	nfsdfs_remove_files(dentry);
-	clear_ncl(inode);
-	dget(dentry);
-	ret = simple_rmdir(dir, dentry);
-	WARN_ON_ONCE(ret);
-	d_drop(dentry);
-	fsnotify_rmdir(dir, dentry);
-	dput(dentry);
-	inode_unlock(dir);
+	simple_recursive_removal(dentry, clear_ncl);
 }
 
 static int nfsd_fill_super(struct super_block *sb, struct fs_context *fc)