On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@xxxxxxx> wrote:
>
> fuse_direct_write_iter is basically duplicating what is already
> in fuse_cache_write_iter/generic_file_direct_write. That can be
> avoided by setting IOCB_DIRECT in fuse_file_write_iter, after that
> fuse_cache_write_iter can be used for the FOPEN_DIRECT_IO code path
> and fuse_direct_write_iter can be removed.
>
> Cc: Hao Xu <howeyxu@xxxxxxxxxxx>
> Cc: Miklos Szeredi <miklos@xxxxxxxxxx>
> Cc: Dharmendra Singh <dsingh@xxxxxxx>
> Cc: linux-fsdevel@xxxxxxxxxxxxxxx
> Signed-off-by: Bernd Schubert <bschubert@xxxxxxx>
> ---
> fs/fuse/file.c | 54 ++++----------------------------------------------
> 1 file changed, 4 insertions(+), 50 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 905ce3bb0047..09277a54b711 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1589,52 +1589,6 @@ static ssize_t fuse_direct_read_iter(struct kiocb *iocb, struct iov_iter *to)
> return res;
> }
>
> -static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from)
> -{
> - struct inode *inode = file_inode(iocb->ki_filp);
> - struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(iocb);
> - ssize_t res;
> - bool exclusive_lock = fuse_dio_wr_exclusive_lock(iocb, from);
> -
> - /*
> - * Take exclusive lock if
> - * - Parallel direct writes are disabled - a user space decision
> - * - Parallel direct writes are enabled and i_size is being extended.
> - * This might not be needed at all, but needs further investigation.
> - */
> - if (exclusive_lock)
> - inode_lock(inode);
> - 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.
> - */
> - if (fuse_direct_write_extending_i_size(iocb, from)) {
> - inode_unlock_shared(inode);
> - inode_lock(inode);
> - exclusive_lock = true;
> - }
> - }
> -
> - res = generic_write_checks(iocb, from);
> - if (res > 0) {
> - if (!is_sync_kiocb(iocb) && iocb->ki_flags & IOCB_DIRECT) {
> - res = fuse_direct_IO(iocb, from);
> - } else {
> - res = fuse_direct_io(&io, from, &iocb->ki_pos,
> - FUSE_DIO_WRITE);
> - fuse_write_update_attr(inode, iocb->ki_pos, res);
While I think this is correct, I'd really like if the code to be
replaced and the replacement are at least somewhat comparable.
Currently fuse_direct_IO() handles all cases (of which are many since
the requester can be sync or async and the server can be sync or
async).
Could this mess be cleaned up somehow?
Also could we make the function names of fuse_direct_IO() and
fuse_direct_io() less similar, as this is a very annoying (though
minor) issue.
Thanks,
Miklos
