Re: [PATCH 3/7] nfs: simplify nfs_folio_find_and_lock_request

Sagi Grimberg
2024-07-02T07:54:20Z

On 01/07/2024 8:26, Christoph Hellwig wrote:
> nfs_folio_find_and_lock_request and the nfs_page_group_lock_head helper
> called by it spend quite some effort to deal with head vs subrequests.
> But given that only the head request can be stashed in the folio private
> data, non of that is required.
>
> Fold the locking logic from nfs_page_group_lock_head into
> nfs_folio_find_and_lock_request and simplify the result based on the
> invariant that we always find the head request in the folio private data.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/nfs/pagelist.c | 19 -------------------
> fs/nfs/write.c | 38 +++++++++++++++++++++-----------------
> include/linux/nfs_page.h | 1 -
> 3 files changed, 21 insertions(+), 37 deletions(-)
>
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index 3b006bcbcc87a2..e48cc69a2361aa 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -187,25 +187,6 @@ nfs_async_iocounter_wait(struct rpc_task *task, struct nfs_lock_context *l_ctx)
> }
> EXPORT_SYMBOL_GPL(nfs_async_iocounter_wait);
>
> -/*
> - * nfs_page_lock_head_request - page lock the head of the page group
> - * @req: any member of the page group
> - */
> -struct nfs_page *
> -nfs_page_group_lock_head(struct nfs_page *req)
> -{
> - struct nfs_page *head = req->wb_head;
> -
> - while (!nfs_lock_request(head)) {
> - int ret = nfs_wait_on_request(head);
> - if (ret < 0)
> - return ERR_PTR(ret);
> - }
> - if (head != req)
> - kref_get(&head->wb_kref);
> - return head;
> -}
> -
> /*
> * nfs_unroll_locks - unlock all newly locked reqs and wait on @req
> * @head: head request of page group, must be holding head lock
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index 5410c18a006937..58e5b78ff436b9 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -197,28 +197,32 @@ static struct nfs_page *nfs_folio_find_head_request(struct folio *folio)
> static struct nfs_page *nfs_folio_find_and_lock_request(struct folio *folio)
> {
> struct inode *inode = folio->mapping->host;
> - struct nfs_page *req, *head;
> + struct nfs_page *head;
> int ret;
>
> - for (;;) {
> - req = nfs_folio_find_head_request(folio);
> - if (!req)
> - return req;
> - head = nfs_page_group_lock_head(req);
> - if (head != req)
> - nfs_release_request(req);
> - if (IS_ERR(head))
> - return head;
> - ret = nfs_cancel_remove_inode(head, inode);
> - if (ret < 0) {
> - nfs_unlock_and_release_request(head);
> +retry:
> + head = nfs_folio_find_head_request(folio);

Is nfs_folio_find_head_reques() an appropriate name here? it doesn't
search and find afaict (aside from internally the pnfs commits search).
Anyways, Reviewed-by: Sagi Grimberg <sagi@grimberg.me>