On Fri, 2022-07-29 at 15:52 -0400, Jeff Layton 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.
>
> Reported-by: Olga Kornieskaia <kolga@xxxxxxxxxx>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2107360
> Fixes: 65294c1f2c5e (nfsd: add a new struct file caching facility to nfsd)
> Cc: <stable@xxxxxxxxxxxxxxx> # 5.4.x : bb283ca18d1e NFSD: Clean up the show_nf_flags() macro
> Cc: <stable@xxxxxxxxxxxxxxx> # 5.4.x
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/nfsd/filecache.c | 24 +++---------------------
> fs/nfsd/filecache.h | 4 +---
> fs/nfsd/trace.h | 2 --
> 3 files changed, 4 insertions(+), 26 deletions(-)
>
> This patch should apply cleanly on top of v5.19-rc8, and fixes the issue
> I was seeing with missing CB_RECALLS. It also applies and works on top of
> v5.18.15, but that requires a prerequisite patch. Hopefully I've got the
> stable prereq tags right here.
>
> I'll resend the tracepoint patches separately.
>
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 9cb2d590c036..8f3c0c567d70 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -184,12 +184,8 @@ nfsd_file_alloc(struct inode *inode, unsigned int may, unsigned int hashval,
> nf->nf_hashval = hashval;
> refcount_set(&nf->nf_ref, 1);
> nf->nf_may = may & NFSD_FILE_MAY_MASK;
> - 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);
> - }
> + __set_bit(NFSD_FILE_HASHED, &nf->nf_flags);
> + __set_bit(NFSD_FILE_PENDING, &nf->nf_flags);
Oof, this bit crept in during the backport. I'll send a v3.
> nf->nf_mark = NULL;
> trace_nfsd_file_alloc(nf);
> }
> @@ -958,21 +954,7 @@ nfsd_do_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>
> 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) {
> *pnf = nf;
> diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
> index 1da0c79a5580..c9e3c6eb4776 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;
> unsigned int nf_hashval;
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index a60ead3b227a..081179fb17e8 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -696,8 +696,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,
--
Jeff Layton <jlayton@xxxxxxxxxx>
