From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Benjamin Coddington <bcodding@redhat.com>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna@kernel.org>,
        Hugh Dickins <hughd@google.com>,
        Linux Memory Management List <linux-mm@kvack.org>
Subject: Re: git regression failures with v6.2-rc NFS client
Thread-Topic: git regression failures with v6.2-rc NFS client
Thread-Index: AQHZNbksRLoeBCG00EyL+s5Kt9KD6665E+wAgAEOhICAABzOgIADD6eAgAAJrwCAAAZBgIAAG46AgAANvACAACDsAIAABuGA
Date:   Fri, 3 Feb 2023 20:25:42 +0000
Message-ID: <966AEC32-A7C9-4B97-A4F7-098AF6EF0067@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>
 <44CB1E86-60E0-4CF0-9FD4-BB7E446542B7@redhat.com>
 <1AAC6854-2591-4B21-952A-BC58180B4091@oracle.com>
 <41813D21-95C8-44E3-BB97-1E9C03CE7FE5@redhat.com>
 <79261B77-35D0-4E36-AA29-C7BF9FB734CC@oracle.com>
 <104B6879-5223-485F-B099-767F741EB15B@redhat.com>
In-Reply-To: <104B6879-5223-485F-B099-767F741EB15B@redhat.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?FUJzTQF0crhfgWqD+oGV+obkP+qYCSKIIQgOtYHXtqCfQhqasC1GfFZrRqIE?=
 =?us-ascii?Q?3ej59sitlG/Zle63O9FlNkhRfo7TDJbGPDys4uYudewRxYuHpYS/835mdeXn?=
 =?us-ascii?Q?3awC569r/KN4GLQr9McWJgffV3AN4mrVLssf7EjVMjzBhUcnqH5OSC8fu8Zu?=
 =?us-ascii?Q?jAPpvX0NG7NOUjdZmBxMfSl2Y1Ls3CTNTh6mq6zr5SUGEhK++qzV3sVT3utb?=
 =?us-ascii?Q?xPZEf/tPP2wyV20dHG+E0sXbwBbB3NMQvnLO+c7VPgF04aEI9Cx7XvQ2hLZI?=
 =?us-ascii?Q?O8M5mw2nnK1g36VMAQgApSbgOmAxjdAf5LfS2gkeLPDrUPY/rxf4+1dKEesB?=
 =?us-ascii?Q?eMkJasZelyIjLThK0o/xGlQ9ChonyI6xE9W3tnrwm78NLJvbzMEdwDQqZbhK?=
 =?us-ascii?Q?FW1Yqcske73si72v8BCkigEZgnOeW5EDG6sLg35uuroCF2L2xUpnT43Gk6Z7?=
 =?us-ascii?Q?K0Pa7B1u8BFU3cJnLNOj5b6N3Qd69ttwYlU18XtkBgqrdKdojdTW/7I1oSS1?=
 =?us-ascii?Q?pNyEmZTUSxmPi0GjQXyFHW4Hlv9ahMuIRuhSK+IO2wqFaHx5rpDvdJF4IVs6?=
 =?us-ascii?Q?pb125iWWV+GXjNx5Vrb9YFsmSBLgBgIKX9Y7SSto0n4Cmt2A98T9v9T1ZC3m?=
 =?us-ascii?Q?D3m/K0eFdVTqSGLINw2PEChVGXDuiDXB0GuALvKVZoFf+FlMBcC97qu3AKfK?=
 =?us-ascii?Q?5t+pscCSp3KfzU+yIuPIExbKzz0UWFCFN9f/8U/IVZc5//Q1+pfbQziFo/tx?=
 =?us-ascii?Q?ERk08E3/oFNAMLPIuNQIUk+uKIERVtu0tADulPCZs6mjQQumeDNakJ//l5qb?=
 =?us-ascii?Q?BjtcO4VHAFg7TfvmmB/Cg3GiUBDLT745yg9s3ivz7qo4PNHMC5j6/oMkFKZd?=
 =?us-ascii?Q?7F4vZ2e/7I7gtTc15vJ7ccgNFKCdq98AZmC3G35JjgtcvDsc4qbxFn+qqszF?=
 =?us-ascii?Q?mZewp8oVNRAZDXOKSDBTLJVtoEDfWJTLxyQz4onG0u+mr3/S7myFGCFZLcI+?=
 =?us-ascii?Q?wLgkg2sZv36bBj8yWEN+1waslkdgt3cvwCIIJTBdv7qZ5X3Tvaf8oCzxKfEf?=
 =?us-ascii?Q?MU4ONTvQU923EH+dsffTyLLl7PWI6fG+GEEoJyLImW04maHXSFdNi/BDmXb3?=
 =?us-ascii?Q?AZN4XUEESR4Z4LyyzB7WMe7myjo2Aul7bluMujO35ypmvPDroeO6dyUqApfg?=
 =?us-ascii?Q?EmGqHmRcKt4SNu8ME0KXNNrH151JCqTjDvsBAYN+4GoHPe9rNIPP4R5t8nLa?=
 =?us-ascii?Q?/Ee63dcKAEwohXbQbWvhZBNLFN7Zam8X4bMbJuIMgNa4NIvAP3s3T3Icze4A?=
 =?us-ascii?Q?Ptn3CcMgJ6Ty+XRtRljj+ThrNVh6Yf2wNm1qXdrTF5YYiqMXoN4FGCRlEzu6?=
 =?us-ascii?Q?iM6Z8OHjKcRNv9+zZvFisu0bl/+L7u2S3a4GOKDUq7HCzKY+DfWMAAh0NyWQ?=
 =?us-ascii?Q?5O40EjBLGYi3COv3q6LGn++bxuoY1WAiopqfP5C/p8IP8k6S2GhPwnceAp41?=
 =?us-ascii?Q?v8D+2jFnvfqiUV8bbBYwcf+2uOaPlrt+GypGkErxvFAt+hWSYc8GKmoQ8Gut?=
 =?us-ascii?Q?jc0zwE5rDsk+MroOUXHRkMC3ZKQVv6wt0z+77wqN9xVg6m29r5aQRV+J35W9?=
 =?us-ascii?Q?kA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <0F45B3665A8B294D871A84BF40290071@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 0VgMwpolCoE0y62hJe4qIyKcqCCGlZUegLhmQGwVmq8m8ZnSVFzFGsfL6x4kuQrrjqk5WykTmdUH9pxangr0AH6PYnSw+4J2OV3MzBcdNDuBPO5kZFfOVhwu9djl699PpJMhMqOJVuJfLL1RbjKeIyuP+bQnHKDAI/yXC9VHL+6nEH926VjYcCSczhN6RWJC4Wp7bmmZ5RIi8QEkJCOMfxPjZJ09OZ4wgIklB1M8ueSEEpS50x1NDH2wpTgV1nbt6s1dlLEuEpGIhPDdp4+MAaI5cBdfUZ47xd9uFBONiMMIDzEo/0tBv86tY5R9mjj9CEPXgo7gHCie8t2F1zYSW7usGzONtqMV3qJdM1A1X4Aw8/7PAdpBEQ3BLijFfE3lDEnRkj9lByiy6Ceayv9VrkmJJjo487eBjOOvBf4n5efV5C27wkikvEZ4rPK4RhPGrZZUQgVt2Cs9Hc0RfpCdW2vlUKjxJfB/vYnb1odIq6DnKb3OBzDUVxJx7l1bgY+xu+VUIXhBeAMlsaxu2+NUoPc4fLtUxrR2wYqrmpL31s92+Tb1xt6dKMHXIoYo+RRYGVK5OLXCo6folZ9GYQf0021XF0siCftMu6n/tCgj2yrfO+zbTy+gE15XXHme7B28RW7o+AyFYpXC9Hb0i4zlXksOriKFcf91Ko8Zwu0YIG9CYKxKBcjN9eNVEGX/lp3/YPyTby7Al+VEgyeiNzSMWu2pIefEmpbFhsZ1Br4ZM3/yQF341xKfC+2h3ORiWaFr+Ri+lTUJR2SVbqyDijy2jpkOM7wmuAd65fYmbA/lyNHulo5FSIYcLYQ45xce8NCEMYb3C1PZVJad5tLu2b9bo/SKvqjgttXHmhBrqIrrpLN4xffZjhYc058//c20D8VDx/JILLAt/fQSYTDW8brr6bQP3tBpBHb6mh0wfvDw608=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3a28bb7f-dccf-49f1-7c0b-08db0624d2af
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Feb 2023 20:25:42.6205
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: GMA0bQy8nOT/c0mFExU8HGmcJBI4mKmcodS24+ba1X7ctd7j0b1+yUoNdxXeIH946kbWRpQdzlXmtneN1mpmEw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR10MB6733
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1
 definitions=2023-02-03_19,2023-02-03_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 adultscore=0
 suspectscore=0 mlxscore=0 spamscore=0 phishscore=0 bulkscore=0
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2212070000 definitions=main-2302030183
X-Proofpoint-ORIG-GUID: 1pBhJYW2P-3gvEGIwGC9DpR61ICajwLK
X-Proofpoint-GUID: 1pBhJYW2P-3gvEGIwGC9DpR61ICajwLK
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Feb 3, 2023, at 3:01 PM, Benjamin Coddington <bcodding@redhat.com> wro=
te:
>=20
> On 3 Feb 2023, at 13:03, Chuck Lever III wrote:
>> Naive suggestion: Would another option be to add server
>> support for the directory verifier?
>=20
> I don't think it would resolve this bug, because what can the client do t=
o
> find its place in the directory stream after getting NFS4ERR_NOT_SAME?
>=20
> Directory verifiers might help another class of bugs, where a linear walk
> through the directory produces duplicate entries.. but I think it only he=
lps
> some of those cases.
>=20
> Knfsd /could/ use the directory verifier to keep a reference to an opened
> directory.  Perhaps knfsd's open file cache can be used.  Then, as long a=
s
> we're doing a linear walk through the directory, the local directory's
> file->private cursor would continue to be valid to produce consistent lin=
ear
> results even if the dentries are removed in between calls to READDIR.
>=20
> .. but that's not how the verifier is supposed to be used - rather it's
> supposed to signal when the client's cookies are invalid, which, for tmpf=
s,
> would be anytime any changes are made to the directory.

