Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <f33ae93b-ccd6-c8db-9646-251512f5b096@oracle.com>
Date:   Mon, 23 May 2022 09:57:56 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.9.0
Subject: Re: [PATCH RFC v25 0/7] NFSD: Initial implementation of NFSv4
 Courteous Server
Content-Language: en-US
To:     "J. Bruce Fields" <bfields@fieldses.org>
Cc:     chuck.lever@oracle.com, jlayton@redhat.com,
        viro@zeniv.linux.org.uk, linux-nfs@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
References: <1651526367-1522-1-git-send-email-dai.ngo@oracle.com>
 <20220503011252.GK30550@fieldses.org> <20220503012132.GL30550@fieldses.org>
 <9b394762-660b-4742-b54a-2b385485b412@oracle.com>
 <20220523154026.GD24163@fieldses.org>
From:   dai.ngo@oracle.com
In-Reply-To: <20220523154026.GD24163@fieldses.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?a3NHZGpuclgwK2VId2d3c2xoRzZjUDA2dll3ZTkzb2RERUpwNldia285VDU1?=
 =?utf-8?B?SnFZUTE5NTQ1Zmxmdm14cFlsK0dHOWUzTWJiTmF3bUMwN0JtNzFaN2lFeGQw?=
 =?utf-8?B?RTRseE5IbzR4QkdvR3FWY1l4Q2Y3VElVYVNuV3hSRUhFbkp2RVZQMUhiMk1j?=
 =?utf-8?B?TzVhYmtJWXNrNjlHOEhTZ3ZETzlRYnFvbmU0QjFaY0lpZVJUZzIrSitHb1Zj?=
 =?utf-8?B?Smk2a2UycFdnNFVCOFlFY2srMWM0VkU4Nm81Rm42dTNROVIvdmRzS0tpY3dN?=
 =?utf-8?B?cnNVR3FBa00yOE5kQnZBZmhnVTdpSWl6SWI3bm1NaVdsYks1OEl4RzNHSUEr?=
 =?utf-8?B?OG51NU9MMDAwMllnSm4zbVVJQmZsY3R0ZEMrY1owd3NwdjlYR3BFeGtJTURp?=
 =?utf-8?B?RFJtQUxyR3p2RFk2WjR2eEt3ZVlzNUlNVi92UllNdXRQeVdyQWtqMHFnbDZU?=
 =?utf-8?B?MTg4bHFQMzVJeGxyMHhqU3IyOEtFbktFVGM4RklzSzRURUYvSlN6QzlHK0My?=
 =?utf-8?B?VVRGWm8wQmF4ZHRVOTFJOEE0Z0NmSzZVQkRiR2cyTkdobFA0dWx6MWRvT1Zr?=
 =?utf-8?B?RG9ocitOTFMzdDkwekZoaUJEa3RWa3ZwQXhCcU9BcFJoMmlvemt1MlhmYXlR?=
 =?utf-8?B?ZEVmODZ0TEwwYTBZOWJSOGZaUUtuY1Y0NmpZdGxiYXZjZWVwME5aeW53emhT?=
 =?utf-8?B?T1ppRGNpWklWNFY2RWp5VktzbHFBTTIrdzVFelFtamxrZmp4ZlFaZ28vMStm?=
 =?utf-8?B?Y2JoY1RpZE01MHRZZEtyVzd1NnZjWGFzUTF4dUhtcVJnT2JoL3JnN2tyZ1F1?=
 =?utf-8?B?TkVYVmdpcmdWUmlWZGExdFJwMlE0ckNBcUk1TlNLdlhKZS9HWHB2TEJNenBq?=
 =?utf-8?B?c1VjUHEwTmdjc1N4T0w4blU2TENpQUR4L1BmUVNYOEdZUWFESWNWQUpSOERP?=
 =?utf-8?B?ZWlEYWRoYmo3eXRuNDNLYkdQUVkyL2FnWHVCcWR5eDRqcUgwNXI5elVJY3J3?=
 =?utf-8?B?aGREY1B3R29BdDBDR01oOUVxT1BBUHlKV2hnU0I2OGZxNkpZVVZDT2puRW51?=
 =?utf-8?B?MmdhYzcwY1hydGRFU2h5UnJwb0tKUCsrSDEzQWRBMG14UUxTTmVwZXNQUndJ?=
 =?utf-8?B?RUM5WmhscEtPZEJOSGpVNmhrbmhHTG1wVHFYdWFXTVlncjBzaHpIZG83aytN?=
 =?utf-8?B?dHdndHh2V3o4eitTb25oNUpkakZpNC9mYlBMdVFJUllwZnd3OHFFZzdhcGlo?=
 =?utf-8?B?bGxOZFVuYjM0SEQ1QUw4YVJlTjRIOHVrbUF5Q3c2QTNpNEtYUjdvSDllUWdr?=
 =?utf-8?B?TytVNUhXYWhRS0w4QWwwbGxFa0lzd1VuQWp6eUpUYXMvNUZET3VhYnhCSjk0?=
 =?utf-8?B?Zk1JYll6SmJjNE9vZ1RNYU1YeHJKNi9mQ2tVVTBRMVpnUzJQQSsxdFpLU09E?=
 =?utf-8?B?Ykx0NzdNd3oyMlFySmVPYlVFL21KejF1eWN6czhjM2VUeGU3Z2tkQ0gzNEkw?=
 =?utf-8?B?ZHlreGVONGRRRGFFbW16Vng1Q0VsVlp6MmlRTEF1b2ZzVmdsY0FmTFdERFVu?=
 =?utf-8?B?dGUwTSthRDRINUkrMitZK2ZyNm5DWWtRd1VHWmFtRXBpeWlGYi9KMFlpRUVY?=
 =?utf-8?B?bmRkNGU3ek5tem4yTmMyb1ovTXlBbXZLZ0FraElHY2JVbkJCMCttbHNXL3Az?=
 =?utf-8?B?eXNVV09RbE93dDF0bFJVcm1yY2F1UEQxOGF5Vmo3QWFkdjRobzB2N0tXQzZO?=
 =?utf-8?B?b1R4YzkrMWsrd2YwQldxbjZqeUkyaThYWFFidC9iRXFYeUNuK3FWRzhleU9P?=
 =?utf-8?B?R1RSVXlramhNRkorbFkwZEJUeG41S2tlL2FCSXdPZllFdkVxRFBQcE5meHhC?=
 =?utf-8?B?V1ZXWEUxOWF3bWl4SlBjRGxrUit3Wk14cmRPdHNjQUMyZGFxcGRqVkFYUCtW?=
 =?utf-8?B?U3ptVUVPZGQyeUpncHl3cWE1T0hXT3dIV0RwaStFek0rbDU4eGhqYWFQR0Iw?=
 =?utf-8?B?dnNOUE9BOURia1pPZCtNYzhrQVdVMlpKeDg5RHV6VVJ6RlpYb0MyNFQ1OEVr?=
 =?utf-8?B?UnhmOWdnc0loR284d3FIcUJEMitQblB6clU4YmpDMnFlODF2UC9MNHVuVUdL?=
 =?utf-8?B?N3hkU0U0KzRoUEh2YU5tRDlyQ0ZxenJTeDFPVDE5bGZvcFNETDV2eG5xVUZz?=
 =?utf-8?B?ZzB4YndKMUNwK3JiWEduV1NFcjhXSlFWRDRXU1BjY0dSQUtGbGZ5VHdjb29m?=
 =?utf-8?B?dzREdWwxa2dleFJtVVR3NXF5c3IyOHZ5Q1JBdjk4bXVyRnhNR0tXK2x4QS9s?=
 =?utf-8?B?VWxISWNseW5HZ1FXZnZweitpT211UldneCtZV0wzUVByK0lBK3hTdz09?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e58de08a-95b2-4f00-6906-08da3cdd68c2
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 May 2022 16:58:07.2484
 (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: W5eTVxe6sA0mkdhrvfhPh+OSxRbiuLU7OKKqLfn5FtJG7rlDxsHayHV8LVEkoy0feeelfx8iiJZ3sYpsQ5MaKA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4751
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.874
 definitions=2022-05-23_07:2022-05-23,2022-05-23 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 spamscore=0 mlxscore=0
 phishscore=0 mlxlogscore=999 adultscore=0 malwarescore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2205230096
X-Proofpoint-GUID: 0BF3-ym4jYnTkGPBGXJ0dMV1-nI3oXKG
X-Proofpoint-ORIG-GUID: 0BF3-ym4jYnTkGPBGXJ0dMV1-nI3oXKG
X-Spam-Status: No, score=-5.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


On 5/23/22 8:40 AM, J. Bruce Fields wrote:
> On Mon, May 02, 2022 at 06:38:03PM -0700, dai.ngo@oracle.com wrote:
>> On 5/2/22 6:21 PM, J. Bruce Fields wrote:
>>> On Mon, May 02, 2022 at 09:12:52PM -0400, J. Bruce Fields wrote:
>>>> Looks good to me.
>>> And the only new test failures are due to the new DELAYs on OPEN.
>>> Somebody'll need to fix up pynfs.  (I'm not volunteering for now.)
>> I will fix it, since I broke it :-)
> By the way, I have three more notes on courtesy server stuff that I
> wanted to dump into email before I forget them:
>
> 1. I do still recommend fixing up those pynfs failures.  The ones I see
>     are in RENEW3, LKU10, CLOSE9, CLOSE8, but there may be others.

