2021-12-22 15:51 GMT+09:00, Vasily Averin <vvs@xxxxxxxxxxxxx>:
> On 22.12.2021 08:25, Namjae Jeon wrote:
>> 2021-12-22 13:32 GMT+09:00, Vasily Averin <vvs@xxxxxxxxxxxxx>:
>>> On 22.12.2021 05:50, Namjae Jeon wrote:
>>>> 2021-12-21 22:08 GMT+09:00, Vasily Averin <vvs@xxxxxxxxxxxxx>:
>>>>> On 21.12.2021 15:02, Namjae Jeon wrote:
>>>>>> 2021-12-19 18:34 GMT+09:00, Vasily Averin <vvs@xxxxxxxxxxxxx>:
>>>>>>> To avoid possible deadlock ksmbd should process locks
>>>>>>> asynchronously.
>>>>>>> Callers expecting vfs_file_locks() to return asynchronously should
>>>>>>> only
>>>>>>> use F_SETLK, not F_SETLKW.
>>>>>> Should I check this patch instead of
>>>>>> [PATCH] ksmbd: force "fail immediately" flag on fs with its own
>>>>>> ->lock
>>>>>> ?
>>>>>
>>>>> no, these patches are independent and both ones are required.
>>>>> current patch fixes incorrect kernel thread behaviour:
>>>>> kernel threads should not use F_SETLKW for locking requests.
>>>> How does this patch work? posix_lock_file in vfs_lock_file() does not
>>>> use
>>>> cmd.
>>>> And your patch still leaves FL_SLEEP.
>>>
>>> "use F_SETLK, not F_SETLKW" was copy-pasted from requirement described
>>> in
>>> comment above vfs_lock_file().
>>>
>>> posix_lock_file() is not used in all ->lock() functions, and use
>>> F_SETLKW
>>> forces some of affected filesystem use blocking locks:
>> What I'm saying is that when we apply "ksmbd: force "fail immediately"
>> flag on fs with its own ->lock ", this patch is meaningless. How is
>> ->lock() with F_SETLKW called?
>
> I got your point finally,
> yes, you are right, now this cannot happen.
> However I'm going to fix all affected filesystems and then revert
> "ksmbd: force "fail immediately" flag on fs with its own ->lock"
> When this happen and ksmbd will still use IS_SETLKW it will trigger the
> problems described below.
If so, You can include one patch(this patch + revert patch) in patch
series for fixing ->lock of all filesystem. But I can still not
understand why we need to revert the patch and apply this patch.
Maybe, I need to check your next patches.
Thanks!
>
> Thank you,
> Vasily Averin
>
>>> fs/ceph/locks.c::ceph_lock()
>>> /* set wait bit as appropriate, then make command as Ceph
>>> expects
>>> it*/
>>> if (IS_GETLK(cmd))
>>> op = CEPH_MDS_OP_GETFILELOCK;
>>> else if (IS_SETLKW(cmd))
>>> wait = 1
>>>
>>> nfs v3 handles it in nlmclnt_proc
>>> fs/lockd/clntproc.c::nlmclnt_proc
>>> if (IS_SETLK(cmd) || IS_SETLKW(cmd)) {
>>> if (fl->fl_type != F_UNLCK) {
>>> call->a_args.block = IS_SETLKW(cmd) ? 1 : 0;
>>>
>>>
>>> nvs v4 handles it in nfs4_retry_setlk()
>>> fs/nfs/nfs4proc.c()::nfs4_retry_setlk()
>>> while(!signalled()) {
>>> status = nfs4_proc_setlk(state, cmd, request);
>>> if ((status != -EAGAIN) || IS_SETLK(cmd))
>>> break;
>>>
>>> gfs2_lock and ocfs calls dlm_posix_lock()
>>> dlm_posix_lock::dlm_posix_lock()
>>> op->info.wait = IS_SETLKW(cmd)
>>>
>>> So if ksmbd will try to export these file systems it can be deadlocked
>>> on
>>> blocking locks processing,
>>> even with my patch dropped FL_SLEEP.
>>>
>>> To be honest, some other filesystems, contrary, ignores cmd and handles
>>> FL_SLEEP instead:
>>> cifs_lock and fuse_setlk. Moreover, locks_lock_file_wait() is widely
>>> used
>>> too,
>>> (and can block if FL_SLEEP is set). Some of these cases looks like bugs,
>>> its careful processing requires some time, therefore right now, to
>>> quickly
>>> work around
>>> all these cases kernel export threads (nfsd/lockd/ksmbd) can drop to
>>> FL_SLEEP flag).
>>>
>>> I think it makes sense to create new bug on bugzilla.kernel.org, explain
>>> all
>>> details of this problem,
>>> and keep you informed about progress with filesystems fixes. When all
>>> file
>>> systems will be fixed,
>>> it allows you to revert "ksmbd: force "fail immediately" flag on fs with
>>> its
>>> own ->lock"
>>>
>>> Thank you,
>>> Vasily Averin
>>>
>>>>> "[PATCH] ksmbd: force "fail immediately" flag on fs with its own
>>>>> ->lock"
>>>>> tries to workaround the incorrect behaviour of some exported
>>>>> filesystems.
>>>>>
>>>>> Currently this way is used in nfsd and lockd, however it is not fully
>>>>> correct,
>>>>> and I still search some better solution, so perhaps
>>>>> '[PATCH] ksmbd: force "fail immediately" flag on fs with its own
>>>>> ->lock'
>>>>> will be dropped later.
>>>>>
>>>>> Thank you,
>>>>> Vasily Averin
>>>>>
>>>>>>> Signed-off-by: Vasily Averin <vvs@xxxxxxxxxxxxx>
>>>>>>> ---
>>>>>>> fs/ksmbd/smb2pdu.c | 4 ++--
>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>>>>>>> index 0c020deb76bb..34f333549767 100644
>>>>>>> --- a/fs/ksmbd/smb2pdu.c
>>>>>>> +++ b/fs/ksmbd/smb2pdu.c
>>>>>>> @@ -6646,13 +6646,13 @@ static int smb2_set_flock_flags(struct
>>>>>>> file_lock
>>>>>>> *flock, int flags)
>>>>>>> switch (flags) {
>>>>>>> case SMB2_LOCKFLAG_SHARED:
>>>>>>> ksmbd_debug(SMB, "received shared request\n");
>>>>>>> - cmd = F_SETLKW;
>>>>>>> + cmd = F_SETLK;
>>>>>>> flock->fl_type = F_RDLCK;
>>>>>>> flock->fl_flags |= FL_SLEEP;
>>>>>>> break;
>>>>>>> case SMB2_LOCKFLAG_EXCLUSIVE:
>>>>>>> ksmbd_debug(SMB, "received exclusive request\n");
>>>>>>> - cmd = F_SETLKW;
>>>>>>> + cmd = F_SETLK;
>>>>>>> flock->fl_type = F_WRLCK;
>>>>>>> flock->fl_flags |= FL_SLEEP;
>>>>>>> break;
>>>>>>> --
>>>>>>> 2.25.1
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>
>
