Re: [PATCH v2 1/2] FUSE: Implement atomic lookup + open

[Dharmendra Hans]

On Mon, Apr 25, 2022 at 4:13 PM Dharmendra Hans <dharamhans87@xxxxxxxxx> wrote:
>
> On Mon, Apr 25, 2022 at 1:08 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> >
> > On Mon, 25 Apr 2022 at 07:26, Dharmendra Hans <dharamhans87@xxxxxxxxx> wrote:
> > >
> > > On Fri, Apr 22, 2022 at 8:59 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> > > >
> > > > On Tue, 22 Mar 2022 at 12:52, Dharmendra Singh <dharamhans87@xxxxxxxxx> wrote:
> > > > >
> > > > > From: Dharmendra Singh <dsingh@xxxxxxx>
> > > > >
> > > > > There are couple of places in FUSE where we do agressive
> > > > > lookup.
> > > > > 1) When we go for creating a file (O_CREAT), we do lookup
> > > > > for non-existent file. It is very much likely that file
> > > > > does not exists yet as O_CREAT is passed to open(). This
> > > > > lookup can be avoided and can be performed  as part of
> > > > > open call into libfuse.
> > > > >
> > > > > 2) When there is normal open for file/dir (dentry is
> > > > > new/negative). In this case since we are anyway going to open
> > > > > the file/dir with USER space, avoid this separate lookup call
> > > > > into libfuse and combine it with open.
> > > > >
> > > > > This lookup + open in single call to libfuse and finally to
> > > > > USER space has been named as atomic open. It is expected
> > > > > that USER space open the file and fills in the attributes
> > > > > which are then used to make inode stand/revalidate in the
> > > > > kernel cache.
> > > > >
> > > > > Signed-off-by: Dharmendra Singh <dsingh@xxxxxxx>
> > > > > ---
> > > > > v2 patch includes:
> > > > > - disabled o-create atomicity when the user space file system
> > > > >   does not have an atomic_open implemented. In principle lookups
> > > > >   for O_CREATE also could be optimized out, but there is a risk
> > > > >   to break existing fuse file systems. Those file system might
> > > > >   not expect open O_CREATE calls for exiting files, as these calls
> > > > >   had been so far avoided as lookup was done first.
> > > >
> > > > So we enabling atomic lookup+create only if FUSE_DO_ATOMIC_OPEN is
> > > > set.  This logic is a bit confusing as CREATE is unrelated to
> > > > ATOMIC_OPEN.   It would be cleaner to have a separate flag for atomic
> > > > lookup+create.  And in fact FUSE_DO_ATOMIC_OPEN could be dropped and
> > > > the usual logic of setting fc->no_atomic_open if ENOSYS is returned
> > > > could be used instead.
> > >
> > > I am aware that ATOMIC_OPEN is not directly related to CREATE. But
> > > This is more of feature enabling by using the flag. If we do not
> > > FUSE_DO_ATOMIC_OPEN, CREATE calls would not know that it need to
> > > optimize lookup calls otherwise as we know only from open call that
> > > atomic open is implemented.
> >
> > Right.  So because the atomic lookup+crteate would need a new flag to
> > return whether the file was created or not, this is probably better
> > implemented as a completely new request type (FUSE_ATOMIC_CREATE?)
> >
> > No new INIT flags needed at all, since we can use the ENOSYS mechanism
> > to determine whether the filesystem has atomic open/create ops or not.
>
> Yes, it sounds good to have a separate request type for CREATE. I
> would separate out the patch into two for create and open.  Will omit
> INIT flags. Also, I would change libfuse code accordingly.

Actually when writing the code, I observe that not having INIT flags
works fine for atomic create but it does not work well for atomic
open case considering specially 3rd  patch which optimises
d_revalidate() lookups.
(https://lore.kernel.org/linux-fsdevel/20220322115148.3870-3-dharamhans87@xxxxxxxxx/,
 we did not receive any comments on it so far).
So it looks like we need INIT flags in atomic open case at least
considering that 3rd patch would go in as well.