On 8/15/23 21:33, Miklos Szeredi via fuse-devel wrote:
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.
In open_last_lookups there is
BUG_ON(nd->flags & LOOKUP_RCU)
(which is why I had noticed in the first place the issue in RCU mode).
In principle we could change the code to give up RCU mode after
DCACHE_ATOMIC_OPEN, but the vfs functions certainly wouldn't get
simpler with that
BTW I don't see DCACHE_ATOMIC_OPEN being cleared in this patchset,
which just shows that it's quite safe ;)
Oh right, it might go a code path later that would have an issue.
Actually Dharmendras version had removed the flag in
_fuse_atomic_open(), but I thought we wouldn't need to unset it and had
removed that part. Also better shouldn't be part of fuse to remove the
flag, but in the vfs - here in this patch, which introduces the flag.
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.
Also fine with me, although it probably would bring back the 'hack' in
lookup_fast(), as the caller does not get the return code from
d_revalidate . Or we we add it in.
Going to send an updated version in the morning.
Thanks,
Bernd
