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>
 <c6fcbdb7-f232-f888-674b-7a45d98ad92a@virtuozzo.com>
 <CAKYAXd-zBBF_J-z1Ph4ruRD3Fb3EORsKfDjKw0yX42D0SCD_Ow@mail.gmail.com>
 <4e61a114-d55a-3c14-e019-83891c5b1f0a@virtuozzo.com>
 <846ff849-58a9-ff03-8144-d7d47775553b@virtuozzo.com>
 <CAKYAXd8=EoOnMHURFPfwvCHmit+Xg+tKdsjWN7wrEXDa53aVxg@mail.gmail.com>
From:   Vasily Averin <vvs@virtuozzo.com>
Message-ID: <567591e3-1f1a-f883-e610-4f99e982f4b2@virtuozzo.com>
Date:   Mon, 27 Dec 2021 10:08:58 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <CAKYAXd8=EoOnMHURFPfwvCHmit+Xg+tKdsjWN7wrEXDa53aVxg@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?OXZnTGw3dG8xUm4vcTFCUlMwNlg5cVZMVnJHNFpwUGxGR2xta1V2OUVQaU00?=
 =?utf-8?B?NEJwSTJOODZEeVRyalorMDV0SVd5cWJja0hZNlBGYmc3ckE4NkdxSmM1Nncv?=
 =?utf-8?B?SDdzeTc5VzB6bHA2STlUNzkrMHh2Y1d2UzJwY3cxYWhQL25rbjkwclJDNVFN?=
 =?utf-8?B?OUh4TTVMMmVqQTMvYW1QODFRb2Fpa2ROUlNFQ1lBSnV1bng5c0srNi9aVU02?=
 =?utf-8?B?c25HdWt4Q2VTeDViU1M1aTd2RVJWekU3M1RSdWd3K1hhOHhvYTZvemRMK0RS?=
 =?utf-8?B?bC83UEc2bXRIdTZWLy8zQU0wditmUGFFUmZEVGltM1AwbWFGNEV1YlBkTXFW?=
 =?utf-8?B?QjhzYUNaa3ZLMS9zZDNvMVZxbGdXclJRRGZLeGhqelBKOXRneUR0bzFpOFJ5?=
 =?utf-8?B?NlFpSFNUT0xMcFpvODRzcFp6RldYZDdmRHdaMUNzZk9nNmxlK0VMTEUrRmZ4?=
 =?utf-8?B?YTRNME5LamE4UGlzcmhwT2V6b29mS25JODVQbFVBUFk3Qlo4VERQMExmUDhy?=
 =?utf-8?B?UmVDZHhLK3pCSnZuNURPT053SzJBSjRPeFRTeXZ0VmJFam1RcU1wZi8wNXNY?=
 =?utf-8?B?a2R3QzBJa1R1Rkx2M1lqY1RkYzl4M093clZZbjlvbzVJaGFSUjM0WExkZFM5?=
 =?utf-8?B?ZHQweHlOaWRHYlFqaGpEdGZlTFp0dW1GRTUyM3hXbllRRnRFem5GbjI1bUEv?=
 =?utf-8?B?N1hvSzE5TFFYRHF1OFJ6UGNMU3ZiSHRNMmd5bGpybS9vWW8wc2ZUZktYektx?=
 =?utf-8?B?N1U1d3lQdFNHVEVxbjVnbkZNZDYwT0tDTEJSK0plam1KMkx5VUZNamFGS0hv?=
 =?utf-8?B?QUltL0VmaG4xbm56c0RRZkg5UHJDMlVJRVFlc0hvbkJlSExiOWREeTExa044?=
 =?utf-8?B?TnFKQWhjZVRRanpYOTVNQkRMdHBrWlVhTW9NRDVNc0JzcDQyb3hWNFFqL3lN?=
 =?utf-8?B?ZWE5VTY0Sm5jRXgwK1BObEFTdEY5YWxMdzREV0tGaWVhVTU5MWphc2lHb0Ez?=
 =?utf-8?B?ZWY0YVkvdXRCaWtKVk1tbXVhRWNxY3lSMUMyVGNtYXlna1MxY3ZpRFZIMVdZ?=
 =?utf-8?B?TEtqcUFBYW1zVURQd1UyNkNuRFRnT3A1S1B0NnpFNkkwTVVCdnhZb3VQY2Zh?=
 =?utf-8?B?MmE4Q3BBc1MyaVovRDd6VFBkWVRJU01KSnFwb1QyazN2Mjg5VUk0VlV3RE4r?=
 =?utf-8?B?ZDY2cFVCbEp6L21sM2h3a0xqcldMajE4ZXJBc3ZZSVFUbEQ5NHlHVUE4SkZJ?=
 =?utf-8?B?VUFHblZHSStrNC9palU0TURHYW9CVjNRZWlMOU12ZUhGc3NNZHIxLytaQVVY?=
 =?utf-8?B?Rjg4VXlvaHhrbERMd0ZQbGxVTllBbGRlSS9sSlRBTVhGUlBmL2tTMlN1dFlk?=
 =?utf-8?B?TmovZ1hDMXpnWmlGRDY1Q0JVOENnVWp1cHltL0xENnhrbys1amc4TW83MTNt?=
 =?utf-8?B?VnM3OXc5Z2VLWHpqQ0xVK3dnaTZxa0tRa3RMRTd0TStneU44QkQwNUFKU2VU?=
 =?utf-8?B?MU52cjNzVStwZW91dTB4RWcyd3p6SXB5d0k2N1hUa29jVmk5RHRrVWVKM1NR?=
 =?utf-8?B?ZUc3SnlYUG80TDMxWHFBT2RKQ3RiYUkyZHp1T3NIekFwU3R4b0dwUE55cVV0?=
 =?utf-8?B?eDlZbHhYRWJWZVo2WWVzSUZjRmhOL3RMeFFqb291eDZoYjdoODFROVg5Q1hq?=
 =?utf-8?B?aEtYdnZobEdHQUgzdkhxaU5vczA2ZjkwSlNicGpjZEYzamxvR3IwQjUzK1lX?=
 =?utf-8?B?bDZSeVUwZGIzb3hVMUNEUWw2bi9sUzlpVStXYmFuRE1PYUcyTnRFZ2lQOUVV?=
 =?utf-8?B?WXVoMXA5Rnd3NkhLcEZoZDdrWUNoMVRCSDg5MXhzbUlwc3Q4RENUeUVKdlBl?=
 =?utf-8?B?K0R5SnpBTE5iOGU0VER2aW41UEVteU4rbWhoaERGa0NjV28vc0dFZmlpdUNp?=
 =?utf-8?B?b2QzdkNVMzk5UGpsb0FIMzA4Yk5DZWFkYUtCVG5MbmxJS2RoYVdJZ29XNm5E?=
 =?utf-8?B?YU1hRmVQS3I3cmtNNG45aGpuLzE4OEgvMGxsVHA3YWtzMzh3RHYvQ1RTYW5t?=
 =?utf-8?B?NS85aklZT0FWb2JhcmgrUGRFcHhXRklzalUwWEI2TDhUcklyV1VRRUIwODA2?=
 =?utf-8?B?MjNBamRUSEpaSG9YYnRrNTRHRncvM1YybndBQjB4ZWZrQUw3RVNXOTdnT2dC?=
 =?utf-8?Q?rALNrIDQ6alfexnZjAU4XdA=3D?=
X-OriginatorOrg: virtuozzo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 90f2d867-e56a-45ee-d391-08d9c907c15c
X-MS-Exchange-CrossTenant-AuthSource: AM9PR08MB6241.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Dec 2021 07:08:59.8331
 (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: h+/3tX9Pom3cfGak8oUi8XjZnKOd5/U+NFpE8FSyxV72CueV9ygd63FvOknijV3CQKADGaYVUaRl+3+0T9bObA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR08MB4468
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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