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@xxxxxxxxxxxxxxx>
> >
> > If the DS is local to this client use localio to write the data.
> >
> > Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> > ---
> > 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.
