Commit b571cfcb authored by Christoph Hellwig's avatar Christoph Hellwig Committed by Anna Schumaker
Browse files

nfs: don't reuse partially completed requests in nfs_lock_and_join_requests 



When NFS requests are split into sub-requests, nfs_inode_remove_request
calls nfs_page_group_sync_on_bit to set PG_REMOVE on this sub-request and
only completes the head requests once PG_REMOVE is set on all requests.
This means that when nfs_lock_and_join_requests sees a PG_REMOVE bit, I/O
on the request is in progress and has partially completed.   If such a
request is returned to nfs_try_to_update_request, it could be extended
with the newly dirtied region and I/O for the combined range will be
re-scheduled, leading to extra I/O.

Change the logic to instead restart the search for a request when any
PG_REMOVE bit is set, as the completion handler will remove the request
as soon as it can take the page group lock.  This not only avoid
extending the I/O but also does the right thing for the callers that
want to cancel or flush the request.

Signed-off-by: default avatarChristoph Hellwig <hch@lst.de>
Signed-off-by: default avatarAnna Schumaker <Anna.Schumaker@Netapp.com>
parent f1b7c755

fs/nfs/write.c

+20 −29
Original line number Diff line number Diff line
@@ -144,31 +144,6 @@ static void nfs_io_completion_put(struct nfs_io_completion *ioc) 
		kref_put(&ioc->refcount, nfs_io_completion_release);

}



static void

nfs_page_set_inode_ref(struct nfs_page *req, struct inode *inode)

{

	if (!test_and_set_bit(PG_INODE_REF, &req->wb_flags)) {

		kref_get(&req->wb_kref);

		atomic_long_inc(&NFS_I(inode)->nrequests);

	}

}



static int

nfs_cancel_remove_inode(struct nfs_page *req, struct inode *inode)

{

	int ret;



	if (!test_bit(PG_REMOVE, &req->wb_flags))

		return 0;

	ret = nfs_page_group_lock(req);

	if (ret)

		return ret;

	if (test_and_clear_bit(PG_REMOVE, &req->wb_flags))

		nfs_page_set_inode_ref(req, inode);

	nfs_page_group_unlock(req);

	return 0;

}



/**

 * nfs_folio_find_head_request - find head request associated with a folio

 * @folio: pointer to folio
@@ -566,6 +541,7 @@ static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio) 
	struct inode *inode = folio->mapping->host;

	struct nfs_page *head, *subreq;

	struct nfs_commit_info cinfo;

	bool removed;

	int ret;



	/*
@@ -590,18 +566,18 @@ static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio) 
		goto retry;

	}



	ret = nfs_cancel_remove_inode(head, inode);

	if (ret < 0)

		goto out_unlock;



	ret = nfs_page_group_lock(head);

	if (ret < 0)

		goto out_unlock;



	removed = test_bit(PG_REMOVE, &head->wb_flags);



	/* lock each request in the page group */

	for (subreq = head->wb_this_page;

	     subreq != head;

	     subreq = subreq->wb_this_page) {

		if (test_bit(PG_REMOVE, &subreq->wb_flags))

			removed = true;

		ret = nfs_page_group_lock_subreq(head, subreq);

		if (ret < 0)

			goto out_unlock;
@@ -609,6 +585,21 @@ static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio) 


	nfs_page_group_unlock(head);



	/*

	 * If PG_REMOVE is set on any request, I/O on that request has

	 * completed, but some requests were still under I/O at the time

	 * we locked the head request.

	 *

	 * In that case the above wait for all requests means that all I/O

	 * has now finished, and we can restart from a clean slate.  Let the

	 * old requests go away and start from scratch instead.

	 */

	if (removed) {

		nfs_unroll_locks(head, head);

		nfs_unlock_and_release_request(head);

		goto retry;

	}



	nfs_init_cinfo_from_inode(&cinfo, inode);

	nfs_join_page_group(head, &cinfo, inode);

	return head;