From:   Benjamin Coddington <bcodding@redhat.com>
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna@kernel.org>
Subject: Re: git regression failures with v6.2-rc NFS client
Date:   Fri, 03 Feb 2023 10:13:04 -0500
Message-ID: <44CB1E86-60E0-4CF0-9FD4-BB7E446542B7@redhat.com>
In-Reply-To: <B90C62F2-1D3A-40E0-8E33-8C349C6FFD3D@oracle.com>
References: <9A4A5673-691D-47EC-BC44-C43BE7E50A48@oracle.com>
 <D0404F55-2692-4DB6-8DD6-CAC004331AC1@redhat.com>
 <5FF4061F-108C-4555-A32D-DDBFA80EE4E7@redhat.com>
 <F1833EA0-263F-46DF-8001-747A871E5757@redhat.com>
 <B90C62F2-1D3A-40E0-8E33-8C349C6FFD3D@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 3 Feb 2023, at 9:38, Chuck Lever III wrote:

>> On Feb 1, 2023, at 10:53 AM, Benjamin Coddington <bcodding@redhat.com>=
 wrote:
>>
>> On 1 Feb 2023, at 9:10, Benjamin Coddington wrote:
>>>
>>> Working on a fix..
>>
>> .. actually, I have no idea how to fix this - if tmpfs is going to mod=
ify
>> the position of its dentries, I can't think of a way for the client to=
 loop
>> through getdents() and remove every file reliably.
>>
>> The patch you bisected into just makes this happen on directories with=
 18
>> entries instead of 127 which can be verified by changing COUNT in the
>> reproducer.
>>
>> As Trond pointed out in:
>> https://lore.kernel.org/all/eb2a551096bb3537a9de7091d203e0cbff8dc6be.c=
amel@hammerspace.com/
>>
>>    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
>>
>> The issue here is not quite the same though, we unlink the first batch=
 of
>> entries, then do a second getdents(), which returns zero entries even =
though
>> some still exist.  I don't think POSIX talks about this case directly.=

>>
>> I guess the question now is if we need to drop the "ls -l" improvement=

>> because after it we are going to see this behavior on directories with=
 >17
>> entiries instead of >127 entries.
>
> I don't have any suggestions about how to fix your optimization.

I wasn't trying to fix it.  I was trying to fix your testing setup.

> Technically I think this counts as a regression; Thorsten seems
> to agree with that opinion. It's late in the cycle, so it is
> appropriate to consider reverting 85aa8ddc3818 and trying again
> in v6.3 or v6.4.

Thorsten's bot is just scraping your regression report email, I doubt
they've carefully read this thread.

>> It should be possible to make tmpfs (and friends) generate reliable co=
okies
>> by doing something like hashing out the cursor->d_child into the cooki=
e
>> space.. (waving hands)
>
> Sure, but what if there are non-Linux NFS-exported filesystems
> that behave this way?

Then they would display this same behavior, and claiming it is a server b=
ug
might be defensible position.

The reality as I understand it is that if the server is going to change t=
he
cookie or offset of the dentries in between calls to READDIR, you're neve=
r
going to reliably be able to list the directory completely.  Or maybe we
can, but at least I can't think of any way it can be done.

You can ask Trond/Anna to revert this, but that's only going to fix your
test setup.  The behavior you're claiming is a regression is still there =
-
just at a later offset.

Or we can modify the server to make tmpfs and friends generate stable
cookies/offsets.

Or we can patch git so that it doesn't assume it can walk a directory
completely while simultaneously modifying it.

What do you think?

Ben