I had the pynfs fix ready, I just wait for the courteous server patches
to go in 5.19 then submit the pynfs fix. Or do you want me to send it
out now?

>
> 2. In the lock case, nfsd4_lock() holds an st_mutex while calling
>     vfs_lock_file(), which may end up needing to wait for the laundromat.
>     As I said in review, I don't see a potential deadlock there, so I'm
>     fine with the code going in as is.
>
>     But, as a note for possible cleanup, or if this does turn into a
>     problem later: vfs_lock_file could return to nfsd4_lock(), and
>     nfsd4_lock() could easily drop the st_mutex, wait, and retry.
>
>     I think the only trick part would be deciding on conventions for the
>     caller to tell vfs_lock_file() that it shouldn't wait in this case
>     (non-nfsd callers will still want to wait), and for vfs_lock_file()
>     to indicate the caller needs to retry.  Probably something in
>     fl_flags for the former, and an agreed-on error return for the
>     latter?
>
> 3. One other piece of future work would be optimizing the conflicting
>     lock case.  A very premature optimization at this point, but I'm just
>     leaving my notes here in case someone's interested:
>
>     The loop in posix_lock_inode() is currently O(N^2) in the number of
>     expirable clients holding conflicting locks, because each time we
>     encounter one, we wait and then restart.  In practice I doubt that
>     matters--if you have a lot of clients to expire, the time rescanning
>     the list will likely be trivial compared to the time spent waiting
>     for nfsdcld to commit the expiry of each client to stable storage.
>
>     *However*, it might be a more significant optimization if we first
>     allowed more parallelism in nfsdcld.  And that might also benefit
>     some other cases (e.g., lots of clients reconnecting after a crash).
>     We'd need paralle nfsdcld--no idea what that would involve--and I
>     think it'd also help to update the kernel<->nfsdcld protocol with a
>     separate commit operation, so that nfsd could issue a bunch of client
>     changes and then a single commit to wait for them all.
>
>     That done, we could modify the loop in vfs_lock_file() so that, in
>     the case where multiple clients hold conflicting locks, the loop
>     marks them all for expiry in one pass, then waits just once at the
>     end.

Thank you for your notes Bruce, I will keep these in mind.

-Dai

>
> --b.