On 22.12.2021 11:58, Namjae Jeon wrote: > 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. 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
