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

From: Dharmendra Hans <dharamhans87@gmail.com>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: linux-fsdevel@vger.kernel.org,
	fuse-devel <fuse-devel@lists.sourceforge.net>,
	linux-kernel@vger.kernel.org, Dharmendra Singh <dsingh@ddn.com>,
	Bernd Schubert <bschubert@ddn.com>
Subject: Re: [PATCH v2 1/2] FUSE: Implement atomic lookup + open
Date: Mon, 25 Apr 2022 16:13:02 +0530	[thread overview]
Message-ID: <CACUYsyFrP5UDOJKCLOr+PeHjnh9RV=wWOBRFN31-Fr-gi1d2WA@mail.gmail.com> (raw)
In-Reply-To: <CAJfpegt5qWE4UepoDj9QBuT--ysT6+7E-6ZQvNeZ+bODRHHCvg@mail.gmail.com>

On Mon, Apr 25, 2022 at 1:08 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 25 Apr 2022 at 07:26, Dharmendra Hans <dharamhans87@gmail.com> wrote:
> >
> > On Fri, Apr 22, 2022 at 8:59 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Tue, 22 Mar 2022 at 12:52, Dharmendra Singh <dharamhans87@gmail.com> wrote:
> > > >
> > > > From: Dharmendra Singh <dsingh@ddn.com>
> > > >
> > > > 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@ddn.com>
> > > > ---
> > > > 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.