Re: [PATCH v5 2/5] nfsd: reorganize filecache.c

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2022-11-02 at 07:59 +1100, NeilBrown wrote:
> On Wed, 02 Nov 2022, Jeff Layton wrote:
> > In a coming patch, we're going to rework how the filecache refcounting
> > works. Move some code around in the function to reduce the churn in the
> > later patches, and rename some of the functions with (hopefully) clearer
> > names. This should introduce no functional changes.
> 
> Just for future reference, it can help to list here which functions
> changed names and what the new names are.  It makes review that little
> bit easier.
> I think:
>    nfsd_file_flush -> nfsd_file_fsync
>    nfsd_file_unhash_and_dispose -> nfsd_file_unhash_and_queue
> 

Yep. I think those names are more descriptive. I'll add some more info
to the changelog for the next posting.

> That patch make it look like
>    nfsd_file_unhash -> nfsd_file_get
>    nfsd_file_close_inode_sync -> nfsd_file_close_inode
>    nfsd_file_close_inode -> nfsd_file_close_inode_sync
> as well, but there the content of the functions also change
> so one concludes that you just moved functions, and diff sees the '{'
> and '}' and blank lines are common and aligns on them...
> 
> Why did you just swap the order of those last two ??

Yes. In a later patch, nfsd_file_close_inode_sync calls
nfsd_file_close_inode, so it made sense to swap them.
> 
> You also moved a tracepoint from nfsd_file_put_final to nfsd_file_free,
> but didn't mention it in the commit comment (is that being too picky?).
> 

There is no nfsd_file_put_final. The nfsd_file_put_final tracepoint is
in nfsd_file_free. This patch just renames it to something more suitable
(and moves it up a little in the function).

> But allowing for all that - the patch looks ok.
> 
> Reviewed-by: NeilBrown <neilb@suse.de>
> 

Thanks!

