Re: [PATCH 4/7] nfs: fold nfs_folio_find_and_lock_request into nfs_lock_and_join_requests

Sagi Grimberg
2024-07-02T07:57:39Z

On 01/07/2024 8:26, Christoph Hellwig wrote:
> Fold nfs_folio_find_and_lock_request into the only caller to prepare
> for changes to this code.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/nfs/write.c | 68 ++++++++++++++++++++------------------------------
> 1 file changed, 27 insertions(+), 41 deletions(-)
>
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index 58e5b78ff436b9..2b139493168d87 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -194,38 +194,6 @@ static struct nfs_page *nfs_folio_find_head_request(struct folio *folio)
> return req;
> }
>
> -static struct nfs_page *nfs_folio_find_and_lock_request(struct folio *folio)
> -{
> - struct inode *inode = folio->mapping->host;
> - struct nfs_page *head;
> - int ret;
> -
> -retry:
> - head = nfs_folio_find_head_request(folio);
> - if (!head)
> - return NULL;
> -
> - while (!nfs_lock_request(head)) {
> - ret = nfs_wait_on_request(head);
> - if (ret < 0)
> - return ERR_PTR(ret);
> - }
> -
> - /* Ensure that nobody removed the request before we locked it */
> - if (head != folio->private) {
> - nfs_unlock_and_release_request(head);
> - goto retry;
> - }
> -
> - ret = nfs_cancel_remove_inode(head, inode);
> - if (ret < 0) {
> - nfs_unlock_and_release_request(head);
> - return ERR_PTR(ret);
> - }
> -
> - return head;
> -}
> -
> /* Adjust the file length if we're writing beyond the end */
> static void nfs_grow_file(struct folio *folio, unsigned int offset,
> unsigned int count)
> @@ -530,26 +498,44 @@ static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio)
> struct nfs_commit_info cinfo;
> int ret;
>
> - nfs_init_cinfo_from_inode(&cinfo, inode);
> /*
> * A reference is taken only on the head request which acts as a
> * reference to the whole page group - the group will not be destroyed
> * until the head reference is released.
> */
> - head = nfs_folio_find_and_lock_request(folio);
> - if (IS_ERR_OR_NULL(head))
> - return head;
> +retry:
> + head = nfs_folio_find_head_request(folio);
> + if (!head)
> + return NULL;
>
> - /* lock each request in the page group */
> - ret = nfs_page_group_lock_subrequests(head);
> - if (ret < 0) {
> + while (!nfs_lock_request(head)) {
> + ret = nfs_wait_on_request(head);
> + if (ret < 0)
> + return ERR_PTR(ret);
> + }
> +
> + /* Ensure that nobody removed the request before we locked it */
> + if (head != folio->private) {
> nfs_unlock_and_release_request(head);
> - return ERR_PTR(ret);
> + goto retry;
> }
>
> - nfs_join_page_group(head, &cinfo, inode);
> + ret = nfs_cancel_remove_inode(head, inode);
> + if (ret < 0)
> + goto out_unlock;
>
> + /* lock each request in the page group */
> + ret = nfs_page_group_lock_subrequests(head);
> + if (ret < 0)
> + goto out_unlock;
> +
> + nfs_init_cinfo_from_inode(&cinfo, inode);

Any reason why nfs_init_cinfo_from_inode() changed location?

Otherwise,
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>