Received-SPF: pass (google.com: domain of linux-nfs+bounces-2570-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <09da882f-4bad-4398-a2d0-ef8daefce374@oracle.com>
Date: Sat, 30 Mar 2024 10:46:08 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/1] NFSD: cancel CB_RECALL_ANY call when nfs4_client is
 about to be destroyed
Content-Language: en-US
To: Chuck Lever <chuck.lever@oracle.com>
Cc: jlayton@kernel.org, linux-nfs@vger.kernel.org
References: <1711476809-26248-1-git-send-email-dai.ngo@oracle.com>
 <ZgMToHNkkGEHNb/y@tissot.1015granger.net>
 <69914825-e9d5-4859-a5a8-60d17e8e8bf6@oracle.com>
 <ZgV5zwR0q/vjBAtI@tissot.1015granger.net>
 <88fac8af-c194-452b-94eb-7658b9056246@oracle.com>
 <c97be8b9-c0ba-4f2d-9340-78368008ba4b@oracle.com>
 <ZgbWevtNp8Vust4A@tissot.1015granger.net>
 <97387ec5-bcb4-4c5e-81af-a0038f7fc311@oracle.com>
 <ZgdR48dhdSoUj+VM@tissot.1015granger.net>
From: Dai Ngo <dai.ngo@oracle.com>
In-Reply-To: <ZgdR48dhdSoUj+VM@tissot.1015granger.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?eEdpQ0w2aUx1YlNldFB0M2EweG93ZXBseWladWg4WVd3cUROWHh3dXVEc1Ft?=
 =?utf-8?B?cmpCSFJCOXIxQ29JNExiRGpBdFJSdmVVcHZOcElrdFJVY0VNRHpDT0FIQ2Rn?=
 =?utf-8?B?c0xwbk1MNGdKYVgralpneGwxTS80TkdlZ1ZIUG9BVW0wQSsvUWdTTWNXTDhr?=
 =?utf-8?B?VWtZVm15dzc2bThUaktmUTdSM1FrRGJNd050SDZqbjVxNTFqa21qQ2ZzZHJj?=
 =?utf-8?B?UzNiUTVxMEFFWkU3NGd3dUdaMGVUcHQvdVcwZEk1NVlER2xUYkR0UFE1Ym1B?=
 =?utf-8?B?TFNwMXNmdUhHNDFjaFdkQUZwbFRoRXpFc0JJY21GYWVWZVRZaktYakdCdHJ1?=
 =?utf-8?B?d0ZRS0VyaFZ3NVdtbERtcFFudmVaZDJVdmZmTXk1RnZ5c3M0Z0FHZVljRjZa?=
 =?utf-8?B?VHM1bVcxaTVKeUZ1K0N2SjhFcVlaZ1M5SEh5NkNpRFlnWUQ5cXU0MjVnL2Vz?=
 =?utf-8?B?d0w1ZEg0MEF2ZnZ4WUc5OG5XaTNqYS9pMFU0MGpsTFN2amR6M1BMbmdZVVhY?=
 =?utf-8?B?WnNhaW51MTNlTE1DWEV6QitoRW92dk91Qnh2YUd4bjhoUjh1MWVPR09mcmk3?=
 =?utf-8?B?RVBpN0xSYjl3V3QwVjk0TVVEVlF6SlIrUTJMOGo0SUhKaWZlNTZKVGdGRjF2?=
 =?utf-8?B?djZLMUk0Z3N0QjBmNElBbk11SDRoak1WQmRocDJOS3I1bUJZQ1c2R2ViZHNK?=
 =?utf-8?B?bmJXeis0VmJ1R1dmNDh2emRseGpzSEpVNXQ5ZFIvRnRidDJ3cllpQVJGSnlx?=
 =?utf-8?B?NllGYkpGVStmTjEvYmFlNXg4ZzRuZDRCeXJJOGh2a2pXcjNNSUhtblR4UTF0?=
 =?utf-8?B?SDI1V2pCdC9haXVGYzVEMlF0MDJBTS9UOGdpT2FzemgzUkR4MC8yb2ExYUpL?=
 =?utf-8?B?SHZuWjdzRU8rMVpMMVlXUTVUU0JGc3BwcTZWSFhLYW1KR3RwUURkakZSV0ZI?=
 =?utf-8?B?TTVZejJ4QnJ4MXNKcC9RKy9HNlRsS01NanB5SHNaeVlwRDRqb1N5R0ttaVRH?=
 =?utf-8?B?MWVwa3JNbWxNYlNwTDR4Mm1Teml0N2pTNEF0UFV6RmJ2NWxMWHBITWZuMGZI?=
 =?utf-8?B?aW5EWDZhNnVRQWp1WWczL1VsdjVjLytURlhJSHJhNTgyK3RRdktheGhOWGRj?=
 =?utf-8?B?ZzE1Sy8vVXFBYTZsd3NYVS9vNHJxNU5Qdmd2RHkwMENQTG5RZWJOT3NuVlVV?=
 =?utf-8?B?ZVJrSThSbUp5SzlSZnBZY1VVY0pQWjhuZXFPaFVjWS9abEZRMnp6RFp5QjM0?=
 =?utf-8?B?WTM4Sjk4NE5ERlRpQ3RDT0MxRW5nVUN2OGZjSjFUOXVvZmNLUHhVajMwaTFI?=
 =?utf-8?B?N1o3djZPU1ZCK2h1NnRWZ3liY2N4MC94Zm1mVlVkUENNTWtnMm42dkliR1Fk?=
 =?utf-8?B?ZHo5bzhiZEd4bDNaaHc5YU5LWm1BVDRxNDRwZjJUZDdqTjhPZnRqbHp4OFJ4?=
 =?utf-8?B?YVdwTXlZcFdlWk9sKytGbFRTSHdiV2ZvN21zbWVtUy9UZUh5eE9YYzB6SWdG?=
 =?utf-8?B?RzhHUlFqVlNndjRVcGVtZFBQQW0ycXdvL1JEMnVoa3hRRDRnT1hkUG5uMUlZ?=
 =?utf-8?B?bFJDYitkRU55SVQySW51dXo3WXhZMzI5WTFXcTNvZDN0anRWWG5jMjZGN3VH?=
 =?utf-8?B?M3VINFlnTlRYMFM1aEIrS0xaTmFqTkZHLzJLN09MR3BkODFWUW9kZWlEaHdY?=
 =?utf-8?B?cTZxMFhjSWt4UjVWeFkvby9qaHRSNWNEZ2xzRXhmNGNoZkVjUTBoaXF2bUNt?=
 =?utf-8?B?ZlpoSzRtdFRMeDVETjloM3pTM203d3EydE05d2FNWDRVd3NPNEVKaDljQjJ1?=
 =?utf-8?B?d0tFSjcxcjNGN2ltdDlzRGdJRzZFSGQrbmg4c3krQTVXdzN1cXBwS0pFVHdh?=
 =?utf-8?B?bGN6Tm1hZUFXQkl5T2oxWVV4V2UyZVlpOW1zMDNxQldIVUQ2T2FucVhDa0JL?=
 =?utf-8?B?THlRZ2lNNEtqWW04Z080QTZKbFVmM2FJZVFFYTc5VzhWbHk3RmkyWllxbnUr?=
 =?utf-8?B?TU95R2pGcUxPT20yajZqS25UdHNJbXd0aW4xWkpQRlBqQytqVCtzSUhxQWRL?=
 =?utf-8?B?MnJ1TytoaWpiQ2FxTE1Tc1AyWHJXa05YVVRWZ3RiMG44WEJaWGtxYjUrUmhZ?=
 =?utf-8?Q?i/3IDWaEDNaEuHh14EBvOmr4t?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	kJ22GjlEOxA3HK4u0v9DvZJTRvbTmeJ8E97+N/Y8UefuWaoDpfNecGcgyJ9OJmYkhirkgQQrl2Mmd+x8pEz4NToVIGCZ3WIGWHsvffvQtOmNTbE9b9DiV39qFcIqVa/NMuPgdnKb5BTQX1HVfynGu4swIM80sBNaw8c9o+N8bbdSYV/HoR0u2bh8SuYFPav4+YP1vd5h3j54FA473UudtvL+8rrd3U++AG841FIQ6d/aBelJt4gk9Oich5KRJuz864WgmBFqwpX98UFUWRDT0wXlED8H9wSK2jbXUI2rxU1kPA81MDcHdovkxscWT3CiMtftWyqTmK6nrKYzdgwmmnRFWKOZvV9OrhDnQoqUEicQ5Zpk0CtjTcL1b5+5QD0fQxx4vfTEFidy4g6PC7BauYcA4NQfdHCo0JcVvsiKTVLBlp0sZK1zBv9McW6yI8kVe3va6hLG+YlefGZIuDgRe081bnQgvJykSfwt1mi5JxsA/PXOEI6oz/LRT7IzTGkNFsFYDVPrQESxZlB12//ZQbM1lMu37iE92GPWezuTkMJEqfiA8rexpmUoixw0ujo3KJunKWzk0DMOL1tAeG1a3Vst3ZQTjwZvNpwrENCE3UM=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f9d8ce4e-7de4-42ec-d080-08dc50e14a30
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Mar 2024 17:46:12.4250
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: H6tAUHUO+o6uuNrOQeQIGKouVIDxsTABmf0DPfKZ1PBmSfTYcWbYPE0WFUOCsXBevCXttzgt1ZK3Z45NwHPgng==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR10MB5807
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-03-30_12,2024-03-28_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 phishscore=0 spamscore=0
 bulkscore=0 adultscore=0 mlxlogscore=999 suspectscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2403210000
 definitions=main-2403300145
