Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v3 2/3] nfs4: handle async processing of F_SETLK with
 FL_SLEEP
To:     "J. Bruce Fields" <bfields@fieldses.org>
Cc:     Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna.schumaker@netapp.com>, kernel@openvz.org,
        linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org,
        Chuck Lever <chuck.lever@oracle.com>
References: <1f354cec-d2d6-ddf5-56e0-325c10fe26ee@virtuozzo.com>
 <00d1e0fa-55dd-82a5-2607-70d4552cc7f4@virtuozzo.com>
 <20220103194012.GF21514@fieldses.org>
From:   Vasily Averin <vvs@virtuozzo.com>
Message-ID: <b332a1dd-f96d-2d83-80ca-67058eeaa1af@virtuozzo.com>
Date:   Sun, 16 Jan 2022 15:25:43 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <20220103194012.GF21514@fieldses.org>
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?aGpkMUJacWhTSVFjdmRMcUZMRlRWSmg5RW9oR0FmQjBEdkxxd2lUNWVWODIz?=
 =?utf-8?B?cGgwQUVEK1RsSWE2aEI3SUpHMml3N3FKVm1kdWJkSGphSTV2TktrcmE2MEpr?=
 =?utf-8?B?Nngva2ttSm12NGxlREVxaGdnSFh1Um05NEdtanlnQ2RPZ1ZKZjB2SHd0cTNo?=
 =?utf-8?B?NlAwblRQZ1dtS29WdFJLUE9ka0x2R2x2dEljdDJhbUhWY3ZVbEdhV0x1TUxG?=
 =?utf-8?B?RWlPa3FkZHVRNEdKMzdqN0orK1JwQWpIY3p5R3graWl6Vyt3ZTF1WWRKWjNQ?=
 =?utf-8?B?S1VpN1FMcWQrSUNHZTNhVTVGZlltdFdqWlNBVE95N0d1MDA5YWlaZStpc1BZ?=
 =?utf-8?B?MEVQRElESER5VCtVYkxZaWJJajVYZ3hIcVhyOExBV2ZrajZEMmkvanhTUWE3?=
 =?utf-8?B?aDlpZkozL0VxcGNXNjdXOWRVejdsenJ6UnR1TkFUMDN0K2RMTkRwRmJXNVQ1?=
 =?utf-8?B?MDdtZk1SWTYzWUgrUDdVenBLdE11ZXc1NW5ja2syd1psZnorR2tvRkxPYW50?=
 =?utf-8?B?bUtkOXpCc0JyWDhsNUNIYUZSV2FsMU1HZ3FRbjNqY2pyVXFFZkwrNUs4VFg2?=
 =?utf-8?B?L01oeTY1N3VYR3ltSnFyOUdaV3RMOWFsVUw2NU5mV01rZHhDOWU2ZlVYWk1C?=
 =?utf-8?B?UVlVMFBMdzBMRHpUZ1dkS2F4QlJKUnpYNVVxdWp3c3RZcFI0bVUzSktESGlr?=
 =?utf-8?B?RGtyV3dIK3MxWXo5T3RyS1orSENlekMxZkRxNXhCMnpQMzgzNCs3QVlsVE12?=
 =?utf-8?B?WDM5Mk9NdDM2TWN5WEpFYkNxQWFpUFA1N0s4bUhRU1JMZjJFVDJqNFZZa1BB?=
 =?utf-8?B?djhuVzFzbEZ5dUg5RWF1ekw4R0VKU09vOXhiZ3orY2RVZkZITjlYdzdvaFE0?=
 =?utf-8?B?SzhTZE02WU5NNGp6YmJjbGlZYW1qL2FldXVEQlpQR2pFNi9pOWdVeUlQQnpG?=
 =?utf-8?B?TUFhVU5TaVhTNldidXFIOGNmMUI2VE05NmJwejdzTFZjMHU0MzhxUGs2cmd2?=
 =?utf-8?B?OVhNOHNXTWFERnpJOTBsZ0dWQmI2Sk00Nnc0SDFiaERNM0hnK1M2emNxRENP?=
 =?utf-8?B?THRMR1pveUFsL2x6N1lRQmQ3bW5JUDJ4YWUrbzJacXgyVDZpZForQXMwcFlJ?=
 =?utf-8?B?QkFmRjhnZ2IzQlIyVEhvMlgrN0pVaWZSc2V3M1ZYakdCVjh3dUxiKzZhcTRz?=
 =?utf-8?B?c0l5MDk3NzJEMFJUTXkwZVlzUHRNdWhUUkViYlI4WHBtWXA0Ull2OURVYzkz?=
 =?utf-8?B?TUd0NDhLWkpxb2lqZFpGa1lmVittWVloV01RVGdCYnh4WmFjMWUwakd1bEMv?=
 =?utf-8?B?R2Z3QWpYRm1YemlmL3hpc0Q5blQ2bkFKYnorSVpXUWF5ZkpDbThxUWhOTkpK?=
 =?utf-8?B?MG5hUWxUUjZ6cTZySlpHWVErVE5leU1yemVwNUpsR0M5SXBHV1pMamtWVktR?=
 =?utf-8?B?WlEvbi8vWDY5Nno4MngwSlFLa1l1OWRmUUtnU0J2U0M4d2p3WnpIakN1eUxq?=
 =?utf-8?B?VnlxZTlYQW83QzNNSDMybVAvbGhpVUtyTjZ6cWpqSVIxemZkYVV3SDdkVmpv?=
 =?utf-8?B?MkhQR3g0NXR2dWtzOHZlbkYyUnNaaTMvQVVpN3dnY3FzanFjeWtjUzVRdDBv?=
 =?utf-8?B?dVQwVWwwQ1ljWnZ6MVMvSHhtVnVJVW9JRk13dTZjTU1icHZoOXpBTm9Gall2?=
 =?utf-8?B?N3c1eXQvVDlLQkVaWCtZSWhVT2ZydFFwRUg3a2RKQ1lUZVkxMGRFNmJ2QTVs?=
 =?utf-8?B?RG9XNy9iUGxHMTV1cEMzVk4vTHp3TXVSUk9VakxKbFd1N0lIOVZnOHFncUZr?=
 =?utf-8?B?T1dEYk52VkNISSsxUXI2MTRleG5LSEdhYjJMQXAxSlBlempRY3VKYVNCSzhx?=
 =?utf-8?B?a2NweWZWdDRzMnVZbUlCVFljUm81dGZYQkprSWxnM01OQ1NaZlRTWXVoMVRi?=
 =?utf-8?B?bWRSZkcwS3FqVlg5WGVkQ3BDQTdDM3F5a24vR1o4d1o5VGZad2dnVDhMZVh4?=
 =?utf-8?B?Y0Jtd3lERWh3Z1UwdmVVTURmQi80T3VoL2pJWWp1NXhQTjM1MnFCdEluTzRD?=
 =?utf-8?B?MlN2bUtMK09CUys0b2lSTm1Hd0NadjF0dFVaVEpxK2VhK3RCekZlQjNzTXRk?=
 =?utf-8?B?dEs4bjFPZGFOUXJPVGphMUJXaC92N2JUMklWUVNpcktEeVhxeHhuSWsxbHBN?=
 =?utf-8?Q?g+Gg5RAuUQbOuXJVwhw6nEc=3D?=
