Re: [PATCH v2] nfsd: eliminate the NFSD_FILE_BREAK_* flags

[Jeff Layton <jlayton@kernel.org>]

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@netapp.com>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2107360
> Fixes: 65294c1f2c5e (nfsd: add a new struct file caching facility to nfsd)
> Cc: <stable@vger.kernel.org> # 5.4.x : bb283ca18d1e NFSD: Clean up the show_nf_flags() macro
> Cc: <stable@vger.kernel.org> # 5.4.x
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  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@kernel.org>