[Devel] [PATCH vz7] fuse: relax i_mutex coverage in fuse_fsync

[Alexey Kuznetsov]

Hello!

I do not think you got it right.

i_mutex in fsync is not about some atomicity,
it is about stopping data feed while fsync is executed
to prevent livelock.

I cannot tell anything about mtime update, it is just some voodoo
magic for me.

What's about fsync semantics, I see two different ways:

A.

1. Remove useless write_inode_now. Its work is done
    by filemap_write_and_wait_range(), there is no need to repeat it
under mutex.
2. move mutex_lock _after_  fuse_sync_writes(), which is essentially
    fuse continuation forfilemap_write_and_wait_range().
3. i_mutex is preserved only around fsync call.

B.
1. Remove  write_inode_now as well.
2. Remove i_mutex _completely_. (No idea about mtime voodo though)
2. Replace fuse_sync_writes() with fuse_set_nowrite()
    and add release after call to FSYNC.

Both prevent livelock. B is obviosly optimal.

But A preserves historic fuse protocol semantics.
F.e. I have no idea would user space survive truncate
racing with fsync. pstorage should survice, though this
path was never tested.





On Wed, Nov 30, 2016 at 4:02 AM, Maxim Patlasov <mpatlasov at virtuozzo.com> wrote:
> fuse_fsync_common() does need i_mutex for fuse_sync_writes() and
> fuse_flush_mtime(). But when those operations are done, it's actually
> doesn't matter whether to hold the lock over fuse_request_send(FUSE_FSYNC)
> or not: we ensured that all relevant data were already seen by
> userspace fuse daemon, and so it will sync them (by handling FUSE_FSYNC)
> anyway; if the user screws up by leaking new data updates in-between, it
> is up to the user and doesn't violate fsync(2) semantics.
>
> https://jira.sw.ru/browse/PSBM-55919
>
> Signed-off-by: Maxim Patlasov <mpatlasov at virtuozzo.com>
> ---
>  fs/fuse/file.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 464b2f5..559dfd9 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -697,6 +697,8 @@ int fuse_fsync_common(struct file *file, loff_t start, loff_t end,
>                 goto out;
>         }
>
> +       mutex_unlock(&inode->i_mutex);
> +
>         memset(&inarg, 0, sizeof(inarg));
>         inarg.fh = ff->fh;
>         inarg.fsync_flags = datasync ? 1 : 0;
> @@ -715,6 +717,7 @@ int fuse_fsync_common(struct file *file, loff_t start, loff_t end,
>                         fc->no_fsync = 1;
>                 err = 0;
>         }
> +       return err;
>  out:
>         mutex_unlock(&inode->i_mutex);
>         return err;
>