RFC: Fix nfs_generic_pgio page vector issues

This patchset fixes a few issues with the creation of nfs_pageio_descriptor page vectors, which are passed to nfs_pgio_header structures.

These were first found by Toralf Förster <[email protected]> by running trinity, but I've since received a report of a real world usecase that hit a related issue: kvm with cache=none.

There are two problems that are closely related and seem to only be triggered by direct i/o writev()/readv() calls.

The fixes are:
- do not coalesce pages unless they are contiguous in file position *and* within / betwen pages.

- do not allow duplicated pages in the pagevector

I'm still testing these, but I thought I'd share what I have with the list.

-dros

Weston Andros Adamson (2):
nfs: disallow duplicate pages in pgio page vectors
nfs: can_coalesce_requests must enforce contiguity

 fs/nfs/pagelist.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

1.8.5.2 (Apple Git-48)

Adjacent requests that share the same page are allowed, but should only use one entry in the page vector. This avoids overruning the page vector - it is sized based on how many bytes there are, not by request count.

This fixes issues that manifest as "Redzone overwritten" bugs (the vector overrun) and hangs waiting on page read / write, as it waits on the same page more than once.

This also adds bounds checking to the page vector with a graceful failure (WARN_ON_ONCE and pgio error returned to application).

---
 fs/nfs/pagelist.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

index e0c2e72..73476df 100644
@@ -733,10 +733,11 @@ int nfs_generic_pgio(struct nfs_pageio_descriptor *desc,
 		     struct nfs_pgio_header *hdr)
 {
 	struct nfs_page		*req;
-	struct page		**pages;
+	struct page		**pages,
+				*last_page;
 	struct list_head *head = &desc->pg_list;
 	struct nfs_commit_info cinfo;
-	unsigned int pagecount;
+	unsigned int pagecount, pageused;
 
 	pagecount = nfs_page_array_len(desc->pg_base, desc->pg_count);
 	if (!nfs_pgarray_set(&hdr->page_array, pagecount))

@@ -744,11 +745,26 @@ int nfs_generic_pgio(struct nfs_pageio_descriptor *desc,
 
 	nfs_init_cinfo(&cinfo, desc->pg_inode, desc->pg_dreq);
 	pages = hdr->page_array.pagevec;
+	last_page = NULL;
+	pageused = 0;
 	while (!list_empty(head)) {
 		req = nfs_list_entry(head->next);
 		nfs_list_remove_request(req);
 		nfs_list_add_request(req, &hdr->pages);
-		*pages++ = req->wb_page;
+
+		if (pageused >= pagecount) {
+			WARN_ON_ONCE(1);
+			return nfs_pgio_error(desc, hdr);
+		}
+
+		if (!last_page || last_page != req->wb_page) {
+			*pages++ = last_page = req->wb_page;
+			pageused++;
+		}
+	}
+	if (pageused != pagecount) {
+		WARN_ON_ONCE(1);
+		return nfs_pgio_error(desc, hdr);
 	}
 
 	if ((desc->pg_ioflags & FLUSH_COND_STABLE) &&

1.8.5.2 (Apple Git-48)

Commit 6094f83864c1d1296566a282cba05ba613f151ee
"nfs: allow coalescing of subpage requests" got rid of the requirement that requests cover whole pages, but it made some incorrect assumptions.

It turns out that callers of this interface can map adjacent requests (by file position as seen by req_offset + req->wb_bytes) to different pages, even when they could share a page. An example is the direct I/O interface - iov_iter_get_pages_alloc may return one segment with a partial page filled and the next segment (which is adjacent in the file position) starts with a new page.

---
 fs/nfs/pagelist.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index 73476df..d512887 100644
@@ -836,6 +836,14 @@ static bool nfs_can_coalesce_requests(struct nfs_page *prev,
 			return false;
 		if (req_offset(req) != req_offset(prev) + prev->wb_bytes)
 			return false;
+		if (req->wb_page == prev->wb_page) {
+			if (req->wb_pgbase != prev->wb_pgbase + prev->wb_bytes)
+				return false;
+		} else {
+			if (req->wb_pgbase != 0 ||
+			    prev->wb_pgbase + prev->wb_bytes != PAGE_CACHE_SIZE)
+				return false;
+		}
 	}
 	size = pgio->pg_ops->pg_test(pgio, prev, req);
 	WARN_ON_ONCE(size > req->wb_bytes);

1.8.5.2 (Apple Git-48)

This survived a full day of running trinity on one system and a full day of various iozone / fio on another.

It’s good to go from my perspective.

-dros

Test passed here too within 32 bit UML guests.

Toralf