diff mbox series

[19/25] NFS: Replace custom error reporting mechanism with generic one

Message ID 20190328205239.29674-20-trond.myklebust@hammerspace.com (mailing list archive)
State New, archived
Headers show
Series Fix up soft mounts for NFSv4.x | expand

Commit Message

Trond Myklebust March 28, 2019, 8:52 p.m. UTC
Replace the NFS custom error reporting mechanism with the generic
mapping_set_error().

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/file.c          | 25 ++++---------------------
 fs/nfs/internal.h      |  6 ------
 fs/nfs/write.c         | 37 ++++++++++++++++++++-----------------
 include/linux/nfs_fs.h |  1 -
 4 files changed, 24 insertions(+), 45 deletions(-)

Comments

Dan Carpenter April 1, 2019, 6:36 a.m. UTC | #1
Hi Trond,

url:    https://github.com/0day-ci/linux/commits/Trond-Myklebust/Fix-up-soft-mounts-for-NFSv4-x/20190331-130454
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next

New smatch warnings:
fs/nfs/write.c:1849 nfs_commit_release_pages() error: we previously assumed 'req->wb_page' could be null (see line 1840)
fs/nfs/write.c:1850 nfs_commit_release_pages() warn: variable dereferenced before check 'req->wb_page' (see line 1849)


# https://github.com/0day-ci/linux/commit/2f2cfd52810bf732da40334bf590dbadda75d6cf
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 2f2cfd52810bf732da40334bf590dbadda75d6cf
vim +1849 fs/nfs/write.c

c9d8f89d9 Trond Myklebust 2008-04-15  1829  
f453a54a0 Fred Isaman     2012-04-20  1830  static void nfs_commit_release_pages(struct nfs_commit_data *data)
c9d8f89d9 Trond Myklebust 2008-04-15  1831  {
c9d8f89d9 Trond Myklebust 2008-04-15  1832  	struct nfs_page	*req;
c9d8f89d9 Trond Myklebust 2008-04-15  1833  	int status = data->task.tk_status;
f453a54a0 Fred Isaman     2012-04-20  1834  	struct nfs_commit_info cinfo;
353db7966 NeilBrown       2014-09-24  1835  	struct nfs_server *nfss;
788e7a89a Trond Myklebust 2006-03-20  1836  
^1da177e4 Linus Torvalds  2005-04-16  1837  	while (!list_empty(&data->pages)) {
^1da177e4 Linus Torvalds  2005-04-16  1838  		req = nfs_list_entry(data->pages.next);
^1da177e4 Linus Torvalds  2005-04-16  1839  		nfs_list_remove_request(req);
67911c8f1 Anna Schumaker  2016-01-19 @1840  		if (req->wb_page)
                                                            ^^^^^^^^^^^^

d6d6dc7cd Fred Isaman     2012-03-08  1841  			nfs_clear_page_commit(req->wb_page);
^1da177e4 Linus Torvalds  2005-04-16  1842  
1e8968c5b Niels de Vos    2013-12-17  1843  		dprintk("NFS:       commit (%s/%llu %d@%lld)",
3d4ff43d8 Al Viro         2011-06-22  1844  			req->wb_context->dentry->d_sb->s_id,
2b0143b5c David Howells   2015-03-17  1845  			(unsigned long long)NFS_FILEID(d_inode(req->wb_context->dentry)),
^1da177e4 Linus Torvalds  2005-04-16  1846  			req->wb_bytes,
^1da177e4 Linus Torvalds  2005-04-16  1847  			(long long)req_offset(req));
c9d8f89d9 Trond Myklebust 2008-04-15  1848  		if (status < 0) {
2f2cfd528 Trond Myklebust 2019-03-28 @1849  			nfs_mapping_set_error(req->wb_page, status);
                                                                                      ^^^^^^^^^^^^
Dereferenced inside function probably?

38a33101d Kinglong Mee    2017-03-09 @1850  			if (req->wb_page)
                                                                    ^^^^^^^^^^^^
Too late.

^1da177e4 Linus Torvalds  2005-04-16  1851  				nfs_inode_remove_request(req);
ddeaa6379 Joe Perches     2016-10-15  1852  			dprintk_cont(", error = %d\n", status);
^1da177e4 Linus Torvalds  2005-04-16  1853  			goto next;
^1da177e4 Linus Torvalds  2005-04-16  1854  		}
^1da177e4 Linus Torvalds  2005-04-16  1855  

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
diff mbox series

Patch

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index f807e8643ae6..144e183250c3 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -199,13 +199,6 @@  EXPORT_SYMBOL_GPL(nfs_file_mmap);
  * Flush any dirty pages for this process, and check for write errors.
  * The return status from this call provides a reliable indication of
  * whether any write errors occurred for this process.
- *
- * Notice that it clears the NFS_CONTEXT_ERROR_WRITE before synching to
- * disk, but it retrieves and clears ctx->error after synching, despite
- * the two being set at the same time in nfs_context_set_write_error().
- * This is because the former is used to notify the _next_ call to
- * nfs_file_write() that a write error occurred, and hence cause it to
- * fall back to doing a synchronous write.
  */
 static int
 nfs_file_fsync_commit(struct file *file, int datasync)
@@ -220,11 +213,8 @@  nfs_file_fsync_commit(struct file *file, int datasync)
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend = test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
 	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	if (test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags)) {
-		ret = xchg(&ctx->error, 0);
-		if (ret)
-			goto out;
-	}
+	if (status == 0)
+		status = file_check_and_advance_wb_err(file);
 	if (status < 0) {
 		ret = status;
 		goto out;
@@ -245,13 +235,7 @@  nfs_file_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 	trace_nfs_fsync_enter(inode);
 
 	do {
-		struct nfs_open_context *ctx = nfs_file_open_context(file);
-		ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
-		if (test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags)) {
-			int ret2 = xchg(&ctx->error, 0);
-			if (ret2)
-				ret = ret2;
-		}
+		ret = file_write_and_wait_range(file, start, end);
 		if (ret != 0)
 			break;
 		ret = nfs_file_fsync_commit(file, datasync);
@@ -600,8 +584,7 @@  static int nfs_need_check_write(struct file *filp, struct inode *inode)
 	struct nfs_open_context *ctx;
 
 	ctx = nfs_file_open_context(filp);
-	if (test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags) ||
-	    nfs_ctx_key_to_expire(ctx, inode))
+	if (nfs_ctx_key_to_expire(ctx, inode))
 		return 1;
 	return 0;
 }
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 3cefd0ed01be..196534634c3a 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -773,9 +773,3 @@  static inline bool nfs_error_is_fatal(int err)
 	}
 }
 
