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

[Alexey Kuznetsov]

Sorry, missed that pair fuse_set_nowrite/fuse_release_writes
can be done only under i_mutex.

IMHO it is only due to bad implementation.
If fuse_set_nowrite would be done with separate
count instead of adding negative bias, it would
be possible.


On Wed, Nov 30, 2016 at 3:47 PM, Alexey Kuznetsov <kuznet at virtuozzo.com> wrote:
> 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;
>>