Received-SPF: softfail (google.com: domain of transitioning linux-nfs-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   "Benjamin Coddington" <bcodding@redhat.com>
To:     "Trond Myklebust" <trondmy@hammerspace.com>
Cc:     linux-nfs@vger.kernel.org
Subject: Re: [PATCH v7 05/21] NFS: Store the change attribute in the directory
 page cache
Date:   Fri, 25 Feb 2022 09:44:16 -0500
Message-ID: <11744FC6-5EFB-427A-ADB4-D211BA1C74F4@redhat.com>
In-Reply-To: <eb2a551096bb3537a9de7091d203e0cbff8dc6be.camel@hammerspace.com>
References: <20220223211305.296816-1-trondmy@kernel.org>
 <20220223211305.296816-2-trondmy@kernel.org>
 <20220223211305.296816-3-trondmy@kernel.org>
 <20220223211305.296816-4-trondmy@kernel.org>
 <20220223211305.296816-5-trondmy@kernel.org>
 <20220223211305.296816-6-trondmy@kernel.org>
 <0DBE97BF-3A88-49FD-B078-012B5EDA5849@redhat.com>
 <1ca16f7e7be9588d15525e3afa4f7a80eb66b12b.camel@hammerspace.com>
 <9506801a7b7b6330ce2807721da5e03d77cf5c78.camel@hammerspace.com>
 <640B2705-35C6-4E9E-89D2-CC3D0E10EC3A@redhat.com>
 <eb2a551096bb3537a9de7091d203e0cbff8dc6be.camel@hammerspace.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 25 Feb 2022, at 8:10, Trond Myklebust wrote:

> On Fri, 2022-02-25 at 06:38 -0500, Benjamin Coddington wrote:
>> On 24 Feb 2022, at 22:51, Trond Myklebust wrote:
>>
>>> On Fri, 2022-02-25 at 02:26 +0000, Trond Myklebust wrote:
>>>> On Thu, 2022-02-24 at 09:53 -0500, Benjamin Coddington wrote:
>>>>> On 23 Feb 2022, at 16:12, trondmy@kernel.org=C2=A0wrote:
>>>>>
>>>>>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>>>>>>
>>>>>> Use the change attribute and the first cookie in a directory
>>>>>> page
>>>>>> cache
>>>>>> entry to validate that the page is up to date.
>>>>>>
>>>>>> Suggested-by: Benjamin Coddington <bcodding@redhat.com>
>>>>>> Signed-off-by: Trond Myklebust
>>>>>> <trond.myklebust@hammerspace.com>
>>>>>> ---
>>>>>> =C2=A0fs/nfs/dir.c | 68
>>>>>> ++++++++++++++++++++++++++++------------------------
>>>>>> =C2=A01 file changed, 37 insertions(+), 31 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
>>>>>> index f2258e926df2..5d9367d9b651 100644
>>>>>> --- a/fs/nfs/dir.c
>>>>>> +++ b/fs/nfs/dir.c
>>>>>> @@ -139,6 +139,7 @@ struct nfs_cache_array_entry {
>>>>>> =C2=A0};
>>>>>>
>>>>>> =C2=A0struct nfs_cache_array {
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0u64 change_attr;
>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0u64 last_cookie;
>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned int size;=

>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned char page=
_full : 1,
>>>>>> @@ -175,7 +176,8 @@ static void nfs_readdir_array_init(struct
>>>>>> nfs_cache_array *array)
>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0memset(array, 0, s=
izeof(struct nfs_cache_array));
>>>>>> =C2=A0}
>>>>>>
>>>>>> -static void nfs_readdir_page_init_array(struct page *page,
>>>>>> u64
>>>>>> last_cookie)
>>>>>> +static void nfs_readdir_page_init_array(struct page *page,
>>>>>> u64
>>>>>> last_cookie,
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0u64
>>>>>> change_attr)
>>>>>> =C2=A0{
>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct nfs_cache_a=
rray *array;
>>>>>
>>>>>
>>>>> There's a hunk missing here, something like:
>>>>>
>>>>> @@ -185,6 +185,7 @@ static void
>>>>> nfs_readdir_page_init_array(struct
>>>>> page
>>>>> *page, u64 last_cookie,
>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 nfs_readdir_array_=
init(array);
>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 array->last_cookie=
 =3D last_cookie;
>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 array->cookies_are=
_ordered =3D 1;
>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 array->change_attr =3D change=
_attr;
>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 kunmap_atomic(arra=
y);
>>>>> =C2=A0 }
>>>>>
>>>>>>
>>>>>> @@ -207,7 +209,7 @@ nfs_readdir_page_array_alloc(u64
>>>>>> last_cookie,
>>>>>> gfp_t gfp_flags)
>>>>>> =C2=A0{
>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct page *page =
=3D alloc_page(gfp_flags);
>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (page)
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0nfs_readdir_page_init_array(page,
>>>>>> last_cookie);
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0nfs_readdir_page_init_array(page,
>>>>>> last_cookie,
>>>>>> 0);
>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return page;
>>>>>> =C2=A0}
>>>>>>
>>>>>> @@ -304,19 +306,44 @@ int nfs_readdir_add_to_array(struct
>>>>>> nfs_entry
>>>>>> *entry, struct page *page)
>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return ret;
>>>>>> =C2=A0}
>>>>>>
>>>>>> +static bool nfs_readdir_page_cookie_match(struct page *page,
>>>>>> u64
>>>>>> last_cookie,
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0
>>>>>> u64 change_attr)
>>>>>
>>>>> How about "nfs_readdir_page_valid()"?=C2=A0 There's more going on
>>>>> than a
>>>>> cookie match.
>>>>>
>>>>>
>>>>>> +{
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct nfs_cache_array =
*array =3D kmap_atomic(page);
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int ret =3D true;
>>>>>> +
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (array->change_attr =
!=3D change_attr)
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0ret =3D false;
>>>>>
>>>>> Can we skip the next test if ret =3D false?
>>>>
>>>> I'd expect the compiler to do that.
>>>>
>>>>>
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (array->size > 0 && =
array->array[0].cookie !=3D
>>>>>> last_cookie)
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0ret =3D false;
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0kunmap_atomic(array);
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static void nfs_readdir_page_unlock_and_put(struct page
>>>>>> *page)
>>>>>> +{
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unlock_page(page);
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0put_page(page);
>>>>>> +}
>>>>>> +
>>>>>> =C2=A0static struct page *nfs_readdir_page_get_locked(struct
>>>>>> address_space
>>>>>> *mapping,
>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0p=
goff_t
>>>>>> index,
>>>>>> u64
>>>>>> last_cookie)
>>>>>> =C2=A0{
>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct page *page;=

>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0u64 change_attr;
>>>>>>
>>>>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0page =3D grab_cach=
e_page(mapping, index);
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (page && !PageUptoda=
te(page)) {
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0nfs_readdir_page_init_array(page,
>>>>>> last_cookie);
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0if
>>>>>> (invalidate_inode_pages2_range(mapping, index
>>>>>> +
>>>>>> 1, -1) < 0)
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0n=
fs_zap_mapping(mapping->host,
>>>>>> mapping);
>>>>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0SetPageUptodate(page);
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!page)
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0return NULL;
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0change_attr =3D
>>>>>> inode_peek_iversion_raw(mapping->host);
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (PageUptodate(page))=
 {
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0if
>>>>>> (nfs_readdir_page_cookie_match(page,
>>>>>> last_cookie,
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0
>>>>>> change_attr))
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0r=
eturn page;
>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0nfs_readdir_clear_array(page);
>>>>>
>>>>>
>>>>> Why use i_version rather than nfs_save_change_attribute?=C2=A0 Seem=
s
>>>>> having a
>>>>> consistent value across the pachecache and dir_verifiers would
>>>>> help
>>>>> debugging, and we've already have a bunch of machinery around
>>>>> the
>>>>> change_attribute.
>>>>
>>>> The directory cache_change_attribute is not reported in
>>>> tracepoints
>>>> because it is a directory-specific field, so it's not as useful
>>>> for
>>>> debugging.
>>>>
>>>> The inode change attribute is what we have traditionally used for
>>>> determining cache consistency, and when to invalidate the cache.
>>>
>>> I should probably elaborate a little further on the differences
>>> between
>>> the inode change attribute and the cache_change_attribute.
>>>
>>> One of the main reasons for introducing the latter was to have
>>> something that allows us to track changes to the directory, but to
>>> avoid forcing unnecessary revalidations of the dcache.
>>>
>>> What this means is that when we create or remove a file, and the
>>> pre/post-op attributes tell us that there were no third party
>>> changes
>>> to the directory, we update the dcache, but we do _not_ update the
>>> cache_change_attribute, because we know that the rest of the
>>> directory
>>> contents are valid, and so we don't have to revalidate the
>>> dentries.
>>> However in that case, we _do_ want to update the readdir cache to
>>> reflect the fact that an entry was added or deleted. While we could
>>> figure out how to remove an entry (at least for the case where the
>>> filesystem is case-sensitive), we do not know where the filesystem
>>> added the new file, or what cookies was assigned.
>>>
>>> This is why the inode change attribute is more appropriate for
>>> indexing
>>> the page cache pages. It reflects the cases where we want to
>>> revalidate
>>> the readdir cache, as opposed to the dcache.
>>
>> Ok, thanks for explaining this.
>>
>> I've noticed that you haven't responded about my concerns about not
>> checking
>> the directory for changes with every v4 READDIR.=C2=A0 For v3, we have=

>> post-op
>> updates to the directory, but with v4 the directory can change and
>> we'll
>> end up with entries in the cache that are marked with an old
>> change_attr.
>>
>
> Then they will be rejected by nfs_readdir_page_cookie_match() if a =

> user
> looks up that page again after we've revalidated the change attribute
> on the directory.
>
> ...and note that NFSv4 does returns a struct change_info4 for all
> operations that change the directory, so we will update the change
> attribute in all those cases.

I'm not worried about changes from the same client.

> If the change is made on the server, well then we will detect it
> through the standard revalidation process that usually decides when to
> invalidate the directory page cache.

The environments I'm concerned about are setup very frequently: they =

look
like multiple NFS clients co-ordinating on a directory with millions of
files.  Some clients are adding files as they do work, other clients are
then looking for those files by walking the directory entries to =

validate
their existence.  The systems that do this have a "very bad time" if =

some
of them produce listings that are _dramatically_ and transiently =

different
from a listing they produced before.

That can happen really easily with what we've got here, and it can =

create a
huge problem for these setups.  And it won't be easily reproduceable, =

and it
will be hard to find.  It will cost everyone involved a lot of time and
effort to track down, and we can fix it easily.

>> I'm pretty positive that not checking for changes to the directory
>> (not
>> sending GETATTR with READDIR) is going to create cases of double-
>> listed
>> and
>> truncated-listings for dirctory listers.=C2=A0 Not handling those case=
s
>> means
>> I'm
>> going to have some very unhappy customers that complain about their
>> files
>> disappearing/reappearing on NFS.
>>
>> If you need me to prove that its an issue, I can take the time to
>> write
>> up
>> program that shows this problem.
>>
>
> If you label the page contents with an attribute that was retrieved
> _after_ the READDIR op, then you will introduce this as a problem for
> your customers.

No the problem is already here, we're not introducing it.  By labeling =

the
page contents with every call we're shifting the race window from the =

client
where it's a very large window to the server where the window is small.

Its still possible, but *much* less likely.

> The reason is that there is no atomicity between operations in a
> COMPOUND. Worse, the implementation of readdir in scalable modern
> systems, including Linux, does not even guarantee atomicity of the
> readdir operation itself. Instead each readdir entry is filled without
> holding any locks or preventing any changes to the directory or to the
> object itself.

I understand all this, but its not a reason to make the problem worse.

> POSIX states very explicitly that if you're making changes to the
> directory after the call to opendir() or rewinddir(), then the
> behaviour w.r.t. whether that file appears in the readdir() call is
> unspecified. See
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html=


Yes, but again - just because the problem exists doesn't give us reason =

to
amplify it when we can easily make a better choice for almost no cost.

Here are my reasons for wanting the GETATTR added:
  - it makes it *much* less likely for this problem to occur, with the =

minor
    downside of decreased caching for unstable directories.
  - it makes v3 and v4 readdir pagecache behavior consistent WRT =

changing
    directories.

I spent a non-trivial amount of time working on this problem, and saw =

this
exact issue appear.  Its definitely something that's going to come back =

and
bite us if we don't fix it.

How can I convince you?  I've offered to produce a working example of =

this
problem.  Will you review those results?  If I cannot convince you, I =

feel
I'll have to pursue distro-specific changes for this work.

Ben