Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7a98c3e70bae70c44418ce8ac4b84f387b4ff850.camel@kernel.org>
Subject: Re: [PATCH] NFS: Handle missing attributes in OPEN reply
From:   Trond Myklebust <trondmy@kernel.org>
To:     NeilBrown <neilb@suse.de>,
        Trond Myklebust <trondmy@hammerspace.com>,
        Anna Schumaker <anna@kernel.org>
Cc:     Olga Kornievskaia <aglo@umich.edu>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Date:   Tue, 03 Jan 2023 19:46:25 -0500
In-Reply-To: <167279203612.13974.15063003557908413815@noble.neil.brown.name>
References: <167279203612.13974.15063003557908413815@noble.neil.brown.name>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
User-Agent: Evolution 3.46.2 (3.46.2-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Wed, 2023-01-04 at 11:27 +1100, NeilBrown wrote:
> 
> If a NFSv4 OPEN reply reports that the file was successfully opened
> but
> the subsequent GETATTR fails, Linux-NFS will attempt a stand-alone
> GETATTR request.  If that also fails, handling of the reply is
> aborted
> with error -EAGAIN and the open is attempted again from the start.
> 
> This leaves the server with an active state (because the OPEN
> operation
> succeeded) which the client doesn't know about.  If the open-owner
> (local user) did not have the file already open, this has minimal
> consequences for the client and only causes the server to spend
> resources on an open state that will never be used or explicitly
> closed.
> 
> If the open-owner DID already have the file open, then it will hold a
> reference to the open-state for that file, but the seq-id in the
> state-id will now be out-of-sync with the server.  The server will
> have
> incremented the seq-id, but the client will not have noticed.  So
> when
> the client next attempts to access the file using that state (READ,
> WRITE, SETATTR), the attempt will fail with NFS4ERR_OLD_STATEID.
> 
> The Linux-client assumes this error is due to a race and simply
> retries
> on the basis that the local state-id information should have been
> updated by another thread.  This basis is invalid in this case and
> the
> result is an infinite loop attempting IO and getting OLD_STATEID.
> 
> This has been observed with a NetApp Filer as the server (ONTAP 9.8
> p5,
> using NFSv4.0).  The client is creating, writing, and unlinking a
> particular file from multiple clients (.bash_history).  If a new OPEN
> from one client races with a REMOVE from another client while the
> first
> client already has the file open, the Filer can report success for
> the
> OPEN op, but NFS4ERR_STALE for the ACCESS or GETATTR ops in the OPEN
> request.  This gets the seq-id out-of-sync and a subsequent write to
> the
> other open on the first client causes the infinite loop to occur.
> 
> The reason that the client returns -EAGAIN is that it needs to find
> the
> inode so it can find the associated state to update the seq-id, but
> the
> inode lookup requires the file-id which is provided in the GETATTR
> reply.  Without the file-id normal inode lookup cannot be used.
> 
> This patch changes the lookup so that when the file-id is not
> available
> the list of states owned by the open-owner is examined to find the
> state
> with the correct state-id (ignoring the seq-id part of the state-id).
> If this is found it is used just as when a normal inode lookup finds
> an
> inode.  If it isn't found, -EAGAIN is returned as before.
> 
> This bug can be demonstrated by modifying the Linux NFS server as
> follows:
> 
> 1/ The second time a file is opened, unlink it.  This simulates
>    a race with another client, without needing to have a race:
> 
>     --- a/fs/nfsd/nfs4proc.c
>     +++ b/fs/nfsd/nfs4proc.c
>     @@ -594,6 +594,12 @@ nfsd4_open(struct svc_rqst *rqstp, struct
> nfsd4_compound_state *cstate,
>         if (reclaim && !status)
>                 nn->somebody_reclaimed = true;
>      out:
>     +   if (!status && open->op_stateid.si_generation > 1) {
>     +           printk("Opening gen %d\n", (int)open-
> >op_stateid.si_generation);
>     +           vfs_unlink(mnt_user_ns(resfh->fh_export-
> >ex_path.mnt),
>     +                      resfh->fh_dentry->d_parent->d_inode,
>     +                      resfh->fh_dentry, NULL);
>     +   }
>         if (open->op_filp) {
>                 fput(open->op_filp);
>                 open->op_filp = NULL;
> 
> 2/ When a GETATTR op is attempted on an unlinked file, return ESTALE
> 
>     @@ -852,6 +858,11 @@ nfsd4_getattr(struct svc_rqst *rqstp, struct
> nfsd4_compound_state *cstate,
>         if (status)
>                 return status;
> 
>     +   if (cstate->current_fh.fh_dentry->d_inode->i_nlink == 0) {
>     +           printk("Return Estale for unlinked file\n");
>     +           return nfserr_stale;
>     +   }
>     +
>         if (getattr->ga_bmval[1] & NFSD_WRITEONLY_ATTRS_WORD1)
>                 return nfserr_inval;
> 
> Then mount the filesystem and
> 
>   Thread 1            Thread 2
>   open a file
>                       open the same file (will fail)
>   write to that file
> 
> I use this shell fragment, using 'sleep' for synchronisation.
> The use of /bin/echo ensures the write is flushed when /bin/echo
> closes
> the fd on exit.
> 
>     (
>         /bin/echo hello
>         sleep 3
>         /bin/echo there
>     ) > /import/A/afile &
>     sleep 3
>     cat /import/A/afile
> 
> Probably when the OPEN succeeds, the GETATTR fails, and we don't
> already
> have the state open, we should explicitly close the state.  Leaving
> it
> open could cause problems if, for example, the server revoked it and
> signalled the client that there was a revoked state.  The client
> would
> not be able to find the state that needed to be relinquished.  I
> haven't
> attempted to implement this.


If the server starts to reply NFS4ERR_STALE to GETATTR requests, why do
we care about stateid values? Just mark the inode as stale and drop it
on the floor.

If the server tries to declare the state as revoked, then it is clearly
borken, so let NetApp fix their own bug.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com