Right. Change the verifier whenever a directory is modified.

(Make it an export -> callback per filesystem type).

>> Well, let's not argue semantics. The optimization exposes
>> this (previously known) bug to a wider set of workloads.
>=20
> Ok, yes.
>=20
>> Please, open some bugs that document it. Otherwise this stuff
>> will never get addressed. Can't fix what we don't know about.
>>=20
>> I'm not claiming tmpfs is perfect. But the optimization seems
>> to make things worse for more workloads. That's always been a
>> textbook definition of regression, and a non-starter for
>> upstream.
>=20
> I guess I can - my impression has been there's no interest in fixing tmpf=
s
> for use over NFS..  after my last round of work on it I resolved to tell =
any
> customers that asked for it to do:
>=20
> [root@fedora ~]# modprobe brd rd_size=3D1048576 rd_nr=3D1
> [root@fedora ~]# mkfs.xfs /dev/ram0
>=20
> .. instead.  Though, I think that tmpfs is going to have better performan=
ce.
>=20
>>> I spent a great deal of time making points about it and arguing that th=
e
>>> client should try to work around them, and ultimately was told exactly =
this:
>>> https://lore.kernel.org/linux-nfs/a9411640329ed06dd7306bbdbdf251097c5e3=
411.camel@hammerspace.com/
>>=20
>> Ah, well "client should work around them" is going to be a
>> losing argument every time.
>=20
> Yeah, I agree with that, though at the time I naively thought the issues
> could be solved by better validation of changing directories.
>=20
> So.. I guess I lost?  I wasn't aware of the stable cookie issues fully
> until Trond pointed them out and I compared tmpfs and xfs.  At that point=
, I
> saw there's not really much of a path forward for tmpfs, unless we want t=
o
> work pretty hard at it.  But why?  Any production server wanting performa=
nce
> and stability on an NFS export isn't going to use tmpfs on knfsd.  There =
are
> good memory-speed alternatives.

