NFS: Clean up and fix page zeroing when we have short reads
The code that is supposed to zero the uninitialised partial pages when the
server returns a short read is currently broken: it looks at the nfs_page
wb_pgbase and wb_bytes fields instead of the equivalent nfs_read_data
values when deciding where to start truncating the page.
Also ensure that we are more careful about setting PG_uptodate
before retrying a short read: the retry will change the nfs_read_data
args.pgbase and args.count.
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 624ca71..4b5f58d 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -104,6 +104,28 @@
return 0;
}
+static void nfs_readpage_truncate_uninitialised_page(struct nfs_read_data *data)
+{
+ unsigned int remainder = data->args.count - data->res.count;
+ unsigned int base = data->args.pgbase + data->res.count;
+ unsigned int pglen;
+ struct page **pages;
+
+ if (data->res.eof == 0 || remainder == 0)
+ return;
+ /*
+ * Note: "remainder" can never be negative, since we check for
+ * this in the XDR code.
+ */
+ pages = &data->args.pages[base >> PAGE_CACHE_SHIFT];
+ base &= ~PAGE_CACHE_MASK;
+ pglen = PAGE_CACHE_SIZE - base;
+ if (pglen < remainder)
+ memclear_highpage_flush(*pages, base, pglen);
+ else
+ memclear_highpage_flush(*pages, base, remainder);
+}
+
/*
* Read a page synchronously.
*/
@@ -177,11 +199,9 @@
NFS_I(inode)->cache_validity |= NFS_INO_INVALID_ATIME;
spin_unlock(&inode->i_lock);
- if (count)
- memclear_highpage_flush(page, rdata->args.pgbase, count);
- SetPageUptodate(page);
- if (PageError(page))
- ClearPageError(page);
+ nfs_readpage_truncate_uninitialised_page(rdata);
+ if (rdata->res.eof || rdata->res.count == rdata->args.count)
+ SetPageUptodate(page);
result = 0;
io_error:
@@ -436,20 +456,12 @@
struct nfs_page *req = data->req;
struct page *page = req->wb_page;
+ if (likely(task->tk_status >= 0))
+ nfs_readpage_truncate_uninitialised_page(data);
+ else
+ SetPageError(page);
if (nfs_readpage_result(task, data) != 0)
return;
- if (task->tk_status >= 0) {
- unsigned int request = data->args.count;
- unsigned int result = data->res.count;
-
- if (result < request) {
- memclear_highpage_flush(page,
- data->args.pgbase + result,
- request - result);
- }
- } else
- SetPageError(page);
-
if (atomic_dec_and_test(&req->wb_complete)) {
if (!PageError(page))
SetPageUptodate(page);
@@ -462,6 +474,40 @@
.rpc_release = nfs_readdata_release,
};
+static void nfs_readpage_set_pages_uptodate(struct nfs_read_data *data)
+{
+ unsigned int count = data->res.count;
+ unsigned int base = data->args.pgbase;
+ struct page **pages;
+
+ if (unlikely(count == 0))
+ return;
+ pages = &data->args.pages[base >> PAGE_CACHE_SHIFT];
+ base &= ~PAGE_CACHE_MASK;
+ count += base;
+ for (;count >= PAGE_CACHE_SIZE; count -= PAGE_CACHE_SIZE, pages++)
+ SetPageUptodate(*pages);
+ /*
+ * Was this an eof or a short read? If the latter, don't mark the page
+ * as uptodate yet.
+ */
+ if (count > 0 && (data->res.eof || data->args.count == data->res.count))
+ SetPageUptodate(*pages);
+}
+
+static void nfs_readpage_set_pages_error(struct nfs_read_data *data)
+{
+ unsigned int count = data->args.count;
+ unsigned int base = data->args.pgbase;
+ struct page **pages;
+
+ pages = &data->args.pages[base >> PAGE_CACHE_SHIFT];
+ base &= ~PAGE_CACHE_MASK;
+ count += base;
+ for (;count >= PAGE_CACHE_SIZE; count -= PAGE_CACHE_SIZE, pages++)
+ SetPageError(*pages);
+}
+
/*
* This is the callback from RPC telling us whether a reply was
* received or some error occurred (timeout or socket shutdown).
@@ -469,27 +515,24 @@
static void nfs_readpage_result_full(struct rpc_task *task, void *calldata)
{
struct nfs_read_data *data = calldata;
- unsigned int count = data->res.count;
+ /*
+ * Note: nfs_readpage_result may change the values of
+ * data->args. In the multi-page case, we therefore need
+ * to ensure that we call the next nfs_readpage_set_page_uptodate()
+ * first in the multi-page case.
+ */
+ if (likely(task->tk_status >= 0)) {
+ nfs_readpage_truncate_uninitialised_page(data);
+ nfs_readpage_set_pages_uptodate(data);
+ } else
+ nfs_readpage_set_pages_error(data);
if (nfs_readpage_result(task, data) != 0)
return;
while (!list_empty(&data->pages)) {
struct nfs_page *req = nfs_list_entry(data->pages.next);
- struct page *page = req->wb_page;
- nfs_list_remove_request(req);
- if (task->tk_status >= 0) {
- if (count < PAGE_CACHE_SIZE) {
- if (count < req->wb_bytes)
- memclear_highpage_flush(page,
- req->wb_pgbase + count,
- req->wb_bytes - count);
- count = 0;
- } else
- count -= PAGE_CACHE_SIZE;
- SetPageUptodate(page);
- } else
- SetPageError(page);
+ nfs_list_remove_request(req);
nfs_readpage_release(req);
}
}