-static inline void nfs_context_set_write_error(struct nfs_open_context *ctx, int error)
-{
-	ctx->error = error;
-	smp_wmb();
-	set_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
-}
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 64cf6a340ba6..627a2b6ed1fe 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -244,6 +244,12 @@  static void nfs_set_pageerror(struct address_space *mapping)
 	nfs_zap_mapping(mapping->host, mapping);
 }
 
+static void nfs_mapping_set_error(struct page *page, int error)
+{
+	SetPageError(page);
+	mapping_set_error(page_file_mapping(page), error);
+}
+
 /*
  * nfs_page_group_search_locked
  * @head - head request of page group
@@ -582,9 +588,9 @@  nfs_lock_and_join_requests(struct page *page)
 	return ERR_PTR(ret);
 }
 
-static void nfs_write_error_remove_page(struct nfs_page *req)
+static void nfs_write_error(struct nfs_page *req, int error)
 {
-	SetPageError(req->wb_page);
+	nfs_mapping_set_error(req->wb_page, error);
 	nfs_end_page_writeback(req);
 	nfs_release_request(req);
 }
@@ -608,6 +614,7 @@  nfs_error_is_fatal_on_server(int err)
 static int nfs_page_async_flush(struct nfs_pageio_descriptor *pgio,
 				struct page *page)
 {
+	struct address_space *mapping;
 	struct nfs_page *req;
 	int ret = 0;
 
@@ -621,19 +628,19 @@  static int nfs_page_async_flush(struct nfs_pageio_descriptor *pgio,
 	nfs_set_page_writeback(page);
 	WARN_ON_ONCE(test_bit(PG_CLEAN, &req->wb_flags));
 
-	ret = req->wb_context->error;
 	/* If there is a fatal error that covers this write, just exit */
-	if (nfs_error_is_fatal_on_server(ret))
+	ret = 0;
+	mapping = page_file_mapping(page);
+	if (test_bit(AS_ENOSPC, &mapping->flags) ||
+	    test_bit(AS_EIO, &mapping->flags))
 		goto out_launder;
 
-	ret = 0;
 	if (!nfs_pageio_add_request(pgio, req)) {
 		ret = pgio->pg_error;
 		/*
 		 * Remove the problematic req upon fatal errors on the server
 		 */
 		if (nfs_error_is_fatal(ret)) {
-			nfs_context_set_write_error(req->wb_context, ret);
 			if (nfs_error_is_fatal_on_server(ret))
 				goto out_launder;
 		} else
@@ -645,7 +652,7 @@  static int nfs_page_async_flush(struct nfs_pageio_descriptor *pgio,
 out:
 	return ret;
 out_launder:
-	nfs_write_error_remove_page(req);
+	nfs_write_error(req, ret);
 	return 0;
 }
 
@@ -998,7 +1005,7 @@  static void nfs_write_completion(struct nfs_pgio_header *hdr)
 		if (test_bit(NFS_IOHDR_ERROR, &hdr->flags) &&
 		    (hdr->good_bytes < bytes)) {
 			nfs_set_pageerror(page_file_mapping(req->wb_page));
-			nfs_context_set_write_error(req->wb_context, hdr->error);
+			nfs_mapping_set_error(req->wb_page, hdr->error);
 			goto remove_req;
 		}
 		if (nfs_write_need_commit(hdr)) {
@@ -1422,14 +1429,10 @@  static void nfs_async_write_error(struct list_head *head, int error)
 	while (!list_empty(head)) {
 		req = nfs_list_entry(head->next);
 		nfs_list_remove_request(req);
-		if (nfs_error_is_fatal(error)) {
-			nfs_context_set_write_error(req->wb_context, error);
-			if (nfs_error_is_fatal_on_server(error)) {
-				nfs_write_error_remove_page(req);
-				continue;
-			}
-		}
-		nfs_redirty_request(req);
+		if (nfs_error_is_fatal(error))
+			nfs_write_error(req, error);
+		else
+			nfs_redirty_request(req);
 	}
 }
 
@@ -1843,7 +1846,7 @@  static void nfs_commit_release_pages(struct nfs_commit_data *data)
 			req->wb_bytes,
 			(long long)req_offset(req));
 		if (status < 0) {
-			nfs_context_set_write_error(req->wb_context, status);
+			nfs_mapping_set_error(req->wb_page, status);
 			if (req->wb_page)
 				nfs_inode_remove_request(req);
 			dprintk_cont(", error = %d\n", status);
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 40e30376130b..d363d5765cdf 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -76,7 +76,6 @@  struct nfs_open_context {
 	fmode_t mode;
 
 	unsigned long flags;
-#define NFS_CONTEXT_ERROR_WRITE		(0)
 #define NFS_CONTEXT_RESEND_WRITES	(1)
 #define NFS_CONTEXT_BAD			(2)
 #define NFS_CONTEXT_UNLOCK	(3)