Re: [PATCH 1/2] NFSv4: Fix CLOSE races with OPEN

[Olga Kornievskaia <aglo@umich.edu>]

On Wed, Feb 14, 2018 at 11:59 AM, Trond Myklebust
<trondmy@primarydata.com> wrote:
> On Wed, 2018-02-14 at 11:06 -0500, Olga Kornievskaia wrote:
>> On Wed, Feb 14, 2018 at 10:42 AM, Benjamin Coddington
>> <bcodding@redhat.com> wrote:
>> > On 14 Feb 2018, at 10:29, Trond Myklebust wrote:
>> > > On Wed, 2018-02-14 at 10:21 -0500, Jeff Layton wrote:
>> > > > On Wed, 2018-02-14 at 10:05 -0500, Benjamin Coddington wrote:
>> > > > > Hi Olga,
>> > > > >
>> > > > > On 13 Feb 2018, at 15:08, Olga Kornievskaia wrote:
>> > > > >
>> > > > > > On Mon, Nov 14, 2016 at 11:19 AM, Trond Myklebust
>> > > > > > <trond.myklebust@primarydata.com> wrote:
>> > > > > > ...
>> > > > >
>> > > > > Ah, good question there..  Even though the stateid is not
>> > > > > useful
>> > > > > for
>> > > > > operations that follow, I think the sequenceid should be
>> > > > > incremented because
>> > > > > the CLOSE is an operation that modifies the set of locks or
>> > > > > state:
>> > > > >
>> > > >
>> > > > It doesn't matter.
>> >
>> > Yes, good condensed point.  It doesn't matter.
>> >
>> > > > > ...
>> > >
>> > > Is there a proposal to change the current client behaviour here?
>> > > As far
>> > > as I can tell, the answer is "no". Am I missing something?
>> >
>> > Not that I can see.  I think we're just comparing linux and netapp
>> > server
>> > behaviors..
>> >
>> > Ben
>>
>> I just found very surprising that in nfs4_close_done() which calls
>> eventually calls nfs_clear_open_stateid_locked() we change the state
>> based on the stateid received from the CLOSE.
>> nfs_clear_open_stateid_locked() is only called from nfs4_close_done()
>> and no other function.
>>
>> I guess I'm not wondering if we had this problem described in this
>> patch of the delayed CLOSE, if we didn't update the state after
>> receiving the close... (so yes this is a weak proposal).
>>
>
> nfs4_close_done() doesn't only deal with CLOSE. It also has to handle
> OPEN_DOWNGRADE.

What about doing an update for OPEN_DOWNGRADE but not for the CLOSE (untested):

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index f8a2b226..5868a6a 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1472,7 +1472,7 @@ static void nfs_clear_open_stateid_locked(struct
nfs4_state *state,
        if (stateid == NULL)
                return;
        /* Handle OPEN+OPEN_DOWNGRADE races */
-       if (nfs4_stateid_match_other(stateid, &state->open_stateid) &&
+       if (fmode && nfs4_stateid_match_other(stateid, &state->open_stateid) &&
            !nfs4_stateid_is_newer(stateid, &state->open_stateid)) {
                nfs_resync_open_stateid_locked(state);
                goto out;

>
> --
> Trond Myklebust
> Linux NFS client maintainer, PrimaryData
> trond.myklebust@primarydata.com