Linux-NFS Archive on lore.kernel.org
 help / Atom feed
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> (raw)
In-Reply-To: <CAN-5tyFzGGFhuFriN=a-U8X1r-A9+q1V6V4XQM_tmbUdFPyFxg@mail.gmail.com>

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

      reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-12 14:45 Benjamin Coddington
2019-06-13 14:51 ` bfields
2019-06-13 16:02   ` Olga Kornievskaia
2019-06-14 10:28     ` Benjamin Coddington [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7A5AA6C0-CA02-49DD-8D80-066B0D83B2F2@redhat.com \
    --to=bcodding@redhat.com \
    --cc=aglo@umich.edu \
    --cc=anna.schumaker@netapp.com \
    --cc=bfields@fieldses.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=trond.myklebust@hammerspace.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-NFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nfs/0 linux-nfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nfs linux-nfs/ https://lore.kernel.org/linux-nfs \
		linux-nfs@vger.kernel.org linux-nfs@archiver.kernel.org
	public-inbox-index linux-nfs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-nfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox