On Tue, 15 Aug 2023 at 11:56, Bernd Schubert <bschubert@xxxxxxx> wrote: > > On 8/15/23 10:03, Miklos Szeredi wrote: > > On Fri, 11 Aug 2023 at 20:38, Bernd Schubert <bschubert@xxxxxxx> wrote: > >> > >> From: Miklos Szeredi <miklos@xxxxxxxxxx> > >> > >> atomic_open() will do an open-by-name or create-and-open > >> depending on the flags. > >> > >> If file was created, then the old positive dentry is obviously > >> stale, so it will be invalidated and a new one will be allocated. > >> > >> If not created, then check whether it's the same inode (same as in > >> ->d_revalidate()) and if not, invalidate & allocate new dentry. > >> > >> Changes from Miklos initial patch (by Bernd): > >> - LOOKUP_ATOMIC_REVALIDATE was added and is set for revalidate > >> calls into the file system when revalidate by atomic open is > >> supported - this is to avoid that ->d_revalidate() would skip > >> revalidate and set DCACHE_ATOMIC_OPEN, although vfs > >> does not supported it in the given code path (for example > >> when LOOKUP_RCU is set)). > > > > I don't get it. We don't get so far as to set DCACHE_ATOMIC_OPEN if > > LOOKUP_RCU is set. > > > See lookup_fast, there are two calls to d_revalidate() that have > LOOKUP_ATOMIC_REVALIDATE and one in RCU mode that does not. > With the new flag LOOKUP_ATOMIC_REVALIDATE we tell ->revalidate() > that we are in code path that supports revalidating atomically. > > Sure, ror RCU we can/should always return -ECHILD in fuse_dentry_revalidate when > LOOKUP_RCU is set. But then it is also easy to miss that - at a minimum we > need to document that DCACHE_ATOMIC_OPEN must not be set in RCU mode. I wouldn't say "must not". Setting DCACHE_ATOMIC_OPEN would result in ATOMIC_OPEN being used instead of plain OPEN. This may be less optimal in cases when the dentry didn't need to be revalidated, but AFAICS it wouldn't result in a bug. BTW I don't see DCACHE_ATOMIC_OPEN being cleared in this patchset, which just shows that it's quite safe ;) I'm not at all sure that DCACHE_ATOMIC_OPEN is the best mechanism for this job. Returning a specific value from ->d_revalidate might be better. Thanks, Miklos
