Re: [PATCH v13 15/19] pnfs/flexfiles: enable localio support

[Mike Snitzer <snitzer@kernel.org>]

On Mon, Aug 26, 2024 at 11:39:31AM +1000, NeilBrown wrote:
> On Sat, 24 Aug 2024, Mike Snitzer wrote:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > If the DS is local to this client use localio to write the data.
> > 
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > ---
> >  fs/nfs/flexfilelayout/flexfilelayout.c    | 136 +++++++++++++++++++++-
> >  fs/nfs/flexfilelayout/flexfilelayout.h    |   2 +
> >  fs/nfs/flexfilelayout/flexfilelayoutdev.c |   6 +
> >  3 files changed, 140 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
> > index 01ee52551a63..d91b640f6c05 100644
> > --- a/fs/nfs/flexfilelayout/flexfilelayout.c
> > +++ b/fs/nfs/flexfilelayout/flexfilelayout.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/nfs_mount.h>
> >  #include <linux/nfs_page.h>
> >  #include <linux/module.h>
> > +#include <linux/file.h>
> >  #include <linux/sched/mm.h>
> >  
> >  #include <linux/sunrpc/metrics.h>
> > @@ -162,6 +163,72 @@ decode_name(struct xdr_stream *xdr, u32 *id)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * A dummy definition to make RCU (and non-LOCALIO compilation) happy.
> > + * struct nfsd_file should never be dereferenced in this file.
> > + */
> > +struct nfsd_file {
> > +       int undefined__;
> > +};
> 
> I removed this and tried building both with and without LOCALIO enabled
> and the compiler didn't complain.
> Could you tell me what to do to see the unhappiness you mention?

Sorry, I can remove the dummy definition for upstream.  That was
leftover from the backport I did to 5.15.y stable@ kernel.  Older
kernels' RCU code dereferences what should just be an opaque pointer
and (ab)use typeof.  So without the dummy definition compiling against
5.15.y fails with:

  CC [M]  fs/nfs/flexfilelayout/flexfilelayout.o
In file included from ./include/linux/rbtree.h:24,
                 from ./include/linux/mm_types.h:10,
                 from ./include/linux/mmzone.h:21,
                 from ./include/linux/gfp.h:6,
                 from ./include/linux/mm.h:10,
                 from ./include/linux/nfs_fs.h:23,
                 from fs/nfs/flexfilelayout/flexfilelayout.c:10:
fs/nfs/flexfilelayout/flexfilelayout.c: In function `ff_local_open_fh´:
./include/linux/rcupdate.h:441:9: error: dereferencing pointer to incomplete type `struct nfsd_file´
  typeof(*p) *________p1 = (typeof(*p) *__force)READ_ONCE(p); \
         ^
./include/linux/rcupdate.h:580:2: note: in expansion of macro `__rcu_dereference_check´
  __rcu_dereference_check((p), (c) || rcu_read_lock_held(), __rcu)
  ^~~~~~~~~~~~~~~~~~~~~~~
./include/linux/rcupdate.h:648:28: note: in expansion of macro `rcu_dereference_check´
 #define rcu_dereference(p) rcu_dereference_check(p, 0)
                            ^~~~~~~~~~~~~~~~~~~~~
fs/nfs/flexfilelayout/flexfilelayout.c:193:7: note: in expansion of macro `rcu_dereference´
  nf = rcu_dereference(*pnf);
       ^~~~~~~~~~~~~~~

> > diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h b/fs/nfs/flexfilelayout/flexfilelayout.h
> > index f84b3fb0dddd..562e7e27a8b5 100644
> > --- a/fs/nfs/flexfilelayout/flexfilelayout.h
> > +++ b/fs/nfs/flexfilelayout/flexfilelayout.h
> > @@ -82,7 +82,9 @@ struct nfs4_ff_layout_mirror {
> >  	struct nfs_fh			*fh_versions;
> >  	nfs4_stateid			stateid;
> >  	const struct cred __rcu		*ro_cred;
> > +	struct nfsd_file __rcu		*ro_file;
> >  	const struct cred __rcu		*rw_cred;
> > +	struct nfsd_file __rcu		*rw_file;
> 
> What is the lifetime of a layout_mirror?  Does it live for longer than a
> single IO request?  If so we have a problem as this will pin the
> nfsd_file until the layout is returned.

Ah, yeah lifetime is longer than an IO... so we have the issue of pnfs
(flexfileslayout) holding nfsd_files open in the client; which will
prevent backing filesystem from being unmounted.  I haven't done that
same unmount test (which you reported I fixed for normal NFS) against
pNFS with flexfiles.  Will sort it out.