On Fri, 2022-07-29 at 17:46 +0000, Chuck Lever III wrote:
>
> > On Jul 29, 2022, at 1:34 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >
> > On Fri, 2022-07-29 at 17:21 +0000, Chuck Lever III wrote:
> > >
> > > > On Jul 29, 2022, at 12:47 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > > >
> > > > We had a report from the spring Bake-a-thon of data corruption in some
> > > > nfstest_interop tests. Looking at the traces showed the NFS server
> > > > allowing a v3 WRITE to proceed while a read delegation was still
> > > > outstanding.
> > > >
> > > > Currently, we only set NFSD_FILE_BREAK_* flags if
> > > > NFSD_MAY_NOT_BREAK_LEASE was set when we call nfsd_file_alloc.
> > > > NFSD_MAY_NOT_BREAK_LEASE was intended to be set when finding files for
> > > > COMMIT ops, where we need a writeable filehandle but don't need to
> > > > break read leases.
> > > >
> > > > It doesn't make any sense to consult that flag when allocating a file
> > > > since the file may be used on subsequent calls where we do want to break
> > > > the lease (and the usage of it here seems to be reverse from what it
> > > > should be anyway).
> > > >
> > > > Also, after calling nfsd_open_break_lease, we don't want to clear the
> > > > BREAK_* bits. A lease could end up being set on it later (more than
> > > > once) and we need to be able to break those leases as well.
> > > >
> > > > This means that the NFSD_FILE_BREAK_* flags now just mirror
> > > > NFSD_MAY_{READ,WRITE} flags, so there's no need for them at all. Just
> > > > drop those flags and unconditionally call nfsd_open_break_lease every
> > > > time.
> > > >
> > > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2107360
> > > > Fixes: 65294c1f2c5e (nfsd: add a new struct file caching facility to nfsd)
> > > > Reported-by: Olga Kornieskaia <kolga@xxxxxxxxxx>
> > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > >
> > > I'm going to go out on a limb and predict this will conflict
> > > heavily with the filecache overhaul patches I have queued for
> > > next. :-)
> > >
> > > Do you believe this is something that urgently needs to be
> > > backported to stable kernels, or can it be rebased on top of
> > > the filecache overhaul work?
> > >
> > >
> >
> > I based this on top of your for-next branch and I think the filecache is
> > already in there.
> >
> > It's a pretty nasty bug that we probably will want backported, so it
> > might make sense to respin this on top of mainline and put it in ahead
> > of the filecache overhaul.
>
> I am a generally a proponent of enabling fix backports.
>
> I encourage you to test the respin on 5.19 and 5.18 at least
> because the moment that patch hits upstream, Sasha and Greg
> will pull it into stable. I don't relish the idea of having
> to fix the fix, if you catch my drift.
>
> And perhaps when you repost, the fix should be reordered
> before the patches that add the tracepoints.
>
That all sounds good. I'll do that in a bit, and will send a v2.
>
> > Thoughts?
>
> Rebasing all that is mechanically straightforward to do.
>
> The only issue is that the filecache work and the first PR
> tag is already in my for-next branch. If you don't think it
> will trigger massive heartburn for Linus and Stephen, I can
> pull that stuff out now, and postpone my first PR until the
> second week of the merge window.
>
That may be best. I think we want to see this fix go in almost
everywhere.
>
> > > > ---
> > > > fs/nfsd/filecache.c | 26 +++-----------------------
> > > > fs/nfsd/filecache.h | 4 +---
> > > > fs/nfsd/trace.h | 2 --
> > > > 3 files changed, 4 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > > > index 4758c2a3fcf8..7e566ddca388 100644
> > > > --- a/fs/nfsd/filecache.c
> > > > +++ b/fs/nfsd/filecache.c
> > > > @@ -283,7 +283,7 @@ nfsd_file_mark_find_or_create(struct nfsd_file *nf, struct inode *inode)
> > > > }
> > > >
> > > > static struct nfsd_file *
> > > > -nfsd_file_alloc(struct nfsd_file_lookup_key *key, unsigned int may)
> > > > +nfsd_file_alloc(struct nfsd_file_lookup_key *key)
> > > > {
> > > > struct nfsd_file *nf;
> > > >
> > > > @@ -301,12 +301,6 @@ nfsd_file_alloc(struct nfsd_file_lookup_key *key, unsigned int may)
> > > > /* nf_ref is pre-incremented for hash table */
> > > > refcount_set(&nf->nf_ref, 2);
> > > > nf->nf_may = key->need;
> > > > - if (may & NFSD_MAY_NOT_BREAK_LEASE) {
> > > > - if (may & NFSD_MAY_WRITE)
> > > > - __set_bit(NFSD_FILE_BREAK_WRITE, &nf->nf_flags);
> > > > - if (may & NFSD_MAY_READ)
> > > > - __set_bit(NFSD_FILE_BREAK_READ, &nf->nf_flags);
> > > > - }
> > > > nf->nf_mark = NULL;
> > > > }
> > > > return nf;
> > > > @@ -1090,7 +1084,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > > > if (nf)
> > > > goto wait_for_construction;
> > > >
> > > > - new = nfsd_file_alloc(&key, may_flags);
> > > > + new = nfsd_file_alloc(&key);
> > > > if (!new) {
> > > > status = nfserr_jukebox;
> > > > goto out_status;
> > > > @@ -1130,21 +1124,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > > > nfsd_file_lru_remove(nf);
> > > > this_cpu_inc(nfsd_file_cache_hits);
> > > >
> > > > - if (!(may_flags & NFSD_MAY_NOT_BREAK_LEASE)) {
> > > > - bool write = (may_flags & NFSD_MAY_WRITE);
> > > > -
> > > > - if (test_bit(NFSD_FILE_BREAK_READ, &nf->nf_flags) ||
> > > > - (test_bit(NFSD_FILE_BREAK_WRITE, &nf->nf_flags) && write)) {
> > > > - status = nfserrno(nfsd_open_break_lease(
> > > > - file_inode(nf->nf_file), may_flags));
> > > > - if (status == nfs_ok) {
> > > > - clear_bit(NFSD_FILE_BREAK_READ, &nf->nf_flags);
> > > > - if (write)
> > > > - clear_bit(NFSD_FILE_BREAK_WRITE,
> > > > - &nf->nf_flags);
> > > > - }
> > > > - }
> > > > - }
> > > > + status = nfserrno(nfsd_open_break_lease(file_inode(nf->nf_file), may_flags));
> > > > out:
> > > > if (status == nfs_ok) {
> > > > if (open)
> > > > diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
> > > > index d534b76cb65b..8e8c0c47d67d 100644
> > > > --- a/fs/nfsd/filecache.h
> > > > +++ b/fs/nfsd/filecache.h
> > > > @@ -37,9 +37,7 @@ struct nfsd_file {
> > > > struct net *nf_net;
> > > > #define NFSD_FILE_HASHED (0)
> > > > #define NFSD_FILE_PENDING (1)
> > > > -#define NFSD_FILE_BREAK_READ (2)
> > > > -#define NFSD_FILE_BREAK_WRITE (3)
> > > > -#define NFSD_FILE_REFERENCED (4)
> > > > +#define NFSD_FILE_REFERENCED (2)
> > > > unsigned long nf_flags;
> > > > struct inode *nf_inode; /* don't deref */
> > > > refcount_t nf_ref;
> > > > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> > > > index e9c5d0f56977..2bd867a96eba 100644
> > > > --- a/fs/nfsd/trace.h
> > > > +++ b/fs/nfsd/trace.h
> > > > @@ -758,8 +758,6 @@ DEFINE_CLID_EVENT(confirmed_r);
> > > > __print_flags(val, "|", \
> > > > { 1 << NFSD_FILE_HASHED, "HASHED" }, \
> > > > { 1 << NFSD_FILE_PENDING, "PENDING" }, \
> > > > - { 1 << NFSD_FILE_BREAK_READ, "BREAK_READ" }, \
> > > > - { 1 << NFSD_FILE_BREAK_WRITE, "BREAK_WRITE" }, \
> > > > { 1 << NFSD_FILE_REFERENCED, "REFERENCED"})
> > > >
> > > > DECLARE_EVENT_CLASS(nfsd_file_class,
> > > > --
> > > > 2.37.1
> > > >
> > >
> > > --
> > > Chuck Lever
> > >
> > >
> > >
> >
> > --
> > Jeff Layton <jlayton@xxxxxxxxxx>
>
> --
> Chuck Lever
>
>
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