Just curious, what are they? I have xfs on a pair of NVMe
add-in cards, and it's quite fast. But that's an expensive
replacement for tmpfs.

My point is, until now, I had thought that tmpfs was a fully
supported filesystem type for NFS export. What I'm hearing
is that some people don't agree, and worse, it's not been
documented anywhere.

If we're not going to support tmpfs enough to fix these
significant problems, then it should be made unexportable,
and that deprecation needs to be properly documented.


>>> The optimization you'd like to revert fixes a performance regression on
>>> workloads across exports of all filesystems.  This is a fix we've had m=
any
>>> folks asking for repeatedly.
>>=20
>> Does the community agree that tmpfs has never been a first-tier
>> filesystem for exporting? That's news to me. I don't recall us
>> deprecating support for tmpfs.
>=20
> I'm definitely not telling folks to use it as exported from knfsd.  I'm
> instead telling them, "Directory listings are going to be unstable, you'l=
l
> see problems." That includes any filesystems with file_operations of
> simple_dir_operations.

> That said, the whole opendir, getdents, unlink, getdents pattern is maybe
> fine for a test that can assume it has exclusive rights (writes?) to a
> directory, but also probably insane for anything else that wants to relia=
bly
> remove the thing, and we'll find that's why `rm -rf` still works.  It doe=
s
> opendir, getdents, getdents, getdents, unlink, unlink, unlink, .. rmdir.
> This more closely corresponds to POSIX stating:
>=20
>    If a file is removed from or added to the directory after the most rec=
ent
>    call to opendir() or rewinddir(), whether a subsequent call to readdir=
()
>    returns an entry for that file is unspecified.
>=20
>=20
>> If an optimization broke ext4, xfs, or btrfs, it would be
>> reverted until the situation was properly addressed. I don't
>> see why the situation is different for tmpfs just because it
>> has more bugs.
>=20
> Maybe it isn't?  We've yet to hear from any authoritative sources on this=
.

