Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220502102521.22875-1-dharamhans87@gmail.com>
 <20220502102521.22875-3-dharamhans87@gmail.com> <YnLD1ibtU0FM5nlS@redhat.com>
In-Reply-To: <YnLD1ibtU0FM5nlS@redhat.com>
From:   Dharmendra Hans <dharamhans87@gmail.com>
Date:   Thu, 5 May 2022 12:09:10 +0530
Message-ID: <CACUYsyGOwTjyOZYuAK1sxO9VFFBp-qPOjbQXJgQj2DK-_X1ynA@mail.gmail.com>
Subject: Re: [PATCH v4 2/3] FUSE: Implement atomic lookup + open
To:     Vivek Goyal <vgoyal@redhat.com>
Cc:     Miklos Szeredi <miklos@szeredi.hu>, linux-fsdevel@vger.kernel.org,
        fuse-devel <fuse-devel@lists.sourceforge.net>,
        linux-kernel@vger.kernel.org, Bernd Schubert <bschubert@ddn.com>,
        Dharmendra Singh <dsingh@ddn.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, May 4, 2022 at 11:50 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Mon, May 02, 2022 at 03:55:20PM +0530, Dharmendra Singh wrote:
> > From: Dharmendra Singh <dsingh@ddn.com>
> >
> > We can optimize aggressive lookups which are triggered when
> > there is normal open for file/dir (dentry is new/negative).
> >
> > Here 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 call into libfuse.
> >
> > This lookup + open in single call to libfuse has been named
> > as atomic open. It is expected that USER SPACE opens 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>
> > ---
> >  fs/fuse/dir.c             | 78 ++++++++++++++++++++++++++++++---------
> >  fs/fuse/fuse_i.h          |  3 ++
> >  fs/fuse/inode.c           |  4 +-
> >  include/uapi/linux/fuse.h |  2 +
> >  4 files changed, 69 insertions(+), 18 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index cad3322a007f..6879d3a86796 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -516,18 +516,18 @@ static int get_security_context(struct dentry *entry, umode_t mode,
> >  }
> >
> >  /*
> > - * Atomic create+open operation
> > - *
> > - * If the filesystem doesn't support this, then fall back to separate
> > - * 'mknod' + 'open' requests.
> > + * This is common function for initiating atomic operations into libfuse.
> > + * Currently being used by Atomic create(atomic lookup + create) and
> > + * Atomic open(atomic lookup + open).
> >   */
> > -static int fuse_create_open(struct inode *dir, struct dentry *entry,
> > -                         struct file *file, unsigned int flags,
> > -                         umode_t mode, uint32_t opcode)
> > +static int fuse_atomic_do_common(struct inode *dir, struct dentry *entry,
> > +                              struct dentry **alias, struct file *file,
> > +                              unsigned int flags, umode_t mode, uint32_t opcode)
>
> If this common function is really useful for atomic create + atomic open,
> I will first add a patch which adds a common function and makes FUSE_CREATE_EXT use that function. And in later patch add functionality to do atomic open.
> Just makes code more readable.

This func is commonly used by atomic create and atomic open, both.
atomic open has fuse_do_atomic_open wrapper over it. I just renamed
this func except doing it in a separate patch (just to avoid another
patch though I understand that it might be a little hard to read the
code).


