Re: [PATCH v2 1/1] Allow non-extending parallel direct writes

[Miklos Szeredi]

On Mon, 9 May 2022 at 12:59, Dharmendra Singh <dharamhans87@xxxxxxxxx> wrote:
>
> From: Dharmendra Singh <dsingh@xxxxxxx>
>
> In general, as of now, in FUSE, direct writes on the same file are
> serialized over inode lock i.e we hold inode lock for the full duration
> of the write request. I could not found in fuse code a comment which
> clearly explains why this exclusive lock is taken for direct writes.
> Our guess is some USER space fuse implementations might be relying
> on this lock for seralization and also it protects for the issues
> arising due to file size assumption or write failures.  This patch
> relaxes this exclusive lock in some cases of direct writes.
>
> With these changes, we allows non-extending parallel direct writes
> on the same file with the help of a flag called FOPEN_PARALLEL_WRITES.
> If this flag is set on the file (flag is passed from libfuse to fuse
> kernel as part of file open/create), we do not take exclusive lock instead
> use shared lock so that all non-extending writes can run in parallel.
>
> Best practise would be to enable parallel direct writes of all kinds
> including extending writes as well but we see some issues such as
> when one write completes and other fails, how we should truncate(if
> needed) the file if underlying file system does not support holes
> (For file systems which supports holes, there might be a possibility
> of enabling parallel writes for all cases).
>
> FUSE implementations which rely on this inode lock for serialisation
> can continue to do so and this is default behaviour i.e no parallel
> direct writes.
>
> Signed-off-by: Dharmendra Singh <dsingh@xxxxxxx>
> ---
>  fs/fuse/file.c            | 45 ++++++++++++++++++++++++++++++++++++---
>  include/uapi/linux/fuse.h |  2 ++
>  2 files changed, 44 insertions(+), 3 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 829094451774..495138a68306 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1541,14 +1541,48 @@ static ssize_t fuse_direct_read_iter(struct kiocb *iocb, struct iov_iter *to)
>         return res;
>  }
>
> +static bool fuse_direct_write_extending_i_size(struct kiocb *iocb,
> +                                              struct iov_iter *iter)
> +{
> +       struct inode *inode = file_inode(iocb->ki_filp);
> +       loff_t i_size;
> +       loff_t offset;
> +       size_t count;
> +
> +       if (iocb->ki_flags & IOCB_APPEND)
> +               return true;
> +
> +       offset = iocb->ki_pos;
> +       count = iov_iter_count(iter);
> +       i_size = i_size_read(inode);
> +
> +       return offset + count <= i_size ? false : true;
> +}

This could be rewritten in much fewer lines:

static bool fuse_is_extending_write(struct kiocb *iocb, struct iov_iter *iter)
{
    struct inode *inode = file_inode(iocb->ki_filp);

    return (iocb->ki_flags & IOCB_APPEND) ||
        iocb->ki_pos + iov_iter_count(iter) > i_size_read(inode);
}


> +
>  static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  {
>         struct inode *inode = file_inode(iocb->ki_filp);
> +       struct file *file = iocb->ki_filp;
> +       struct fuse_file *ff = file->private_data;
>         struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(iocb);
>         ssize_t res;
> +       bool p_write = ff->open_flags & FOPEN_PARALLEL_WRITES ? true : false;

Please just use "bool v = expr" instead of "bool v = expr ? true :
false" as they are equivalent.

> +       bool exclusive_lock = !p_write ||
> +                              fuse_direct_write_extending_i_size(iocb, from) ?
> +                              true : false;

Same.

> +
> +       /*
> +        * Take exclusive lock if
> +        * - parallel writes are disabled.
> +        * - parallel writes are enabled and i_size is being extended
> +        * Take shared lock if
> +        * - parallel writes are enabled but i_size does not extend.
> +        */
> +       if (exclusive_lock)
> +               inode_lock(inode);
> +       else
> +               inode_lock_shared(inode);
>
> -       /* Don't allow parallel writes to the same file */
> -       inode_lock(inode);
>         res = generic_write_checks(iocb, from);
>         if (res > 0) {
>                 if (!is_sync_kiocb(iocb) && iocb->ki_flags & IOCB_DIRECT) {
> @@ -1559,7 +1593,10 @@ static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from)
>                         fuse_write_update_attr(inode, iocb->ki_pos, res);
>                 }
>         }
> -       inode_unlock(inode);
> +       if (exclusive_lock)
> +               inode_unlock(inode);
> +       else
> +               inode_unlock_shared(inode);
>
>         return res;
>  }
> @@ -2900,7 +2937,9 @@ fuse_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>         kref_put(&io->refcnt, fuse_io_release);
>
>         if (iov_iter_rw(iter) == WRITE) {
> +

Unnecessary empty line.

>                 fuse_write_update_attr(inode, pos, ret);
> +               /* For extending writes we already hold exclusive lock */
>                 if (ret < 0 && offset + count > i_size)
>                         fuse_do_truncate(file);
>         }
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index d6ccee961891..ee5379d41906 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -301,6 +301,7 @@ struct fuse_file_lock {
>   * FOPEN_CACHE_DIR: allow caching this directory
>   * FOPEN_STREAM: the file is stream-like (no file position at all)
>   * FOPEN_NOFLUSH: don't flush data cache on close (unless FUSE_WRITEBACK_CACHE)
> + * FOPEN_PARALLEL_WRITES: Allow concurrent writes on the same inode
>   */
>  #define FOPEN_DIRECT_IO                (1 << 0)
>  #define FOPEN_KEEP_CACHE       (1 << 1)
> @@ -308,6 +309,7 @@ struct fuse_file_lock {
>  #define FOPEN_CACHE_DIR                (1 << 3)
>  #define FOPEN_STREAM           (1 << 4)
>  #define FOPEN_NOFLUSH          (1 << 5)
> +#define FOPEN_PARALLEL_WRITES  (1 << 6)
>
>  /**
>   * INIT request/reply flags
> --
> 2.17.1
>