X-OriginatorOrg: virtuozzo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a7b068b6-de1c-44fd-45da-08d9d8eb51c2
X-MS-Exchange-CrossTenant-AuthSource: AM9PR08MB6241.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Jan 2022 12:25:45.4286
 (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: X8qA6MNI+348vOgGwHM7KLprs4kHv/55dtMoIbOAWTid8qDTHd++RmzyjBC14yHtvCanI575QqU3BjKpa87BZA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAXPR08MB6335
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 03.01.2022 22:40, J. Bruce Fields wrote:
> On Wed, Dec 29, 2021 at 11:24:43AM +0300, Vasily Averin wrote:
>> nfsd and lockd use F_SETLK cmd with the FL_SLEEP flag set to request
>> asynchronous processing of blocking locks.
>>
>> Currently nfs4 use locks_lock_inode_wait() function which is blocked
>> for such requests. To handle them correctly FL_SLEEP flag should be
>> temporarily reset before executing the locks_lock_inode_wait() function.
>>
>> Additionally block flag is forced to set, to translate blocking lock to
>> remote nfs server, expecting it supports async processing of the blocking
>> locks too.
> 
> Seems like an improvement, but is there some way to make this more
> straightforward by just calling a function that doesn't sleep in the
> first place?  (posix_lock_inode(), maybe?)

There are few problems:
1) posix_lock_inode() is static in fs/locks.c
2) exported posix_lock_file() used posix_lock_inode() inside requires file pointer,
and I do not understand how to get it.
3) _nfs4_do_setlk() is called from do_setlk and handles flocks too, 
therefore any posix-only calls requires additional checks or branches.

On the other hand all that is required to handle F_SETLK with FL_SLEEP correctly :
to avoid blocking on exiting lock. We can reach this goal here by drop of FL_SLEEP flag
before locks_lock_inode_wait() execution.

Thank you,
	Vasily Averin

PS I'm worry for a long delay with answer,
in Russia we have long holidays after New Year,
then I dealt with urgent tasks accumulated over the holidays
then I forgot the context of this patch and 
I was need to spend some time to re-member the details.

>> https://bugzilla.kernel.org/show_bug.cgi?id=215383
>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
>> ---
>>  fs/nfs/nfs4proc.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index ee3bc79f6ca3..9b1380c4223c 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -7094,7 +7094,7 @@ static int _nfs4_do_setlk(struct nfs4_state *state, int cmd, struct file_lock *f
>>  			recovery_type == NFS_LOCK_NEW ? GFP_KERNEL : GFP_NOFS);
>>  	if (data == NULL)
>>  		return -ENOMEM;
>> -	if (IS_SETLKW(cmd))
>> +	if (IS_SETLKW(cmd) || (fl->fl_flags & FL_SLEEP))
>>  		data->arg.block = 1;
>>  	nfs4_init_sequence(&data->arg.seq_args, &data->res.seq_res, 1,
>>  				recovery_type > NFS_LOCK_NEW);
>> @@ -7200,6 +7200,9 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
>>  	int status;
>>  
>>  	request->fl_flags |= FL_ACCESS;
>> +	if (((fl_flags & FL_SLEEP_POSIX) == FL_SLEEP_POSIX) && IS_SETLK(cmd))
>> +		request->fl_flags &= ~FL_SLEEP;
>> +
>>  	status = locks_lock_inode_wait(state->inode, request);
>>  	if (status < 0)
>>  		goto out;
>> -- 
>> 2.25.1