X-Proofpoint-ORIG-GUID: 1tCrRE7IytrowuihnKSc7BcpmuEmawds
X-Proofpoint-GUID: 1tCrRE7IytrowuihnKSc7BcpmuEmawds


On 3/29/24 4:42 PM, Chuck Lever wrote:
> On Fri, Mar 29, 2024 at 10:57:22AM -0700, Dai Ngo wrote:
>> On 3/29/24 7:55 AM, Chuck Lever wrote:
>>> On Thu, Mar 28, 2024 at 05:31:02PM -0700, Dai Ngo wrote:
>>>> On 3/28/24 11:14 AM, Dai Ngo wrote:
>>>>> On 3/28/24 7:08 AM, Chuck Lever wrote:
>>>>>> On Wed, Mar 27, 2024 at 06:09:28PM -0700, Dai Ngo wrote:
>>>>>>> On 3/26/24 11:27 AM, Chuck Lever wrote:
>>>>>>>> On Tue, Mar 26, 2024 at 11:13:29AM -0700, Dai Ngo wrote:
>>>>>>>>> Currently when a nfs4_client is destroyed we wait for
>>>>>>>>> the cb_recall_any
>>>>>>>>> callback to complete before proceed. This adds
>>>>>>>>> unnecessary delay to the
>>>>>>>>> __destroy_client call if there is problem communicating
>>>>>>>>> with the client.
>>>>>>>> By "unnecessary delay" do you mean only the seven-second RPC
>>>>>>>> retransmit timeout, or is there something else?
>>>>>>> when the client network interface is down, the RPC task takes ~9s to
>>>>>>> send the callback, waits for the reply and gets ETIMEDOUT. This process
>>>>>>> repeats in a loop with the same RPC task before being stopped by
>>>>>>> rpc_shutdown_client after client lease expires.
>>>>>> I'll have to review this code again, but rpc_shutdown_client
>>>>>> should cause these RPCs to terminate immediately and safely. Can't
>>>>>> we use that?
>>>>> rpc_shutdown_client works, it terminated the RPC call to stop the loop.
>>>>>
>>>>>>> It takes a total of about 1m20s before the CB_RECALL is terminated.
>>>>>>> For CB_RECALL_ANY and CB_OFFLOAD, this process gets in to a infinite
>>>>>>> loop since there is no delegation conflict and the client is allowed
>>>>>>> to stay in courtesy state.
>>>>>>>
>>>>>>> The loop happens because in nfsd4_cb_sequence_done if cb_seq_status
>>>>>>> is 1 (an RPC Reply was never received) it calls nfsd4_mark_cb_fault
>>>>>>> to set the NFSD4_CB_FAULT bit. It then sets cb_need_restart to true.
>>>>>>> When nfsd4_cb_release is called, it checks cb_need_restart bit and
>>>>>>> re-queues the work again.
>>>>>> Something in the sequence_done path should check if the server is
>>>>>> tearing down this callback connection. If it doesn't, that is a bug
>>>>>> IMO.
>>>> TCP terminated the connection after retrying for 16 minutes and
>>>> notified the RPC layer which deleted the nfsd4_conn.
>>> The server should have closed this connection already.
>> Since there is no delegation conflict, the client remains in courtesy
>> state.
>>
>>>    Is it stuck
>>> waiting for the client to respond to a FIN or something?
>> No, in this case since the client network interface was down server
>> TCP did not even receive ACK for the transmit so the server TCP
>> kept retrying.
> It sounds like the server attempts to maintain the client's
> transport while the client is in courtesy state?

Yes, TCP keeps retryingwhile the client is in courtesy state.
  

>   I thought the
> purpose of courteous server was to more gracefully handle network
> partitions, in which case, the transport is not reliable.
>
> If a courtesy client hasn't re-established a connection with a
> backchannel by the time a conflicting open/lock request arrives,
> the server has no choice but to expire that client's courtesy
> state immediately. Right?

Yes, that is the case for CB_RECALL but not for CB_RECALL_ANY.

>
> But maybe that's a side-bar.
>
>
>>>> But when nfsd4_run_cb_work ran again, it got into the infinite
>>>> loop caused by:
>>>>        /*
>>>>         * XXX: Ideally, we could wait for the client to
>>>>         *      reconnect, but I haven't figured out how
>>>>         *      to do that yet.
>>>>         */
>>>>         nfsd4_queue_cb_delayed(cb, 25);
>>>>
>>>> which was introduced by c1ccfcf1a9bf. Note that I'm using 6.9-rc1.
>>> The whole paragraph is:
>>>
>>> 1503         clnt = clp->cl_cb_client;
>>> 1504         if (!clnt) {
>>> 1505                 if (test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags))
>>> 1506                         nfsd41_destroy_cb(cb);
>>> 1507                 else {
>>> 1508                         /*
>>> 1509                          * XXX: Ideally, we could wait for the client to
>>> 1510                          *      reconnect, but I haven't figured out how
>>> 1511                          *      to do that yet.
>>> 1512                          */
>>> 1513                         nfsd4_queue_cb_delayed(cb, 25);
>>> 1514                 }
>>> 1515                 return;
>>> 1516         }
>>>
>>> When there's no rpc_clnt and CB_KILL is set, the callback
>>> operation should be destroyed immediately. CB_KILL is set by
>>> nfsd4_shutdown_callback. It's only caller is
>>> __destroy_client().
>>>
>>> Why isn't NFSD4_CLIENT_CB_KILL set?
>> Since __destroy_client was not called, for the reason mentioned above,
>> nfsd4_shutdown_callback was not called so NFSD4_CLIENT_CB_KILL was not
>> set.
> Well, then I'm missing something. You said, above:
>
>> Currently when a nfs4_client is destroyed we wait for
> And __destroy_client() invokes nfsd4_shutdown_callback(). Can you
> explain the usage scenario(s) to me again?

__destroy_client is not called in the case of CB_RECALL_ANY since
there is no open/lock conflict associated the the client.

>
>
>> Since the nfsd callback_wq was created with alloc_ordered_workqueue,
>> once this loop happens the nfsd callback_wq is stuck since this workqueue
>> executes at most one work item at any given time.
>>
>> This means that if a nfs client is shutdown and the server is in this
>> state then all other clients are effected; all callbacks to other
>> clients are stuck in the workqueue. And if a callback for a client is
>> stuck in the workqueue then that client can not unmount its shares.
>>
>> This is the symptom that was reported recently on this list. I think
>> the nfsd callback_wq should be created as a normal workqueue that allows
>> multiple work items to be executed at the same time so a problem with
>> one client does not effect other clients.
> My impression is that the callback_wq is single-threaded to avoid
> locking complexity. I'm not yet convinced it would be safe to simply
> change that workqueue to permit multiple concurrent work items.

Do you have any specific concern about lock complexity related to
callback operation?

>
> It could be straightforward, however, to move the callback_wq into
> struct nfs4_client so that each client can have its own workqueue.
> Then we can take some time and design something less brittle and
> more scalable (and maybe come up with some test infrastructure so
> this stuff doesn't break as often).

IMHO I don't see why the callback workqueue has to be different
from the laundry_wq or nfsd_filecache_wq used by nfsd.

-Dai

>
>