Message-ID: <cdf6317b-aa42-539a-bc7f-3150e83cbc60@oracle.com>
Date:   Wed, 8 Dec 2021 09:29:31 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.3.2
Subject: Re: [PATCH RFC v6 2/2] nfsd: Initial implementation of NFSv4
 Courteous Server
Content-Language: en-US
To:     "bfields@fieldses.org" <bfields@fieldses.org>
Cc:     Trond Myklebust <trondmy@hammerspace.com>,
        "chuck.lever@oracle.com" <chuck.lever@oracle.com>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
References: <20211206175942.47326-1-dai.ngo@oracle.com>
 <20211206175942.47326-3-dai.ngo@oracle.com>
 <4025C4F6-258F-4A2E-8715-05FD77052275@oracle.com>
 <f6d65309-6679-602a-19b8-414ce29c691a@oracle.com>
 <ba637e0c64b6a2b53c8b5bf197ce02d239cdc0d2.camel@hammerspace.com>
 <605c2aef-3140-6e1a-4953-fd318dbcc277@oracle.com>
 <20211208163937.GA29555@fieldses.org>
From:   dai.ngo@oracle.com
In-Reply-To: <20211208163937.GA29555@fieldses.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Y2V6eWVCQ0ZsU0RqZS8zbm5jcUdwRGNzL2w5Y2J5V0xMc0tiVWFRTXUrVUpF?=
 =?utf-8?B?Y00vQUJEVHVjd2lKdHc2Y0llNTlwODE2YUxaUnFKQ0hySE5IcGlGb3l5OUJu?=
 =?utf-8?B?bUZGWVJ0Nk9ieGQ0dENnb0tLSG1Zc2NzbmVOQkN1T1QyRUhDVGhlejdBQ2RG?=
 =?utf-8?B?NWFvYmc3Zi9Bd1c3dks2cWFUWVVLUCtTNlVaZTd0RkZkbldKcVJqYmhFUWdu?=
 =?utf-8?B?SExSSTlZcW5JY0F6elhJVGtPZnk5YzA4cFJiTjRyNFl1Nlo4U0JjbXl1T3NM?=
 =?utf-8?B?M291V055ZUY2aGVZUVpJcFB6SGR1L1FGMnFic1ZrNTdHTm0yRkFKbjFBUk9Q?=
 =?utf-8?B?ckt3VkZkcERTVW5XQVBSWmVGQ0tlWVFRYVo3aU5iV0kwL3dqWHVhSEJTNVBW?=
 =?utf-8?B?MDhBdDNMck1PTWROaG53SFdmYjlQbWlpSmg0L2dPTnBScXZDeTg0RGo4VEFM?=
 =?utf-8?B?cGJpakJlem1ya1cxWjBLeXhld0lxSWlNeGIwVTFFVlhGS1c5WnNLSU8xeVh0?=
 =?utf-8?B?VW1lUEpzNUI0djltVmluVWFlRUd0Qk1wbysxUEtVVjhRR20yZUY3dngyWDhu?=
 =?utf-8?B?aUVjSGVkL2tuVWlZaEhHZStNd29lNjlzNEduT3N3M0F2NG0yL0R3RmtKRTJF?=
 =?utf-8?B?aldTVTlnMFo1SFkzbFJFeEhxYlY1VGx5Qy9ZdksrZndOaXJ0aEJLNWFnTDdJ?=
 =?utf-8?B?dWVMam4wcWZoUmxOcVZ0L1duUVcwYTF3dy9pdU5qY3lTY2M0SWczbVp1ZHBJ?=
 =?utf-8?B?YzRiZkcyaXIxd3lKZi9obDVlbVp2M3RFbVM3dHJSMUppTFBRMkJlU0FEWHRo?=
 =?utf-8?B?NWsrZHNva2RabzB0UW5DTjgweS9Yb0ROSkthQU1taUY0WGJmTDJDQ1Z6QSsx?=
 =?utf-8?B?Yzcwd1ljaGFwdHUxUEJYYmxOZG9BY1ZFUzBFQUhDcEV5NnBGN0ljZnNLemIx?=
 =?utf-8?B?S1VBVnQ1VFRUVVVxdXd1ZFNFNlg3dllzbWFiNXlJK3BvK29BeEx1b3NCRUJG?=
 =?utf-8?B?d0hsVi9acFo3RXVXRkw2OW1SQ1VEV3FIK3RHdGdOdEQ3U05LMUZWbVhFODJm?=
 =?utf-8?B?SUZYUlR2T2oxK1RuKzRJV3R5R0FnWXFHR1V6bkF2N1lxSzlCSk9Ua1B2Y3VT?=
 =?utf-8?B?Nnd0RmVuSXBlaVZ0OHBpdXNTZ2RwOUNLdmFJeXc5TndIZUVMc2lPVlRNc09v?=
 =?utf-8?B?L3ZaTnpKNXVVSmJtM3RrM3dHNnJwZVZCL3V3TU5XL3pRR0ZDYVROQ3R3QWtz?=
 =?utf-8?B?MEJpdUVkVjdOZ3V4bmJNSnVKU2w3Uk9hWWtJTlFHbHN5cGJhd2NPYWlBWTBB?=
 =?utf-8?B?dEszZ1VMRk9DRVhXSUY2M3V0eDJkd3d6RXNmVzhNSE1CZWZrZW5CM0YwWlor?=
 =?utf-8?B?dFhwaGZOYkhabDNoTjB5cWgxeFdlUi9BRDUzS3daZ0xiaXpnN3Q5NjV5djRj?=
 =?utf-8?B?a05jYzU1YXFGWlg5Tm1aYmRKZG1wUGw5R3VtVkZJdXhxeDN5M0JBazQ1MzBK?=
 =?utf-8?B?ZVYrRlU4MlB2MmpDRXRNVEl0aW9FOGtiRUNsQnB0UkVKY2xuVy9KOUtVQ1dy?=
 =?utf-8?B?ZDliNnRiajRkOTg4SEVyTGFzam0zUmNNK1h0Mi93Z1lEcEJ5ODBPSFp5ZHlZ?=
 =?utf-8?B?Mlh3bFU2amFQRXprTy91WlhPR0poa0U0U2JmTDNZV3JVT3JmY2I2MmZYSEpN?=
 =?utf-8?B?dkFWczZhUlRXVTFJK1hrdmt4RXNrZW5GZGkxMDc3UEltN2I0VzVTN2tkeG1Q?=
 =?utf-8?B?UTNnYjNWdTdOY2xkYUFadE5YV1RtTkFIWEd2enVHU3R6UTdCWjJnTE1mNm9h?=
 =?utf-8?B?b3NleGRHZ29yV09sZVNpUGNJRUxDbXRFK0dBdUc1eWlmc0ZaTmd5KzBqb3RH?=
 =?utf-8?B?SHI3TXV5QWRUVHFZNVpnZmhtQVJYQTJkZ0pIZ0pGQW1LMXZqcjJaUUtaa0xC?=
 =?utf-8?B?UmFiRGRFMU9DaFEwZTdBcHhKNW56U0pVcytsbEZOR0tiWTJsalRmZFg1Mjd0?=
 =?utf-8?B?L3NtNU5LRE9OcXNvbVYwaGE5WndDMmlzMGdJSHRmS29uN1NtOE5Ib3dZczJL?=
 =?utf-8?B?YndnakFsSkVsaFIwNkRyaVpESlpSSnV6aG51bmdDTWdCdW8ySk5CS1o0S1Fh?=
 =?utf-8?B?dWhlN2JDcG5wSUpKN00zZUhHdmw3a0dVK0oyQW9WdU41OUxXMVVaT0xPOFNx?=
 =?utf-8?Q?MUYXw/QdViUAb3Xeui/K6lg=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 52276e66-2a5b-441e-ce77-08d9ba704d01
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2021 17:29:34.6014
 (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: JnsEWVqazwCYjr08oCnp0KQkImuhB5T9QQDMycZYJlghhwnUQt6VjADanT4bdT5KLYxQ/hgJSdzEi9vKkoNoqg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4653
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10192 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 mlxlogscore=999
 phishscore=0 bulkscore=0 malwarescore=0 spamscore=0 suspectscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2110150000 definitions=main-2112080099
X-Proofpoint-GUID: 4T5okRx7YFAWpJluGu0mvEHxZpYHlAe3
X-Proofpoint-ORIG-GUID: 4T5okRx7YFAWpJluGu0mvEHxZpYHlAe3
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


On 12/8/21 8:39 AM, bfields@fieldses.org wrote:
> On Wed, Dec 08, 2021 at 08:25:28AM -0800, dai.ngo@oracle.com wrote:
>> On 12/8/21 8:16 AM, Trond Myklebust wrote:
>>> On Wed, 2021-12-08 at 07:54 -0800, dai.ngo@oracle.com wrote:
>>>> On 12/6/21 11:55 AM, Chuck Lever III wrote:
>>>>
>>>>>> +
>>>>>> +/*
>>>>>> + * Function to check if the nfserr_share_denied error for 'fp'
>>>>>> resulted
>>>>>> + * from conflict with courtesy clients then release their state to
>>>>>> resolve
>>>>>> + * the conflict.
>>>>>> + *
>>>>>> + * Function returns:
>>>>>> + *      0 -  no conflict with courtesy clients
>>>>>> + *     >0 -  conflict with courtesy clients resolved, try
>>>>>> access/deny check again
>>>>>> + *     -1 -  conflict with courtesy clients being resolved in
>>>>>> background
>>>>>> + *            return nfserr_jukebox to NFS client
>>>>>> + */
>>>>>> +static int
>>>>>> +nfs4_destroy_clnts_with_sresv_conflict(struct svc_rqst *rqstp,
>>>>>> +                       struct nfs4_file *fp, struct
>>>>>> nfs4_ol_stateid *stp,
>>>>>> +                       u32 access, bool share_access)
>>>>>> +{
>>>>>> +       int cnt = 0;
>>>>>> +       int async_cnt = 0;
>>>>>> +       bool no_retry = false;
>>>>>> +       struct nfs4_client *cl;
>>>>>> +       struct list_head *pos, *next, reaplist;
>>>>>> +       struct nfsd_net *nn = net_generic(SVC_NET(rqstp),
>>>>>> nfsd_net_id);
>>>>>> +
>>>>>> +       INIT_LIST_HEAD(&reaplist);
>>>>>> +       spin_lock(&nn->client_lock);
>>>>>> +       list_for_each_safe(pos, next, &nn->client_lru) {
>>>>>> +               cl = list_entry(pos, struct nfs4_client, cl_lru);
>>>>>> +               /*
>>>>>> +                * check all nfs4_ol_stateid of this client
>>>>>> +                * for conflicts with 'access'mode.
>>>>>> +                */
>>>>>> +               if (nfs4_check_deny_bmap(cl, fp, stp, access,
>>>>>> share_access)) {
>>>>>> +                       if (!test_bit(NFSD4_COURTESY_CLIENT, &cl-
>>>>>>> cl_flags)) {
>>>>>> +                               /* conflict with non-courtesy
>>>>>> client */
>>>>>> +                               no_retry = true;
>>>>>> +                               cnt = 0;
>>>>>> +                               goto out;
>>>>>> +                       }
>>>>>> +                       /*
>>>>>> +                        * if too many to resolve synchronously
>>>>>> +                        * then do the rest in background
>>>>>> +                        */
>>>>>> +                       if (cnt > 100) {
>>>>>> +                               set_bit(NFSD4_DESTROY_COURTESY_CLIE
>>>>>> NT, &cl->cl_flags);
>>>>>> +                               async_cnt++;
>>>>>> +                               continue;
>>>>>> +                       }
>>>>>> +                       if (mark_client_expired_locked(cl))
>>>>>> +                               continue;
>>>>>> +                       cnt++;
>>>>>> +                       list_add(&cl->cl_lru, &reaplist);
>>>>>> +               }
>>>>>> +       }
>>>>> Bruce suggested simply returning NFS4ERR_DELAY for all cases.
>>>>> That would simplify this quite a bit for what is a rare edge
>>>>> case.
>>>> If we always do this asynchronously by returning NFS4ERR_DELAY
>>>> for all cases then the following pynfs tests need to be modified
>>>> to handle the error:
>>>>
>>>> RENEW3   st_renew.testExpired                                     :
>>>> FAILURE
>>>> LKU10    st_locku.testTimedoutUnlock                              :
>>>> FAILURE
>>>> CLOSE9   st_close.testTimedoutClose2                              :
>>>> FAILURE
>>>>
>>>> and any new tests that opens file have to be prepared to handle
>>>> NFS4ERR_DELAY due to the lack of destroy_clientid in 4.0.
>>>>
>>>> Do we still want to take this approach?
>>> NFS4ERR_DELAY is a valid error for both CLOSE and LOCKU (see RFC7530
>>> section 13.2 https://urldefense.com/v3/__https://datatracker.ietf.org/doc/html/rfc7530*section-13.2__;Iw!!ACWV5N9M2RV99hQ!f8vZHAJophxXdSSJvnxDCSBSRpWFxEOZBo2ZLvjPzXLVrvMYR8RKcc0_Jvjhng$
>>> ) so if pynfs complains, then it needs fixing regardless.
>>>
>>> RENEW, on the other hand, cannot return NFS4ERR_DELAY, but why would it
>>> need to? Either the lease is still valid, or else someone is already
>>> trying to tear it down due to an expiration event. I don't see why
>>> courtesy locks need to add any further complexity to that test.
>> RENEW fails in the 2nd open:
>>
>>      c.create_confirm(t.word(), access=OPEN4_SHARE_ACCESS_BOTH,
>>                       deny=OPEN4_SHARE_DENY_BOTH)     <<======   DENY_BOTH
>>      sleeptime = c.getLeaseTime() * 2
>>      env.sleep(sleeptime)
>>      c2 = env.c2
>>      c2.init_connection()
>>      c2.open_confirm(t.word(), access=OPEN4_SHARE_ACCESS_READ,    <<=== needs to handle NFS4ERR_DELAY
>>                      deny=OPEN4_SHARE_DENY_NONE)
>>
>> CLOSE and LOCKU also fail in the OPEN, similar to the RENEW test.
>> Any new pynfs 4.0 test that does open might get NFS4ERR_DELAY.
> So it's a RENEW test, not the RENEW operation.
>
> A general-purpose client always has to be prepared for DELAY on OPEN.
> But pynfs isn't a general-purpose client, and it assumes that it's the
> only one touching the files and directories it creates.
>
> Within pynfs we've got a problem that the tests don't necessarily clean
> up after themselves completely, so in theory a test could interfere with
> later results.
>
> But each test uses its own files--e.g. in the fragment above note that
> the file it's testing gets the name t.word(), which is by design unique
> to that test.  So it shouldn't be hitting any conflicts with state held
> by previous tests.  Am I missing something?

Both calls, c.create_confirm and c2.open_confirm, use the same file name
t.word().

However, it's strange that if I run RENEW3 by itself then it passes.
 From the network trace, env.c2/c2.init_connection generates a SETCLIENTID
which might get rid off all the states of the previous client since
they're the same.

I will investigate why it fails when running 'all' with NFS4ERR_DELAY.

Do you know if there is an option to specify a list of tests to run,
instead of 'all'?

-Dai