Re: [for-6.13 PATCH v2 06/15] nfs: cache all open LOCALIO nfsd_file(s) in client

[Mike Snitzer <snitzer@kernel.org>]

On Fri, Nov 15, 2024 at 02:12:46PM +1100, NeilBrown wrote:
> On Thu, 14 Nov 2024, Mike Snitzer wrote:
> > This commit switches from leaning heavily on NFSD's filecache (in
> > terms of GC'd nfsd_files) back to caching nfsd_files in the
> > client. A later commit will add the callback mechanism needed to
> > allow NFSD to force the NFS client to cleanup all caches files.
> > 
> > Add nfs_fh_localio_init() and 'struct nfs_fh_localio' to cache opened
> > nfsd_file(s) (both a RO and RW nfsd_file is able to be opened and
> > cached for a given nfs_fh).
> > 
> > Update nfs_local_open_fh() to cache the nfsd_file once it is opened
> > using __nfs_local_open_fh().
> > 
> > Introduce nfs_close_local_fh() to clear the cached open nfsd_files and
> > call nfs_to_nfsd_file_put_local().
> > 
> > Refcounting is such that:
> > - nfs_local_open_fh() is paired with nfs_close_local_fh().
> > - __nfs_local_open_fh() is paired with nfs_to_nfsd_file_put_local().
> > - nfs_local_file_get() is paired with nfs_local_file_put().
> > 
> > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > ---
> >  fs/nfs/flexfilelayout/flexfilelayout.c | 29 +++++----
> >  fs/nfs/flexfilelayout/flexfilelayout.h |  1 +
> >  fs/nfs/inode.c                         |  3 +
> >  fs/nfs/internal.h                      |  4 +-
> >  fs/nfs/localio.c                       | 89 +++++++++++++++++++++-----
> >  fs/nfs/pagelist.c                      |  5 +-
> >  fs/nfs/write.c                         |  3 +-
> >  fs/nfs_common/nfslocalio.c             | 52 ++++++++++++++-
> >  include/linux/nfs_fs.h                 | 22 ++++++-
> >  include/linux/nfslocalio.h             | 18 +++---
> >  10 files changed, 181 insertions(+), 45 deletions(-)
> > 
> > diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
> > index f78115c6c2c12..451f168d882be 100644
> > --- a/fs/nfs/flexfilelayout/flexfilelayout.c
> > +++ b/fs/nfs/flexfilelayout/flexfilelayout.c
> > @@ -164,18 +164,17 @@ decode_name(struct xdr_stream *xdr, u32 *id)
> >  }
> >  
> >  static struct nfsd_file *
> > -ff_local_open_fh(struct nfs_client *clp, const struct cred *cred,
> > +ff_local_open_fh(struct pnfs_layout_segment *lseg, u32 ds_idx,
> > +		 struct nfs_client *clp, const struct cred *cred,
> >  		 struct nfs_fh *fh, fmode_t mode)
> >  {
> > -	if (mode & FMODE_WRITE) {
> > -		/*
> > -		 * Always request read and write access since this corresponds
> > -		 * to a rw layout.
> > -		 */
> > -		mode |= FMODE_READ;
> > -	}
> > +#if IS_ENABLED(CONFIG_NFS_LOCALIO)
> > +	struct nfs4_ff_layout_mirror *mirror = FF_LAYOUT_COMP(lseg, ds_idx);
> >  
> > -	return nfs_local_open_fh(clp, cred, fh, mode);
> > +	return nfs_local_open_fh(clp, cred, fh, &mirror->nfl, mode);
> > +#else
> > +	return NULL;
> > +#endif
> >  }
> >  
> >  static bool ff_mirror_match_fh(const struct nfs4_ff_layout_mirror *m1,
> > @@ -247,6 +246,9 @@ static struct nfs4_ff_layout_mirror *ff_layout_alloc_mirror(gfp_t gfp_flags)
> >  		spin_lock_init(&mirror->lock);
> >  		refcount_set(&mirror->ref, 1);
> >  		INIT_LIST_HEAD(&mirror->mirrors);
> > +#if IS_ENABLED(CONFIG_NFS_LOCALIO)
> > +		nfs_localio_file_init(&mirror->nfl);
> > +#endif
> 
> Can we make nfs_localio_file_init() a #define in the no-NFS_LOCALIO
> case, we don't need the #if.
> (every time you write #if in a .c file think to your self "Neil will
> hate this".  See also coding-style.rst. 21) Conditional Compilation.

It already was defined in header, and doesn't need wrapping in caller,
I just mistakenly wrapped it again in ff_layout_alloc_mirror().

Fixed.

> >  	}
> >  	return mirror;
> >  }
> > @@ -257,6 +259,9 @@ static void ff_layout_free_mirror(struct nfs4_ff_layout_mirror *mirror)
> >  
> >  	ff_layout_remove_mirror(mirror);
> >  	kfree(mirror->fh_versions);
> > +#if IS_ENABLED(CONFIG_NFS_LOCALIO)
> > +	nfs_close_local_fh(&mirror->nfl);
> > +#endif
> >  	cred = rcu_access_pointer(mirror->ro_cred);
> >  	put_cred(cred);
> >  	cred = rcu_access_pointer(mirror->rw_cred);

I fixed this call to nfs_close_local_fh() to not use #if also.

> > diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h b/fs/nfs/flexfilelayout/flexfilelayout.h
> > index f84b3fb0dddd8..095df09017a57 100644
> > --- a/fs/nfs/flexfilelayout/flexfilelayout.h
> > +++ b/fs/nfs/flexfilelayout/flexfilelayout.h
> > @@ -83,6 +83,7 @@ struct nfs4_ff_layout_mirror {
> >  	nfs4_stateid			stateid;
> >  	const struct cred __rcu		*ro_cred;
> >  	const struct cred __rcu		*rw_cred;
> > +	struct nfs_file_localio		nfl;
> 
> This probably wants a #if around it though - it is in a .h after all.

I unconditionall defined 'struct nfs_file_localio' otherwise the
calls that access this nfl member (nfs_localio_file_init) _will_ need
special treatment.

> 
> >  	refcount_t			ref;
> >  	spinlock_t			lock;
> >  	unsigned long			flags;

<snip>

> > diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c
> > index abc132166742e..35a2e48731df6 100644
> > --- a/fs/nfs_common/nfslocalio.c
> > +++ b/fs/nfs_common/nfslocalio.c
> > @@ -151,9 +151,18 @@ void nfs_localio_invalidate_clients(struct list_head *cl_uuid_list)
> >  }
> >  EXPORT_SYMBOL_GPL(nfs_localio_invalidate_clients);
> >  
> > +static void nfs_uuid_add_file(nfs_uuid_t *nfs_uuid, struct nfs_file_localio *nfl)
> > +{
> > +	spin_lock(&nfs_uuid_lock);
> > +	if (!nfl->nfs_uuid)
> > +		rcu_assign_pointer(nfl->nfs_uuid, nfs_uuid);
> > +	spin_unlock(&nfs_uuid_lock);
> > +}
> > +
> >  struct nfsd_file *nfs_open_local_fh(nfs_uuid_t *uuid,
> >  		   struct rpc_clnt *rpc_clnt, const struct cred *cred,
> > -		   const struct nfs_fh *nfs_fh, const fmode_t fmode)
> > +		   const struct nfs_fh *nfs_fh, struct nfs_file_localio *nfl,
> > +		   const fmode_t fmode)
> >  {
> >  	struct net *net;
> >  	struct nfsd_file *localio;
> > @@ -180,11 +189,50 @@ struct nfsd_file *nfs_open_local_fh(nfs_uuid_t *uuid,
> >  					     cred, nfs_fh, fmode);
> >  	if (IS_ERR(localio))
> >  		nfs_to_nfsd_net_put(net);
> > +	else
> > +		nfs_uuid_add_file(uuid, nfl);
> >  
> >  	return localio;
> >  }
> >  EXPORT_SYMBOL_GPL(nfs_open_local_fh);
> >  
> > +void nfs_close_local_fh(struct nfs_file_localio *nfl)
> > +{
> > +	struct nfsd_file *ro_nf = NULL;
> > +	struct nfsd_file *rw_nf = NULL;
> > +	nfs_uuid_t *nfs_uuid;
> > +
> > +	rcu_read_lock();
> > +	nfs_uuid = rcu_dereference(nfl->nfs_uuid);
> 
> nfl->nfs_uuid is a void*.  Why do you assign it to an 'nfs_uuid_t *' ??

Yeah, I made it void* to not have to play games with nfs_uuid_t in
include/linux/nfs_fs.h

It is assigned to 'nfs_uuid_t *' because I dereference it to get its
spinlock in later patch (that splits the nfs_uuid_lock).

> And why do we need rcu here?  We never dereference that pointer.

As I just said, I do in the patch that splits the nfs_uuid_lock.

And I made nfl->nfs_uuid management RCU protected given it being NULL
or not is significant and I'd rather not require other synchronization
via other locking.

SO while I appreciate your review here, the code in final form (once
nfs_uuid_lock lock split patch applied) does need RCU as I've written
it.

> I would just have
> 
>   if (!nfl->nfs_uuid || (!nfl->ro_file && !nfl->rw_file))
>         return;
> 
> then take the spinlock and do it the real work.
> 
> > +	if (!nfs_uuid) {
> > +		/* regular (non-LOCALIO) NFS will hammer this */
> > +		rcu_read_unlock();
> > +		return;
> > +	}
> > +
> > +	ro_nf = rcu_access_pointer(nfl->ro_file);
> > +	rw_nf = rcu_access_pointer(nfl->rw_file);
> > +	if (ro_nf || rw_nf) {
> > +		spin_lock(&nfs_uuid_lock);
> > +		if (ro_nf)
> > +			ro_nf = rcu_dereference_protected(xchg(&nfl->ro_file, NULL), 1);
> > +		if (rw_nf)
> > +			rw_nf = rcu_dereference_protected(xchg(&nfl->rw_file, NULL), 1);
> > +
> > +		rcu_assign_pointer(nfl->nfs_uuid, NULL);
> > +		spin_unlock(&nfs_uuid_lock);
> > +		rcu_read_unlock();
> > +
> > +		if (ro_nf)
> > +			nfs_to_nfsd_file_put_local(ro_nf);
> > +		if (rw_nf)
> > +			nfs_to_nfsd_file_put_local(rw_nf);
> > +		return;
> > +	}
> > +	rcu_read_unlock();
> > +}
> > +EXPORT_SYMBOL_GPL(nfs_close_local_fh);
> > +
> >  /*
> >   * The NFS LOCALIO code needs to call into NFSD using various symbols,
> >   * but cannot be statically linked, because that will make the NFS
> > diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> > index 039898d70954f..67ae2c3f41d20 100644
> > --- a/include/linux/nfs_fs.h
> > +++ b/include/linux/nfs_fs.h
> > @@ -77,6 +77,23 @@ struct nfs_lock_context {
> >  	struct rcu_head	rcu_head;
> >  };
> >  
> > +struct nfs_file_localio {
> > +	struct nfsd_file __rcu *ro_file;
> > +	struct nfsd_file __rcu *rw_file;
> > +	struct list_head list;
> > +	void __rcu *nfs_uuid; /* opaque pointer to 'nfs_uuid_t' */
> 
> I've said it above but just to be clear:  No "__rcu" here.

Please look at final form of nfs_close_local_fh() with all patches
applied.  I wanted to avoid churn in nfs_close_local_fh(), but yeah it
does have some needless RCU-ification in this intermediate patch.

That said, could be there is still room for cleanup even with the
final nfs_close_local_fh()...

Thanks,
Mike