Re: [PATCH] NFSD: reduce code duplication between pool_stats_operations and nfsd_rpc_status_operations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2023-08-08 at 10:05 +0200, Lorenzo Bianconi wrote:
> Introduce nfsd_stats_open utility routine in order to reduce code
> duplication between pool_stats_operations and
> nfsd_rpc_status_operations.
> Rename nfsd_pool_stats_release in nfsd_stats_release.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> ---
>  fs/nfsd/nfsctl.c |  4 ++--
>  fs/nfsd/nfsd.h   |  2 +-
>  fs/nfsd/nfssvc.c | 35 ++++++++++++++++++++---------------
>  3 files changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 6bf168b6a410..83eb5c6d894e 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -179,7 +179,7 @@ static const struct file_operations pool_stats_operations = {
>  	.open		= nfsd_pool_stats_open,
>  	.read		= seq_read,
>  	.llseek		= seq_lseek,
> -	.release	= nfsd_pool_stats_release,
> +	.release	= nfsd_stats_release,
>  };
>  
>  DEFINE_SHOW_ATTRIBUTE(nfsd_reply_cache_stats);
> @@ -200,7 +200,7 @@ static const struct file_operations nfsd_rpc_status_operations = {
>  	.open		= nfsd_rpc_status_open,
>  	.read		= seq_read,
>  	.llseek		= seq_lseek,
> -	.release	= nfsd_pool_stats_release,
> +	.release	= nfsd_stats_release,
>  };
>  
>  /*
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index 38c390fb2cf9..3e8a47b93fd4 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -93,7 +93,7 @@ int		nfsd_nrpools(struct net *);
>  int		nfsd_get_nrthreads(int n, int *, struct net *);
>  int		nfsd_set_nrthreads(int n, int *, struct net *);
>  int		nfsd_pool_stats_open(struct inode *, struct file *);
> -int		nfsd_pool_stats_release(struct inode *, struct file *);
> +int		nfsd_stats_release(struct inode *, struct file *);
>  int		nfsd_rpc_status_open(struct inode *inode, struct file *file);
>  void		nfsd_shutdown_threads(struct net *net);
>  
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 51a6f7a8d3f7..33ad91dd3a2d 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -1079,23 +1079,34 @@ bool nfssvc_encode_voidres(struct svc_rqst *rqstp, struct xdr_stream *xdr)
>  	return true;
>  }
>  
> -int nfsd_pool_stats_open(struct inode *inode, struct file *file)
> +static int nfsd_stats_open(struct inode *inode)
>  {
> -	int ret;
>  	struct nfsd_net *nn = net_generic(inode->i_sb->s_fs_info, nfsd_net_id);
>  
>  	mutex_lock(&nfsd_mutex);
> -	if (nn->nfsd_serv == NULL) {
> +	if (!nn->nfsd_serv) {
>  		mutex_unlock(&nfsd_mutex);
>  		return -ENODEV;
>  	}
> +
>  	svc_get(nn->nfsd_serv);
> -	ret = svc_pool_stats_open(nn->nfsd_serv, file);

Note that svc_pool_stats_open used to be called under the nfsd_mutex and
it won't be after this change. I think that's ok though since you hold a
reference to the serv.

>  	mutex_unlock(&nfsd_mutex);
> -	return ret;
> +
> +	return 0;
>  }
>  
> -int nfsd_pool_stats_release(struct inode *inode, struct file *file)
> +int nfsd_pool_stats_open(struct inode *inode, struct file *file)
> +{
> +	struct nfsd_net *nn = net_generic(inode->i_sb->s_fs_info, nfsd_net_id);
> +	int ret = nfsd_stats_open(inode);
> +
> +	if (ret)
> +		return ret;
> +
> +	return svc_pool_stats_open(nn->nfsd_serv, file);
> +}
> +
> +int nfsd_stats_release(struct inode *inode, struct file *file)
>  {
>  	int ret = seq_release(inode, file);
>  	struct net *net = inode->i_sb->s_fs_info;
> @@ -1217,16 +1228,10 @@ static int nfsd_rpc_status_show(struct seq_file *m, void *v)
>   */
>  int nfsd_rpc_status_open(struct inode *inode, struct file *file)
>  {
> -	struct nfsd_net *nn = net_generic(inode->i_sb->s_fs_info, nfsd_net_id);
> +	int ret = nfsd_stats_open(inode);
>  
> -	mutex_lock(&nfsd_mutex);
> -	if (!nn->nfsd_serv) {
> -		mutex_unlock(&nfsd_mutex);
> -		return -ENODEV;
> -	}
> -
> -	svc_get(nn->nfsd_serv);
> -	mutex_unlock(&nfsd_mutex);
> +	if (ret)
> +		return ret;
>  
>  	return single_open(file, nfsd_rpc_status_show, inode->i_private);
>  }

Meh. I'm not sure this change is really worth it, but it seems to be
correct.

Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>




[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux