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).
Reported-by: Toralf Förster <[email protected]>
Signed-off-by: Weston Andros Adamson <[email protected]>
---
fs/nfs/pagelist.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index e0c2e72..73476df 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -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)
On 08/15/2014 04:36 PM, Weston Andros Adamson wrote:
> 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
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
On Aug 14, 2014, at 5:39 PM, Weston Andros Adamson <[email protected]> wrote:
> 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)
>
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.
Reported-by: Toralf Förster <[email protected]>
Signed-off-by: Weston Andros Adamson <[email protected]>
---
fs/nfs/pagelist.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 73476df..d512887 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -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)