>
> >  {
> >       int err;
> >       struct inode *inode;
> >       struct fuse_mount *fm = get_fuse_mount(dir);
> > +     struct fuse_conn *fc = get_fuse_conn(dir);
> >       FUSE_ARGS(args);
> >       struct fuse_forget_link *forget;
> >       struct fuse_create_in inarg;
> > @@ -539,9 +539,13 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >       void *security_ctx = NULL;
> >       u32 security_ctxlen;
> >       bool atomic_create = (opcode == FUSE_ATOMIC_CREATE ? true : false);
> > +     bool create_op = (opcode == FUSE_CREATE ||
> > +                       opcode == FUSE_ATOMIC_CREATE) ? true : false;
> > +     if (alias)
> > +             *alias = NULL;
> >
> >       /* Userspace expects S_IFREG in create mode */
> > -     BUG_ON((mode & S_IFMT) != S_IFREG);
> > +     BUG_ON(create_op && (mode & S_IFMT) != S_IFREG);
> >
> >       forget = fuse_alloc_forget();
> >       err = -ENOMEM;
> > @@ -553,7 +557,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >       if (!ff)
> >               goto out_put_forget_req;
> >
> > -     if (!fm->fc->dont_mask)
> > +     if (!fc->dont_mask)
> >               mode &= ~current_umask();
> >
> >       flags &= ~O_NOCTTY;
> > @@ -642,8 +646,9 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >                               err = PTR_ERR(res);
> >                               goto out_err;
> >                       }
> > -                     /* res is expected to be NULL since its REG file */
> > -                     WARN_ON(res);
> > +                     entry = res;
> > +                     if (alias)
> > +                             *alias = res;
> >               }
> >       }
> >       fuse_change_entry_timeout(entry, &outentry);
> > @@ -681,8 +686,8 @@ static int fuse_atomic_create(struct inode *dir, struct dentry *entry,
> >       if (fc->no_atomic_create)
> >               return -ENOSYS;
> >
> > -     err = fuse_create_open(dir, entry, file, flags, mode,
> > -                            FUSE_ATOMIC_CREATE);
> > +     err = fuse_atomic_do_common(dir, entry, NULL, file, flags, mode,
> > +                                 FUSE_ATOMIC_CREATE);
> >       /* If atomic create is not implemented then indicate in fc so that next
> >        * request falls back to normal create instead of going into libufse and
> >        * returning with -ENOSYS.
> > @@ -694,6 +699,29 @@ static int fuse_atomic_create(struct inode *dir, struct dentry *entry,
> >       return err;
> >  }
> >
> > +static int fuse_do_atomic_open(struct inode *dir, struct dentry *entry,
> > +                             struct dentry **alias, struct file *file,
> > +                             unsigned int flags, umode_t mode)
> > +{
> > +     int err;
> > +     struct fuse_conn *fc = get_fuse_conn(dir);
> > +
> > +     if (!fc->do_atomic_open)
> > +             return -ENOSYS;
> > +
> > +     err = fuse_atomic_do_common(dir, entry, alias, file, flags, mode,
> > +                                 FUSE_ATOMIC_OPEN);
> > +     /* Atomic open imply atomic trunc as well i.e truncate should be performed
> > +      * as part of atomic open call itself.
> > +      */
> > +     if (!fc->atomic_o_trunc) {
> > +             if (fc->do_atomic_open)
> > +                     fc->atomic_o_trunc = 1;
> > +     }
> > +
> > +     return err;
> > +}
> > +
> >  static int fuse_mknod(struct user_namespace *, struct inode *, struct dentry *,
> >                     umode_t, dev_t);
> >  static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> > @@ -702,12 +730,23 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> >  {
> >       int err;
> >       struct fuse_conn *fc = get_fuse_conn(dir);
> > -     struct dentry *res = NULL;
> > +     struct dentry *res = NULL, *alias = NULL;
> >       bool create = flags & O_CREAT ? true : false;
> >
> >       if (fuse_is_bad(dir))
> >               return -EIO;
> >
> > +     if (!create) {
> > +             err = fuse_do_atomic_open(dir, entry, &alias,
> > +                                       file, flags, mode);
>
> So this is the core change of behavior. As of now, if we are not doing
> create operation, we just lookup and return. But now you are doing
> open + lookup in a single operation. Interesting. I am not sure if
> it breaks anything in VFS.

Yes, here we are handling optimizing lookups for open calls. If atomic
open is implemented underlying we use it otherwise falls back to
normal open.

>
> > +             res = alias;
> > +             if (!err || err != -ENOSYS)
> > +                     goto out_dput;
> > +     }
> > +      /*
> > +       * ENOSYS fall back for open- user space does not have full
> > +       * atomic open.
> > +       */
>
> This ENOSYS stuff all the place is making these patches look very unclean
> to me. A part of me says that should we negotiate these as feature bits.
> Having said that feature bits are precious and should not be used for
> trivial purposes. Hmm..., may be we can make handle ENOSYS little better
> in general.

Actually atomic open is based upon init bits considering the 3rd patch
which optimizes lookups in d_revalidate(). I would see if I can clean
ENOSYS a bit.
>
> >       if ((!create || fc->no_atomic_create) && d_in_lookup(entry)) {
> >               res = fuse_lookup(dir, entry, 0);
> >               if (IS_ERR(res))
> > @@ -730,9 +769,14 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> >       /* Libfuse/user space has not implemented atomic create, therefore
> >        * fall back to normal create.
> >        */
> > -     if (err == -ENOSYS)
> > -             err = fuse_create_open(dir, entry, file, flags, mode,
> > -                                    FUSE_CREATE);
> > +     /* Atomic create+open operation
> > +      * If the filesystem doesn't support this, then fall back to separate
> > +      * 'mknod' + 'open' requests.
> > +      */
> > +     if (err == -ENOSYS) {
> > +             err = fuse_atomic_do_common(dir, entry, NULL, file, flags,
> > +                                         mode, FUSE_CREATE);
> > +     }
> >       if (err == -ENOSYS) {
> >               fc->no_create = 1;
> >               goto mknod;
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index d577a591ab16..24793b82303f 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -669,6 +669,9 @@ struct fuse_conn {
> >       /** Is open/release not implemented by fs? */
> >       unsigned no_open:1;
> >
> > +     /** Is atomic open implemented by fs ? */
> > +     unsigned do_atomic_open : 1;
> > +
> >       /** Is atomic create not implemented by fs? */
> >       unsigned no_atomic_create:1;
> >
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index ee846ce371d8..5f667de69115 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -1190,6 +1190,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> >                               fc->setxattr_ext = 1;
> >                       if (flags & FUSE_SECURITY_CTX)
> >                               fc->init_security = 1;
> > +                     if (flags & FUSE_DO_ATOMIC_OPEN)
> > +                             fc->do_atomic_open = 1;
>
> Hey you are negotiating FUSE_DO_ATOMIC_OPEN flag. If that's the case why
> do you have to deal with -ENOSYS in fuse_do_atomic_open(). You should
> be able to just check.
>
> if (!create && fc->do_atomic_open) {
>         fuse_do_atomic_open().
> }

You are right. This happens due to the reason that we have auto atomic
create check in atomic create but yes, we do not need ENOSYS here. I
will clean it.


> I have yet to check what happens if file does not exist.
>
> >               } else {
> >                       ra_pages = fc->max_read / PAGE_SIZE;
> >                       fc->no_lock = 1;
> > @@ -1235,7 +1237,7 @@ void fuse_send_init(struct fuse_mount *fm)
> >               FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
> >               FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
> >               FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_INIT_EXT |
> > -             FUSE_SECURITY_CTX;
> > +             FUSE_SECURITY_CTX | FUSE_DO_ATOMIC_OPEN;
> >  #ifdef CONFIG_FUSE_DAX
> >       if (fm->fc->dax)
> >               flags |= FUSE_MAP_ALIGNMENT;
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index e4b56004b148..ab91e391241a 100644
> > --- a/include/uapi/linux/fuse.h
> > +++ b/include/uapi/linux/fuse.h
> > @@ -391,6 +391,7 @@ struct fuse_file_lock {
> >  /* bits 32..63 get shifted down 32 bits into the flags2 field */
> >  #define FUSE_SECURITY_CTX    (1ULL << 32)
> >  #define FUSE_HAS_INODE_DAX   (1ULL << 33)
> > +#define FUSE_DO_ATOMIC_OPEN  (1ULL << 34)
> >
> >  /**
> >   * CUSE INIT request/reply flags
> > @@ -540,6 +541,7 @@ enum fuse_opcode {
> >       FUSE_REMOVEMAPPING      = 49,
> >       FUSE_SYNCFS             = 50,
> >       FUSE_ATOMIC_CREATE      = 51,
> > +     FUSE_ATOMIC_OPEN        = 52,
> >
> >       /* CUSE specific operations */
> >       CUSE_INIT               = 4096,
> > --
> > 2.17.1
> >
>