Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   "Benjamin Coddington" <bcodding@redhat.com>
To:     "Olga Kornievskaia" <aglo@umich.edu>
Cc:     "J. Bruce Fields" <bfields@fieldses.org>,
        trond.myklebust@hammerspace.com,
        "Anna Schumaker" <anna.schumaker@netapp.com>,
        linux-nfs <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] NFS: Don't skip lookup when holding a delegation
Date:   Fri, 14 Jun 2019 06:28:55 -0400
Message-ID: <7A5AA6C0-CA02-49DD-8D80-066B0D83B2F2@redhat.com>
In-Reply-To: <CAN-5tyFzGGFhuFriN=a-U8X1r-A9+q1V6V4XQM_tmbUdFPyFxg@mail.gmail.com>
References: <bcb2d38fe9c9bb15aeb9baa811aeb9a8697ea141.1560348835.git.bcodding@redhat.com>
 <20190613145149.GD2145@fieldses.org>
 <CAN-5tyFzGGFhuFriN=a-U8X1r-A9+q1V6V4XQM_tmbUdFPyFxg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; format=flowed
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On 13 Jun 2019, at 12:02, Olga Kornievskaia wrote:

> On Thu, Jun 13, 2019 at 11:00 AM J. Bruce Fields 
> <bfields@fieldses.org> wrote:
>>
>> On Wed, Jun 12, 2019 at 10:45:13AM -0400, Benjamin Coddington wrote:
>>> If we skip lookup revalidation while holding a delegation, we might 
>>> miss
>>> that the file has changed directories on the server.
>>
>> The delegation should prevent the file disappearing from this 
>> directory,
>> so if I've been following the discussion, the bug was due to 
>> overlooking
>> the case where the change happened before we got the delegation.  
>> Given
>> that history it seems worth calling out that case specifically?
>>
>> Maybe a comment along the lines of:
>>
>>                 /*
>>                  * Note that the file can't move while we hold a
>>                  * delegation.  But this dentry could have been 
>> cached
>>                  * before we got a delegation.  So it's only safe to
>>                  * skip revalidation when the parent directory is
>>                  * unchanged:
>>                  */
>>
>> But maybe there's a pithier way to say that.

I wish I had pith.  I cannot improve on this comment.. I'm OK with or
without it.

> What is preventing the file from disappearing from the directory while
> holding the delegation: is it the server's responsibility to recall
> the delegation when it gets a move or is it client's responsibility
> not to rely on the cached attributes?

The server should recall the delegation if the file moves, since moving 
it
means that additional calls to OPEN that file at that location should 
fail.
The client shouldn't continue to independently handle those OPENs 
(unless by
filehandle.. but how can the server know how the client intends to 
handle
further delegated OPENs?)

> According to this patch it's client's responsibility, in the case, I
> find the working " file can't move" confusing as they imply to me that
> client can assume file isn't moved (ie, server will prevent it from
> happening).

I think that it looks like its the client's responsibility only because 
the
client lacks an easy way to determine what order delegations were 
acquired
with respect to directory modifications.  We could try to track all of 
that,
but the structures and memory used would be hideous.

Ben