NFS: Clean up and fix page zeroing when we have short reads From: Trond Myklebust 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 --- fs/nfs/read.c | 107 ++++++++++++++++++++++++++++++++++++++++----------------- 1 files changed, 75 insertions(+), 32 deletions(-) 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 @@ int nfs_return_empty_page(struct page *p 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 @@ static int nfs_readpage_sync(struct nfs_ 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 @@ static void nfs_readpage_result_partial( 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 @@ static const struct rpc_call_ops nfs_rea .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 const struct rpc_call_ops nfs_rea 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); } }