Subject: Re: [PATCH] ksmbd: use F_SETLK to force vfs_file_lock() to return
 asynchronously
To:     Namjae Jeon <linkinjeon@kernel.org>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steve French <sfrench@samba.org>,
        Hyunchul Lee <hyc.lee@gmail.com>, kernel@openvz.org,
        linux-cifs@vger.kernel.org, linux-kernel@vger.kernel.org
References: <e2aef4e7-a9b9-e44e-94a2-29ed6bc20091@virtuozzo.com>
 <CAKYAXd8pCfUBPRXT-44N_g2GD_BKtjboiepgTGz5DQ93YEiz-A@mail.gmail.com>
 <bf92bd1f-d03b-1fb6-ff62-53cca4b441e8@virtuozzo.com>
 <CAKYAXd_z9i6VtMsHmR_FQDwBzGHcjHLv=zicAsddjur=_A071g@mail.gmail.com>
 <f210483a-69f7-1983-65cf-f3f5bd4112ac@virtuozzo.com>
 <CAKYAXd97orfDc7VA+fae7tLjTufMksQTepYKazY=gX7UkG0+rw@mail.gmail.com>
From:   Vasily Averin <vvs@virtuozzo.com>
Message-ID: <c6fcbdb7-f232-f888-674b-7a45d98ad92a@virtuozzo.com>
Date:   Wed, 22 Dec 2021 09:51:14 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <CAKYAXd97orfDc7VA+fae7tLjTufMksQTepYKazY=gX7UkG0+rw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NTdqcEhRTDNoYnoyWCtOWXF6d3oweloxNTBGZi9kK1d4Znl3cWNNMG56Njhv?=
 =?utf-8?B?NnFZRFR4SWkyVkJTM1laREpIdFpNdWdVVCtDQmRna1RYWUdCMDJ2TzBMTFAx?=
 =?utf-8?B?SS9GZXBzMUNqRUVYY3BGeDlLWkFSWEZVcmpiUEswdUZNSGVNUWhiOXZzT2lN?=
 =?utf-8?B?QVB3clRMSi82ZmozNHdzSE5ORUg2VWtDME5OeHdFS0dWa3JHZzNDRlpVSkpS?=
 =?utf-8?B?eGtPK3R3MzRBZ2RjYjZhNm1OQXQ2UUFjN3RhZ1lwYXdaQVFzQ2tDTEpIclR5?=
 =?utf-8?B?anJzb1gzRHNuUFkrZTBRUzFtZHl6d3lId0FwVXpkcjlVUlZLSFZLOHhzOFpJ?=
 =?utf-8?B?aGkyVWk3T3FTMm92L0NpZ3QxMFhGL2I1MSs2dGpSa000bE9OUjdSL0RrOFI2?=
 =?utf-8?B?RndLamdvblAvZEpaR0JDaTFXalZDQklycXgzVTBwU1FPR3NnZ2ZXK3kvV29m?=
 =?utf-8?B?emx4YkZFeDdqakEyYTBRT05DVnQ3eEdaWjhmdGduYzlHaXFUQnBoK3BoNjli?=
 =?utf-8?B?V2hFZno2Tmc1ZWpiV3JydmhJVWZ1SFIrcWZIWDJ4TzRRaUNSWmtQdXVHTDZT?=
 =?utf-8?B?bi9nSzhvQjdNakptak9ObnI5ek5EVFhFRHlhZm1hODFJUWRqZ0U4dDFyajhP?=
 =?utf-8?B?Q2NQSy9BVWR0MkhZOElDdTBkbVdMZXUvcHhqWURvQXZiM1JySmhIdVNaQkhU?=
 =?utf-8?B?SGo0S2RHbE9pTVRRc2h0d3pZYnRmWkxETDBMeHJjbGhmdkZ2Q0owZ3duVUs1?=
 =?utf-8?B?bWJDa25NMTNtNWVrSzNSMzJNWVBoYm5ISXZzdHRzK2krcjdFTXE3dXNUVFZn?=
 =?utf-8?B?SGpaa3lwYm9FZER6b0k4NDlDTlpreDFLQUIwMHVtSGZqWkhKSDhhM1ArQXBS?=
 =?utf-8?B?ejJockFVVEZBK1pHSGVqcU9RWkRSOWVlcXdIdlgwRW9FOHFTNW9INGdBUnp2?=
 =?utf-8?B?elg2Rkk0STFQd3hNQzBRcGl6QXZEYVIwRWMyT1kwTFFZRWN0VzZhT1dBVUpB?=
 =?utf-8?B?K1dmUVY5ZHFGT0lnbiticUFUdldmdWI3YU5ONmxiWXhaenR4dllzd3pTWkFm?=
 =?utf-8?B?ZjV3Mm96Wkk3SUM2RVZ4QXJqcndva1FTeVY3ZlFnUWQ0L3A5cDFNMXFsQ2Rv?=
 =?utf-8?B?VEJoam41RGFWS0hBYTVZY0NKeVRsaVFCRmZzRDhNTy92dXpBbmNBQ0VqZnhm?=
 =?utf-8?B?VDBzRkVEMmRhM3BoNUVndVJwRHNQU1ZEOTQ1UWN4OWZnWHZhN2JQYXdBR2p4?=
 =?utf-8?B?WWVudXJyaEQ1d3pOanVFZmpTMnFkQ1ZFM2JIbGVVY3RLbEtOekZBT3daM25k?=
 =?utf-8?B?eGZwY2Jqdm9XR0FJdzZJbVF4S1Jod3BIVGJxYVlna1A5WFN4ZCtrOHlxajZP?=
 =?utf-8?B?WElYRUE4S3IzWU10SHo4Q1JjR1hQSUFkNVZSWUIzVUhBUktZQlZCVURJd21y?=
 =?utf-8?B?VWJpLytockp0VFRNOVdQb0hOZDZ1SXBIY3h5bTFnSWFITW1LMitma0RjUjVY?=
 =?utf-8?B?YWdMMWFkL1A3dExmNmNIMytjaW80TW5zT0VINkUzd09RazA1T29QdWI4cGNU?=
 =?utf-8?B?Z2RjSEdFNG9YZ1F3bWc0N3ZsQWw2aE9QTUw2dS9Wa0FZbWJlYVNlcWVRc3JG?=
 =?utf-8?B?UmwzRFFZN29LZjYzMXRNRkplQUcyZW9Td0ErTWFyR1owQ09HOHp5N0F5OFJn?=
 =?utf-8?B?SHVFeFVrYzlLa2tIYU1lbG10b1JqL3ZNOE52VzBYM1MyZzBPVVhjc0lCUDl4?=
 =?utf-8?B?OUxhOUM5UXMxM25YSEkyblZMSHRZVkVpdVBNSHl3YXh4clFRYmhPbW1INzBy?=
 =?utf-8?B?UlNIKzJzUTZqZVpMcCtqayt5WHJ4ZWdMRGY2eFE3T3BKY3VOaitXclpZV0dt?=
 =?utf-8?B?WUR2Qm82eG9KNVZPQi9kVXpLdzFNa2VwTEZxUERyZk1YZVluMGVlU3J6eEx6?=
 =?utf-8?B?ZnpoOTNpSEc1VWhYU3JZME8wWEw5K210ZkRmRFViT1ljdGJxc1JmRHdKTi9G?=
 =?utf-8?B?WkxacG1SLzlhY3FUamlJMlF3MnovSVJxY1JRcVhYUUNITCtscCtlR2VYb1Jh?=
 =?utf-8?B?ZldsNis1MkZZcUZISFRBSkZNVWhSdFhrUnpsR3JqQVVlTEZHZTJrSmM3TStC?=
 =?utf-8?B?QUIrdldJS2s0eTg0aEJDWG1zQTNYZHY5eWFHd0FxSlNIOWFjSG5VUnhOalNH?=
 =?utf-8?Q?z1AaFoCun/8rrpb+BRgzKoY=3D?=
X-OriginatorOrg: virtuozzo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7b5867ef-605e-4a5e-a89a-08d9c5177338
X-MS-Exchange-CrossTenant-AuthSource: DB9PR08MB6619.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Dec 2021 06:51:16.1899
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0bc7f26d-0264-416e-a6fc-8352af79c58f
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: bb/rlNpxo9N7agOiPenLk6bg9S09PxqROE0/yKZ48+cpBAUhPtKTYLh7g7nik6A+ZcQXs6a++qWnPh6oa+Jy4A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR08MB6699
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>