>>> I hope the maintainers decide not to revert
>>> it, and that we can also find a way to make readdir work reliably on tm=
pfs.
>>=20
>> IMO the guidelines go the other way: readdir on tmpfs needs to
>> be addressed first. Reverting is a last resort, but I don't see
>> a fix coming before v6.2. Am I wrong?
>=20
> Is your opinion wrong?  :)  IMO, yes, because this is just another round =
of
> "we don't fix the client for broken server behaviors".

In general, we don't fix broken servers on the client, true.

In this case, though, this is a regression. A client change
broke workloads that were working in v6.1.

I feel that makes this different than "we're not changing
the client to work around a server bug."


> If we're going to
> disagree, know that its entirely amicable from my side.  :)
>=20
>> What I fear will happen is that the optimization will go in, and
>> then nobody will address (or even document) the tmpfs problem,
>> making it unusable. That would be unfortunate and wrong.
>>=20
>> Please look at tmpfs and see how difficult it will be to address
>> the cookie problem properly.
>=20
> I think tmpfs doesn't have any requirement to report consistent offsets i=
n
> between calls to close(dir) and open(dir)

It has that requirement if we want to continue to make it
exportable by NFS.


> - so if you re-open the directory
> a second time and seek to some position, you're not going to be able to
> connect some earlier part of the stream to what you see now.
>=20
> So, I don't think its a tmpfs problem - its a bit of a combination of iss=
ues
> that contrive to make READDIR results inconsistent on NFS.  The best plac=
e
> to improve things might be to have knfsd try to keep the directory open i=
n
> between calls to READDIR for filesystems of simple_dir_operations.
>=20
> Really, the root of the problem is that there's no stateful way to walk a
> directory that's changing, especially if you're expecting things to be th=
e
> same in between close() and open().  XFS and ext4 do a pretty good job of
> making that problem disappear by hashing out their dentries across a very
> large range, but without some previous state or versioning, it seems a ha=
rd
> problem to solve.

Adding the tmpfs folks for an opinion. The issue is that
removing directory entries from a tmpfs directory while
directory scanning is in process results in undefined
behavior because tmpfs permits directory offsets to
change between closedir and opendir.

NFS does not have an explicit opendir or closedir operation.

--
Chuck Lever