Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <154149743811.18189.2448546280541143340.stgit@localhost.localdomain>
References: <154149673368.18189.14752124997476502777.stgit@localhost.localdomain>
 <154149743811.18189.2448546280541143340.stgit@localhost.localdomain>
From:   Miklos Szeredi <miklos@szeredi.hu>
Date:   Wed, 7 Nov 2018 15:12:15 +0100
Message-ID: <CAJfpegspUywVumOs-HcJVdm7CiGAHeNGfn4Kqg0dLup_5ufi8Q@mail.gmail.com>
Subject: Re: [PATCH 4/6] fuse: Implement fuse_attr_version_inc()
To:     Kirill Tkhai <ktkhai@virtuozzo.com>
Cc:     linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Nov 6, 2018 at 10:43 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> For cleanup purpose archive repeating pattern:
>
>         spin_lock(&fc->lock);
>         fi->attr_version = ++fc->attr_version;
>         spin_unlock(&fc->lock);
>
> into separate function.
>
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  fs/fuse/dir.c    |    8 ++------
>  fs/fuse/file.c   |   16 ++++------------
>  fs/fuse/fuse_i.h |   11 +++++++++++
>  fs/fuse/inode.c  |    4 +---
>  4 files changed, 18 insertions(+), 21 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 794b9f48fb8e..35f3b3d1e044 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -674,9 +674,7 @@ static int fuse_unlink(struct inode *dir, struct dentry *entry)
>                 struct fuse_inode *fi = get_fuse_inode(inode);
>
>                 spin_lock(&fi->lock);
> -               spin_lock(&fc->lock);
> -               fi->attr_version = ++fc->attr_version;
> -               spin_unlock(&fc->lock);
> +               fi->attr_version = fuse_attr_version_inc_return(fc);
>                 /*
>                  * If i_nlink == 0 then unlink doesn't make sense, yet this can
>                  * happen if userspace filesystem is careless.  It would be
> @@ -830,9 +828,7 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
>                 struct fuse_inode *fi = get_fuse_inode(inode);
>
>                 spin_lock(&fi->lock);
> -               spin_lock(&fc->lock);
> -               fi->attr_version = ++fc->attr_version;
> -               spin_unlock(&fc->lock);
> +               fi->attr_version = fuse_attr_version_inc_return(fc);
>                 inc_nlink(inode);
>                 spin_unlock(&fi->lock);
>                 fuse_invalidate_attr(inode);
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 48e2889ba6a6..1164d3580c62 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -186,9 +186,7 @@ void fuse_finish_open(struct inode *inode, struct file *file)
>                 struct fuse_inode *fi = get_fuse_inode(inode);
>
>                 spin_lock(&fi->lock);
> -               spin_lock(&fc->lock);
> -               fi->attr_version = ++fc->attr_version;
> -               spin_unlock(&fc->lock);
> +               fi->attr_version = fuse_attr_version_inc_return(fc);
>                 i_size_write(inode, 0);
>                 spin_unlock(&fi->lock);
>                 fuse_invalidate_attr(inode);
> @@ -604,9 +602,7 @@ static void fuse_aio_complete(struct fuse_io_priv *io, int err, ssize_t pos)
>                         struct fuse_inode *fi = get_fuse_inode(inode);
>
>                         spin_lock(&fi->lock);
> -                       spin_lock(&fc->lock);
> -                       fi->attr_version = ++fc->attr_version;
> -                       spin_unlock(&fc->lock);
> +                       fi->attr_version = fuse_attr_version_inc_return(fc);
>                         spin_unlock(&fi->lock);
>                 }
>
> @@ -685,9 +681,7 @@ static void fuse_read_update_size(struct inode *inode, loff_t size,
>         spin_lock(&fi->lock);
>         if (attr_ver == fi->attr_version && size < inode->i_size &&
>             !test_bit(FUSE_I_SIZE_UNSTABLE, &fi->state)) {
> -               spin_lock(&fc->lock);
> -               fi->attr_version = ++fc->attr_version;
> -               spin_unlock(&fc->lock);
> +               fi->attr_version = fuse_attr_version_inc_return(fc);
>                 i_size_write(inode, size);
>         }
>         spin_unlock(&fi->lock);
> @@ -1006,9 +1000,7 @@ bool fuse_write_update_size(struct inode *inode, loff_t pos)
>         bool ret = false;
>
>         spin_lock(&fi->lock);
> -       spin_lock(&fc->lock);
> -       fi->attr_version = ++fc->attr_version;
> -       spin_unlock(&fc->lock);
> +       fi->attr_version = fuse_attr_version_inc_return(fc);
>         if (pos > inode->i_size) {
>                 i_size_write(inode, pos);
>                 ret = true;
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 075da40499b8..f7442bcecbb0 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -775,6 +775,17 @@ static inline int invalid_nodeid(u64 nodeid)
>         return !nodeid || nodeid == FUSE_ROOT_ID;
>  }
>
> +static inline u64 fuse_attr_version_inc_return(struct fuse_conn *fc)
> +{
> +       u64 attr_version;
> +
> +       spin_lock(&fc->lock);
> +       attr_version = ++fc->attr_version;
> +       spin_unlock(&fc->lock);
> +
> +       return attr_version;
> +}

atomic64_t?

And move this patch one up, 3/6 is complicated as is without having to
add extra locking around fc->attr_version.


> +
>  /** Device operations */
>  extern const struct file_operations fuse_dev_operations;
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index f62d45686b42..5f488b019cd9 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -167,9 +167,7 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
>
>         lockdep_assert_held(&fi->lock);
>
> -       spin_lock(&fc->lock);
> -       fi->attr_version = ++fc->attr_version;
> -       spin_unlock(&fc->lock);
> +       fi->attr_version = fuse_attr_version_inc_return(fc);
>
>         fi->i_time = attr_valid;
>         WRITE_ONCE(fi->inval_mask, 0);
>