Re: [PATCH 2/4] fuse: Create helper function if DIO write needs exclusive lock

[Amir Goldstein <amir73il@gmail.com>]

On Sun, Dec 24, 2023 at 12:49 PM Bernd Schubert <bschubert@ddn.com> wrote:
>
> This makes the code a bit easier to read and allows to more
> easily add more conditions when an exclusive lock is needed.
>
> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/fuse/file.c | 64 +++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 45 insertions(+), 19 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 174aa16407c4b..546254aaab19f 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1298,6 +1298,45 @@ static ssize_t fuse_perform_write(struct kiocb *iocb, struct iov_iter *ii)
>         return res;
>  }
>
> +static bool fuse_io_past_eof(struct kiocb *iocb, struct iov_iter *iter)
> +{
> +       struct inode *inode = file_inode(iocb->ki_filp);
> +
> +       return iocb->ki_pos + iov_iter_count(iter) > i_size_read(inode);
> +}
> +
> +/*
> + * @return true if an exclusive lock for direct IO writes is needed
> + */
> +static bool fuse_dio_wr_exclusive_lock(struct kiocb *iocb, struct iov_iter *from)
> +{
> +       struct file *file = iocb->ki_filp;
> +       struct fuse_file *ff = file->private_data;
> +       struct inode *inode = file_inode(iocb->ki_filp);
> +
> +       /* server side has to advise that it supports parallel dio writes */
> +       if (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES))
> +               return true;
> +
> +       /* append will need to know the eventual eof - always needs an
> +        * exclusive lock
> +        */
> +       if (iocb->ki_flags & IOCB_APPEND)
> +               return true;
> +
> +       /* combination opf page access and direct-io difficult, shared
> +        * locks actually introduce a conflict.
> +        */
> +       if (get_fuse_conn(inode)->direct_io_allow_mmap)
> +               return true;
> +
> +       /* parallel dio beyond eof is at least for now not supported */
> +       if (fuse_io_past_eof(iocb, from))
> +               return true;
> +
> +       return false;
> +}
> +
>  static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  {
>         struct file *file = iocb->ki_filp;
> @@ -1557,26 +1596,12 @@ 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);
> -
> -       return 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 exclusive_lock =
> -               !(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES) ||
> -               get_fuse_conn(inode)->direct_io_allow_mmap ||
> -               iocb->ki_flags & IOCB_APPEND ||
> -               fuse_direct_write_extending_i_size(iocb, from);
> +       bool exclusive_lock = fuse_dio_wr_exclusive_lock(iocb, from);
>
>         /*
>          * Take exclusive lock if
> @@ -1590,10 +1615,10 @@ static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from)
>         else {
>                 inode_lock_shared(inode);
>
> -               /* A race with truncate might have come up as the decision for
> -                * the lock type was done without holding the lock, check again.
> +               /*
> +                * Previous check was without any lock and might have raced.
>                  */
> -               if (fuse_direct_write_extending_i_size(iocb, from)) {
> +               if (fuse_dio_wr_exclusive_lock(iocb, from)) {
>                         inode_unlock_shared(inode);
>                         inode_lock(inode);
>                         exclusive_lock = true;
> @@ -2467,7 +2492,8 @@ static int fuse_file_mmap(struct file *file, struct vm_area_struct *vma)
>                 return fuse_dax_mmap(file, vma);
>
>         if (ff->open_flags & FOPEN_DIRECT_IO) {
> -               /* Can't provide the coherency needed for MAP_SHARED
> +               /*
> +                * Can't provide the coherency needed for MAP_SHARED
>                  * if FUSE_DIRECT_IO_ALLOW_MMAP isn't set.
>                  */
>                 if ((vma->vm_flags & VM_MAYSHARE) && !fc->direct_io_allow_mmap)
> --
> 2.40.1
>