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@xxxxxxxxxx>
> > ---
> > 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
