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
>
