> 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?
> ---
> 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
