| Message ID | e2aef4e7-a9b9-e44e-94a2-29ed6bc20091@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ksmbd: use F_SETLK to force vfs_file_lock() to return asynchronously | expand |
2021-12-19 18:34 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: > 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 ? > > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> > --- > 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 > >
On 21.12.2021 15:02, Namjae Jeon wrote: > 2021-12-19 18:34 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >> 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. "[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@virtuozzo.com> >> --- >> 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 >> >>
2021-12-21 22:08 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: > On 21.12.2021 15:02, Namjae Jeon wrote: >> 2021-12-19 18:34 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>> 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. > > "[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@virtuozzo.com> >>> --- >>> 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 >>> >>> > >
On 22.12.2021 05:50, Namjae Jeon wrote: > 2021-12-21 22:08 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >> On 21.12.2021 15:02, Namjae Jeon wrote: >>> 2021-12-19 18:34 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>> 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: 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@virtuozzo.com> >>>> --- >>>> 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 >>>> >>>> >> >>
2021-12-22 13:32 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: > On 22.12.2021 05:50, Namjae Jeon wrote: >> 2021-12-21 22:08 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>> On 21.12.2021 15:02, Namjae Jeon wrote: >>>> 2021-12-19 18:34 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>> 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? > > 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@virtuozzo.com> >>>>> --- >>>>> 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 >>>>> >>>>> >>> >>> > >
On 22.12.2021 08:25, Namjae Jeon wrote: > 2021-12-22 13:32 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >> On 22.12.2021 05:50, Namjae Jeon wrote: >>> 2021-12-21 22:08 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>> On 21.12.2021 15:02, Namjae Jeon wrote: >>>>> 2021-12-19 18:34 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>>> 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. 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@virtuozzo.com> >>>>>> --- >>>>>> 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 >>>>>> >>>>>> >>>> >>>> >> >>
On 22.12.2021 09:51, Vasily Averin wrote: > On 22.12.2021 08:25, Namjae Jeon wrote: >> 2021-12-22 13:32 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>> On 22.12.2021 05:50, Namjae Jeon wrote: >>>> 2021-12-21 22:08 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>> On 21.12.2021 15:02, Namjae Jeon wrote: >>>>>> 2021-12-19 18:34 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>>>> 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. I've created https://bugzilla.kernel.org/show_bug.cgi?id=215383
2021-12-22 15:51 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: > On 22.12.2021 08:25, Namjae Jeon wrote: >> 2021-12-22 13:32 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>> On 22.12.2021 05:50, Namjae Jeon wrote: >>>> 2021-12-21 22:08 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>> On 21.12.2021 15:02, Namjae Jeon wrote: >>>>>> 2021-12-19 18:34 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>>>> 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@virtuozzo.com> >>>>>>> --- >>>>>>> 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 >>>>>>> >>>>>>> >>>>> >>>>> >>> >>> > >
On 22.12.2021 11:58, Namjae Jeon wrote: > 2021-12-22 15:51 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >> On 22.12.2021 08:25, Namjae Jeon wrote: >>> 2021-12-22 13:32 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>> On 22.12.2021 05:50, Namjae Jeon wrote: >>>>> 2021-12-21 22:08 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>>> On 21.12.2021 15:02, Namjae Jeon wrote: >>>>>>> 2021-12-19 18:34 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>>>>> 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. Ok. let's do it. > 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. 1) it is ideologically incorrect to call F_SETLKW from kernel thread: W here means "if a conflicting lock is held on the file, then wait for that lock to be released". However this can cause server deadlock. 2) nobody handles F_SETLKW cmd. It is set only if exported file systems does not define own ->lock() function, and so this request is processed by posix_lock_file() ignored cmd. So there is no sense to set it. 3) when all affected fileystem will be fixed, it will handle properly FL_SLEEP && F_SETLK combination. this will make unnecessary the force of SMB2_LOCKFLAG_FAIL_IMMEDIATELY and drop FL_SLEEP in ksmbd. Thank you, Vasily Averin
On 22.12.2021 18:17, Vasily Averin wrote: > On 22.12.2021 11:58, Namjae Jeon wrote: >> 2021-12-22 15:51 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>> On 22.12.2021 08:25, Namjae Jeon wrote: >>>> 2021-12-22 13:32 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>> On 22.12.2021 05:50, Namjae Jeon wrote: >>>>>> 2021-12-21 22:08 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>>>> On 21.12.2021 15:02, Namjae Jeon wrote: >>>>>>>> 2021-12-19 18:34 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>>>>>> 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. I've checked how smb2_lock() handles FILE_LOCK_DEFERRED returned by vfs_lock_file() call. It seems for me, working thread will be blocked in ksmbd_vfs_posix_lock_wait(), so whole ksmbd server still can deadlock. Am I wrong probably? Thank you, Vasily Averin
2021-12-24 21:31 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: > On 22.12.2021 18:17, Vasily Averin wrote: >> On 22.12.2021 11:58, Namjae Jeon wrote: >>> 2021-12-22 15:51 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>> On 22.12.2021 08:25, Namjae Jeon wrote: >>>>> 2021-12-22 13:32 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>>> On 22.12.2021 05:50, Namjae Jeon wrote: >>>>>>> 2021-12-21 22:08 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>>>>> On 21.12.2021 15:02, Namjae Jeon wrote: >>>>>>>>> 2021-12-19 18:34 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>>>>>>> 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. > > I've checked how smb2_lock() handles FILE_LOCK_DEFERRED returned by > vfs_lock_file() call. > It seems for me, working thread will be blocked in > ksmbd_vfs_posix_lock_wait(), > so whole ksmbd server still can deadlock. Am I wrong probably? No, Each commands are handled by ksmbd-io kworkers. Thanks! > > Thank you, > Vasily Averin >
On 25.12.2021 02:08, Namjae Jeon wrote: > 2021-12-24 21:31 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >> On 22.12.2021 18:17, Vasily Averin wrote: >>> On 22.12.2021 11:58, Namjae Jeon wrote: >>>> 2021-12-22 15:51 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>> On 22.12.2021 08:25, Namjae Jeon wrote: >>>>>> 2021-12-22 13:32 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>>>> On 22.12.2021 05:50, Namjae Jeon wrote: >>>>>>>> 2021-12-21 22:08 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>>>>>> On 21.12.2021 15:02, Namjae Jeon wrote: >>>>>>>>>> 2021-12-19 18:34 GMT+09:00, Vasily Averin <vvs@virtuozzo.com>: >>>>>>>>>>> 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. >> >> I've checked how smb2_lock() handles FILE_LOCK_DEFERRED returned by >> vfs_lock_file() call. >> It seems for me, working thread will be blocked in >> ksmbd_vfs_posix_lock_wait(), >> so whole ksmbd server still can deadlock. Am I wrong probably? > No, Each commands are handled by ksmbd-io kworkers. In this case ksmbd can do not require async lock processing and you can drop my previous patches. Is there any difference where thread will be blocked: inside ->lock() function of exported filesystem or in ksmbd_vfs_posix_lock_wait? I think it isn't. Thank you, Vasily Averin
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;
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. Signed-off-by: Vasily Averin <vvs@virtuozzo.com> --- fs/ksmbd/smb2pdu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)