> Thanks,
> NeilBrown
> 
> 
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/filecache.c | 135 ++++++++++++++++++++++----------------------
> >  fs/nfsd/trace.h     |   4 +-
> >  2 files changed, 70 insertions(+), 69 deletions(-)
> > 
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index 90e62042d6d6..0bf3727455e2 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -311,16 +311,59 @@ nfsd_file_alloc(struct nfsd_file_lookup_key *key, unsigned int may)
> >  	return nf;
> >  }
> >  
> > +static void
> > +nfsd_file_fsync(struct nfsd_file *nf)
> > +{
> > +	struct file *file = nf->nf_file;
> > +
> > +	if (!file || !(file->f_mode & FMODE_WRITE))
> > +		return;
> > +	if (vfs_fsync(file, 1) != 0)
> > +		nfsd_reset_write_verifier(net_generic(nf->nf_net, nfsd_net_id));
> > +}
> > +
> > +static int
> > +nfsd_file_check_write_error(struct nfsd_file *nf)
> > +{
> > +	struct file *file = nf->nf_file;
> > +
> > +	if (!file || !(file->f_mode & FMODE_WRITE))
> > +		return 0;
> > +	return filemap_check_wb_err(file->f_mapping, READ_ONCE(file->f_wb_err));
> > +}
> > +
> > +static void
> > +nfsd_file_hash_remove(struct nfsd_file *nf)
> > +{
> > +	trace_nfsd_file_unhash(nf);
> > +
> > +	if (nfsd_file_check_write_error(nf))
> > +		nfsd_reset_write_verifier(net_generic(nf->nf_net, nfsd_net_id));
> > +	rhashtable_remove_fast(&nfsd_file_rhash_tbl, &nf->nf_rhash,
> > +			       nfsd_file_rhash_params);
> > +}
> > +
> > +static bool
> > +nfsd_file_unhash(struct nfsd_file *nf)
> > +{
> > +	if (test_and_clear_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
> > +		nfsd_file_hash_remove(nf);
> > +		return true;
> > +	}
> > +	return false;
> > +}
> > +
> >  static bool
> >  nfsd_file_free(struct nfsd_file *nf)
> >  {
> >  	s64 age = ktime_to_ms(ktime_sub(ktime_get(), nf->nf_birthtime));
> >  	bool flush = false;
> >  
> > +	trace_nfsd_file_free(nf);
> > +
> >  	this_cpu_inc(nfsd_file_releases);
> >  	this_cpu_add(nfsd_file_total_age, age);
> >  
> > -	trace_nfsd_file_put_final(nf);
> >  	if (nf->nf_mark)
> >  		nfsd_file_mark_put(nf->nf_mark);
> >  	if (nf->nf_file) {
> > @@ -354,27 +397,6 @@ nfsd_file_check_writeback(struct nfsd_file *nf)
> >  		mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
> >  }
> >  
> > -static int
> > -nfsd_file_check_write_error(struct nfsd_file *nf)
> > -{
> > -	struct file *file = nf->nf_file;
> > -
> > -	if (!file || !(file->f_mode & FMODE_WRITE))
> > -		return 0;
> > -	return filemap_check_wb_err(file->f_mapping, READ_ONCE(file->f_wb_err));
> > -}
> > -
> > -static void
> > -nfsd_file_flush(struct nfsd_file *nf)
> > -{
> > -	struct file *file = nf->nf_file;
> > -
> > -	if (!file || !(file->f_mode & FMODE_WRITE))
> > -		return;
> > -	if (vfs_fsync(file, 1) != 0)
> > -		nfsd_reset_write_verifier(net_generic(nf->nf_net, nfsd_net_id));
> > -}
> > -
> >  static void nfsd_file_lru_add(struct nfsd_file *nf)
> >  {
> >  	set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
> > @@ -388,31 +410,18 @@ static void nfsd_file_lru_remove(struct nfsd_file *nf)
> >  		trace_nfsd_file_lru_del(nf);
> >  }
> >  
> > -static void
> > -nfsd_file_hash_remove(struct nfsd_file *nf)
> > -{
> > -	trace_nfsd_file_unhash(nf);
> > -
> > -	if (nfsd_file_check_write_error(nf))
> > -		nfsd_reset_write_verifier(net_generic(nf->nf_net, nfsd_net_id));
> > -	rhashtable_remove_fast(&nfsd_file_rhash_tbl, &nf->nf_rhash,
> > -			       nfsd_file_rhash_params);
> > -}
> > -
> > -static bool
> > -nfsd_file_unhash(struct nfsd_file *nf)
> > +struct nfsd_file *
> > +nfsd_file_get(struct nfsd_file *nf)
> >  {
> > -	if (test_and_clear_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
> > -		nfsd_file_hash_remove(nf);
> > -		return true;
> > -	}
> > -	return false;
> > +	if (likely(refcount_inc_not_zero(&nf->nf_ref)))
> > +		return nf;
> > +	return NULL;
> >  }
> >  
> >  static void
> > -nfsd_file_unhash_and_dispose(struct nfsd_file *nf, struct list_head *dispose)
> > +nfsd_file_unhash_and_queue(struct nfsd_file *nf, struct list_head *dispose)
> >  {
> > -	trace_nfsd_file_unhash_and_dispose(nf);
> > +	trace_nfsd_file_unhash_and_queue(nf);
> >  	if (nfsd_file_unhash(nf)) {
> >  		/* caller must call nfsd_file_dispose_list() later */
> >  		nfsd_file_lru_remove(nf);
> > @@ -450,7 +459,7 @@ nfsd_file_put(struct nfsd_file *nf)
> >  		nfsd_file_unhash_and_put(nf);
> >  
> >  	if (!test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
> > -		nfsd_file_flush(nf);
> > +		nfsd_file_fsync(nf);
> >  		nfsd_file_put_noref(nf);
> >  	} else if (nf->nf_file && test_bit(NFSD_FILE_GC, &nf->nf_flags)) {
> >  		nfsd_file_put_noref(nf);
> > @@ -459,14 +468,6 @@ nfsd_file_put(struct nfsd_file *nf)
> >  		nfsd_file_put_noref(nf);
> >  }
> >  
> > -struct nfsd_file *
> > -nfsd_file_get(struct nfsd_file *nf)
> > -{
> > -	if (likely(refcount_inc_not_zero(&nf->nf_ref)))
> > -		return nf;
> > -	return NULL;
> > -}
> > -
> >  static void
> >  nfsd_file_dispose_list(struct list_head *dispose)
> >  {
> > @@ -475,7 +476,7 @@ nfsd_file_dispose_list(struct list_head *dispose)
> >  	while(!list_empty(dispose)) {
> >  		nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
> >  		list_del_init(&nf->nf_lru);
> > -		nfsd_file_flush(nf);
> > +		nfsd_file_fsync(nf);
> >  		nfsd_file_put_noref(nf);
> >  	}
> >  }
> > @@ -489,7 +490,7 @@ nfsd_file_dispose_list_sync(struct list_head *dispose)
> >  	while(!list_empty(dispose)) {
> >  		nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
> >  		list_del_init(&nf->nf_lru);
> > -		nfsd_file_flush(nf);
> > +		nfsd_file_fsync(nf);
> >  		if (!refcount_dec_and_test(&nf->nf_ref))
> >  			continue;
> >  		if (nfsd_file_free(nf))
> > @@ -689,7 +690,7 @@ __nfsd_file_close_inode(struct inode *inode, struct list_head *dispose)
> >  				       nfsd_file_rhash_params);
> >  		if (!nf)
> >  			break;
> > -		nfsd_file_unhash_and_dispose(nf, dispose);
> > +		nfsd_file_unhash_and_queue(nf, dispose);
> >  		count++;
> >  	} while (1);
> >  	rcu_read_unlock();
> > @@ -697,37 +698,37 @@ __nfsd_file_close_inode(struct inode *inode, struct list_head *dispose)
> >  }
> >  
> >  /**
> > - * nfsd_file_close_inode_sync - attempt to forcibly close a nfsd_file
> > + * nfsd_file_close_inode - attempt a delayed close of a nfsd_file
> >   * @inode: inode of the file to attempt to remove
> >   *
> > - * Unhash and put, then flush and fput all cache items associated with @inode.
> > + * Unhash and put all cache item associated with @inode.
> >   */
> > -void
> > -nfsd_file_close_inode_sync(struct inode *inode)
> > +static void
> > +nfsd_file_close_inode(struct inode *inode)
> >  {
> >  	LIST_HEAD(dispose);
> >  	unsigned int count;
> >  
> >  	count = __nfsd_file_close_inode(inode, &dispose);
> > -	trace_nfsd_file_close_inode_sync(inode, count);
> > -	nfsd_file_dispose_list_sync(&dispose);
> > +	trace_nfsd_file_close_inode(inode, count);
> > +	nfsd_file_dispose_list_delayed(&dispose);
> >  }
> >  
> >  /**
> > - * nfsd_file_close_inode - attempt a delayed close of a nfsd_file
> > + * nfsd_file_close_inode_sync - attempt to forcibly close a nfsd_file
> >   * @inode: inode of the file to attempt to remove
> >   *
> > - * Unhash and put all cache item associated with @inode.
> > + * Unhash and put, then flush and fput all cache items associated with @inode.
> >   */
> > -static void
> > -nfsd_file_close_inode(struct inode *inode)
> > +void
> > +nfsd_file_close_inode_sync(struct inode *inode)
> >  {
> >  	LIST_HEAD(dispose);
> >  	unsigned int count;
> >  
> >  	count = __nfsd_file_close_inode(inode, &dispose);
> > -	trace_nfsd_file_close_inode(inode, count);
> > -	nfsd_file_dispose_list_delayed(&dispose);
> > +	trace_nfsd_file_close_inode_sync(inode, count);
> > +	nfsd_file_dispose_list_sync(&dispose);
> >  }
> >  
> >  /**
> > @@ -891,7 +892,7 @@ __nfsd_file_cache_purge(struct net *net)
> >  		nf = rhashtable_walk_next(&iter);
> >  		while (!IS_ERR_OR_NULL(nf)) {
> >  			if (!net || nf->nf_net == net)
> > -				nfsd_file_unhash_and_dispose(nf, &dispose);
> > +				nfsd_file_unhash_and_queue(nf, &dispose);
> >  			nf = rhashtable_walk_next(&iter);
> >  		}
> >  
> > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> > index b09ab4f92d43..940252482fd4 100644
> > --- a/fs/nfsd/trace.h
> > +++ b/fs/nfsd/trace.h
> > @@ -903,10 +903,10 @@ DEFINE_EVENT(nfsd_file_class, name, \
> >  	TP_PROTO(struct nfsd_file *nf), \
> >  	TP_ARGS(nf))
> >  
> > -DEFINE_NFSD_FILE_EVENT(nfsd_file_put_final);
> > +DEFINE_NFSD_FILE_EVENT(nfsd_file_free);
> >  DEFINE_NFSD_FILE_EVENT(nfsd_file_unhash);
> >  DEFINE_NFSD_FILE_EVENT(nfsd_file_put);
> > -DEFINE_NFSD_FILE_EVENT(nfsd_file_unhash_and_dispose);
> > +DEFINE_NFSD_FILE_EVENT(nfsd_file_unhash_and_queue);
> >  
> >  TRACE_EVENT(nfsd_file_alloc,
> >  	TP_PROTO(
> > -- 
> > 2.38.1
> > 
> > 

-- 
Jeff Layton <jlayton@kernel.org>