nfs: track writeback errors with errseq_t

PATCH In review Jeff Layton 7 participants 26 messages Jul 20, 2017 – Sep 28, 2017 2 messages not in the archive

Subsystems (4)

VFS NFS client NFS server Man-pages: manual pages for Linux -- Sections 2, 3, 4, 5, and 7

lore

Share & tools

lore

Jeff Layton

author maintainer

2017-07-20 19:42 UTC #0 lore raw

PATCH 1/1 nfs: track writeback errors with errseq_t

From: Jeff Layton <[email protected]>

There is some ambiguity in nfs about how writeback errors are tracked.

For instance, nfs_pageio_add_request calls mapping_set_error when the add fails, but we track errors that occur after adding the request with a dedicated int error in the open context.

Now that we have better infrastructure for the vfs layer, this latter int is now unnecessary. Just have nfs_context_set_write_error set the error in the mapping when one occurs.

Have NFS use file_write_and_wait_range to initiate and wait on writeback of the data, and then check again after issuing the commit(s).

With this, we also don't need to pay attention to the ERROR_WRITE flag for reporting, and just clear it to indicate to subsequent writers that they should try to go asynchronous again.

In nfs_page_async_flush, sample the error before locking and joining the requests, and check for errors since that point.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfs/file.c          | 24 +++++++++++-------------
 fs/nfs/inode.c         |  3 +--
 fs/nfs/write.c         |  8 ++++++--
 include/linux/nfs_fs.h |  1 -
 4 files changed, 18 insertions(+), 18 deletions(-)

I have a baling wire and duct tape solution for testing this with xfstests (using iptables REJECT targets and soft mounts). This seems to make nfs do the right thing.

fs/nfs/file.c +11 ~~-13~~

index 5713eb32a45e..15d3c6faafd3 100644
@@ -212,25 +212,23 @@ nfs_file_fsync_commit(struct file *file, loff_t start, loff_t end, int datasync)
 {
 	struct nfs_open_context *ctx = nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
-	int ret = 0;
+	int do_resend, status;
+	int ret;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file, datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend = test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
-	have_error = test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
-	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
-	if (have_error) {
-		ret = xchg(&ctx->error, 0);
-		if (ret)
-			goto out;
-	}
-	if (status < 0) {
+	clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
+	ret = nfs_commit_inode(inode, FLUSH_SYNC);
+
+	/* Recheck and advance after the commit */
+	status = file_check_and_advance_wb_err(file);
+	if (!ret)
 		ret = status;
+	if (ret)
 		goto out;
-	}
+
 	do_resend |= test_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
 	if (do_resend)
 		ret = -EAGAIN;

@@ -247,7 +245,7 @@ nfs_file_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 	trace_nfs_fsync_enter(inode);
 
 	do {
-		ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
+		ret = file_write_and_wait_range(file, start, end);
 		if (ret != 0)
 			break;
 		ret = nfs_file_fsync_commit(file, start, end, datasync);

fs/nfs/inode.c +1 -2

index 109279d6d91b..c48f673c5cc9 100644
@@ -900,7 +900,6 @@ struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry,
 	ctx->state = NULL;
 	ctx->mode = f_mode;
 	ctx->flags = 0;
-	ctx->error = 0;
 	ctx->flock_owner = (fl_owner_t)filp;
 	nfs_init_lock_context(&ctx->lock_context);
 	ctx->lock_context.open_context = ctx;

@@ -1009,7 +1008,7 @@ void nfs_file_clear_open_context(struct file *filp)
 		 * We fatal error on write before. Try to writeback
 		 * every page again.
 		 */
-		if (ctx->error < 0)
+		if (filemap_check_wb_err(inode->i_mapping, filp->f_wb_err))
 			invalidate_inode_pages2(inode->i_mapping);
 		filp->private_data = NULL;
 		spin_lock(&inode->i_lock);

fs/nfs/write.c +6 -2

index b1af5dee5e0a..c2fcaf07cd24 100644
@@ -149,7 +149,9 @@ static void nfs_io_completion_put(struct nfs_io_completion *ioc)
 
 static void nfs_context_set_write_error(struct nfs_open_context *ctx, int error)
 {
-	ctx->error = error;
+	struct inode *inode = d_inode(ctx->dentry);
+
+	mapping_set_error(inode->i_mapping, error);
 	smp_wmb();
 	set_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
 }

@@ -628,6 +630,8 @@ static int nfs_page_async_flush(struct nfs_pageio_descriptor *pgio,
 {
 	struct nfs_page *req;
 	int ret = 0;
+	struct address_space *mapping = page_file_mapping(page);
+	errseq_t since = filemap_sample_wb_err(mapping);
 
 	req = nfs_lock_and_join_requests(page, nonblock);
 	if (!req)

@@ -641,7 +645,7 @@ static int nfs_page_async_flush(struct nfs_pageio_descriptor *pgio,
 
 	ret = 0;
 	/* If there is a fatal error that covers this write, just exit */
-	if (nfs_error_is_fatal_on_server(req->wb_context->error))
+	if (nfs_error_is_fatal_on_server(filemap_check_wb_err(mapping, since)))
 		goto out_launder;
 
 	if (!nfs_pageio_add_request(pgio, req)) {

include/linux/nfs_fs.h +0 -1

index e52cc55ac300..a96b0bd52b32 100644
@@ -77,7 +77,6 @@ struct nfs_open_context {
 #define NFS_CONTEXT_RESEND_WRITES	(1)
 #define NFS_CONTEXT_BAD			(2)
 #define NFS_CONTEXT_UNLOCK	(3)
-	int error;
 
 	struct list_head list;
 	struct nfs4_threshold	*mdsthreshold;

Signature · 1 line

2.13.3

Jeff Layton

2017-08-25 17:59 UTC #1 lore raw

Quoted 142 lines · Jeff Layton

From: Jeff Layton <[email protected]>

There is some ambiguity in nfs about how writeback errors are tracked.

For instance, nfs_pageio_add_request calls mapping_set_error when the add fails, but we track errors that occur after adding the request with a dedicated int error in the open context.

Now that we have better infrastructure for the vfs layer, this latter int is now unnecessary. Just have nfs_context_set_write_error set the error in the mapping when one occurs.

Have NFS use file_write_and_wait_range to initiate and wait on writeback of the data, and then check again after issuing the commit(s).

With this, we also don't need to pay attention to the ERROR_WRITE flag for reporting, and just clear it to indicate to subsequent writers that they should try to go asynchronous again.

In nfs_page_async_flush, sample the error before locking and joining the requests, and check for errors since that point.

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfs/file.c          | 24 +++++++++++-------------
 fs/nfs/inode.c         |  3 +--
 fs/nfs/write.c         |  8 ++++++--
 include/linux/nfs_fs.h |  1 -
 4 files changed, 18 insertions(+), 18 deletions(-)

I have a baling wire and duct tape solution for testing this with xfstests (using iptables REJECT targets and soft mounts). This seems to make nfs do the right thing.

fs/nfs/file.c +11 ~~-13~~

index 5713eb32a45e..15d3c6faafd3 100644
@@ -212,25 +212,23 @@ nfs_file_fsync_commit(struct file *file, loff_t start, loff_t end, int datasync)
 {
 	struct nfs_open_context *ctx = nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
-	int ret = 0;
+	int do_resend, status;
+	int ret;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file, datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend = test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
-	have_error = test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
-	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
-	if (have_error) {
-		ret = xchg(&ctx->error, 0);
-		if (ret)
-			goto out;
-	}
-	if (status < 0) {
+	clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
+	ret = nfs_commit_inode(inode, FLUSH_SYNC);
+
+	/* Recheck and advance after the commit */
+	status = file_check_and_advance_wb_err(file);
+	if (!ret)
 		ret = status;
+	if (ret)
 		goto out;
-	}
+
 	do_resend |= test_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
 	if (do_resend)
 		ret = -EAGAIN;

@@ -247,7 +245,7 @@ nfs_file_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 	trace_nfs_fsync_enter(inode);
 
 	do {
-		ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
+		ret = file_write_and_wait_range(file, start, end);
 		if (ret != 0)
 			break;
 		ret = nfs_file_fsync_commit(file, start, end, datasync);

fs/nfs/inode.c +1 -2

index 109279d6d91b..c48f673c5cc9 100644
@@ -900,7 +900,6 @@ struct nfs_open_context *alloc_nfs_open_context(struct dentry *dentry,
 	ctx->state = NULL;
 	ctx->mode = f_mode;
 	ctx->flags = 0;
-	ctx->error = 0;
 	ctx->flock_owner = (fl_owner_t)filp;
 	nfs_init_lock_context(&ctx->lock_context);
 	ctx->lock_context.open_context = ctx;

@@ -1009,7 +1008,7 @@ void nfs_file_clear_open_context(struct file *filp)
 		 * We fatal error on write before. Try to writeback
 		 * every page again.
 		 */
-		if (ctx->error < 0)
+		if (filemap_check_wb_err(inode->i_mapping, filp->f_wb_err))
 			invalidate_inode_pages2(inode->i_mapping);
 		filp->private_data = NULL;
 		spin_lock(&inode->i_lock);

fs/nfs/write.c +6 -2

index b1af5dee5e0a..c2fcaf07cd24 100644
@@ -149,7 +149,9 @@ static void nfs_io_completion_put(struct nfs_io_completion *ioc)
 
 static void nfs_context_set_write_error(struct nfs_open_context *ctx, int error)
 {
-	ctx->error = error;
+	struct inode *inode = d_inode(ctx->dentry);
+
+	mapping_set_error(inode->i_mapping, error);
 	smp_wmb();
 	set_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
 }

@@ -628,6 +630,8 @@ static int nfs_page_async_flush(struct nfs_pageio_descriptor *pgio,
 {
 	struct nfs_page *req;
 	int ret = 0;
+	struct address_space *mapping = page_file_mapping(page);
+	errseq_t since = filemap_sample_wb_err(mapping);
 
 	req = nfs_lock_and_join_requests(page, nonblock);
 	if (!req)

@@ -641,7 +645,7 @@ static int nfs_page_async_flush(struct nfs_pageio_descriptor *pgio,
 
 	ret = 0;
 	/* If there is a fatal error that covers this write, just exit */
-	if (nfs_error_is_fatal_on_server(req->wb_context->error))
+	if (nfs_error_is_fatal_on_server(filemap_check_wb_err(mapping, since)))
 		goto out_launder;
 
 	if (!nfs_pageio_add_request(pgio, req)) {

include/linux/nfs_fs.h +0 -0

index e52cc55ac300..a96b0bd52b32 100644

include/linux/nfs_fs.h struct nfs_open_context { · line 82

Quoted 7 lines

include/linux/nfs_fs.h +0 -1

@@ -77,7 +77,6 @@ struct nfs_open_context {
 #define NFS_CONTEXT_RESEND_WRITES	(1)
 #define NFS_CONTEXT_BAD			(2)
 #define NFS_CONTEXT_UNLOCK	(3)
-	int error;
 
 	struct list_head list;
 	struct nfs4_threshold	*mdsthreshold;

Anna and Trond,

Ping? I haven't seen any word on this patch, and it hasn't shown up in any branches. Do you have concerns with it, or is this good to go for v4.14?

Thanks,

Signature · 1 line

Jeff Layton <[email protected]>

NeilBrown

2017-08-27 23:25 UTC #2 lore raw

Quoted 50 lines · Jeff Layton

Quoted 49 lines · Jeff Layton
From: Jeff Layton <[email protected]>
There is some ambiguity in nfs about how writeback errors are tracked.
For instance, nfs_pageio_add_request calls mapping_set_error when the add fails, but we track errors that occur after adding the request with a dedicated int error in the open context.
Now that we have better infrastructure for the vfs layer, this latter int is now unnecessary. Just have nfs_context_set_write_error set the error in the mapping when one occurs.
Have NFS use file_write_and_wait_range to initiate and wait on writeback of the data, and then check again after issuing the commit(s).
With this, we also don't need to pay attention to the ERROR_WRITE flag for reporting, and just clear it to indicate to subsequent writers that they should try to go asynchronous again.
In nfs_page_async_flush, sample the error before locking and joining the requests, and check for errors since that point.
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfs/file.c          | 24 +++++++++++-------------
 fs/nfs/inode.c         |  3 +--
 fs/nfs/write.c         |  8 ++++++--
 include/linux/nfs_fs.h |  1 -
 4 files changed, 18 insertions(+), 18 deletions(-)
I have a baling wire and duct tape solution for testing this with xfstests (using iptables REJECT targets and soft mounts). This seems to make nfs do the right thing.
fs/nfs/file.c +2 -2
index 5713eb32a45e..15d3c6faafd3 100644
@@ -212,25 +212,23 @@ nfs_file_fsync_commit(struct file *file, loff_t start, loff_t end, int datasync)
 {
 	struct nfs_open_context *ctx = nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
-	int ret = 0;
+	int do_resend, status;
+	int ret;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file, datasync);
 

Quoted 16 lines

Quoted 6 lines

fs/nfs/file.c +0 -4

 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend = test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
-	have_error = test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
-	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
-	if (have_error) {

fs/nfs/file.c nfs_file_fsync_commit(struct file *file, loff_t start, loff_t end, int datasync) · line 226

Quoted 10 lines

fs/nfs/file.c +5 -5

-		ret = xchg(&ctx->error, 0);
-		if (ret)
-			goto out;
-	}
-	if (status < 0) {
+	clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
+	ret = nfs_commit_inode(inode, FLUSH_SYNC);
+
+	/* Recheck and advance after the commit */
+	status = file_check_and_advance_wb_err(file);

This change makes the code inconsistent with the comment above the function, which still references ctx->error. The intent of the comment is still correct, but the details have changed.

Also, there is a call to mapping_set_error() in nfs_pageio_add_request().
I wonder if that should be changed to
nfs_context_set_write_error(req->wb_context, desc->pg_error)
??

Otherwise, patch looks good to me.
Thanks,
NeilBrown

Jeff Layton

2017-08-28 11:47 UTC #3 lore raw

Quoted 70 lines · NeilBrown

Quoted 68 lines · Jeff Layton
Quoted 67 lines · Jeff Layton
From: Jeff Layton <[email protected]>
There is some ambiguity in nfs about how writeback errors are tracked.
For instance, nfs_pageio_add_request calls mapping_set_error when the
add fails, but we track errors that occur after adding the request
with a dedicated int error in the open context.
Now that we have better infrastructure for the vfs layer, this latter int is now unnecessary. Just have nfs_context_set_write_error set
the error in the mapping when one occurs.
Have NFS use file_write_and_wait_range to initiate and wait on writeback
of the data, and then check again after issuing the commit(s).
With this, we also don't need to pay attention to the ERROR_WRITE flag for reporting, and just clear it to indicate to subsequent writers that they should try to go asynchronous again.
In nfs_page_async_flush, sample the error before locking and joining
the requests, and check for errors since that point.
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfs/file.c          | 24 +++++++++++-------------
 fs/nfs/inode.c         |  3 +--
 fs/nfs/write.c         |  8 ++++++--
 include/linux/nfs_fs.h |  1 -
 4 files changed, 18 insertions(+), 18 deletions(-)
I have a baling wire and duct tape solution for testing this with xfstests (using iptables REJECT targets and soft mounts). This seems to
make nfs do the right thing.
fs/nfs/file.c +2 -5
index 5713eb32a45e..15d3c6faafd3 100644
@@ -212,25 +212,23 @@ nfs_file_fsync_commit(struct file *file,
 loff_t start, loff_t end, int datasync)
 {
 	struct nfs_open_context *ctx =
 nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
-	int ret = 0;
+	int do_resend, status;
+	int ret;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file,
 datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend =
 test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
-	have_error = test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE,
 &ctx->flags);
-	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
 >flags);

Quoted 17 lines

Quoted 11 lines
fs/nfs/file.c nfs_file_fsync_commit(struct file *file, loff_t start, loff_t end, int datasync) · line 226
Quoted 11 lines
fs/nfs/file.c +5 -6
-	if (have_error) {
-		ret = xchg(&ctx->error, 0);
-		if (ret)
-			goto out;
-	}
-	if (status < 0) {
+	clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
+	ret = nfs_commit_inode(inode, FLUSH_SYNC);
+
+	/* Recheck and advance after the commit */
+	status = file_check_and_advance_wb_err(file);
This change makes the code inconsistent with the comment above the function, which still references ctx->error. The intent of the comment
is still correct, but the details have changed.

Good catch. I'll fix that up in a respin.

Quoted 6 lines

Also, there is a call to mapping_set_error() in nfs_pageio_add_request().
I wonder if that should be changed to
nfs_context_set_write_error(req->wb_context, desc->pg_error)
??

Trickier question...

I'm not quite sure what semantics we're looking for with NFS_CONTEXT_ERROR_WRITE. I know that it forces writes to be synchronous, but I'm not quite sure why it gets cleared the way it does. It's set on any error but cleared before issuing a commit.

I added a similar flag to Ceph inodes recently, but only clear it when a write succeeds. Wouldn't that make more sense here as well?

Signature · 1 line

Jeff Layton <[email protected]>

NeilBrown

2017-08-29 01:23 UTC #4 lore raw

Quoted 100 lines · Jeff Layton

Quoted 70 lines · NeilBrown
Quoted 68 lines · Jeff Layton
Quoted 67 lines · Jeff Layton
From: Jeff Layton <[email protected]>
There is some ambiguity in nfs about how writeback errors are tracked.
For instance, nfs_pageio_add_request calls mapping_set_error when the
add fails, but we track errors that occur after adding the request
with a dedicated int error in the open context.
Now that we have better infrastructure for the vfs layer, this latter int is now unnecessary. Just have nfs_context_set_write_error set
the error in the mapping when one occurs.
Have NFS use file_write_and_wait_range to initiate and wait on writeback
of the data, and then check again after issuing the commit(s).
With this, we also don't need to pay attention to the ERROR_WRITE flag for reporting, and just clear it to indicate to subsequent writers that they should try to go asynchronous again.
In nfs_page_async_flush, sample the error before locking and joining
the requests, and check for errors since that point.
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfs/file.c          | 24 +++++++++++-------------
 fs/nfs/inode.c         |  3 +--
 fs/nfs/write.c         |  8 ++++++--
 include/linux/nfs_fs.h |  1 -
 4 files changed, 18 insertions(+), 18 deletions(-)
I have a baling wire and duct tape solution for testing this with xfstests (using iptables REJECT targets and soft mounts). This seems to
make nfs do the right thing.
fs/nfs/file.c +2 -5
index 5713eb32a45e..15d3c6faafd3 100644
@@ -212,25 +212,23 @@ nfs_file_fsync_commit(struct file *file,
 loff_t start, loff_t end, int datasync)
 {
 	struct nfs_open_context *ctx =
 nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
-	int ret = 0;
+	int do_resend, status;
+	int ret;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file,
 datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend =
 test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
-	have_error = test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE,
 &ctx->flags);
-	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
 >flags);
Quoted 17 lines
Quoted 11 lines
fs/nfs/file.c nfs_file_fsync_commit(struct file *file, loff_t start, loff_t end, int datasync) · line 226
Quoted 11 lines
fs/nfs/file.c +5 -6
-	if (have_error) {
-		ret = xchg(&ctx->error, 0);
-		if (ret)
-			goto out;
-	}
-	if (status < 0) {
+	clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
+	ret = nfs_commit_inode(inode, FLUSH_SYNC);
+
+	/* Recheck and advance after the commit */
+	status = file_check_and_advance_wb_err(file);
This change makes the code inconsistent with the comment above the function, which still references ctx->error. The intent of the comment
is still correct, but the details have changed.
Good catch. I'll fix that up in a respin.
Quoted 6 lines
Also, there is a call to mapping_set_error() in nfs_pageio_add_request().
I wonder if that should be changed to
nfs_context_set_write_error(req->wb_context, desc->pg_error)
??
Trickier question...

Quoted 7 lines

I'm not quite sure what semantics we're looking for with NFS_CONTEXT_ERROR_WRITE. I know that it forces writes to be synchronous, but I'm not quite sure why it gets cleared the way it does. It's set on any error but cleared before issuing a commit.
I added a similar flag to Ceph inodes recently, but only clear it when a write succeeds. Wouldn't that make more sense here as well?

It is a bit hard to wrap one's mind around.

In the original code (commit 7b159fc18d417980) it looks like:
- test-and-clear bit
- write and sync
- test-bit

This does, I think, seem safer than "clear on successful write" as the writes could complete out-of-order and I wouldn't be surprised if the unsuccessful ones completed with an error before the successful one - particularly with an error like EDQUOT.

However the current code does the writes before the test-and-clear, and only does the commit afterwards. That makes it less clear why the current sequence is a good idea.

However ... nfs_file_fsync_commit() is only called if filemap_write_and_wait_range() returned with success, so we only clear the flag after successful writes(?).

Oh....
This patch from me:

Commit: 2edb6bc3852c ("NFS - fix recent breakage to NFS error handling.")

seems to have been reverted by

Commit: 7b281ee02655 ("NFS: fsync() must exit with an error if page writeback failed")

which probably isn't good. It appears that this code is very fragile and easily broken.
Maybe we need to work out exactly what is required, and document it - so we can stop breaking it.
Or maybe we need some unit tests.....

NeilBrown

Jeff Layton

2017-08-29 10:54 UTC #5 lore raw

Quoted 138 lines · NeilBrown

Quoted 101 lines · Jeff Layton
Quoted 71 lines · NeilBrown
Quoted 69 lines · Jeff Layton
Quoted 68 lines · Jeff Layton
From: Jeff Layton <[email protected]>
There is some ambiguity in nfs about how writeback errors are tracked.
For instance, nfs_pageio_add_request calls mapping_set_error when the
add fails, but we track errors that occur after adding the request
with a dedicated int error in the open context.
Now that we have better infrastructure for the vfs layer, this latter int is now unnecessary. Just have nfs_context_set_write_error set
the error in the mapping when one occurs.
Have NFS use file_write_and_wait_range to initiate and wait on writeback
of the data, and then check again after issuing the commit(s).
With this, we also don't need to pay attention to the ERROR_WRITE flag for reporting, and just clear it to indicate to subsequent writers that they should try to go asynchronous again.
In nfs_page_async_flush, sample the error before locking and joining
the requests, and check for errors since that point.
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfs/file.c          | 24 +++++++++++-------------
 fs/nfs/inode.c         |  3 +--
 fs/nfs/write.c         |  8 ++++++--
 include/linux/nfs_fs.h |  1 -
 4 files changed, 18 insertions(+), 18 deletions(-)
I have a baling wire and duct tape solution for testing this with xfstests (using iptables REJECT targets and soft mounts). This seems to
make nfs do the right thing.
fs/nfs/file.c +2 -5
index 5713eb32a45e..15d3c6faafd3 100644
@@ -212,25 +212,23 @@ nfs_file_fsync_commit(struct file *file,
 loff_t start, loff_t end, int datasync)
 {
 	struct nfs_open_context *ctx =
 nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
-	int ret = 0;
+	int do_resend, status;
+	int ret;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file,
 datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend =
 test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
-	have_error = test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE,
 &ctx->flags);
-	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
 > flags);
 
Quoted 17 lines
Quoted 11 lines
fs/nfs/file.c nfs_file_fsync_commit(struct file *file, loff_t start, loff_t end, int datasync) · line 226
Quoted 11 lines
fs/nfs/file.c +5 -6
-	if (have_error) {
-		ret = xchg(&ctx->error, 0);
-		if (ret)
-			goto out;
-	}
-	if (status < 0) {
+	clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
+	ret = nfs_commit_inode(inode, FLUSH_SYNC);
+
+	/* Recheck and advance after the commit */
+	status = file_check_and_advance_wb_err(file);
This change makes the code inconsistent with the comment above the function, which still references ctx->error. The intent of the comment
is still correct, but the details have changed.
Good catch. I'll fix that up in a respin.
Quoted 6 lines
Also, there is a call to mapping_set_error() in nfs_pageio_add_request().
I wonder if that should be changed to
nfs_context_set_write_error(req->wb_context, desc->pg_error)
??
Trickier question...
Quoted 7 lines
I'm not quite sure what semantics we're looking for with NFS_CONTEXT_ERROR_WRITE. I know that it forces writes to be synchronous, but I'm not quite sure why it gets cleared the way it does. It's set on any error but cleared before issuing a commit.
I added a similar flag to Ceph inodes recently, but only clear it when a write succeeds. Wouldn't that make more sense here as well?
It is a bit hard to wrap one's mind around.
In the original code (commit 7b159fc18d417980) it looks like:
- test-and-clear bit
- write and sync
- test-bit
This does, I think, seem safer than "clear on successful write" as the writes could complete out-of-order and I wouldn't be surprised if the unsuccessful ones completed with an error before the successful one - particularly with an error like EDQUOT.
However the current code does the writes before the test-and-clear, and only does the commit afterwards. That makes it less clear why the current sequence is a good idea.
However ... nfs_file_fsync_commit() is only called if filemap_write_and_wait_range() returned with success, so we only clear the flag after successful writes(?).
Oh....
This patch from me:
Commit: 2edb6bc3852c ("NFS - fix recent breakage to NFS error handling.")
seems to have been reverted by

Quoted 8 lines

Commit: 7b281ee02655 ("NFS: fsync() must exit with an error if page writeback failed")
which probably isn't good. It appears that this code is very fragile and easily broken.
Maybe we need to work out exactly what is required, and document it - so we can stop breaking it.
Or maybe we need some unit tests.....

Yes, laying out what's necessary for this would be very helpful. We clearly want to set the flag when an error occurs. Under what circumstances should we be clearing it?

I'm not sure we can really do much better than clearing it on a successful write. With Ceph, was that this is just a hint to the write submission mechanism and we generally aren't too concerned if a few slip past in either direction.

Signature · 1 line

Jeff Layton <[email protected]>

NeilBrown

2017-09-07 03:37 UTC #6 lore raw

Quoted 135 lines · Jeff Layton

Quoted 134 lines · NeilBrown
Quoted 101 lines · Jeff Layton
Quoted 71 lines · NeilBrown
Quoted 69 lines · Jeff Layton
Quoted 68 lines · Jeff Layton
From: Jeff Layton <[email protected]>
There is some ambiguity in nfs about how writeback errors are tracked.
For instance, nfs_pageio_add_request calls mapping_set_error when the
add fails, but we track errors that occur after adding the request
with a dedicated int error in the open context.
Now that we have better infrastructure for the vfs layer, this latter int is now unnecessary. Just have nfs_context_set_write_error set
the error in the mapping when one occurs.
Have NFS use file_write_and_wait_range to initiate and wait on writeback
of the data, and then check again after issuing the commit(s).
With this, we also don't need to pay attention to the ERROR_WRITE flag for reporting, and just clear it to indicate to subsequent writers that they should try to go asynchronous again.
In nfs_page_async_flush, sample the error before locking and joining
the requests, and check for errors since that point.
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfs/file.c          | 24 +++++++++++-------------
 fs/nfs/inode.c         |  3 +--
 fs/nfs/write.c         |  8 ++++++--
 include/linux/nfs_fs.h |  1 -
 4 files changed, 18 insertions(+), 18 deletions(-)
I have a baling wire and duct tape solution for testing this with xfstests (using iptables REJECT targets and soft mounts). This seems to
make nfs do the right thing.
fs/nfs/file.c +2 -5
index 5713eb32a45e..15d3c6faafd3 100644
@@ -212,25 +212,23 @@ nfs_file_fsync_commit(struct file *file,
 loff_t start, loff_t end, int datasync)
 {
 	struct nfs_open_context *ctx =
 nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
-	int ret = 0;
+	int do_resend, status;
+	int ret;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file,
 datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend =
 test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
-	have_error = test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE,
 &ctx->flags);
-	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
 > flags);
 
Quoted 17 lines
Quoted 11 lines
fs/nfs/file.c nfs_file_fsync_commit(struct file *file, loff_t start, loff_t end, int datasync) · line 226
Quoted 11 lines
fs/nfs/file.c +5 -6
-	if (have_error) {
-		ret = xchg(&ctx->error, 0);
-		if (ret)
-			goto out;
-	}
-	if (status < 0) {
+	clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
+	ret = nfs_commit_inode(inode, FLUSH_SYNC);
+
+	/* Recheck and advance after the commit */
+	status = file_check_and_advance_wb_err(file);
This change makes the code inconsistent with the comment above the function, which still references ctx->error. The intent of the comment
is still correct, but the details have changed.
Good catch. I'll fix that up in a respin.
Quoted 6 lines
Also, there is a call to mapping_set_error() in nfs_pageio_add_request().
I wonder if that should be changed to
nfs_context_set_write_error(req->wb_context, desc->pg_error)
??
Trickier question...
Quoted 7 lines
I'm not quite sure what semantics we're looking for with NFS_CONTEXT_ERROR_WRITE. I know that it forces writes to be synchronous, but I'm not quite sure why it gets cleared the way it does. It's set on any error but cleared before issuing a commit.
I added a similar flag to Ceph inodes recently, but only clear it when a write succeeds. Wouldn't that make more sense here as well?
It is a bit hard to wrap one's mind around.
In the original code (commit 7b159fc18d417980) it looks like:
- test-and-clear bit
- write and sync
- test-bit
This does, I think, seem safer than "clear on successful write" as the writes could complete out-of-order and I wouldn't be surprised if the unsuccessful ones completed with an error before the successful one - particularly with an error like EDQUOT.
However the current code does the writes before the test-and-clear, and only does the commit afterwards. That makes it less clear why the current sequence is a good idea.
However ... nfs_file_fsync_commit() is only called if filemap_write_and_wait_range() returned with success, so we only clear the flag after successful writes(?).
Oh....
This patch from me:

Quoted 8 lines

Quoted 8 lines
Commit: 2edb6bc3852c ("NFS - fix recent breakage to NFS error handling.")
seems to have been reverted by
Commit: 7b281ee02655 ("NFS: fsync() must exit with an error if page writeback failed")
which probably isn't good. It appears that this code is very fragile and easily broken.

On further investigation, I think the problem that I fixed and then we reintroduced will be fixed again - more permanently - by your patch. The root problem is that nfs keeps error codes in a different way to the MM core. By unifying those, the problem goes.
(The specific problem is that writes which hit EDQUOT on the server can
report EIO on the client).

Quoted 8 lines

Quoted 4 lines
Maybe we need to work out exactly what is required, and document it - so we can stop breaking it.
Or maybe we need some unit tests.....
Yes, laying out what's necessary for this would be very helpful. We clearly want to set the flag when an error occurs. Under what circumstances should we be clearing it?

Well.... looking back at 7b159fc18d417980f57ae which introduced the flag, prior to that write errors (ctx->error) were only reported by nfs_file_flush and nfs_fsync, so only one close() and fsync().

After that commit, setting the flag would mean that errors could be returned by 'write'. So clearing as part of returning the error makes perfect sense.

As long as the error gets recorded, and gets returned when it is recorded, it doesn't much matter when the flag is cleared. With your patches we don't need to flag any more to get errors reliably reported.

Leaving the flag set means that writes go more slowly - we don't get large queue of background rights building up but destined for failure. This is the main point made in the comment message when the flag was introduced.
Of course, by the time we first get an error there could already by a large queue, so we probably want that to drain completely before allowing async writes again.

It might make sense to have 2 flags. One which says "writes should be synchronous", another that says "There was an error recently". We clear the error flag before calling nfs_fsync, and if it is still clear afterwards, we clear the sync-writes flag. Maybe that is more complex than needed though.

I'm leaning towards your suggestion that it doesn't matter very much when it gets cleared, and clearing it on any successful write is simplest.

So I'm still in favor of using nfs_context_set_write_error() in nfs_pageio_add_request(), primarily because it is most consistent - we don't need exceptions.

Thanks,
NeilBrown

Quoted 7 lines

I'm not sure we can really do much better than clearing it on a successful write. With Ceph, was that this is just a hint to the write submission mechanism and we generally aren't too concerned if a few slip past in either direction.
Signature · 1 line
Jeff Layton <[email protected]>

Jeff Layton

2017-09-07 11:35 UTC #7 lore raw

Quoted 189 lines · NeilBrown

Quoted 135 lines · Jeff Layton
Quoted 134 lines · NeilBrown
Quoted 101 lines · Jeff Layton
Quoted 71 lines · NeilBrown
Quoted 69 lines · Jeff Layton
Quoted 68 lines · Jeff Layton
From: Jeff Layton <[email protected]>
There is some ambiguity in nfs about how writeback errors are tracked.
For instance, nfs_pageio_add_request calls mapping_set_error when the
add fails, but we track errors that occur after adding the request
with a dedicated int error in the open context.
Now that we have better infrastructure for the vfs layer, this latter int is now unnecessary. Just have nfs_context_set_write_error set
the error in the mapping when one occurs.
Have NFS use file_write_and_wait_range to initiate and wait on writeback
of the data, and then check again after issuing the commit(s).
With this, we also don't need to pay attention to the ERROR_WRITE flag for reporting, and just clear it to indicate to subsequent writers that they should try to go asynchronous again.
In nfs_page_async_flush, sample the error before locking and joining
the requests, and check for errors since that point.
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfs/file.c          | 24 +++++++++++-------------
 fs/nfs/inode.c         |  3 +--
 fs/nfs/write.c         |  8 ++++++--
 include/linux/nfs_fs.h |  1 -
 4 files changed, 18 insertions(+), 18 deletions(-)
I have a baling wire and duct tape solution for testing this with xfstests (using iptables REJECT targets and soft mounts). This seems to
make nfs do the right thing.
fs/nfs/file.c +2 -5
index 5713eb32a45e..15d3c6faafd3 100644
@@ -212,25 +212,23 @@ nfs_file_fsync_commit(struct file *file,
 loff_t start, loff_t end, int datasync)
 {
 	struct nfs_open_context *ctx =
 nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
-	int ret = 0;
+	int do_resend, status;
+	int ret;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file,
 datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend =
 test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
-	have_error = test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE,
 &ctx->flags);
-	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
 > flags);
 
Quoted 17 lines
Quoted 11 lines
fs/nfs/file.c nfs_file_fsync_commit(struct file *file,
Quoted 11 lines
fs/nfs/file.c +5 -6
-	if (have_error) {
-		ret = xchg(&ctx->error, 0);
-		if (ret)
-			goto out;
-	}
-	if (status < 0) {
+	clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
+	ret = nfs_commit_inode(inode, FLUSH_SYNC);
+
+	/* Recheck and advance after the commit */
+	status = file_check_and_advance_wb_err(file);
This change makes the code inconsistent with the comment above the function, which still references ctx->error. The intent of the comment
is still correct, but the details have changed.
Good catch. I'll fix that up in a respin.
Quoted 6 lines
Also, there is a call to mapping_set_error() in nfs_pageio_add_request().
I wonder if that should be changed to
nfs_context_set_write_error(req->wb_context, desc->pg_error)
??
Trickier question...
Quoted 7 lines
I'm not quite sure what semantics we're looking for with NFS_CONTEXT_ERROR_WRITE. I know that it forces writes to be synchronous, but I'm not quite sure why it gets cleared the way it does. It's set on any error but cleared before issuing a commit.
I added a similar flag to Ceph inodes recently, but only clear it when a write succeeds. Wouldn't that make more sense here as well?
It is a bit hard to wrap one's mind around.
In the original code (commit 7b159fc18d417980) it looks like:
- test-and-clear bit
- write and sync
- test-bit
This does, I think, seem safer than "clear on successful write" as the writes could complete out-of-order and I wouldn't be surprised if the unsuccessful ones completed with an error before the successful one - particularly with an error like EDQUOT.
However the current code does the writes before the test-and-clear, and only does the commit afterwards. That makes it less clear why the current sequence is a good idea.
However ... nfs_file_fsync_commit() is only called if filemap_write_and_wait_range() returned with success, so we only clear the flag after successful writes(?).
Oh....
This patch from me:
Quoted 8 lines
Quoted 8 lines
Commit: 2edb6bc3852c ("NFS - fix recent breakage to NFS error handling.")
seems to have been reverted by
Commit: 7b281ee02655 ("NFS: fsync() must exit with an error if page writeback failed")
which probably isn't good. It appears that this code is very fragile and easily broken.
On further investigation, I think the problem that I fixed and then we reintroduced will be fixed again - more permanently - by your patch. The root problem is that nfs keeps error codes in a different way to the MM core. By unifying those, the problem goes.
(The specific problem is that writes which hit EDQUOT on the server can
report EIO on the client).
Quoted 8 lines
Quoted 4 lines
Maybe we need to work out exactly what is required, and document it - so we can stop breaking it.
Or maybe we need some unit tests.....
Yes, laying out what's necessary for this would be very helpful. We clearly want to set the flag when an error occurs. Under what circumstances should we be clearing it?
Well.... looking back at 7b159fc18d417980f57ae which introduced the flag, prior to that write errors (ctx->error) were only reported by nfs_file_flush and nfs_fsync, so only one close() and fsync().
After that commit, setting the flag would mean that errors could be returned by 'write'. So clearing as part of returning the error makes perfect sense.
As long as the error gets recorded, and gets returned when it is recorded, it doesn't much matter when the flag is cleared. With your patches we don't need to flag any more to get errors reliably reported.
Leaving the flag set means that writes go more slowly - we don't get large queue of background rights building up but destined for failure. This is the main point made in the comment message when the flag was introduced.
Of course, by the time we first get an error there could already by a large queue, so we probably want that to drain completely before allowing async writes again.
It might make sense to have 2 flags. One which says "writes should be synchronous", another that says "There was an error recently". We clear the error flag before calling nfs_fsync, and if it is still clear afterwards, we clear the sync-writes flag. Maybe that is more complex than needed though.

Quoted 7 lines

I'm leaning towards your suggestion that it doesn't matter very much when it gets cleared, and clearing it on any successful write is simplest.
So I'm still in favor of using nfs_context_set_write_error() in nfs_pageio_add_request(), primarily because it is most consistent - we don't need exceptions.

Thanks for taking a closer look. I can easily make the change above, and I do think that keeping this mechanism as simple as possible will make it easier to prevent bitrot.

That said... NFS_CONTEXT_ERROR_WRITE is a per ctx flag, and the ctx is a per open file description object.

Is that the correct way to track this? All of the ctx's will share the same inode. If we're getting writeback errors for one context, it's quite likely that we'll be seeing them via others.

I suppose the counterargument is when we have things like expiring krb5 tickets. Write failures via an expiring set of creds may have no effect on writeback via other creds.

Still, I think a per-inode flag might make more sense here.

Thoughts?

Signature · 1 line

Jeff Layton <[email protected]>

Trond Myklebust

2017-09-07 14:54 UTC #8 lore raw

Quoted 218 lines · Jeff Layton

Quoted 217 lines · NeilBrown
Quoted 167 lines · Jeff Layton
Quoted 166 lines · NeilBrown
Quoted 128 lines · Jeff Layton
Quoted 95 lines · NeilBrown
Quoted 93 lines · Jeff Layton
Quoted 92 lines · Jeff Layton
From: Jeff Layton <[email protected]>
There is some ambiguity in nfs about how writeback errors are
tracked.
For instance, nfs_pageio_add_request calls mapping_set_error when
the
add fails, but we track errors that occur after adding the
request
with a dedicated int error in the open context.
Now that we have better infrastructure for the vfs layer, this
latter int is now unnecessary. Just have nfs_context_set_write_error set
the error in the mapping when one occurs.
Have NFS use file_write_and_wait_range to initiate and wait on
writeback
of the data, and then check again after issuing the commit(s).
With this, we also don't need to pay attention to the ERROR_WRITE
flag for reporting, and just clear it to indicate to subsequent
writers that they should try to go asynchronous again.
In nfs_page_async_flush, sample the error before locking and
joining
the requests, and check for errors since that point.
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfs/file.c          | 24 +++++++++++-------------
 fs/nfs/inode.c         |  3 +--
 fs/nfs/write.c         |  8 ++++++--
 include/linux/nfs_fs.h |  1 -
 4 files changed, 18 insertions(+), 18 deletions(-)
I have a baling wire and duct tape solution for testing this with
xfstests (using iptables REJECT targets and soft mounts). This
seems to
make nfs do the right thing.
fs/nfs/file.c +2 -2
index 5713eb32a45e..15d3c6faafd3 100644
@@ -212,25 +212,23 @@ nfs_file_fsync_commit(struct file
 *file,
 loff_t start, loff_t end, int datasync)
 {
 	struct nfs_open_context *ctx =
 nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
-	int ret = 0;
+	int do_resend, status;
+	int ret;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n",
 file,
 datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend =
test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx-
Quoted 1 line
flags);
fs/nfs/file.c +0 -1
-	have_error =
test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE,
&ctx->flags);
fs/nfs/file.c +0 -2
-	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |=
test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
Quoted 1 line
flags);
fs/nfs/file.c +1 -6
-	if (have_error) {
-		ret = xchg(&ctx->error, 0);
-		if (ret)
-			goto out;
-	}
-	if (status < 0) {
+	clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
Quoted 1 line
flags);
Quoted 12 lines
Quoted 4 lines
fs/nfs/file.c
Quoted 4 lines
fs/nfs/file.c +4 -0
+	ret = nfs_commit_inode(inode, FLUSH_SYNC);
+
+	/* Recheck and advance after the commit */
+	status = file_check_and_advance_wb_err(file);
This change makes the code inconsistent with the comment above the
function, which still references ctx->error. The intent of the
comment
is still correct, but the details have changed.
Good catch. I'll fix that up in a respin.
Quoted 7 lines
Also, there is a call to mapping_set_error() in nfs_pageio_add_request().
I wonder if that should be changed to
nfs_context_set_write_error(req->wb_context, desc-
Quoted 1 line
pg_error)
??
Trickier question...
I'm not quite sure what semantics we're looking for with NFS_CONTEXT_ERROR_WRITE. I know that it forces writes to be synchronous, but I'm not quite sure why it gets cleared the way it
does. It's set on any error but cleared before issuing a commit.
Quoted 3 lines
I added a similar flag to Ceph inodes recently, but only clear it when
a write succeeds. Wouldn't that make more sense here as well?
It is a bit hard to wrap one's mind around.
In the original code (commit 7b159fc18d417980) it looks like:
- test-and-clear bit
- write and sync
- test-bit
This does, I think, seem safer than "clear on successful write" as the
writes could complete out-of-order and I wouldn't be surprised if the
unsuccessful ones completed with an error before the successful one -
particularly with an error like EDQUOT.
However the current code does the writes before the test-and- clear, and
only does the commit afterwards. That makes it less clear why the
current sequence is a good idea.
However ... nfs_file_fsync_commit() is only called if filemap_write_and_wait_range() returned with success, so we only clear
the flag after successful writes(?).
Oh....
This patch from me:
Commit: 2edb6bc3852c ("NFS - fix recent breakage to NFS error handling.")
Quoted 8 lines
Quoted 8 lines
seems to have been reverted by
Commit: 7b281ee02655 ("NFS: fsync() must exit with an error if page writeback failed")
which probably isn't good. It appears that this code is very fragile
and easily broken.
On further investigation, I think the problem that I fixed and then we
reintroduced will be fixed again - more permanently - by your patch.
The root problem is that nfs keeps error codes in a different way to the
MM core. By unifying those, the problem goes.
(The specific problem is that writes which hit EDQUOT on the server can
report EIO on the client).
Quoted 10 lines
Quoted 5 lines
Maybe we need to work out exactly what is required, and document it - so
we can stop breaking it.
Or maybe we need some unit tests.....
Yes, laying out what's necessary for this would be very helpful. We
clearly want to set the flag when an error occurs. Under what circumstances should we be clearing it?
Well.... looking back at 7b159fc18d417980f57ae which introduced the
flag, prior to that write errors (ctx->error) were only reported by nfs_file_flush and nfs_fsync, so only one close() and fsync().
After that commit, setting the flag would mean that errors could be returned by 'write'. So clearing as part of returning the error makes
perfect sense.
As long as the error gets recorded, and gets returned when it is recorded, it doesn't much matter when the flag is cleared. With your
patches we don't need to flag any more to get errors reliably reported.

Quoted 11 lines

Quoted 11 lines
Leaving the flag set means that writes go more slowly - we don't get
large queue of background rights building up but destined for failure.
This is the main point made in the comment message when the flag was
introduced.
Of course, by the time we first get an error there could already by a large queue, so we probably want that to drain completely before
allowing async writes again.

We already have this functionality implemented in the existing code.

Quoted 10 lines

Quoted 10 lines
It might make sense to have 2 flags. One which says "writes should be
synchronous", another that says "There was an error recently". We clear the error flag before calling nfs_fsync, and if it is still
clear afterwards, we clear the sync-writes flag. Maybe that is more
complex than needed though.

We also need to preserve the NFS_CONTEXT_RESEND_WRITES flag. I don't see any global mechanism that will replace that.

Quoted 31 lines

Quoted 9 lines
I'm leaning towards your suggestion that it doesn't matter very much
when it gets cleared, and clearing it on any successful write is simplest.
So I'm still in favor of using nfs_context_set_write_error() in nfs_pageio_add_request(), primarily because it is most consistent - we
don't need exceptions.
Thanks for taking a closer look. I can easily make the change above, and
I do think that keeping this mechanism as simple as possible will make
it easier to prevent bitrot.
That said... NFS_CONTEXT_ERROR_WRITE is a per ctx flag, and the ctx is a
per open file description object.
Is that the correct way to track this? All of the ctx's will share the
same inode. If we're getting writeback errors for one context, it's quite likely that we'll be seeing them via others.
I suppose the counterargument is when we have things like expiring krb5
tickets. Write failures via an expiring set of creds may have no effect
on writeback via other creds.

Quoted 3 lines

Still, I think a per-inode flag might make more sense here.
Thoughts?

As far as I'm concerned, that would be a regression. The most common problem when flushing writeback data to the server aside from ENOSPC (and possibly ESTALE) is EACCES, which is particular to the file descriptor that opened the file.

File contexts, and NFS_CONTEXT_ERROR_WRITE solve that problem by being private to the file descriptor.

Signature · 3 lines

Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]

NeilBrown

2017-09-11 03:25 UTC #9 lore raw

Quoted 282 lines · Trond Myklebust

Quoted 218 lines · Jeff Layton
Quoted 217 lines · NeilBrown
Quoted 167 lines · Jeff Layton
Quoted 166 lines · NeilBrown
Quoted 128 lines · Jeff Layton
Quoted 95 lines · NeilBrown
Quoted 93 lines · Jeff Layton
Quoted 92 lines · Jeff Layton
From: Jeff Layton <[email protected]>
There is some ambiguity in nfs about how writeback errors are
tracked.
For instance, nfs_pageio_add_request calls mapping_set_error when
the
add fails, but we track errors that occur after adding the
request
with a dedicated int error in the open context.
Now that we have better infrastructure for the vfs layer, this
latter int is now unnecessary. Just have nfs_context_set_write_error set
the error in the mapping when one occurs.
Have NFS use file_write_and_wait_range to initiate and wait on
writeback
of the data, and then check again after issuing the commit(s).
With this, we also don't need to pay attention to the ERROR_WRITE
flag for reporting, and just clear it to indicate to subsequent
writers that they should try to go asynchronous again.
In nfs_page_async_flush, sample the error before locking and
joining
the requests, and check for errors since that point.
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfs/file.c          | 24 +++++++++++-------------
 fs/nfs/inode.c         |  3 +--
 fs/nfs/write.c         |  8 ++++++--
 include/linux/nfs_fs.h |  1 -
 4 files changed, 18 insertions(+), 18 deletions(-)
I have a baling wire and duct tape solution for testing this with
xfstests (using iptables REJECT targets and soft mounts). This
seems to
make nfs do the right thing.
fs/nfs/file.c +2 -2
index 5713eb32a45e..15d3c6faafd3 100644
@@ -212,25 +212,23 @@ nfs_file_fsync_commit(struct file
 *file,
 loff_t start, loff_t end, int datasync)
 {
 	struct nfs_open_context *ctx =
 nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
-	int ret = 0;
+	int do_resend, status;
+	int ret;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n",
 file,
 datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend =
test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx-
Quoted 1 line
flags);
fs/nfs/file.c +0 -1
-	have_error =
test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE,
&ctx->flags);
fs/nfs/file.c +0 -2
-	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |=
test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
Quoted 1 line
flags);
fs/nfs/file.c +1 -6
-	if (have_error) {
-		ret = xchg(&ctx->error, 0);
-		if (ret)
-			goto out;
-	}
-	if (status < 0) {
+	clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
Quoted 1 line
flags);
Quoted 12 lines
Quoted 4 lines
fs/nfs/file.c
Quoted 4 lines
fs/nfs/file.c +4 -0
+	ret = nfs_commit_inode(inode, FLUSH_SYNC);
+
+	/* Recheck and advance after the commit */
+	status = file_check_and_advance_wb_err(file);
This change makes the code inconsistent with the comment above the
function, which still references ctx->error. The intent of the
comment
is still correct, but the details have changed.
Good catch. I'll fix that up in a respin.
Quoted 7 lines
Also, there is a call to mapping_set_error() in nfs_pageio_add_request().
I wonder if that should be changed to
nfs_context_set_write_error(req->wb_context, desc-
Quoted 1 line
pg_error)
??
Trickier question...
I'm not quite sure what semantics we're looking for with NFS_CONTEXT_ERROR_WRITE. I know that it forces writes to be synchronous, but I'm not quite sure why it gets cleared the way it
does. It's set on any error but cleared before issuing a commit.
Quoted 3 lines
I added a similar flag to Ceph inodes recently, but only clear it when
a write succeeds. Wouldn't that make more sense here as well?
It is a bit hard to wrap one's mind around.
In the original code (commit 7b159fc18d417980) it looks like:
- test-and-clear bit
- write and sync
- test-bit
This does, I think, seem safer than "clear on successful write" as the
writes could complete out-of-order and I wouldn't be surprised if the
unsuccessful ones completed with an error before the successful one -
particularly with an error like EDQUOT.
However the current code does the writes before the test-and- clear, and
only does the commit afterwards. That makes it less clear why the
current sequence is a good idea.
However ... nfs_file_fsync_commit() is only called if filemap_write_and_wait_range() returned with success, so we only clear
the flag after successful writes(?).
Oh....
This patch from me:
Commit: 2edb6bc3852c ("NFS - fix recent breakage to NFS error handling.")
Quoted 8 lines
Quoted 8 lines
seems to have been reverted by
Commit: 7b281ee02655 ("NFS: fsync() must exit with an error if page writeback failed")
which probably isn't good. It appears that this code is very fragile
and easily broken.
On further investigation, I think the problem that I fixed and then we
reintroduced will be fixed again - more permanently - by your patch.
The root problem is that nfs keeps error codes in a different way to the
MM core. By unifying those, the problem goes.
(The specific problem is that writes which hit EDQUOT on the server can
report EIO on the client).
Quoted 10 lines
Quoted 5 lines
Maybe we need to work out exactly what is required, and document it - so
we can stop breaking it.
Or maybe we need some unit tests.....
Yes, laying out what's necessary for this would be very helpful. We
clearly want to set the flag when an error occurs. Under what circumstances should we be clearing it?
Well.... looking back at 7b159fc18d417980f57ae which introduced the
flag, prior to that write errors (ctx->error) were only reported by nfs_file_flush and nfs_fsync, so only one close() and fsync().
After that commit, setting the flag would mean that errors could be returned by 'write'. So clearing as part of returning the error makes
perfect sense.
As long as the error gets recorded, and gets returned when it is recorded, it doesn't much matter when the flag is cleared. With your
patches we don't need to flag any more to get errors reliably reported.
Quoted 11 lines
Quoted 11 lines
Leaving the flag set means that writes go more slowly - we don't get
large queue of background rights building up but destined for failure.
This is the main point made in the comment message when the flag was
introduced.
Of course, by the time we first get an error there could already by a large queue, so we probably want that to drain completely before
allowing async writes again.
We already have this functionality implemented in the existing code.
Quoted 10 lines
Quoted 10 lines
It might make sense to have 2 flags. One which says "writes should be
synchronous", another that says "There was an error recently". We clear the error flag before calling nfs_fsync, and if it is still
clear afterwards, we clear the sync-writes flag. Maybe that is more
complex than needed though.
We also need to preserve the NFS_CONTEXT_RESEND_WRITES flag. I don't see any global mechanism that will replace that.
Quoted 31 lines
Quoted 9 lines
I'm leaning towards your suggestion that it doesn't matter very much
when it gets cleared, and clearing it on any successful write is simplest.
So I'm still in favor of using nfs_context_set_write_error() in nfs_pageio_add_request(), primarily because it is most consistent - we
don't need exceptions.
Thanks for taking a closer look. I can easily make the change above, and
I do think that keeping this mechanism as simple as possible will make
it easier to prevent bitrot.
That said... NFS_CONTEXT_ERROR_WRITE is a per ctx flag, and the ctx is a
per open file description object.
Is that the correct way to track this? All of the ctx's will share the
same inode. If we're getting writeback errors for one context, it's quite likely that we'll be seeing them via others.
I suppose the counterargument is when we have things like expiring krb5
tickets. Write failures via an expiring set of creds may have no effect
on writeback via other creds.
Quoted 3 lines
Still, I think a per-inode flag might make more sense here.
Thoughts?

Quoted 7 lines

As far as I'm concerned, that would be a regression. The most common problem when flushing writeback data to the server aside from ENOSPC (and possibly ESTALE) is EACCES, which is particular to the file descriptor that opened the file.
File contexts, and NFS_CONTEXT_ERROR_WRITE solve that problem by being private to the file descriptor.

Thanks for the reminder that errors are per-context and this patch drops this. The per-context nature of errors in NFS was the reason that I nagged Jeff to make errseq_t a stand-alone type rather than just a part of address_space. I had envisaged that it would be embedded in the open_context as well.
We still could do that, but as there is precisely one open-file for each open_context, the gains are not great.

However, while looking over the code to make sure I really understood it and all the possible consequences of changing to errseq_t I found a few anomalies. The patch below addresses them all.

Would you see if they may sense to you?

Thanks,
NeilBrown

From: NeilBrown <[email protected]>
Date: Mon, 11 Sep 2017 13:15:50 +1000
Subject: [PATCH] NFS: various changes relating to reporting IO errors.

1/ remove 'start' and 'end' args from nfs_file_fsync_commit().
They aren't used.

2/ Make nfs_context_set_write_error() a "static inline" in internal.h
so we can...

3/ Use nfs_context_set_write_error() instead of mapping_set_error()
if nfs_pageio_add_request() fails before sending any request. NFS generally keeps errors in the open_context, not the mapping, so this is more consistent.

4/ If filemap_write_and_write_range() reports any error, still
check ctx->error. The value in ctx->error is likely to be more useful. As part of this, NFS_CONTEXT_ERROR_WRITE is cleared slightly earlier, before nfs_file_fsync_commit() is called, rather than at the start of that function.

Signed-off-by NeilBrown <[email protected]>

---
 fs/nfs/file.c     | 16 ++++++++++------
 fs/nfs/internal.h |  7 +++++++
 fs/nfs/pagelist.c |  4 ++--
 fs/nfs/write.c    |  7 -------
 4 files changed, 19 insertions(+), 15 deletions(-)

fs/nfs/file.c +10 -6

index af330c31f627..ab324f14081f 100644
@@ -208,21 +208,19 @@ EXPORT_SYMBOL_GPL(nfs_file_mmap);
  * fall back to doing a synchronous write.
  */
 static int
-nfs_file_fsync_commit(struct file *file, loff_t start, loff_t end, int datasync)
+nfs_file_fsync_commit(struct file *file, int datasync)
 {
 	struct nfs_open_context *ctx = nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
+	int do_resend, status;
 	int ret = 0;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file, datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend = test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
-	have_error = test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
 	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
-	if (have_error) {
+	if (test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags)) {
 		ret = xchg(&ctx->error, 0);
 		if (ret)
 			goto out;

@@ -247,10 +245,16 @@ 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;
+		}
 		if (ret != 0)
 			break;
-		ret = nfs_file_fsync_commit(file, start, end, datasync);
+		ret = nfs_file_fsync_commit(file, datasync);
 		if (!ret)
 			ret = pnfs_sync_inode(inode, !!datasync);
 		/*

fs/nfs/internal.h +7 -0

index dc456416d2be..44c8962fec91 100644
@@ -769,3 +769,10 @@ static inline bool nfs_error_is_fatal(int err)
 		return false;
 	}
 }
+
+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);
+}

fs/nfs/pagelist.c +2 -2

index de9066a92c0d..0ebd26b9a6bd 100644
@@ -1198,8 +1198,8 @@ out_failed:
 
 		/* remember fatal errors */
 		if (nfs_error_is_fatal(desc->pg_error))
-			mapping_set_error(desc->pg_inode->i_mapping,
-					  desc->pg_error);
+			nfs_context_set_write_error(req->wb_context,
+						    desc->pg_error);
 
 		func = desc->pg_completion_ops->error_cleanup;
 		for (midx = 0; midx < desc->pg_mirror_count; midx++) {

fs/nfs/write.c +0 -7

index b1af5dee5e0a..f702bf2def79 100644
@@ -147,13 +147,6 @@ static void nfs_io_completion_put(struct nfs_io_completion *ioc)
 		kref_put(&ioc->refcount, nfs_io_completion_release);
 }
 
-static 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);
-}
-
 /*
  * nfs_page_find_head_request_locked - find head request associated with @page
  *

Signature · 1 line

2.14.0.rc0.dirty

Jeff Layton

2017-09-11 10:46 UTC #10 lore raw

Quoted 427 lines · NeilBrown

Quoted 285 lines · Trond Myklebust
Quoted 221 lines · Jeff Layton
Quoted 220 lines · NeilBrown
Quoted 170 lines · Jeff Layton
Quoted 169 lines · NeilBrown
Quoted 131 lines · Jeff Layton
Quoted 97 lines · NeilBrown
Quoted 95 lines · Jeff Layton
Quoted 94 lines · Jeff Layton
From: Jeff Layton <[email protected]>
There is some ambiguity in nfs about how writeback errors are
tracked.
For instance, nfs_pageio_add_request calls mapping_set_error when
the
add fails, but we track errors that occur after adding the
request
with a dedicated int error in the open context.
Now that we have better infrastructure for the vfs layer, this
latter int is now unnecessary. Just have nfs_context_set_write_error set
the error in the mapping when one occurs.
Have NFS use file_write_and_wait_range to initiate and wait on
writeback
of the data, and then check again after issuing the commit(s).
With this, we also don't need to pay attention to the ERROR_WRITE
flag for reporting, and just clear it to indicate to subsequent
writers that they should try to go asynchronous again.
In nfs_page_async_flush, sample the error before locking and
joining
the requests, and check for errors since that point.
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfs/file.c          | 24 +++++++++++-------------
 fs/nfs/inode.c         |  3 +--
 fs/nfs/write.c         |  8 ++++++--
 include/linux/nfs_fs.h |  1 -
 4 files changed, 18 insertions(+), 18 deletions(-)
I have a baling wire and duct tape solution for testing this with
xfstests (using iptables REJECT targets and soft mounts). This
seems to
make nfs do the right thing.
fs/nfs/file.c +2 -2
index 5713eb32a45e..15d3c6faafd3 100644
@@ -212,25 +212,23 @@ nfs_file_fsync_commit(struct file
 *file,
 loff_t start, loff_t end, int datasync)
 {
 	struct nfs_open_context *ctx =
 nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
-	int ret = 0;
+	int do_resend, status;
+	int ret;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n",
 file,
 datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend =
test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx-
Quoted 1 line
flags);
fs/nfs/file.c +0 -1
-	have_error =
test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE,
&ctx->flags);
fs/nfs/file.c +0 -2
-	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |=
test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
Quoted 1 line
flags);
fs/nfs/file.c +1 -6
-	if (have_error) {
-		ret = xchg(&ctx->error, 0);
-		if (ret)
-			goto out;
-	}
-	if (status < 0) {
+	clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
Quoted 1 line
flags);
Quoted 12 lines
Quoted 4 lines
fs/nfs/file.c
Quoted 4 lines
fs/nfs/file.c +4 -0
+	ret = nfs_commit_inode(inode, FLUSH_SYNC);
+
+	/* Recheck and advance after the commit */
+	status = file_check_and_advance_wb_err(file);
This change makes the code inconsistent with the comment above the
function, which still references ctx->error. The intent of the
comment
is still correct, but the details have changed.
Good catch. I'll fix that up in a respin.
Quoted 8 lines
Also, there is a call to mapping_set_error() in nfs_pageio_add_request().
I wonder if that should be changed to
nfs_context_set_write_error(req->wb_context, desc-
Quoted 1 line
pg_error)
??
Trickier question...
I'm not quite sure what semantics we're looking for with NFS_CONTEXT_ERROR_WRITE. I know that it forces writes to be synchronous, but I'm not quite sure why it gets cleared the way it
does. It's set on any error but cleared before issuing a commit.
Quoted 3 lines
I added a similar flag to Ceph inodes recently, but only clear it when
a write succeeds. Wouldn't that make more sense here as well?
It is a bit hard to wrap one's mind around.
In the original code (commit 7b159fc18d417980) it looks like:
- test-and-clear bit
- write and sync
- test-bit
This does, I think, seem safer than "clear on successful write" as the
writes could complete out-of-order and I wouldn't be surprised if the
unsuccessful ones completed with an error before the successful one -
particularly with an error like EDQUOT.
However the current code does the writes before the test-and- clear, and
only does the commit afterwards. That makes it less clear why the
current sequence is a good idea.
However ... nfs_file_fsync_commit() is only called if filemap_write_and_wait_range() returned with success, so we only clear
the flag after successful writes(?).
Oh....
This patch from me:
Commit: 2edb6bc3852c ("NFS - fix recent breakage to NFS error handling.")
Quoted 8 lines
Quoted 8 lines
seems to have been reverted by
Commit: 7b281ee02655 ("NFS: fsync() must exit with an error if page writeback failed")
which probably isn't good. It appears that this code is very fragile
and easily broken.
On further investigation, I think the problem that I fixed and then we
reintroduced will be fixed again - more permanently - by your patch.
The root problem is that nfs keeps error codes in a different way to the
MM core. By unifying those, the problem goes.
(The specific problem is that writes which hit EDQUOT on the server can
report EIO on the client).
Quoted 10 lines
Quoted 5 lines
Maybe we need to work out exactly what is required, and document it - so
we can stop breaking it.
Or maybe we need some unit tests.....
Yes, laying out what's necessary for this would be very helpful. We
clearly want to set the flag when an error occurs. Under what circumstances should we be clearing it?
Well.... looking back at 7b159fc18d417980f57ae which introduced the
flag, prior to that write errors (ctx->error) were only reported by nfs_file_flush and nfs_fsync, so only one close() and fsync().
After that commit, setting the flag would mean that errors could be returned by 'write'. So clearing as part of returning the error makes
perfect sense.
As long as the error gets recorded, and gets returned when it is recorded, it doesn't much matter when the flag is cleared. With your
patches we don't need to flag any more to get errors reliably reported.
Quoted 11 lines
Quoted 11 lines
Leaving the flag set means that writes go more slowly - we don't get
large queue of background rights building up but destined for failure.
This is the main point made in the comment message when the flag was
introduced.
Of course, by the time we first get an error there could already by a large queue, so we probably want that to drain completely before
allowing async writes again.
We already have this functionality implemented in the existing code.
Quoted 10 lines
Quoted 10 lines
It might make sense to have 2 flags. One which says "writes should be
synchronous", another that says "There was an error recently". We clear the error flag before calling nfs_fsync, and if it is still
clear afterwards, we clear the sync-writes flag. Maybe that is more
complex than needed though.
We also need to preserve the NFS_CONTEXT_RESEND_WRITES flag. I don't see any global mechanism that will replace that.
Quoted 31 lines
Quoted 9 lines
I'm leaning towards your suggestion that it doesn't matter very much
when it gets cleared, and clearing it on any successful write is simplest.
So I'm still in favor of using nfs_context_set_write_error() in nfs_pageio_add_request(), primarily because it is most consistent - we
don't need exceptions.
Thanks for taking a closer look. I can easily make the change above, and
I do think that keeping this mechanism as simple as possible will make
it easier to prevent bitrot.
That said... NFS_CONTEXT_ERROR_WRITE is a per ctx flag, and the ctx is a
per open file description object.
Is that the correct way to track this? All of the ctx's will share the
same inode. If we're getting writeback errors for one context, it's quite likely that we'll be seeing them via others.
I suppose the counterargument is when we have things like expiring krb5
tickets. Write failures via an expiring set of creds may have no effect
on writeback via other creds.
Quoted 3 lines
Still, I think a per-inode flag might make more sense here.
Thoughts?
Quoted 7 lines
As far as I'm concerned, that would be a regression. The most common problem when flushing writeback data to the server aside from ENOSPC (and possibly ESTALE) is EACCES, which is particular to the file descriptor that opened the file.
File contexts, and NFS_CONTEXT_ERROR_WRITE solve that problem by being private to the file descriptor.
Thanks for the reminder that errors are per-context and this patch drops this. The per-context nature of errors in NFS was the reason that I nagged Jeff to make errseq_t a stand-alone type rather than just a part of address_space. I had envisaged that it would be embedded in the open_context as well.
We still could do that, but as there is precisely one open-file for each open_context, the gains are not great.
However, while looking over the code to make sure I really understood it and all the possible consequences of changing to errseq_t I found a few anomalies. The patch below addresses them all.
Would you see if they may sense to you?
Thanks,
NeilBrown
From: NeilBrown <[email protected]>
Date: Mon, 11 Sep 2017 13:15:50 +1000
Subject: [PATCH] NFS: various changes relating to reporting IO errors.
1/ remove 'start' and 'end' args from nfs_file_fsync_commit().
They aren't used.
2/ Make nfs_context_set_write_error() a "static inline" in internal.h
so we can...
3/ Use nfs_context_set_write_error() instead of mapping_set_error()
if nfs_pageio_add_request() fails before sending any request. NFS generally keeps errors in the open_context, not the mapping, so this is more consistent.
4/ If filemap_write_and_write_range() reports any error, still
check ctx->error. The value in ctx->error is likely to be more useful. As part of this, NFS_CONTEXT_ERROR_WRITE is cleared slightly earlier, before nfs_file_fsync_commit() is called, rather than at the start of that function.
Signed-off-by NeilBrown <[email protected]>
---
 fs/nfs/file.c     | 16 ++++++++++------
 fs/nfs/internal.h |  7 +++++++
 fs/nfs/pagelist.c |  4 ++--
 fs/nfs/write.c    |  7 -------
 4 files changed, 19 insertions(+), 15 deletions(-)
fs/nfs/file.c +10 -6
index af330c31f627..ab324f14081f 100644
@@ -208,21 +208,19 @@ EXPORT_SYMBOL_GPL(nfs_file_mmap);
  * fall back to doing a synchronous write.
  */
 static int
-nfs_file_fsync_commit(struct file *file, loff_t start, loff_t end, int datasync)
+nfs_file_fsync_commit(struct file *file, int datasync)
 {
 	struct nfs_open_context *ctx = nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
+	int do_resend, status;
 	int ret = 0;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file, datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend = test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
-	have_error = test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
 	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
-	if (have_error) {
+	if (test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags)) {
 		ret = xchg(&ctx->error, 0);
 		if (ret)
 			goto out;
@@ -247,10 +245,16 @@ 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;
+		}
 		if (ret != 0)
 			break;
-		ret = nfs_file_fsync_commit(file, start, end, datasync);
+		ret = nfs_file_fsync_commit(file, datasync);
 		if (!ret)
 			ret = pnfs_sync_inode(inode, !!datasync);
 		/*
fs/nfs/internal.h +7 -0
index dc456416d2be..44c8962fec91 100644
@@ -769,3 +769,10 @@ static inline bool nfs_error_is_fatal(int err)
 		return false;
 	}
 }
+
+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);
+}
fs/nfs/pagelist.c +2 -2
index de9066a92c0d..0ebd26b9a6bd 100644
@@ -1198,8 +1198,8 @@ out_failed:
 
 		/* remember fatal errors */
 		if (nfs_error_is_fatal(desc->pg_error))
-			mapping_set_error(desc->pg_inode->i_mapping,
-					  desc->pg_error);
+			nfs_context_set_write_error(req->wb_context,
+						    desc->pg_error);
 
 		func = desc->pg_completion_ops->error_cleanup;
 		for (midx = 0; midx < desc->pg_mirror_count; midx++) {
fs/nfs/write.c +0 -0
index b1af5dee5e0a..f702bf2def79 100644
@@ -147,13 +147,6 @@ static void nfs_io_completion_put(struct nfs_io_completion *ioc)
 		kref_put(&ioc->refcount, nfs_io_completion_release);
 }
 

fs/nfs/write.c static void nfs_io_completion_put(struct nfs_io_completion *ioc) · line 152

Quoted 10 lines

fs/nfs/write.c +0 -7

-static 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);
-}
-
 /*
  * nfs_page_find_head_request_locked - find head request associated with @page
  *

This should probably be broken out into at least a 2-3 different patches.

Ok, so to make sure I understand:

All writeback is done under the aegis of an open context, and writes from different open contexts are not mergeable. We also flush to the server in the case that a dirty page is written via an incompatible open context. So with that we can always tie

In that case, yes...mixing in errseq_t doesn't really buy us much here, and I agree with most of the changes above.

That said...I'm still not thrilled with how NFS_CONTEXT_ERROR_WRITE is handled in this code. That flag is set when a write fails, but is only cleared on fsync.

That seems wrong to me. Why wait for an fsync to start doing async writes again once they start working? What if the application never does an fsync? Clearing that flag on a successful WRITE seems like it'd make more sense.

Signature · 1 line

Jeff Layton <[email protected]>

NeilBrown

2017-09-11 21:52 UTC #11 lore raw

Quoted 443 lines · NeilBrown

Quoted 427 lines · Trond Myklebust
Quoted 286 lines · Jeff Layton
Quoted 221 lines · NeilBrown
Quoted 220 lines · Jeff Layton
Quoted 171 lines · NeilBrown
Quoted 169 lines · Jeff Layton
Quoted 132 lines · NeilBrown
Quoted 97 lines · Jeff Layton
Quoted 96 lines · Jeff Layton
Quoted 94 lines
From: Jeff Layton <[email protected]>
There is some ambiguity in nfs about how writeback errors are
tracked.
For instance, nfs_pageio_add_request calls mapping_set_error when
the
add fails, but we track errors that occur after adding the
request
with a dedicated int error in the open context.
Now that we have better infrastructure for the vfs layer, this
latter int is now unnecessary. Just have nfs_context_set_write_error set
the error in the mapping when one occurs.
Have NFS use file_write_and_wait_range to initiate and wait on
writeback
of the data, and then check again after issuing the commit(s).
With this, we also don't need to pay attention to the ERROR_WRITE
flag for reporting, and just clear it to indicate to subsequent
writers that they should try to go asynchronous again.
In nfs_page_async_flush, sample the error before locking and
joining
the requests, and check for errors since that point.
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfs/file.c          | 24 +++++++++++-------------
 fs/nfs/inode.c         |  3 +--
 fs/nfs/write.c         |  8 ++++++--
 include/linux/nfs_fs.h |  1 -
 4 files changed, 18 insertions(+), 18 deletions(-)
I have a baling wire and duct tape solution for testing this with
xfstests (using iptables REJECT targets and soft mounts). This
seems to
make nfs do the right thing.
fs/nfs/file.c +2 -2
index 5713eb32a45e..15d3c6faafd3 100644
@@ -212,25 +212,23 @@ nfs_file_fsync_commit(struct file
 *file,
 loff_t start, loff_t end, int datasync)
 {
 	struct nfs_open_context *ctx =
 nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
-	int ret = 0;
+	int do_resend, status;
+	int ret;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n",
 file,
 datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend =
test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx-
Quoted 1 line
flags);
fs/nfs/file.c +0 -1
-	have_error =
test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE,
&ctx->flags);
fs/nfs/file.c +0 -2
-	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |=
test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
Quoted 1 line
flags);
fs/nfs/file.c +1 -6
-	if (have_error) {
-		ret = xchg(&ctx->error, 0);
-		if (ret)
-			goto out;
-	}
-	if (status < 0) {
+	clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
Quoted 1 line
flags);
Quoted 12 lines
Quoted 4 lines
fs/nfs/file.c
Quoted 4 lines
fs/nfs/file.c +4 -0
+	ret = nfs_commit_inode(inode, FLUSH_SYNC);
+
+	/* Recheck and advance after the commit */
+	status = file_check_and_advance_wb_err(file);
This change makes the code inconsistent with the comment above the
function, which still references ctx->error. The intent of the
comment
is still correct, but the details have changed.
Good catch. I'll fix that up in a respin.
Quoted 8 lines
Also, there is a call to mapping_set_error() in nfs_pageio_add_request().
I wonder if that should be changed to
nfs_context_set_write_error(req->wb_context, desc-
Quoted 1 line
pg_error)
??
Trickier question...
I'm not quite sure what semantics we're looking for with NFS_CONTEXT_ERROR_WRITE. I know that it forces writes to be synchronous, but I'm not quite sure why it gets cleared the way it
does. It's set on any error but cleared before issuing a commit.
Quoted 3 lines
I added a similar flag to Ceph inodes recently, but only clear it when
a write succeeds. Wouldn't that make more sense here as well?
It is a bit hard to wrap one's mind around.
In the original code (commit 7b159fc18d417980) it looks like:
- test-and-clear bit
- write and sync
- test-bit
This does, I think, seem safer than "clear on successful write" as the
writes could complete out-of-order and I wouldn't be surprised if the
unsuccessful ones completed with an error before the successful one -
particularly with an error like EDQUOT.
However the current code does the writes before the test-and- clear, and
only does the commit afterwards. That makes it less clear why the
current sequence is a good idea.
However ... nfs_file_fsync_commit() is only called if filemap_write_and_wait_range() returned with success, so we only clear
the flag after successful writes(?).
Oh....
This patch from me:
Commit: 2edb6bc3852c ("NFS - fix recent breakage to NFS error handling.")
Quoted 8 lines
Quoted 8 lines
seems to have been reverted by
Commit: 7b281ee02655 ("NFS: fsync() must exit with an error if page writeback failed")
which probably isn't good. It appears that this code is very fragile
and easily broken.
On further investigation, I think the problem that I fixed and then we
reintroduced will be fixed again - more permanently - by your patch.
The root problem is that nfs keeps error codes in a different way to the
MM core. By unifying those, the problem goes.
(The specific problem is that writes which hit EDQUOT on the server can
report EIO on the client).
Quoted 10 lines
Quoted 5 lines
Maybe we need to work out exactly what is required, and document it - so
we can stop breaking it.
Or maybe we need some unit tests.....
Yes, laying out what's necessary for this would be very helpful. We
clearly want to set the flag when an error occurs. Under what circumstances should we be clearing it?
Well.... looking back at 7b159fc18d417980f57ae which introduced the
flag, prior to that write errors (ctx->error) were only reported by nfs_file_flush and nfs_fsync, so only one close() and fsync().
After that commit, setting the flag would mean that errors could be returned by 'write'. So clearing as part of returning the error makes
perfect sense.
As long as the error gets recorded, and gets returned when it is recorded, it doesn't much matter when the flag is cleared. With your
patches we don't need to flag any more to get errors reliably reported.
Quoted 11 lines
Quoted 11 lines
Leaving the flag set means that writes go more slowly - we don't get
large queue of background rights building up but destined for failure.
This is the main point made in the comment message when the flag was
introduced.
Of course, by the time we first get an error there could already by a large queue, so we probably want that to drain completely before
allowing async writes again.
We already have this functionality implemented in the existing code.
Quoted 10 lines
Quoted 10 lines
It might make sense to have 2 flags. One which says "writes should be
synchronous", another that says "There was an error recently". We clear the error flag before calling nfs_fsync, and if it is still
clear afterwards, we clear the sync-writes flag. Maybe that is more
complex than needed though.
We also need to preserve the NFS_CONTEXT_RESEND_WRITES flag. I don't see any global mechanism that will replace that.
Quoted 31 lines
Quoted 9 lines
I'm leaning towards your suggestion that it doesn't matter very much
when it gets cleared, and clearing it on any successful write is simplest.
So I'm still in favor of using nfs_context_set_write_error() in nfs_pageio_add_request(), primarily because it is most consistent - we
don't need exceptions.
Thanks for taking a closer look. I can easily make the change above, and
I do think that keeping this mechanism as simple as possible will make
it easier to prevent bitrot.
That said... NFS_CONTEXT_ERROR_WRITE is a per ctx flag, and the ctx is a
per open file description object.
Is that the correct way to track this? All of the ctx's will share the
same inode. If we're getting writeback errors for one context, it's quite likely that we'll be seeing them via others.
I suppose the counterargument is when we have things like expiring krb5
tickets. Write failures via an expiring set of creds may have no effect
on writeback via other creds.
Quoted 3 lines
Still, I think a per-inode flag might make more sense here.
Thoughts?
Quoted 7 lines
As far as I'm concerned, that would be a regression. The most common problem when flushing writeback data to the server aside from ENOSPC (and possibly ESTALE) is EACCES, which is particular to the file descriptor that opened the file.
File contexts, and NFS_CONTEXT_ERROR_WRITE solve that problem by being private to the file descriptor.
Thanks for the reminder that errors are per-context and this patch drops this. The per-context nature of errors in NFS was the reason that I nagged Jeff to make errseq_t a stand-alone type rather than just a part of address_space. I had envisaged that it would be embedded in the open_context as well.
We still could do that, but as there is precisely one open-file for each open_context, the gains are not great.
However, while looking over the code to make sure I really understood it and all the possible consequences of changing to errseq_t I found a few anomalies. The patch below addresses them all.
Would you see if they may sense to you?
Thanks,
NeilBrown
From: NeilBrown <[email protected]>
Date: Mon, 11 Sep 2017 13:15:50 +1000
Subject: [PATCH] NFS: various changes relating to reporting IO errors.
1/ remove 'start' and 'end' args from nfs_file_fsync_commit().
They aren't used.
2/ Make nfs_context_set_write_error() a "static inline" in internal.h
so we can...
3/ Use nfs_context_set_write_error() instead of mapping_set_error()
if nfs_pageio_add_request() fails before sending any request. NFS generally keeps errors in the open_context, not the mapping, so this is more consistent.
4/ If filemap_write_and_write_range() reports any error, still
check ctx->error. The value in ctx->error is likely to be more useful. As part of this, NFS_CONTEXT_ERROR_WRITE is cleared slightly earlier, before nfs_file_fsync_commit() is called, rather than at the start of that function.
Signed-off-by NeilBrown <[email protected]>
---
 fs/nfs/file.c     | 16 ++++++++++------
 fs/nfs/internal.h |  7 +++++++
 fs/nfs/pagelist.c |  4 ++--
 fs/nfs/write.c    |  7 -------
 4 files changed, 19 insertions(+), 15 deletions(-)
fs/nfs/file.c +10 -6
index af330c31f627..ab324f14081f 100644
@@ -208,21 +208,19 @@ EXPORT_SYMBOL_GPL(nfs_file_mmap);
  * fall back to doing a synchronous write.
  */
 static int
-nfs_file_fsync_commit(struct file *file, loff_t start, loff_t end, int datasync)
+nfs_file_fsync_commit(struct file *file, int datasync)
 {
 	struct nfs_open_context *ctx = nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
+	int do_resend, status;
 	int ret = 0;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file, datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend = test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
-	have_error = test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
 	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
-	if (have_error) {
+	if (test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags)) {
 		ret = xchg(&ctx->error, 0);
 		if (ret)
 			goto out;
@@ -247,10 +245,16 @@ 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;
+		}
 		if (ret != 0)
 			break;
-		ret = nfs_file_fsync_commit(file, start, end, datasync);
+		ret = nfs_file_fsync_commit(file, datasync);
 		if (!ret)
 			ret = pnfs_sync_inode(inode, !!datasync);
 		/*
fs/nfs/internal.h +7 -0
index dc456416d2be..44c8962fec91 100644
@@ -769,3 +769,10 @@ static inline bool nfs_error_is_fatal(int err)
 		return false;
 	}
 }
+
+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);
+}
fs/nfs/pagelist.c +2 -2
index de9066a92c0d..0ebd26b9a6bd 100644
@@ -1198,8 +1198,8 @@ out_failed:
 
 		/* remember fatal errors */
 		if (nfs_error_is_fatal(desc->pg_error))
-			mapping_set_error(desc->pg_inode->i_mapping,
-					  desc->pg_error);
+			nfs_context_set_write_error(req->wb_context,
+						    desc->pg_error);
 
 		func = desc->pg_completion_ops->error_cleanup;
 		for (midx = 0; midx < desc->pg_mirror_count; midx++) {
fs/nfs/write.c +0 -0
index b1af5dee5e0a..f702bf2def79 100644
@@ -147,13 +147,6 @@ static void nfs_io_completion_put(struct nfs_io_completion *ioc)
 		kref_put(&ioc->refcount, nfs_io_completion_release);
 }
 
fs/nfs/write.c static void nfs_io_completion_put(struct nfs_io_completion *ioc) · line 152
Quoted 10 lines
fs/nfs/write.c +0 -7
-static 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);
-}
-
 /*
  * nfs_page_find_head_request_locked - find head request associated with @page
  *
This should probably be broken out into at least a 2-3 different patches.

Quoted 6 lines

Ok, so to make sure I understand:
All writeback is done under the aegis of an open context, and writes from different open contexts are not mergeable. We also flush to the server in the case that a dirty page is written via an incompatible open context. So with that we can always tie

Not quite. Writes from different open contexts are sometimes mergeable, providing the credential is the same and there are no locks that might get in the way. (nfs_flush_incompatible() gets rid of conflicts writes to the same page as part of nfs_write_begin().
When writes are merged, all contexts remain reachable from the request through an 'nfs_page'. nfs_write_completion() iterates over all the nfs_pages attached to the nfs_pgio_header, and sets the context write_error from the hdr->error.

Quoted 4 lines

In that case, yes...mixing in errseq_t doesn't really buy us much here, and I agree with most of the changes above.

Quoted 8 lines

That said...I'm still not thrilled with how NFS_CONTEXT_ERROR_WRITE is handled in this code. That flag is set when a write fails, but is only cleared on fsync.
That seems wrong to me. Why wait for an fsync to start doing async writes again once they start working? What if the application never does an fsync? Clearing that flag on a successful WRITE seems like it'd make more sense.

We don't really 'wait' for an fsync. Having NFS_CONTEXT_ERROR_WRITE means that the very next write will force an fsync (nfs_need_check_write()). So we really just wait for the next write. The current code doesn't seem "obviously right" to me, but it isn't "obviously wrong" either, and I can only make it obviously right to me by making it more complex, and I don't think I can justify that.

Thanks,
NeilBrown

Quoted 2 lines

Signature · 1 line
Jeff Layton <[email protected]>

Jeff Layton

2017-09-12 15:20 UTC #12 lore raw

Quoted 449 lines · NeilBrown

Quoted 442 lines · NeilBrown
Quoted 427 lines · Trond Myklebust
Quoted 286 lines · Jeff Layton
Quoted 221 lines · NeilBrown
Quoted 220 lines · Jeff Layton
Quoted 171 lines · NeilBrown
Quoted 169 lines · Jeff Layton
Quoted 132 lines · NeilBrown
Quoted 97 lines · Jeff Layton
Quoted 96 lines · Jeff Layton
Quoted 94 lines
From: Jeff Layton <[email protected]>
There is some ambiguity in nfs about how writeback errors are
tracked.
For instance, nfs_pageio_add_request calls mapping_set_error when
the
add fails, but we track errors that occur after adding the
request
with a dedicated int error in the open context.
Now that we have better infrastructure for the vfs layer, this
latter int is now unnecessary. Just have nfs_context_set_write_error set
the error in the mapping when one occurs.
Have NFS use file_write_and_wait_range to initiate and wait on
writeback
of the data, and then check again after issuing the commit(s).
With this, we also don't need to pay attention to the ERROR_WRITE
flag for reporting, and just clear it to indicate to subsequent
writers that they should try to go asynchronous again.
In nfs_page_async_flush, sample the error before locking and
joining
the requests, and check for errors since that point.
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfs/file.c          | 24 +++++++++++-------------
 fs/nfs/inode.c         |  3 +--
 fs/nfs/write.c         |  8 ++++++--
 include/linux/nfs_fs.h |  1 -
 4 files changed, 18 insertions(+), 18 deletions(-)
I have a baling wire and duct tape solution for testing this with
xfstests (using iptables REJECT targets and soft mounts). This
seems to
make nfs do the right thing.
fs/nfs/file.c +2 -2
index 5713eb32a45e..15d3c6faafd3 100644
@@ -212,25 +212,23 @@ nfs_file_fsync_commit(struct file
 *file,
 loff_t start, loff_t end, int datasync)
 {
 	struct nfs_open_context *ctx =
 nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
-	int ret = 0;
+	int do_resend, status;
+	int ret;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n",
 file,
 datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend =
test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx-
Quoted 1 line
flags);
fs/nfs/file.c +0 -1
-	have_error =
test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE,
&ctx->flags);
fs/nfs/file.c +0 -2
-	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |=
test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
Quoted 1 line
flags);
fs/nfs/file.c +1 -6
-	if (have_error) {
-		ret = xchg(&ctx->error, 0);
-		if (ret)
-			goto out;
-	}
-	if (status < 0) {
+	clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
Quoted 1 line
flags);
Quoted 12 lines
Quoted 4 lines
fs/nfs/file.c
Quoted 4 lines
fs/nfs/file.c +4 -0
+	ret = nfs_commit_inode(inode, FLUSH_SYNC);
+
+	/* Recheck and advance after the commit */
+	status = file_check_and_advance_wb_err(file);
This change makes the code inconsistent with the comment above the
function, which still references ctx->error. The intent of the
comment
is still correct, but the details have changed.
Good catch. I'll fix that up in a respin.
Quoted 8 lines
Also, there is a call to mapping_set_error() in nfs_pageio_add_request().
I wonder if that should be changed to
nfs_context_set_write_error(req->wb_context, desc-
Quoted 1 line
pg_error)
??
Trickier question...
I'm not quite sure what semantics we're looking for with NFS_CONTEXT_ERROR_WRITE. I know that it forces writes to be synchronous, but I'm not quite sure why it gets cleared the way it
does. It's set on any error but cleared before issuing a commit.
Quoted 3 lines
I added a similar flag to Ceph inodes recently, but only clear it when
a write succeeds. Wouldn't that make more sense here as well?
It is a bit hard to wrap one's mind around.
In the original code (commit 7b159fc18d417980) it looks like:
- test-and-clear bit
- write and sync
- test-bit
This does, I think, seem safer than "clear on successful write" as the
writes could complete out-of-order and I wouldn't be surprised if the
unsuccessful ones completed with an error before the successful one -
particularly with an error like EDQUOT.
However the current code does the writes before the test-and- clear, and
only does the commit afterwards. That makes it less clear why the
current sequence is a good idea.
However ... nfs_file_fsync_commit() is only called if filemap_write_and_wait_range() returned with success, so we only clear
the flag after successful writes(?).
Oh....
This patch from me:
Commit: 2edb6bc3852c ("NFS - fix recent breakage to NFS error handling.")
Quoted 8 lines
Quoted 8 lines
seems to have been reverted by
Commit: 7b281ee02655 ("NFS: fsync() must exit with an error if page writeback failed")
which probably isn't good. It appears that this code is very fragile
and easily broken.
On further investigation, I think the problem that I fixed and then we
reintroduced will be fixed again - more permanently - by your patch.
The root problem is that nfs keeps error codes in a different way to the
MM core. By unifying those, the problem goes.
(The specific problem is that writes which hit EDQUOT on the server can
report EIO on the client).
Quoted 10 lines
Quoted 5 lines
Maybe we need to work out exactly what is required, and document it - so
we can stop breaking it.
Or maybe we need some unit tests.....
Yes, laying out what's necessary for this would be very helpful. We
clearly want to set the flag when an error occurs. Under what circumstances should we be clearing it?
Well.... looking back at 7b159fc18d417980f57ae which introduced the
flag, prior to that write errors (ctx->error) were only reported by nfs_file_flush and nfs_fsync, so only one close() and fsync().
After that commit, setting the flag would mean that errors could be returned by 'write'. So clearing as part of returning the error makes
perfect sense.
As long as the error gets recorded, and gets returned when it is recorded, it doesn't much matter when the flag is cleared. With your
patches we don't need to flag any more to get errors reliably reported.
Quoted 11 lines
Quoted 11 lines
Leaving the flag set means that writes go more slowly - we don't get
large queue of background rights building up but destined for failure.
This is the main point made in the comment message when the flag was
introduced.
Of course, by the time we first get an error there could already by a large queue, so we probably want that to drain completely before
allowing async writes again.
We already have this functionality implemented in the existing code.
Quoted 10 lines
Quoted 10 lines
It might make sense to have 2 flags. One which says "writes should be
synchronous", another that says "There was an error recently". We clear the error flag before calling nfs_fsync, and if it is still
clear afterwards, we clear the sync-writes flag. Maybe that is more
complex than needed though.
We also need to preserve the NFS_CONTEXT_RESEND_WRITES flag. I don't see any global mechanism that will replace that.
Quoted 31 lines
Quoted 9 lines
I'm leaning towards your suggestion that it doesn't matter very much
when it gets cleared, and clearing it on any successful write is simplest.
So I'm still in favor of using nfs_context_set_write_error() in nfs_pageio_add_request(), primarily because it is most consistent - we
don't need exceptions.
Thanks for taking a closer look. I can easily make the change above, and
I do think that keeping this mechanism as simple as possible will make
it easier to prevent bitrot.
That said... NFS_CONTEXT_ERROR_WRITE is a per ctx flag, and the ctx is a
per open file description object.
Is that the correct way to track this? All of the ctx's will share the
same inode. If we're getting writeback errors for one context, it's quite likely that we'll be seeing them via others.
I suppose the counterargument is when we have things like expiring krb5
tickets. Write failures via an expiring set of creds may have no effect
on writeback via other creds.
Quoted 3 lines
Still, I think a per-inode flag might make more sense here.
Thoughts?
Quoted 7 lines
As far as I'm concerned, that would be a regression. The most common problem when flushing writeback data to the server aside from ENOSPC (and possibly ESTALE) is EACCES, which is particular to the file descriptor that opened the file.
File contexts, and NFS_CONTEXT_ERROR_WRITE solve that problem by being private to the file descriptor.
Thanks for the reminder that errors are per-context and this patch drops this. The per-context nature of errors in NFS was the reason that I nagged Jeff to make errseq_t a stand-alone type rather than just a part of address_space. I had envisaged that it would be embedded in the open_context as well.
We still could do that, but as there is precisely one open-file for each open_context, the gains are not great.
However, while looking over the code to make sure I really understood it and all the possible consequences of changing to errseq_t I found a few anomalies. The patch below addresses them all.
Would you see if they may sense to you?
Thanks,
NeilBrown
From: NeilBrown <[email protected]>
Date: Mon, 11 Sep 2017 13:15:50 +1000
Subject: [PATCH] NFS: various changes relating to reporting IO errors.
1/ remove 'start' and 'end' args from nfs_file_fsync_commit().
They aren't used.
2/ Make nfs_context_set_write_error() a "static inline" in internal.h
so we can...
3/ Use nfs_context_set_write_error() instead of mapping_set_error()
if nfs_pageio_add_request() fails before sending any request. NFS generally keeps errors in the open_context, not the mapping, so this is more consistent.
4/ If filemap_write_and_write_range() reports any error, still
check ctx->error. The value in ctx->error is likely to be more useful. As part of this, NFS_CONTEXT_ERROR_WRITE is cleared slightly earlier, before nfs_file_fsync_commit() is called, rather than at the start of that function.
Signed-off-by NeilBrown <[email protected]>
---
 fs/nfs/file.c     | 16 ++++++++++------
 fs/nfs/internal.h |  7 +++++++
 fs/nfs/pagelist.c |  4 ++--
 fs/nfs/write.c    |  7 -------
 4 files changed, 19 insertions(+), 15 deletions(-)
fs/nfs/file.c +10 -6
index af330c31f627..ab324f14081f 100644
@@ -208,21 +208,19 @@ EXPORT_SYMBOL_GPL(nfs_file_mmap);
  * fall back to doing a synchronous write.
  */
 static int
-nfs_file_fsync_commit(struct file *file, loff_t start, loff_t end, int datasync)
+nfs_file_fsync_commit(struct file *file, int datasync)
 {
 	struct nfs_open_context *ctx = nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
+	int do_resend, status;
 	int ret = 0;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file, datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend = test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
-	have_error = test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
 	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
-	if (have_error) {
+	if (test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags)) {
 		ret = xchg(&ctx->error, 0);
 		if (ret)
 			goto out;
@@ -247,10 +245,16 @@ 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;
+		}
 		if (ret != 0)
 			break;
-		ret = nfs_file_fsync_commit(file, start, end, datasync);
+		ret = nfs_file_fsync_commit(file, datasync);
 		if (!ret)
 			ret = pnfs_sync_inode(inode, !!datasync);
 		/*
fs/nfs/internal.h +7 -0
index dc456416d2be..44c8962fec91 100644
@@ -769,3 +769,10 @@ static inline bool nfs_error_is_fatal(int err)
 		return false;
 	}
 }
+
+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);
+}
fs/nfs/pagelist.c +2 -2
index de9066a92c0d..0ebd26b9a6bd 100644
@@ -1198,8 +1198,8 @@ out_failed:
 
 		/* remember fatal errors */
 		if (nfs_error_is_fatal(desc->pg_error))
-			mapping_set_error(desc->pg_inode->i_mapping,
-					  desc->pg_error);
+			nfs_context_set_write_error(req->wb_context,
+						    desc->pg_error);
 
 		func = desc->pg_completion_ops->error_cleanup;
 		for (midx = 0; midx < desc->pg_mirror_count; midx++) {
fs/nfs/write.c +0 -0
index b1af5dee5e0a..f702bf2def79 100644
@@ -147,13 +147,6 @@ static void nfs_io_completion_put(struct nfs_io_completion *ioc)
 		kref_put(&ioc->refcount, nfs_io_completion_release);
 }
 
fs/nfs/write.c static void nfs_io_completion_put(struct nfs_io_completion *ioc) · line 152
Quoted 10 lines
fs/nfs/write.c +0 -7
-static 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);
-}
-
 /*
  * nfs_page_find_head_request_locked - find head request associated with @page
  *
This should probably be broken out into at least a 2-3 different patches.
Quoted 6 lines
Ok, so to make sure I understand:
All writeback is done under the aegis of an open context, and writes from different open contexts are not mergeable. We also flush to the server in the case that a dirty page is written via an incompatible open context. So with that we can always tie

Quoted 9 lines

Not quite. Writes from different open contexts are sometimes mergeable, providing the credential is the same and there are no locks that might get in the way. (nfs_flush_incompatible() gets rid of conflicts writes to the same page as part of nfs_write_begin().
When writes are merged, all contexts remain reachable from the request through an 'nfs_page'. nfs_write_completion() iterates over all the nfs_pages attached to the nfs_pgio_header, and sets the context write_error from the hdr->error.

Ok, by this account, NFS should already have "correct" error reporting semantics on fsync. i.e. when the file is written via multiple fds, you should get back an error on all fds if those writebacks failed.

I have a test for nfs for the new-style error reporting:

https://git.kernel.org/pub/scm/linux/kernel/git/jlayton/xfstests-dev.git/log/?h=wberr

The nfs test is still pretty rickety, using soft mounts and iptables to cause requests to fail. With the patch I originally proposed, this test would pass. When I run this test on normal mainline kernels, it fails:

-------------------------------8<------------------------------

FSTYP         -- nfs
PLATFORM      -- Linux/x86_64 wberr 4.12.11-300.fc26.x86_64
MKFS_OPTIONS  -- knfsdsrv:/export/scratch

MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 knfsdsrv:/export/scratch /mnt/scratch

nfs/002 - output mismatch (see /home/jlayton/git/xfstests/results//nfs/002.out.bad)
--- tests/nfs/002.out 2017-07-19 13:12:59.354561869 -0400
+++ /home/jlayton/git/xfstests/results//nfs/002.out.bad 2017-09-12 11:17:17.335943539 -0400
@@ -1,3 +1,3 @@
QA output created by 002
Format and mount
-Test passed!
+Success on second fsync on fd[1]!
...
(Run 'diff -u tests/nfs/002.out /home/jlayton/git/xfstests/results//nfs/002.out.bad' to see the entire diff)
Ran: nfs/002
Failures: nfs/002
Failed 1 of 1 tests
-------------------------------8<------------------------------

I'm not sure that errors are really propagated to all struct files like you suggest above. I'll plan to look a little more closely at what's happening here, when I get some time.

Quoted 13 lines

Quoted 4 lines
In that case, yes...mixing in errseq_t doesn't really buy us much here, and I agree with most of the changes above.
Quoted 8 lines
That said...I'm still not thrilled with how NFS_CONTEXT_ERROR_WRITE is handled in this code. That flag is set when a write fails, but is only cleared on fsync.
That seems wrong to me. Why wait for an fsync to start doing async writes again once they start working? What if the application never does an fsync? Clearing that flag on a successful WRITE seems like it'd make more sense.

Quoted 6 lines

We don't really 'wait' for an fsync. Having NFS_CONTEXT_ERROR_WRITE means that the very next write will force an fsync (nfs_need_check_write()). So we really just wait for the next write. The current code doesn't seem "obviously right" to me, but it isn't "obviously wrong" either, and I can only make it obviously right to me by making it more complex, and I don't think I can justify that.

Thanks for pointing this out. I missed the bit about it forcing the fsync when this fails. I agree that that should be fine.

Signature · 1 line

Jeff Layton <[email protected]>

NeilBrown

2017-09-12 21:47 UTC #13 lore raw

Quoted 478 lines · Jeff Layton

Quoted 449 lines · NeilBrown
Quoted 442 lines · NeilBrown
Quoted 427 lines · Trond Myklebust
Quoted 286 lines · Jeff Layton
Quoted 221 lines · NeilBrown
Quoted 220 lines · Jeff Layton
Quoted 171 lines · NeilBrown
Quoted 169 lines · Jeff Layton
Quoted 132 lines · NeilBrown
Quoted 97 lines · Jeff Layton
Quoted 96 lines · Jeff Layton
Quoted 94 lines
From: Jeff Layton <[email protected]>
There is some ambiguity in nfs about how writeback
errors are
tracked.
For instance, nfs_pageio_add_request calls
mapping_set_error when
the
add fails, but we track errors that occur after adding
the
request
with a dedicated int error in the open context.
Now that we have better infrastructure for the vfs
layer, this
latter int is now unnecessary. Just have
nfs_context_set_write_error set
the error in the mapping when one occurs.
Have NFS use file_write_and_wait_range to initiate and
wait on
writeback
of the data, and then check again after issuing the
commit(s).
With this, we also don't need to pay attention to the
ERROR_WRITE
flag for reporting, and just clear it to indicate to
subsequent
writers that they should try to go asynchronous again.
In nfs_page_async_flush, sample the error before
locking and
joining
the requests, and check for errors since that point.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfs/file.c | 24 +++++++++++-------------
fs/nfs/inode.c | 3 +--
fs/nfs/write.c | 8 ++++++--
include/linux/nfs_fs.h | 1 -
4 files changed, 18 insertions(+), 18 deletions(-)
I have a baling wire and duct tape solution for testing
this with
xfstests (using iptables REJECT targets and soft
mounts). This
seems to
make nfs do the right thing.
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 5713eb32a45e..15d3c6faafd3 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -212,25 +212,23 @@ nfs_file_fsync_commit(struct file
*file,
loff_t start, loff_t end, int datasync)
{
struct nfs_open_context *ctx =
nfs_file_open_context(file);
struct inode *inode = file_inode(file);
- int have_error, do_resend, status;
- int ret = 0;
+ int do_resend, status;
+ int ret;
dprintk("NFS: fsync file(%pD2) datasync %d\n",
file,
datasync);
nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
do_resend =
test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx-
flags);
- have_error =
test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE,
&ctx->flags);
- status = nfs_commit_inode(inode, FLUSH_SYNC);
- have_error |=
test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
flags);
- if (have_error) {
- ret = xchg(&ctx->error, 0);
- if (ret)
- goto out;
- }
- if (status < 0) {
+ clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
flags);
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
+ ret = nfs_commit_inode(inode, FLUSH_SYNC);
+
+ /* Recheck and advance after the commit */
+ status = file_check_and_advance_wb_err(file);
This change makes the code inconsistent with the comment above the
function, which still references ctx->error. The intent of the
comment
is still correct, but the details have changed.
Good catch. I'll fix that up in a respin.
Quoted 8 lines
Also, there is a call to mapping_set_error() in nfs_pageio_add_request().
I wonder if that should be changed to
nfs_context_set_write_error(req->wb_context, desc-
Quoted 1 line
pg_error)
??
Trickier question...
I'm not quite sure what semantics we're looking for with NFS_CONTEXT_ERROR_WRITE. I know that it forces writes to be synchronous, but I'm not quite sure why it gets cleared the way it
does. It's set on any error but cleared before issuing a commit.
Quoted 3 lines
I added a similar flag to Ceph inodes recently, but only clear it when
a write succeeds. Wouldn't that make more sense here as well?
It is a bit hard to wrap one's mind around.
In the original code (commit 7b159fc18d417980) it looks like:
- test-and-clear bit
- write and sync
- test-bit
This does, I think, seem safer than "clear on successful write" as the
writes could complete out-of-order and I wouldn't be surprised if the
unsuccessful ones completed with an error before the successful one -
particularly with an error like EDQUOT.
However the current code does the writes before the test-and- clear, and
only does the commit afterwards. That makes it less clear why the
current sequence is a good idea.
However ... nfs_file_fsync_commit() is only called if filemap_write_and_wait_range() returned with success, so we only clear
the flag after successful writes(?).
Oh....
This patch from me:
Commit: 2edb6bc3852c ("NFS - fix recent breakage to NFS error handling.")
Quoted 8 lines
Quoted 8 lines
seems to have been reverted by
Commit: 7b281ee02655 ("NFS: fsync() must exit with an error if page writeback failed")
which probably isn't good. It appears that this code is very fragile
and easily broken.
On further investigation, I think the problem that I fixed and then we
reintroduced will be fixed again - more permanently - by your patch.
The root problem is that nfs keeps error codes in a different way to the
MM core. By unifying those, the problem goes.
(The specific problem is that writes which hit EDQUOT on the server can
report EIO on the client).
Quoted 10 lines
Quoted 5 lines
Maybe we need to work out exactly what is required, and document it - so
we can stop breaking it.
Or maybe we need some unit tests.....
Yes, laying out what's necessary for this would be very helpful. We
clearly want to set the flag when an error occurs. Under what circumstances should we be clearing it?
Well.... looking back at 7b159fc18d417980f57ae which introduced the
flag, prior to that write errors (ctx->error) were only reported by nfs_file_flush and nfs_fsync, so only one close() and fsync().
After that commit, setting the flag would mean that errors could be returned by 'write'. So clearing as part of returning the error makes
perfect sense.
As long as the error gets recorded, and gets returned when it is recorded, it doesn't much matter when the flag is cleared. With your
patches we don't need to flag any more to get errors reliably reported.
Quoted 11 lines
Quoted 11 lines
Leaving the flag set means that writes go more slowly - we don't get
large queue of background rights building up but destined for failure.
This is the main point made in the comment message when the flag was
introduced.
Of course, by the time we first get an error there could already by a large queue, so we probably want that to drain completely before
allowing async writes again.
We already have this functionality implemented in the existing code.
Quoted 10 lines
Quoted 10 lines
It might make sense to have 2 flags. One which says "writes should be
synchronous", another that says "There was an error recently". We clear the error flag before calling nfs_fsync, and if it is still
clear afterwards, we clear the sync-writes flag. Maybe that is more
complex than needed though.
We also need to preserve the NFS_CONTEXT_RESEND_WRITES flag. I don't see any global mechanism that will replace that.
Quoted 31 lines
Quoted 9 lines
I'm leaning towards your suggestion that it doesn't matter very much
when it gets cleared, and clearing it on any successful write is simplest.
So I'm still in favor of using nfs_context_set_write_error() in nfs_pageio_add_request(), primarily because it is most consistent - we
don't need exceptions.
Thanks for taking a closer look. I can easily make the change above, and
I do think that keeping this mechanism as simple as possible will make
it easier to prevent bitrot.
That said... NFS_CONTEXT_ERROR_WRITE is a per ctx flag, and the ctx is a
per open file description object.
Is that the correct way to track this? All of the ctx's will share the
same inode. If we're getting writeback errors for one context, it's quite likely that we'll be seeing them via others.
I suppose the counterargument is when we have things like expiring krb5
tickets. Write failures via an expiring set of creds may have no effect
on writeback via other creds.
Quoted 3 lines
Still, I think a per-inode flag might make more sense here.
Thoughts?
Quoted 7 lines
As far as I'm concerned, that would be a regression. The most common problem when flushing writeback data to the server aside from ENOSPC (and possibly ESTALE) is EACCES, which is particular to the file descriptor that opened the file.
File contexts, and NFS_CONTEXT_ERROR_WRITE solve that problem by being private to the file descriptor.
Thanks for the reminder that errors are per-context and this patch drops this. The per-context nature of errors in NFS was the reason that I nagged Jeff to make errseq_t a stand-alone type rather than just a part of address_space. I had envisaged that it would be embedded in the open_context as well.
We still could do that, but as there is precisely one open-file for each open_context, the gains are not great.
However, while looking over the code to make sure I really understood it and all the possible consequences of changing to errseq_t I found a few anomalies. The patch below addresses them all.
Would you see if they may sense to you?
Thanks,
NeilBrown
From: NeilBrown <[email protected]>
Date: Mon, 11 Sep 2017 13:15:50 +1000
Subject: [PATCH] NFS: various changes relating to reporting IO errors.
1/ remove 'start' and 'end' args from nfs_file_fsync_commit().
They aren't used.
2/ Make nfs_context_set_write_error() a "static inline" in internal.h
so we can...
3/ Use nfs_context_set_write_error() instead of mapping_set_error()
if nfs_pageio_add_request() fails before sending any request. NFS generally keeps errors in the open_context, not the mapping, so this is more consistent.
4/ If filemap_write_and_write_range() reports any error, still
check ctx->error. The value in ctx->error is likely to be more useful. As part of this, NFS_CONTEXT_ERROR_WRITE is cleared slightly earlier, before nfs_file_fsync_commit() is called, rather than at the start of that function.
Signed-off-by NeilBrown <[email protected]>
---
 fs/nfs/file.c     | 16 ++++++++++------
 fs/nfs/internal.h |  7 +++++++
 fs/nfs/pagelist.c |  4 ++--
 fs/nfs/write.c    |  7 -------
 4 files changed, 19 insertions(+), 15 deletions(-)
fs/nfs/file.c +10 -6
index af330c31f627..ab324f14081f 100644
@@ -208,21 +208,19 @@ EXPORT_SYMBOL_GPL(nfs_file_mmap);
  * fall back to doing a synchronous write.
  */
 static int
-nfs_file_fsync_commit(struct file *file, loff_t start, loff_t end, int datasync)
+nfs_file_fsync_commit(struct file *file, int datasync)
 {
 	struct nfs_open_context *ctx = nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
+	int do_resend, status;
 	int ret = 0;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file, datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend = test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
-	have_error = test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
 	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
-	if (have_error) {
+	if (test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags)) {
 		ret = xchg(&ctx->error, 0);
 		if (ret)
 			goto out;
@@ -247,10 +245,16 @@ 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;
+		}
 		if (ret != 0)
 			break;
-		ret = nfs_file_fsync_commit(file, start, end, datasync);
+		ret = nfs_file_fsync_commit(file, datasync);
 		if (!ret)
 			ret = pnfs_sync_inode(inode, !!datasync);
 		/*
fs/nfs/internal.h +7 -0
index dc456416d2be..44c8962fec91 100644
@@ -769,3 +769,10 @@ static inline bool nfs_error_is_fatal(int err)
 		return false;
 	}
 }
+
+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);
+}
fs/nfs/pagelist.c +2 -2
index de9066a92c0d..0ebd26b9a6bd 100644
@@ -1198,8 +1198,8 @@ out_failed:
 
 		/* remember fatal errors */
 		if (nfs_error_is_fatal(desc->pg_error))
-			mapping_set_error(desc->pg_inode->i_mapping,
-					  desc->pg_error);
+			nfs_context_set_write_error(req->wb_context,
+						    desc->pg_error);
 
 		func = desc->pg_completion_ops->error_cleanup;
 		for (midx = 0; midx < desc->pg_mirror_count; midx++) {
fs/nfs/write.c +0 -0
index b1af5dee5e0a..f702bf2def79 100644
@@ -147,13 +147,6 @@ static void nfs_io_completion_put(struct nfs_io_completion *ioc)
 		kref_put(&ioc->refcount, nfs_io_completion_release);
 }
 
fs/nfs/write.c static void nfs_io_completion_put(struct nfs_io_completion *ioc) · line 152
Quoted 10 lines
fs/nfs/write.c +0 -7
-static 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);
-}
-
 /*
  * nfs_page_find_head_request_locked - find head request associated with @page
  *
This should probably be broken out into at least a 2-3 different patches.
Quoted 6 lines
Ok, so to make sure I understand:
All writeback is done under the aegis of an open context, and writes from different open contexts are not mergeable. We also flush to the server in the case that a dirty page is written via an incompatible open context. So with that we can always tie
Quoted 9 lines
Not quite. Writes from different open contexts are sometimes mergeable, providing the credential is the same and there are no locks that might get in the way. (nfs_flush_incompatible() gets rid of conflicts writes to the same page as part of nfs_write_begin().
When writes are merged, all contexts remain reachable from the request through an 'nfs_page'. nfs_write_completion() iterates over all the nfs_pages attached to the nfs_pgio_header, and sets the context write_error from the hdr->error.
Ok, by this account, NFS should already have "correct" error reporting semantics on fsync. i.e. when the file is written via multiple fds, you should get back an error on all fds if those writebacks failed.
I have a test for nfs for the new-style error reporting:
https://git.kernel.org/pub/scm/linux/kernel/git/jlayton/xfstests-dev.git/log/?h=wberr
The nfs test is still pretty rickety, using soft mounts and iptables to cause requests to fail. With the patch I originally proposed, this test would pass. When I run this test on normal mainline kernels, it fails:
-------------------------------8<------------------------------
FSTYP         -- nfs
PLATFORM      -- Linux/x86_64 wberr 4.12.11-300.fc26.x86_64
MKFS_OPTIONS  -- knfsdsrv:/export/scratch
MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 knfsdsrv:/export/scratch /mnt/scratch

Quoted 8 lines

nfs/002 - output mismatch (see /home/jlayton/git/xfstests/results//nfs/002.out.bad)
--- tests/nfs/002.out 2017-07-19 13:12:59.354561869 -0400
+++ /home/jlayton/git/xfstests/results//nfs/002.out.bad 2017-09-12 11:17:17.335943539 -0400
@@ -1,3 +1,3 @@
QA output created by 002
Format and mount
-Test passed!
+Success on second fsync on fd[1]!

Interesting.
I haven't reviewed the kernel code yet, but having looked at the test code I see that the one file is opened 10 time and that the *same* block in the file (65k?) is written via each fd.
If I write to a file, and then someone else over-writes all the bytes that I wrote, then the page is written to the server and gets an error, then you could argue that as none of the bytes I wrote were involved in the error, I don't need to see the error status - someone else has taken on responsibility for that range.

Maybe the test should/could write a few bytes with a different offset for each fd ??

NeilBrown

Quoted 40 lines

...
(Run 'diff -u tests/nfs/002.out /home/jlayton/git/xfstests/results//nfs/002.out.bad' to see the entire diff)
Ran: nfs/002
Failures: nfs/002
Failed 1 of 1 tests
-------------------------------8<------------------------------
I'm not sure that errors are really propagated to all struct files like you suggest above. I'll plan to look a little more closely at what's happening here, when I get some time.
Quoted 13 lines
Quoted 4 lines
In that case, yes...mixing in errseq_t doesn't really buy us much here, and I agree with most of the changes above.
Quoted 8 lines
That said...I'm still not thrilled with how NFS_CONTEXT_ERROR_WRITE is handled in this code. That flag is set when a write fails, but is only cleared on fsync.
That seems wrong to me. Why wait for an fsync to start doing async writes again once they start working? What if the application never does an fsync? Clearing that flag on a successful WRITE seems like it'd make more sense.
Quoted 6 lines
We don't really 'wait' for an fsync. Having NFS_CONTEXT_ERROR_WRITE means that the very next write will force an fsync (nfs_need_check_write()). So we really just wait for the next write. The current code doesn't seem "obviously right" to me, but it isn't "obviously wrong" either, and I can only make it obviously right to me by making it more complex, and I don't think I can justify that.
Thanks for pointing this out. I missed the bit about it forcing the fsync when this fails. I agree that that should be fine.
Signature · 5 lines
Jeff Layton <[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff Layton

Reviewed-by

2017-09-13 12:23 UTC #14 lore raw

Quoted 499 lines · NeilBrown

Quoted 478 lines · Jeff Layton
Quoted 449 lines · NeilBrown
Quoted 442 lines · NeilBrown
Quoted 427 lines · Trond Myklebust
Quoted 286 lines · Jeff Layton
Quoted 221 lines · NeilBrown
Quoted 220 lines · Jeff Layton
Quoted 171 lines · NeilBrown
Quoted 169 lines · Jeff Layton
Quoted 132 lines · NeilBrown
Quoted 97 lines · Jeff Layton
Quoted 96 lines · Jeff Layton
From: Jeff Layton <[email protected]>
There is some ambiguity in nfs about how writeback
errors are
tracked.
For instance, nfs_pageio_add_request calls
mapping_set_error when
the
add fails, but we track errors that occur after adding
the
request
with a dedicated int error in the open context.
Now that we have better infrastructure for the vfs
layer, this
latter int is now unnecessary. Just have
nfs_context_set_write_error set
the error in the mapping when one occurs.
Have NFS use file_write_and_wait_range to initiate and
wait on
writeback
of the data, and then check again after issuing the
commit(s).
With this, we also don't need to pay attention to the
ERROR_WRITE
flag for reporting, and just clear it to indicate to
subsequent
writers that they should try to go asynchronous again.
In nfs_page_async_flush, sample the error before
locking and
joining
the requests, and check for errors since that point.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfs/file.c | 24 +++++++++++-------------
fs/nfs/inode.c | 3 +--
fs/nfs/write.c | 8 ++++++--
include/linux/nfs_fs.h | 1 -
4 files changed, 18 insertions(+), 18 deletions(-)
I have a baling wire and duct tape solution for testing
this with
xfstests (using iptables REJECT targets and soft
mounts). This
seems to
make nfs do the right thing.
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 5713eb32a45e..15d3c6faafd3 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -212,25 +212,23 @@ nfs_file_fsync_commit(struct file
*file,
loff_t start, loff_t end, int datasync)
{
struct nfs_open_context *ctx =
nfs_file_open_context(file);
struct inode *inode = file_inode(file);
- int have_error, do_resend, status;
- int ret = 0;
+ int do_resend, status;
+ int ret;
dprintk("NFS: fsync file(%pD2) datasync %d\n",
file,
datasync);
nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
do_resend =
test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx-
flags);
- have_error =
test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE,
&ctx->flags);
- status = nfs_commit_inode(inode, FLUSH_SYNC);
- have_error |=
test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
flags);
- if (have_error) {
- ret = xchg(&ctx->error, 0);
- if (ret)
- goto out;
- }
- if (status < 0) {
+ clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-
flags);
Quoted 12 lines
Quoted 4 lines
+ ret = nfs_commit_inode(inode, FLUSH_SYNC);
+
+ /* Recheck and advance after the commit */
+ status = file_check_and_advance_wb_err(file);
This change makes the code inconsistent with the comment above the
function, which still references ctx->error. The intent of the
comment
is still correct, but the details have changed.
Good catch. I'll fix that up in a respin.
Quoted 8 lines
Also, there is a call to mapping_set_error() in nfs_pageio_add_request().
I wonder if that should be changed to
nfs_context_set_write_error(req->wb_context, desc-
Quoted 1 line
pg_error)
??
Trickier question...
I'm not quite sure what semantics we're looking for with NFS_CONTEXT_ERROR_WRITE. I know that it forces writes to be synchronous, but I'm not quite sure why it gets cleared the way it
does. It's set on any error but cleared before issuing a commit.
Quoted 3 lines
I added a similar flag to Ceph inodes recently, but only clear it when
a write succeeds. Wouldn't that make more sense here as well?
It is a bit hard to wrap one's mind around.
In the original code (commit 7b159fc18d417980) it looks like:
- test-and-clear bit
- write and sync
- test-bit
This does, I think, seem safer than "clear on successful write" as the
writes could complete out-of-order and I wouldn't be surprised if the
unsuccessful ones completed with an error before the successful one -
particularly with an error like EDQUOT.
However the current code does the writes before the test-and- clear, and
only does the commit afterwards. That makes it less clear why the
current sequence is a good idea.
However ... nfs_file_fsync_commit() is only called if filemap_write_and_wait_range() returned with success, so we only clear
the flag after successful writes(?).
Oh....
This patch from me:
Commit: 2edb6bc3852c ("NFS - fix recent breakage to NFS error handling.")
Quoted 8 lines
Quoted 8 lines
seems to have been reverted by
Commit: 7b281ee02655 ("NFS: fsync() must exit with an error if page writeback failed")
which probably isn't good. It appears that this code is very fragile
and easily broken.
On further investigation, I think the problem that I fixed and then we
reintroduced will be fixed again - more permanently - by your patch.
The root problem is that nfs keeps error codes in a different way to the
MM core. By unifying those, the problem goes.
(The specific problem is that writes which hit EDQUOT on the server can
report EIO on the client).
Quoted 10 lines
Quoted 5 lines
Maybe we need to work out exactly what is required, and document it - so
we can stop breaking it.
Or maybe we need some unit tests.....
Yes, laying out what's necessary for this would be very helpful. We
clearly want to set the flag when an error occurs. Under what circumstances should we be clearing it?
Well.... looking back at 7b159fc18d417980f57ae which introduced the
flag, prior to that write errors (ctx->error) were only reported by nfs_file_flush and nfs_fsync, so only one close() and fsync().
After that commit, setting the flag would mean that errors could be returned by 'write'. So clearing as part of returning the error makes
perfect sense.
As long as the error gets recorded, and gets returned when it is recorded, it doesn't much matter when the flag is cleared. With your
patches we don't need to flag any more to get errors reliably reported.
Quoted 11 lines
Quoted 11 lines
Leaving the flag set means that writes go more slowly - we don't get
large queue of background rights building up but destined for failure.
This is the main point made in the comment message when the flag was
introduced.
Of course, by the time we first get an error there could already by a large queue, so we probably want that to drain completely before
allowing async writes again.
We already have this functionality implemented in the existing code.
Quoted 10 lines
Quoted 10 lines
It might make sense to have 2 flags. One which says "writes should be
synchronous", another that says "There was an error recently". We clear the error flag before calling nfs_fsync, and if it is still
clear afterwards, we clear the sync-writes flag. Maybe that is more
complex than needed though.
We also need to preserve the NFS_CONTEXT_RESEND_WRITES flag. I don't see any global mechanism that will replace that.
Quoted 31 lines
Quoted 9 lines
I'm leaning towards your suggestion that it doesn't matter very much
when it gets cleared, and clearing it on any successful write is simplest.
So I'm still in favor of using nfs_context_set_write_error() in nfs_pageio_add_request(), primarily because it is most consistent - we
don't need exceptions.
Thanks for taking a closer look. I can easily make the change above, and
I do think that keeping this mechanism as simple as possible will make
it easier to prevent bitrot.
That said... NFS_CONTEXT_ERROR_WRITE is a per ctx flag, and the ctx is a
per open file description object.
Is that the correct way to track this? All of the ctx's will share the
same inode. If we're getting writeback errors for one context, it's quite likely that we'll be seeing them via others.
I suppose the counterargument is when we have things like expiring krb5
tickets. Write failures via an expiring set of creds may have no effect
on writeback via other creds.
Quoted 3 lines
Still, I think a per-inode flag might make more sense here.
Thoughts?
Quoted 7 lines
As far as I'm concerned, that would be a regression. The most common problem when flushing writeback data to the server aside from ENOSPC (and possibly ESTALE) is EACCES, which is particular to the file descriptor that opened the file.
File contexts, and NFS_CONTEXT_ERROR_WRITE solve that problem by being private to the file descriptor.
Thanks for the reminder that errors are per-context and this patch drops this. The per-context nature of errors in NFS was the reason that I nagged Jeff to make errseq_t a stand-alone type rather than just a part of address_space. I had envisaged that it would be embedded in the open_context as well.
We still could do that, but as there is precisely one open-file for each open_context, the gains are not great.
However, while looking over the code to make sure I really understood it and all the possible consequences of changing to errseq_t I found a few anomalies. The patch below addresses them all.
Would you see if they may sense to you?
Thanks,
NeilBrown
From: NeilBrown <[email protected]>
Date: Mon, 11 Sep 2017 13:15:50 +1000
Subject: [PATCH] NFS: various changes relating to reporting IO errors.
1/ remove 'start' and 'end' args from nfs_file_fsync_commit().
They aren't used.
2/ Make nfs_context_set_write_error() a "static inline" in internal.h
so we can...
3/ Use nfs_context_set_write_error() instead of mapping_set_error()
if nfs_pageio_add_request() fails before sending any request. NFS generally keeps errors in the open_context, not the mapping, so this is more consistent.
4/ If filemap_write_and_write_range() reports any error, still
check ctx->error. The value in ctx->error is likely to be more useful. As part of this, NFS_CONTEXT_ERROR_WRITE is cleared slightly earlier, before nfs_file_fsync_commit() is called, rather than at the start of that function.
Signed-off-by NeilBrown <[email protected]>
---
 fs/nfs/file.c     | 16 ++++++++++------
 fs/nfs/internal.h |  7 +++++++
 fs/nfs/pagelist.c |  4 ++--
 fs/nfs/write.c    |  7 -------
 4 files changed, 19 insertions(+), 15 deletions(-)
fs/nfs/file.c +10 -6
index af330c31f627..ab324f14081f 100644
@@ -208,21 +208,19 @@ EXPORT_SYMBOL_GPL(nfs_file_mmap);
  * fall back to doing a synchronous write.
  */
 static int
-nfs_file_fsync_commit(struct file *file, loff_t start, loff_t end, int datasync)
+nfs_file_fsync_commit(struct file *file, int datasync)
 {
 	struct nfs_open_context *ctx = nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
+	int do_resend, status;
 	int ret = 0;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file, datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend = test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES, &ctx->flags);
-	have_error = test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
 	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
-	if (have_error) {
+	if (test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags)) {
 		ret = xchg(&ctx->error, 0);
 		if (ret)
 			goto out;
@@ -247,10 +245,16 @@ 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;
+		}
 		if (ret != 0)
 			break;
-		ret = nfs_file_fsync_commit(file, start, end, datasync);
+		ret = nfs_file_fsync_commit(file, datasync);
 		if (!ret)
 			ret = pnfs_sync_inode(inode, !!datasync);
 		/*
fs/nfs/internal.h +7 -0
index dc456416d2be..44c8962fec91 100644
@@ -769,3 +769,10 @@ static inline bool nfs_error_is_fatal(int err)
 		return false;
 	}
 }
+
+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);
+}
fs/nfs/pagelist.c +2 -2
index de9066a92c0d..0ebd26b9a6bd 100644
@@ -1198,8 +1198,8 @@ out_failed:
 
 		/* remember fatal errors */
 		if (nfs_error_is_fatal(desc->pg_error))
-			mapping_set_error(desc->pg_inode->i_mapping,
-					  desc->pg_error);
+			nfs_context_set_write_error(req->wb_context,
+						    desc->pg_error);
 
 		func = desc->pg_completion_ops->error_cleanup;
 		for (midx = 0; midx < desc->pg_mirror_count; midx++) {
fs/nfs/write.c +0 -0
index b1af5dee5e0a..f702bf2def79 100644
@@ -147,13 +147,6 @@ static void nfs_io_completion_put(struct nfs_io_completion *ioc)
 		kref_put(&ioc->refcount, nfs_io_completion_release);
 }
 
fs/nfs/write.c static void nfs_io_completion_put(struct nfs_io_completion *ioc) · line 152
Quoted 10 lines
fs/nfs/write.c +0 -7
-static 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);
-}
-
 /*
  * nfs_page_find_head_request_locked - find head request associated with @page
  *
This should probably be broken out into at least a 2-3 different patches.
Quoted 6 lines
Ok, so to make sure I understand:
All writeback is done under the aegis of an open context, and writes from different open contexts are not mergeable. We also flush to the server in the case that a dirty page is written via an incompatible open context. So with that we can always tie
Quoted 9 lines
Not quite. Writes from different open contexts are sometimes mergeable, providing the credential is the same and there are no locks that might get in the way. (nfs_flush_incompatible() gets rid of conflicts writes to the same page as part of nfs_write_begin().
When writes are merged, all contexts remain reachable from the request through an 'nfs_page'. nfs_write_completion() iterates over all the nfs_pages attached to the nfs_pgio_header, and sets the context write_error from the hdr->error.
Ok, by this account, NFS should already have "correct" error reporting semantics on fsync. i.e. when the file is written via multiple fds, you should get back an error on all fds if those writebacks failed.
I have a test for nfs for the new-style error reporting:
https://git.kernel.org/pub/scm/linux/kernel/git/jlayton/xfstests-dev.git/log/?h=wberr
The nfs test is still pretty rickety, using soft mounts and iptables to cause requests to fail. With the patch I originally proposed, this test would pass. When I run this test on normal mainline kernels, it fails:
-------------------------------8<------------------------------
FSTYP         -- nfs
PLATFORM      -- Linux/x86_64 wberr 4.12.11-300.fc26.x86_64
MKFS_OPTIONS  -- knfsdsrv:/export/scratch
MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 knfsdsrv:/export/scratch /mnt/scratch
Quoted 8 lines
nfs/002 - output mismatch (see /home/jlayton/git/xfstests/results//nfs/002.out.bad)
--- tests/nfs/002.out 2017-07-19 13:12:59.354561869 -0400
+++ /home/jlayton/git/xfstests/results//nfs/002.out.bad 2017-09-12 11:17:17.335943539 -0400
@@ -1,3 +1,3 @@
QA output created by 002
Format and mount
-Test passed!
+Success on second fsync on fd[1]!
Interesting.
I haven't reviewed the kernel code yet, but having looked at the test code I see that the one file is opened 10 time and that the *same* block in the file (65k?) is written via each fd.
If I write to a file, and then someone else over-writes all the bytes that I wrote, then the page is written to the server and gets an error, then you could argue that as none of the bytes I wrote were involved in the error, I don't need to see the error status - someone else has taken on responsibility for that range.

Quoted 5 lines

Maybe the test should/could write a few bytes with a different offset for each fd ??
NeilBrown

Yes, that seems to do the trick! I'll send out a patch to xfstests to change that in the test there as it's probably more representative of a real workload.

Ok, so I guess from both this result and starting at the code for a bit that the nfs client will replace one nfs_page with another if the second one covers the entire range of the first.

We should note that that behavior is a little different from other filesystems that use errseq_t now. If two tasks are writing to the same file, and one attempts to overwrite the other's range, then the first one will not see an error on writeback.

Maybe the above is worth a blurb in vfs.txt?

I do (idly) wonder if that behavior could be exploited in some fashion. A process running in a different container that has write access to a file could potentially mask writeback errors for a task running in a different container. It's probably not useful on its own to an attacker, but could be part of a wider exploit? In any case, it doesn't seem like a big deal.

Thanks for all of the patience here. I think your patch looks fine as well. You can add:

Reviewed-by Jeff Layton <[email protected]>

NeilBrown

2017-09-13 23:50 UTC #15 lore raw

RFC PATCH 1/1 write.2, fsync.2, close.2: update description of error codes

Since 4.13, errors from writeback are more reliably reported to all file descriptors that might be relevant.

Add notes to this effect, and also add details about ENOSPC and EDQUOT which can be delayed in a similar manner to EIO - for NFS in particular.

Signed-off-by NeilBrown <[email protected]>

---

This is my summary of recent changes, and details that have been made clear during the exploration of those changes.

I haven't mentioned the fact that EPERM can be returned by write/fsync/close on NFS if the permissions on the server are changed. We probably should ... are there other errors that are worth mentioning along with EPERM, ENOSPC, EDQUOT ??

Thanks,
NeilBronw

 man2/close.2 |  9 +++++++++
 man2/fsync.2 | 19 ++++++++++++++++++-
 man2/write.2 | 20 +++++++++++++++++---
 3 files changed, 44 insertions(+), 4 deletions(-)

man2/close.2 +9 -0

index 751ec322b1f1..9776c839b8b6 100644
@@ -82,6 +82,15 @@ call was interrupted by a signal; see
 .TP
 .B EIO
 An I/O error occurred.
+.TP
+.BR ENOSPC ", " EDQUOT
+On NFS, these errors are not normally reported against the first write
+which exceeds the available storage space, but instead against a
+subsequent
+.BR write (2),
+.BR fsync (2),
+or
+.BR close (2).
 .PP
 See NOTES for a discussion of why
 .BR close ()

man2/fsync.2 +18 -1

index f1a01301da0f..e706a08d360d 100644
@@ -120,12 +120,29 @@ is set appropriately.
 is not a valid open file descriptor.
 .TP
 .B EIO
-An error occurred during synchronization.
+An error occurred during synchronization.  This error may relate
+to data written to some other file descriptor on the same file.
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
+Since Linux 4.13 errors from write-back will be reported to
+all file descriptors that might have written the data which triggered
+the error, and which are still open.  Some filesystems (e.g. NFS)
+keep close track of which data came through which file descriptor,
+and give more precise reporting.  Other filesystems (e.g. most local
+filesystems) will report errors to all file descriptors on the same
+file.
 .TP
 .BR EROFS ", " EINVAL
 .I fd
 is bound to a special file (e.g., a pipe, FIFO, or socket)
 which does not support synchronization.
+.TP
+.BR ENOSPC ", " EDQUOT
+.I fd
+is bound to a file on NFS or another filesystem which does not allocate
+space at the time of a
+.BR write (2)
+system call, and some previous write failed due to insufficient
+storage space.
 .SH CONFORMING TO
 POSIX.1-2001, POSIX.1-2008, 4.3BSD.
 .SH AVAILABILITY

man2/write.2 +17 -3

index 6a39b5b5541d..1a9a86b03b04 100644
@@ -47,7 +47,7 @@ write \- write to a file descriptor
 .BR write ()
 writes up to
 .I count
-bytes from the buffer pointed
+bytes from the buffer starting at
 .I buf
 to the file referred to by the file descriptor
 .IR fd .

@@ -181,6 +181,14 @@ or the file offset is not suitably aligned.
 .TP
 .B EIO
 A low-level I/O error occurred while modifying the inode.
+This error may relate to data written by an earlier
+.BR write (2),
+which may have been issued to a different file descriptor on
+the same file.  Since Linux 4.13 errors from write-back will
+be reported to all file descriptors that might have
+written the data which triggered the error, and which are still
+open.
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
 .TP
 .B ENOSPC
 The device containing the file referred to by

@@ -222,8 +230,14 @@ unsigned and signed integer data types specified by POSIX.1.
 A successful return from
 .BR write ()
 does not make any guarantee that data has been committed to disk.
-In fact, on some buggy implementations, it does not even guarantee
-that space has successfully been reserved for the data.
+On some filesystems, including NFS, it does not even guarantee
+that space has successfully been reserved for the data.  In the case,
+some errors might be delayed to a future
+.BR write (2)
+or to
+.BR fsync (2)
+or even
+.BR close (2).
 The only way to be sure is to call
 .BR fsync (2)
 after you are done writing all your data.

Signature · 1 line

2.14.0.rc0.dirty

A message that is not in the archive (sent off-list or not yet received): 87ingm9n04.fsf-wvvUuzkyo1HefUI2i7LXDhCRmIWqnp/[email protected]

walter harms

2017-09-14 07:59 UTC #17 lore raw

Re: [RFC PATCH manpages] write.2, fsync.2, close.2: update description of error codes

Am 14.09.2017 01:50, schrieb NeilBrown:

Quoted 7 lines

Since 4.13, errors from writeback are more reliably reported to all file descriptors that might be relevant.
Add notes to this effect, and also add details about ENOSPC and EDQUOT which can be delayed in a similar manner to EIO - for NFS in particular.

I never read EDQUOT before but it is real:

see:
http://elixir.free-electrons.com/linux/latest/source/security/keys/request_key.c

-EDQUOT
* if insufficient key quota was available to create a new key

Quoted 110 lines

Signed-off-by NeilBrown <neilb-IBi9RG/[email protected]>

---

This is my summary of recent changes, and details that have been made clear during the exploration of those changes.

Thanks,
NeilBronw

 man2/close.2 |  9 +++++++++
 man2/fsync.2 | 19 ++++++++++++++++++-
 man2/write.2 | 20 +++++++++++++++++---
 3 files changed, 44 insertions(+), 4 deletions(-)

man2/close.2 +9 -0

index 751ec322b1f1..9776c839b8b6 100644
@@ -82,6 +82,15 @@ call was interrupted by a signal; see
 .TP
 .B EIO
 An I/O error occurred.
+.TP
+.BR ENOSPC ", " EDQUOT
+On NFS, these errors are not normally reported against the first write
+which exceeds the available storage space, but instead against a
+subsequent
+.BR write (2),
+.BR fsync (2),
+or
+.BR close (2).
 .PP
 See NOTES for a discussion of why
 .BR close ()

man2/fsync.2 +18 -1

index f1a01301da0f..e706a08d360d 100644
@@ -120,12 +120,29 @@ is set appropriately.
 is not a valid open file descriptor.
 .TP
 .B EIO
-An error occurred during synchronization.
+An error occurred during synchronization.  This error may relate
+to data written to some other file descriptor on the same file.
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
+Since Linux 4.13 errors from write-back will be reported to
+all file descriptors that might have written the data which triggered
+the error, and which are still open.  Some filesystems (e.g. NFS)
+keep close track of which data came through which file descriptor,
+and give more precise reporting.  Other filesystems (e.g. most local
+filesystems) will report errors to all file descriptors on the same
+file.
 .TP
 .BR EROFS ", " EINVAL
 .I fd
 is bound to a special file (e.g., a pipe, FIFO, or socket)
 which does not support synchronization.
+.TP
+.BR ENOSPC ", " EDQUOT
+.I fd
+is bound to a file on NFS or another filesystem which does not allocate
+space at the time of a
+.BR write (2)
+system call, and some previous write failed due to insufficient
+storage space.
 .SH CONFORMING TO
 POSIX.1-2001, POSIX.1-2008, 4.3BSD.
 .SH AVAILABILITY

man2/write.2 +10 -3

index 6a39b5b5541d..1a9a86b03b04 100644
@@ -47,7 +47,7 @@ write \- write to a file descriptor
 .BR write ()
 writes up to
 .I count
-bytes from the buffer pointed
+bytes from the buffer starting at
 .I buf
 to the file referred to by the file descriptor
 .IR fd .

@@ -181,6 +181,14 @@ or the file offset is not suitably aligned.
 .TP
 .B EIO
 A low-level I/O error occurred while modifying the inode.
+This error may relate to data written by an earlier
+.BR write (2),
+which may have been issued to a different file descriptor on
+the same file.  Since Linux 4.13 errors from write-back will
+be reported to all file descriptors that might have
+written the data which triggered the error, and which are still
+open.
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
 .TP
 .B ENOSPC
 The device containing the file referred to by

@@ -222,8 +230,14 @@ unsigned and signed integer data types specified by POSIX.1.
 A successful return from
 .BR write ()
 does not make any guarantee that data has been committed to disk.
-In fact, on some buggy implementations, it does not even guarantee
-that space has successfully been reserved for the data.
+On some filesystems, including NFS, it does not even guarantee

man2/write.2 unsigned and signed integer data types specified by POSIX.1. · line 243

Quoted 10 lines

man2/write.2 +7 -0

+that space has successfully been reserved for the data.  In the case,
+some errors might be delayed to a future
+.BR write (2)
+or to
+.BR fsync (2)
+or even
+.BR close (2).
 The only way to be sure is to call
 .BR fsync (2)
 after you are done writing all your data.

--
To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

NeilBrown

2017-09-14 22:36 UTC #19 lore raw

Re: [RFC PATCH manpages] write.2, fsync.2, close.2: update description of error codes

Quoted 9 lines · walter harms

Am 14.09.2017 01:50, schrieb NeilBrown:
Quoted 7 lines
Since 4.13, errors from writeback are more reliably reported to all file descriptors that might be relevant.
Add notes to this effect, and also add details about ENOSPC and EDQUOT which can be delayed in a similar manner to EIO - for NFS in particular.

Quoted 7 lines

I never read EDQUOT before but it is real:
see:
http://elixir.free-electrons.com/linux/latest/source/security/keys/request_key.c
-EDQUOT
* if insufficient key quota was available to create a new key

EDQUOT existed long before security keys.
It's original purpose is to report Disk QUOTa problems. If you write to a file and you have exhausted you quota, you get EDQUOT.

http://elixir.free-electrons.com/linux/latest/source/fs/quota/dquot.c#L1318

NeilBrown

Jeff Layton

2017-09-14 10:48 UTC #20 lore raw

Re: [RFC PATCH manpages] write.2, fsync.2, close.2: update description of error codes

In reply to NeilBrown

Quoted 12 lines · NeilBrown

Since 4.13, errors from writeback are more reliably reported to all file descriptors that might be relevant.
Add notes to this effect, and also add details about ENOSPC and EDQUOT which can be delayed in a similar manner to EIO - for NFS in particular.
Signed-off-by NeilBrown <neilb-IBi9RG/[email protected]>
---
This is my summary of recent changes, and details that have been made clear during the exploration of those changes.

Quoted 8 lines

I haven't mentioned the fact that EPERM can be returned by write/fsync/close on NFS if the permissions on the server are changed. We probably should ... are there other errors that are worth mentioning along with EPERM, ENOSPC, EDQUOT ??
Thanks,
NeilBronw

Many thanks for doing this! It was on my to-do list. Comments below:

Quoted 31 lines

 man2/close.2 |  9 +++++++++
 man2/fsync.2 | 19 ++++++++++++++++++-
 man2/write.2 | 20 +++++++++++++++++---
 3 files changed, 44 insertions(+), 4 deletions(-)

man2/close.2 +9 -0

index 751ec322b1f1..9776c839b8b6 100644
@@ -82,6 +82,15 @@ call was interrupted by a signal; see
 .TP
 .B EIO
 An I/O error occurred.
+.TP
+.BR ENOSPC ", " EDQUOT
+On NFS, these errors are not normally reported against the first write
+which exceeds the available storage space, but instead against a
+subsequent
+.BR write (2),
+.BR fsync (2),
+or
+.BR close (2).
 .PP
 See NOTES for a discussion of why
 .BR close ()

man2/fsync.2 +0 -0

index f1a01301da0f..e706a08d360d 100644

man2/fsync.2 is set appropriately. · line 128

Quoted 10 lines

man2/fsync.2 +6 -1

@@ -120,12 +120,29 @@ is set appropriately.
 is not a valid open file descriptor.
 .TP
 .B EIO
-An error occurred during synchronization.
+An error occurred during synchronization.  This error may relate
+to data written to some other file descriptor on the same file.
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
+Since Linux 4.13 errors from write-back will be reported to
+all file descriptors that might have written the data which triggered
+the error, and which are still open.

This is a little awkward. How could we report to a fd that was no longer open? How about:

"Since Linux 4.13, errors from write-back will be reported to all file descriptors that were open at the time that the error was recorded."

Quoted 35 lines

man2/fsync.2 +12 -0

  Some filesystems (e.g. NFS)
+keep close track of which data came through which file descriptor,
+and give more precise reporting.  Other filesystems (e.g. most local
+filesystems) will report errors to all file descriptors on the same
+file.
 .TP
 .BR EROFS ", " EINVAL
 .I fd
 is bound to a special file (e.g., a pipe, FIFO, or socket)
 which does not support synchronization.
+.TP
+.BR ENOSPC ", " EDQUOT
+.I fd
+is bound to a file on NFS or another filesystem which does not allocate
+space at the time of a
+.BR write (2)
+system call, and some previous write failed due to insufficient
+storage space.
 .SH CONFORMING TO
 POSIX.1-2001, POSIX.1-2008, 4.3BSD.

.SH AVAILABILITY

man2/write.2 +1 -1

index 6a39b5b5541d..1a9a86b03b04 100644
@@ -47,7 +47,7 @@ write \- write to a file descriptor
 .BR write ()
 writes up to
 .I count
-bytes from the buffer pointed
+bytes from the buffer starting at
 .I buf
 to the file referred to by the file descriptor
 .IR fd .

man2/write.2 or the file offset is not suitably aligned. · line 190

Quoted 10 lines

man2/write.2 +7 -0

@@ -181,6 +181,14 @@ or the file offset is not suitably aligned.
 .TP
 .B EIO
 A low-level I/O error occurred while modifying the inode.
+This error may relate to data written by an earlier
+.BR write (2),
+which may have been issued to a different file descriptor on
+the same file.  Since Linux 4.13 errors from write-back will
+be reported to all file descriptors that might have
+written the data which triggered the error, and which are still
+open.

This is where things get a little more vague.

Some filesystems will return errors on a subsequent write(2) when previous writeback has failed -- some don't. In either case though, write(2) should never advance your errseq_t cursor, so only an fsync will "clear" an earlier error.

I'm not sure how best to convey that in the manpages though.

Quoted 21 lines

man2/write.2 +9 -2

+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
 .TP
 .B ENOSPC
 The device containing the file referred to by

@@ -222,8 +230,14 @@ unsigned and signed integer data types specified by POSIX.1.
 A successful return from
 .BR write ()
 does not make any guarantee that data has been committed to disk.
-In fact, on some buggy implementations, it does not even guarantee
-that space has successfully been reserved for the data.
+On some filesystems, including NFS, it does not even guarantee
+that space has successfully been reserved for the data.  In the case,
+some errors might be delayed to a future
+.BR write (2)
+or to
+.BR fsync (2)
+or even
+.BR close (2).
 The only way to be sure is to call
 .BR fsync (2)
 after you are done writing all your data.

Signature · 5 lines

Jeff Layton <[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michael Kerrisk (man-pages)

2017-09-15 07:50 UTC #21 lore raw

Re: [RFC PATCH manpages] write.2, fsync.2, close.2: update description of error codes

Hi Neil,

Will you revise this patch to incorporate Jeff's comments, or should I try manually editing them in. (I'd prefer the former.)

Cheers,

Michael

Quoted 150 lines · Jeff Layton

Quoted 12 lines · NeilBrown
Since 4.13, errors from writeback are more reliably reported to all file descriptors that might be relevant.
Add notes to this effect, and also add details about ENOSPC and EDQUOT which can be delayed in a similar manner to EIO - for NFS in particular.
Signed-off-by NeilBrown <[email protected]>
---
This is my summary of recent changes, and details that have been made clear during the exploration of those changes.
Quoted 8 lines
I haven't mentioned the fact that EPERM can be returned by write/fsync/close on NFS if the permissions on the server are changed. We probably should ... are there other errors that are worth mentioning along with EPERM, ENOSPC, EDQUOT ??
Thanks,
NeilBronw
Many thanks for doing this! It was on my to-do list. Comments below:
Quoted 31 lines
 man2/close.2 |  9 +++++++++
 man2/fsync.2 | 19 ++++++++++++++++++-
 man2/write.2 | 20 +++++++++++++++++---
 3 files changed, 44 insertions(+), 4 deletions(-)
man2/close.2 +9 -0
index 751ec322b1f1..9776c839b8b6 100644
@@ -82,6 +82,15 @@ call was interrupted by a signal; see
 .TP
 .B EIO
 An I/O error occurred.
+.TP
+.BR ENOSPC ", " EDQUOT
+On NFS, these errors are not normally reported against the first write
+which exceeds the available storage space, but instead against a
+subsequent
+.BR write (2),
+.BR fsync (2),
+or
+.BR close (2).
 .PP
 See NOTES for a discussion of why
 .BR close ()
man2/fsync.2 +0 -0
index f1a01301da0f..e706a08d360d 100644
man2/fsync.2 is set appropriately. · line 128
Quoted 10 lines
man2/fsync.2 +6 -1
@@ -120,12 +120,29 @@ is set appropriately.
 is not a valid open file descriptor.
 .TP
 .B EIO
-An error occurred during synchronization.
+An error occurred during synchronization.  This error may relate
+to data written to some other file descriptor on the same file.
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
+Since Linux 4.13 errors from write-back will be reported to
+all file descriptors that might have written the data which triggered
+the error, and which are still open.
This is a little awkward. How could we report to a fd that was no longer open? How about:
"Since Linux 4.13, errors from write-back will be reported to all file descriptors that were open at the time that the error was recorded."
Quoted 35 lines
man2/fsync.2 +12 -0
  Some filesystems (e.g. NFS)
+keep close track of which data came through which file descriptor,
+and give more precise reporting.  Other filesystems (e.g. most local
+filesystems) will report errors to all file descriptors on the same
+file.
 .TP
 .BR EROFS ", " EINVAL
 .I fd
 is bound to a special file (e.g., a pipe, FIFO, or socket)
 which does not support synchronization.
+.TP
+.BR ENOSPC ", " EDQUOT
+.I fd
+is bound to a file on NFS or another filesystem which does not allocate
+space at the time of a
+.BR write (2)
+system call, and some previous write failed due to insufficient
+storage space.
 .SH CONFORMING TO
 POSIX.1-2001, POSIX.1-2008, 4.3BSD.
.SH AVAILABILITY
man2/write.2 +1 -1
index 6a39b5b5541d..1a9a86b03b04 100644
@@ -47,7 +47,7 @@ write \- write to a file descriptor
 .BR write ()
 writes up to
 .I count
-bytes from the buffer pointed
+bytes from the buffer starting at
 .I buf
 to the file referred to by the file descriptor
 .IR fd .
man2/write.2 or the file offset is not suitably aligned. · line 190
Quoted 10 lines
man2/write.2 +7 -0
@@ -181,6 +181,14 @@ or the file offset is not suitably aligned.
 .TP
 .B EIO
 A low-level I/O error occurred while modifying the inode.
+This error may relate to data written by an earlier
+.BR write (2),
+which may have been issued to a different file descriptor on
+the same file.  Since Linux 4.13 errors from write-back will
+be reported to all file descriptors that might have
+written the data which triggered the error, and which are still
+open.
This is where things get a little more vague.
Some filesystems will return errors on a subsequent write(2) when previous writeback has failed -- some don't. In either case though, write(2) should never advance your errseq_t cursor, so only an fsync will "clear" an earlier error.
I'm not sure how best to convey that in the manpages though.
Quoted 21 lines
man2/write.2 +9 -2
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
 .TP
 .B ENOSPC
 The device containing the file referred to by
@@ -222,8 +230,14 @@ unsigned and signed integer data types specified by POSIX.1.
 A successful return from
 .BR write ()
 does not make any guarantee that data has been committed to disk.
-In fact, on some buggy implementations, it does not even guarantee
-that space has successfully been reserved for the data.
+On some filesystems, including NFS, it does not even guarantee
+that space has successfully been reserved for the data.  In the case,
+some errors might be delayed to a future
+.BR write (2)
+or to
+.BR fsync (2)
+or even
+.BR close (2).
 The only way to be sure is to call
 .BR fsync (2)
 after you are done writing all your data.

Signature · 3 lines

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

NeilBrown

2017-09-15 08:25 UTC #22 lore raw

Re: [RFC PATCH manpages] write.2, fsync.2, close.2: update description of error codes

I'll do something, but maybe not for a few days (24 hour sardine impersonation pending - Europe, here I come...)

NeilBrown

Quoted 167 lines · Michael Kerrisk (man-pages)

Hi Neil,
Will you revise this patch to incorporate Jeff's comments, or should I try manually editing them in. (I'd prefer the former.)
Cheers,
Michael
Quoted 150 lines · Jeff Layton
Quoted 12 lines · NeilBrown
Since 4.13, errors from writeback are more reliably reported to all file descriptors that might be relevant.
Add notes to this effect, and also add details about ENOSPC and EDQUOT which can be delayed in a similar manner to EIO - for NFS in particular.
Signed-off-by NeilBrown <neilb-IBi9RG/[email protected]>
---
This is my summary of recent changes, and details that have been made clear during the exploration of those changes.
Quoted 8 lines
I haven't mentioned the fact that EPERM can be returned by write/fsync/close on NFS if the permissions on the server are changed. We probably should ... are there other errors that are worth mentioning along with EPERM, ENOSPC, EDQUOT ??
Thanks,
NeilBronw
Many thanks for doing this! It was on my to-do list. Comments below:
Quoted 31 lines
 man2/close.2 |  9 +++++++++
 man2/fsync.2 | 19 ++++++++++++++++++-
 man2/write.2 | 20 +++++++++++++++++---
 3 files changed, 44 insertions(+), 4 deletions(-)
man2/close.2 +9 -0
index 751ec322b1f1..9776c839b8b6 100644
@@ -82,6 +82,15 @@ call was interrupted by a signal; see
 .TP
 .B EIO
 An I/O error occurred.
+.TP
+.BR ENOSPC ", " EDQUOT
+On NFS, these errors are not normally reported against the first write
+which exceeds the available storage space, but instead against a
+subsequent
+.BR write (2),
+.BR fsync (2),
+or
+.BR close (2).
 .PP
 See NOTES for a discussion of why
 .BR close ()
man2/fsync.2 +0 -0
index f1a01301da0f..e706a08d360d 100644
man2/fsync.2 is set appropriately. · line 128
Quoted 10 lines
man2/fsync.2 +6 -1
@@ -120,12 +120,29 @@ is set appropriately.
 is not a valid open file descriptor.
 .TP
 .B EIO
-An error occurred during synchronization.
+An error occurred during synchronization.  This error may relate
+to data written to some other file descriptor on the same file.
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
+Since Linux 4.13 errors from write-back will be reported to
+all file descriptors that might have written the data which triggered
+the error, and which are still open.
This is a little awkward. How could we report to a fd that was no longer open? How about:
"Since Linux 4.13, errors from write-back will be reported to all file descriptors that were open at the time that the error was recorded."
Quoted 35 lines
man2/fsync.2 +12 -0
  Some filesystems (e.g. NFS)
+keep close track of which data came through which file descriptor,
+and give more precise reporting.  Other filesystems (e.g. most local
+filesystems) will report errors to all file descriptors on the same
+file.
 .TP
 .BR EROFS ", " EINVAL
 .I fd
 is bound to a special file (e.g., a pipe, FIFO, or socket)
 which does not support synchronization.
+.TP
+.BR ENOSPC ", " EDQUOT
+.I fd
+is bound to a file on NFS or another filesystem which does not allocate
+space at the time of a
+.BR write (2)
+system call, and some previous write failed due to insufficient
+storage space.
 .SH CONFORMING TO
 POSIX.1-2001, POSIX.1-2008, 4.3BSD.
.SH AVAILABILITY
man2/write.2 +1 -1
index 6a39b5b5541d..1a9a86b03b04 100644
@@ -47,7 +47,7 @@ write \- write to a file descriptor
 .BR write ()
 writes up to
 .I count
-bytes from the buffer pointed
+bytes from the buffer starting at
 .I buf
 to the file referred to by the file descriptor
 .IR fd .
man2/write.2 or the file offset is not suitably aligned. · line 190
Quoted 10 lines
man2/write.2 +7 -0
@@ -181,6 +181,14 @@ or the file offset is not suitably aligned.
 .TP
 .B EIO
 A low-level I/O error occurred while modifying the inode.
+This error may relate to data written by an earlier
+.BR write (2),
+which may have been issued to a different file descriptor on
+the same file.  Since Linux 4.13 errors from write-back will
+be reported to all file descriptors that might have
+written the data which triggered the error, and which are still
+open.
This is where things get a little more vague.
Some filesystems will return errors on a subsequent write(2) when previous writeback has failed -- some don't. In either case though, write(2) should never advance your errseq_t cursor, so only an fsync will "clear" an earlier error.
I'm not sure how best to convey that in the manpages though.
Quoted 21 lines
man2/write.2 +9 -2
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
 .TP
 .B ENOSPC
 The device containing the file referred to by
@@ -222,8 +230,14 @@ unsigned and signed integer data types specified by POSIX.1.
 A successful return from
 .BR write ()
 does not make any guarantee that data has been committed to disk.
-In fact, on some buggy implementations, it does not even guarantee
-that space has successfully been reserved for the data.
+On some filesystems, including NFS, it does not even guarantee
+that space has successfully been reserved for the data.  In the case,
+some errors might be delayed to a future
+.BR write (2)
+or to
+.BR fsync (2)
+or even
+.BR close (2).
 The only way to be sure is to call
 .BR fsync (2)
 after you are done writing all your data.
Signature · 3 lines
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

NeilBrown

2017-09-28 03:01 UTC #23 lore raw

Re: [RFC PATCH manpages] write.2, fsync.2, close.2: update description of error codes

In reply to Jeff Layton

Quoted 13 lines · Jeff Layton

Review

Quoted 9 lines

.TP
.B EIO

(quoted diff) +6 -1

-An error occurred during synchronization.
+An error occurred during synchronization.  This error may relate
+to data written to some other file descriptor on the same file.
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
+Since Linux 4.13 errors from write-back will be reported to
+all file descriptors that might have written the data which triggered
+the error, and which are still open.

This is a little awkward. How could we report to a fd that was no longer open? How about:

Quoted 2 lines

"Since Linux 4.13, errors from write-back will be reported to all file descriptors that were open at the time that the error was recorded."

That might be simpler, but it is less correct. As I go on to say, NFS *doesn't* report on all file descriptors that were open at that time.

I've changed it to

-------------------
Since Linux 4.13, errors from write-back will be reported to
all file descriptors that might have written the data which triggered the error. Some filesystems (e.g. NFS) keep close track of which data came through which file descriptor, and give precise reporting. Other filesystems (e.g. most local filesystems) will report errors to all file descriptors that where open on the file when the error was recorded.
------------------

which includes some of your text, and removes the "that are still open" which probably doesn't help.

Quoted 14 lines

Review
Quoted 10 lines
.TP
.B EIO
A low-level I/O error occurred while modifying the inode.
(quoted diff) +7 -0
+This error may relate to data written by an earlier
+.BR write (2),
+which may have been issued to a different file descriptor on
+the same file.  Since Linux 4.13 errors from write-back will
+be reported to all file descriptors that might have
+written the data which triggered the error, and which are still
+open.
This is where things get a little more vague.

Quoted 6 lines

Some filesystems will return errors on a subsequent write(2) when previous writeback has failed -- some don't. In either case though, write(2) should never advance your errseq_t cursor, so only an fsync will "clear" an earlier error.
I'm not sure how best to convey that in the manpages though.

How about:

-------------
This error may relate to the write-back of data written by an earlier
.BR write (2),
which may have been issued to a different file descriptor on the same file. Since Linux 4.13, errors from write-back come with a promise that they
.I may
be reported by subsequent.
.BR write (2)
requests, and
.I will
be reported by a subsequent
.BR fsync (2)
(whether or not they were also reported by
.BR write (2)).
------------
??

Those changes are included in the following.

Thanks,
NeilBrown

From: NeilBrown <neilb-IBi9RG/[email protected]>
Date: Thu, 14 Sep 2017 09:44:43 +1000
Subject: [PATCH] write.2, fsync.2, close.2: update description of error codes

Since 4.13, errors from writeback are more reliably reported to all file descriptors that might be relevant.

Add notes to this effect, and also add detail about ENOSPC and EDQUOT which can be delayed in a similar many to EIO - for NFS in particular.

Signed-off-by NeilBrown <neilb-IBi9RG/[email protected]>

---
 man2/close.2 |  9 +++++++++
 man2/fsync.2 | 18 +++++++++++++++++-
 man2/write.2 | 28 +++++++++++++++++++++++++---
 3 files changed, 51 insertions(+), 4 deletions(-)

man2/close.2 +9 -0

index 55d89ed3dbc7..136bd0be3f67 100644
@@ -82,6 +82,15 @@ call was interrupted by a signal; see
 .TP
 .B EIO
 An I/O error occurred.
+.TP
+.BR ENOSPC ", " EDQUOT
+On NFS, these errors are not normally reported against the first write
+which exceeds the available storage space, but instead against a
+subsequent
+.BR write (2),
+.BR fsync (2),
+or
+.BR close (2).
 .PP
 See NOTES for a discussion of why
 .BR close ()

man2/fsync.2 +17 -1

index eed3c460bea9..c7878bf3496f 100644
@@ -121,7 +121,15 @@ is set appropriately.
 is not a valid open file descriptor.
 .TP
 .B EIO
-An error occurred during synchronization.
+An error occurred during synchronization.  This error may relate
+to data written to some other file descriptor on the same file.
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
+Since Linux 4.13, errors from write-back will be reported to
+all file descriptors that might have written the data which triggered
+the error.  Some filesystems (e.g. NFS) keep close track of which data
+came through which file descriptor, and give more precise reporting.
+Other filesystems (e.g. most local filesystems) will report errors to
+all file descriptors that where open on the file when the error was recorded.
 .TP
 .B ENOSPC
 Disk space was exhausted while synchronizing.

@@ -130,6 +138,14 @@ Disk space was exhausted while synchronizing.
 .I fd
 is bound to a special file (e.g., a pipe, FIFO, or socket)
 which does not support synchronization.
+.TP
+.BR ENOSPC ", " EDQUOT
+.I fd
+is bound to a file on NFS or another filesystem which does not allocate
+space at the time of a
+.BR write (2)
+system call, and some previous write failed due to insufficient
+storage space.
 .SH CONFORMING TO
 POSIX.1-2001, POSIX.1-2008, 4.3BSD.
 .SH AVAILABILITY

man2/write.2 +25 -3

index 061aa70cf590..b1cc3a2cfb17 100644
@@ -47,7 +47,7 @@ write \- write to a file descriptor
 .BR write ()
 writes up to
 .I count
-bytes from the buffer pointed
+bytes from the buffer starting at
 .I buf
 to the file referred to by the file descriptor
 .IR fd .

@@ -181,6 +181,22 @@ or the file offset is not suitably aligned.
 .TP
 .B EIO
 A low-level I/O error occurred while modifying the inode.
+This error may relate to the write-back of data written by an
+earlier
+.BR write (2),
+which may have been issued to a different file descriptor on
+the same file.  Since Linux 4.13, errors from write-back come
+with a promise that they
+.I may
+be reported by subsequent.
+.BR write (2)
+requests, and
+.I will
+be reported by a subsequent
+.BR fsync (2)
+(whether or not they were also reported by
+.BR write (2)).
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
 .TP
 .B ENOSPC
 The device containing the file referred to by

@@ -222,8 +238,14 @@ unsigned and signed integer data types specified by POSIX.1.
 A successful return from
 .BR write ()
 does not make any guarantee that data has been committed to disk.
-In fact, on some buggy implementations, it does not even guarantee
-that space has successfully been reserved for the data.
+On some filesystems, including NFS, it does not even guarantee
+that space has successfully been reserved for the data.  In the case,
+some errors might be delayed to a future
+.BR write (2)
+or to
+.BR fsync (2)
+or even
+.BR close (2).
 The only way to be sure is to call
 .BR fsync (2)
 after you are done writing all your data.

Signature · 1 line

2.14.0.rc0.dirty

Jeff Layton

Reviewed-by

2017-09-28 12:20 UTC #24 lore raw

Re: [RFC PATCH manpages] write.2, fsync.2, close.2: update description of error codes

Quoted 218 lines · NeilBrown

Quoted 15 lines · Jeff Layton
Quoted 10 lines
.TP
.B EIO
man2/fsync.2 +5 -1
-An error occurred during synchronization.
+An error occurred during synchronization.  This error may relate
+to data written to some other file descriptor on the same file.
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
+Since Linux 4.13 errors from write-back will be reported to
+all file descriptors that might have written the data which
triggered
+the error, and which are still open.
This is a little awkward. How could we report to a fd that was no longer
open? How about:
Quoted 4 lines
"Since Linux 4.13, errors from write-back will be reported to all file
descriptors that were open at the time that the error was recorded."
That might be simpler, but it is less correct. As I go on to say, NFS
*doesn't* report on all file descriptors that were open at that time.
I've changed it to
-------------------
Since Linux 4.13, errors from write-back will be reported to all file descriptors that might have written the data which triggered the error. Some filesystems (e.g. NFS) keep close track of which data
came through which file descriptor, and give precise reporting. Other filesystems (e.g. most local filesystems) will report errors to all file descriptors that where open on the file when the error was recorded.
------------------
which includes some of your text, and removes the "that are still open"
which probably doesn't help.
Quoted 14 lines
Review
Quoted 10 lines
.TP
.B EIO
A low-level I/O error occurred while modifying the inode.
(quoted diff) +7 -0
+This error may relate to data written by an earlier
+.BR write (2),
+which may have been issued to a different file descriptor on
+the same file.  Since Linux 4.13 errors from write-back will
+be reported to all file descriptors that might have
+written the data which triggered the error, and which are still
+open.
This is where things get a little more vague.
Quoted 7 lines
Some filesystems will return errors on a subsequent write(2) when previous writeback has failed -- some don't. In either case though, write(2) should never advance your errseq_t cursor, so only an fsync
will "clear" an earlier error.
I'm not sure how best to convey that in the manpages though.
How about:
-------------
This error may relate to the write-back of data written by an earlier
.BR write (2),
which may have been issued to a different file descriptor on the same file. Since Linux 4.13, errors from write-back come with a promise that they
.I may
be reported by subsequent.
.BR write (2)
requests, and
.I will
be reported by a subsequent
.BR fsync (2)
(whether or not they were also reported by
.BR write (2)).
------------
??
Those changes are included in the following.
Thanks,
NeilBrown
From: NeilBrown <neilb-IBi9RG/[email protected]>
Date: Thu, 14 Sep 2017 09:44:43 +1000
Subject: [PATCH] write.2, fsync.2, close.2: update description of error codes
Since 4.13, errors from writeback are more reliably reported to all file descriptors that might be relevant.
Add notes to this effect, and also add detail about ENOSPC and EDQUOT which can be delayed in a similar many to EIO - for NFS in particular.
Signed-off-by NeilBrown <neilb-IBi9RG/[email protected]>
---
 man2/close.2 |  9 +++++++++
 man2/fsync.2 | 18 +++++++++++++++++-
 man2/write.2 | 28 +++++++++++++++++++++++++---
 3 files changed, 51 insertions(+), 4 deletions(-)
man2/close.2 +3 -0
index 55d89ed3dbc7..136bd0be3f67 100644
@@ -82,6 +82,15 @@ call was interrupted by a signal; see
 .TP
 .B EIO
 An I/O error occurred.
+.TP
+.BR ENOSPC ", " EDQUOT
+On NFS, these errors are not normally reported against the first
write
man2/close.2 +6 -0
+which exceeds the available storage space, but instead against a
+subsequent
+.BR write (2),
+.BR fsync (2),
+or
+.BR close (2).
 .PP
 See NOTES for a discussion of why
 .BR close ()
man2/fsync.2 +5 -1
index eed3c460bea9..c7878bf3496f 100644
@@ -121,7 +121,15 @@ is set appropriately.
 is not a valid open file descriptor.
 .TP
 .B EIO
-An error occurred during synchronization.
+An error occurred during synchronization.  This error may relate
+to data written to some other file descriptor on the same file.
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
+Since Linux 4.13, errors from write-back will be reported to
+all file descriptors that might have written the data which
triggered
+the error. Some filesystems (e.g. NFS) keep close track of which data
man2/fsync.2 +2 -0
+came through which file descriptor, and give more precise reporting.
+Other filesystems (e.g. most local filesystems) will report errors
to
+all file descriptors that where open on the file when the error was recorded.
.TP
.B ENOSPC
Disk space was exhausted while synchronizing.
man2/fsync.2 +4 -0
@@ -130,6 +138,14 @@ Disk space was exhausted while synchronizing.
 .I fd
 is bound to a special file (e.g., a pipe, FIFO, or socket)
 which does not support synchronization.
+.TP
+.BR ENOSPC ", " EDQUOT
+.I fd
+is bound to a file on NFS or another filesystem which does not
allocate
man2/fsync.2 +4 -0
+space at the time of a
+.BR write (2)
+system call, and some previous write failed due to insufficient
+storage space.
 .SH CONFORMING TO
 POSIX.1-2001, POSIX.1-2008, 4.3BSD.
 .SH AVAILABILITY
man2/write.2 +17 -1
index 061aa70cf590..b1cc3a2cfb17 100644
@@ -47,7 +47,7 @@ write \- write to a file descriptor
 .BR write ()
 writes up to
 .I count
-bytes from the buffer pointed
+bytes from the buffer starting at
 .I buf
 to the file referred to by the file descriptor
 .IR fd .
@@ -181,6 +181,22 @@ or the file offset is not suitably aligned.
 .TP
 .B EIO
 A low-level I/O error occurred while modifying the inode.
+This error may relate to the write-back of data written by an
+earlier
+.BR write (2),
+which may have been issued to a different file descriptor on
+the same file.  Since Linux 4.13, errors from write-back come
+with a promise that they
+.I may
+be reported by subsequent.
+.BR write (2)
+requests, and
+.I will
+be reported by a subsequent
+.BR fsync (2)
+(whether or not they were also reported by
+.BR write (2)).
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
 .TP
 .B ENOSPC
 The device containing the file referred to by
@@ -222,8 +238,14 @@ unsigned and signed integer data types specified
by POSIX.1.
A successful return from
.BR write ()
does not make any guarantee that data has been committed to disk.
man2/write.2 +2 -2
-In fact, on some buggy implementations, it does not even guarantee
-that space has successfully been reserved for the data.
+On some filesystems, including NFS, it does not even guarantee
+that space has successfully been reserved for the data.  In the
case,

man2/write.2 unsigned and signed integer data types specified by POSIX.1. · line 251

Quoted 9 lines

man2/write.2 +6 -0

+some errors might be delayed to a future
+.BR write (2)
+or to
+.BR fsync (2)
+or even
+.BR close (2).
 The only way to be sure is to call
 .BR fsync (2)
 after you are done writing all your data.

Looks good to me!

Reviewed-by Jeff Layton <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Michael Kerrisk (man-opages)

2017-09-28 16:19 UTC #25 lore raw

Re: [RFC PATCH manpages] write.2, fsync.2, close.2: update description of error codes

In reply to NeilBrown

Hi Neil,

Quoted 86 lines · NeilBrown

Quoted 13 lines · Jeff Layton
man2/fsync.2 is set appropriately.
Quoted 9 lines
.TP
.B EIO
man2/fsync.2 +6 -1
-An error occurred during synchronization.
+An error occurred during synchronization.  This error may relate
+to data written to some other file descriptor on the same file.
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
+Since Linux 4.13 errors from write-back will be reported to
+all file descriptors that might have written the data which triggered
+the error, and which are still open.
This is a little awkward. How could we report to a fd that was no longer open? How about:
Quoted 2 lines
"Since Linux 4.13, errors from write-back will be reported to all file descriptors that were open at the time that the error was recorded."
That might be simpler, but it is less correct. As I go on to say, NFS *doesn't* report on all file descriptors that were open at that time.
I've changed it to
-------------------
Since Linux 4.13, errors from write-back will be reported to
all file descriptors that might have written the data which triggered the error. Some filesystems (e.g. NFS) keep close track of which data came through which file descriptor, and give precise reporting. Other filesystems (e.g. most local filesystems) will report errors to all file descriptors that where open on the file when the error was recorded.
------------------
which includes some of your text, and removes the "that are still open" which probably doesn't help.
Quoted 14 lines
Review
Quoted 10 lines
.TP
.B EIO
A low-level I/O error occurred while modifying the inode.
(quoted diff) +7 -0
+This error may relate to data written by an earlier
+.BR write (2),
+which may have been issued to a different file descriptor on
+the same file.  Since Linux 4.13 errors from write-back will
+be reported to all file descriptors that might have
+written the data which triggered the error, and which are still
+open.
This is where things get a little more vague.
Quoted 6 lines
Some filesystems will return errors on a subsequent write(2) when previous writeback has failed -- some don't. In either case though, write(2) should never advance your errseq_t cursor, so only an fsync will "clear" an earlier error.
I'm not sure how best to convey that in the manpages though.
How about:
-------------
This error may relate to the write-back of data written by an earlier
.BR write (2),
which may have been issued to a different file descriptor on the same file. Since Linux 4.13, errors from write-back come with a promise that they
.I may
be reported by subsequent.
.BR write (2)
requests, and
.I will
be reported by a subsequent
.BR fsync (2)
(whether or not they were also reported by
.BR write (2)).
------------
??
Those changes are included in the following.
Thanks,
NeilBrown
From: NeilBrown <[email protected]>
Date: Thu, 14 Sep 2017 09:44:43 +1000
Subject: [PATCH] write.2, fsync.2, close.2: update description of error codes

Quoted 7 lines

Since 4.13, errors from writeback are more reliably reported to all file descriptors that might be relevant.
Add notes to this effect, and also add detail about ENOSPC and EDQUOT which can be delayed in a similar many to EIO - for NFS in particular.
Signed-off-by NeilBrown <[email protected]>

Thanks! I've applied, and added Jeff's Reviewed-by.

Cheers,

Michael

Quoted 116 lines

---
  man2/close.2 |  9 +++++++++
  man2/fsync.2 | 18 +++++++++++++++++-
  man2/write.2 | 28 +++++++++++++++++++++++++---
  3 files changed, 51 insertions(+), 4 deletions(-)

man2/close.2 +9 -0

index 55d89ed3dbc7..136bd0be3f67 100644
@@ -82,6 +82,15 @@ call was interrupted by a signal; see
  .TP
  .B EIO
  An I/O error occurred.
+.TP
+.BR ENOSPC ", " EDQUOT
+On NFS, these errors are not normally reported against the first write
+which exceeds the available storage space, but instead against a
+subsequent
+.BR write (2),
+.BR fsync (2),
+or
+.BR close (2).
  .PP
  See NOTES for a discussion of why
  .BR close ()

man2/fsync.2 +17 -1

index eed3c460bea9..c7878bf3496f 100644
@@ -121,7 +121,15 @@ is set appropriately.
  is not a valid open file descriptor.
  .TP
  .B EIO
-An error occurred during synchronization.
+An error occurred during synchronization.  This error may relate
+to data written to some other file descriptor on the same file.
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
+Since Linux 4.13, errors from write-back will be reported to
+all file descriptors that might have written the data which triggered
+the error.  Some filesystems (e.g. NFS) keep close track of which data
+came through which file descriptor, and give more precise reporting.
+Other filesystems (e.g. most local filesystems) will report errors to
+all file descriptors that where open on the file when the error was recorded.
  .TP
  .B ENOSPC
  Disk space was exhausted while synchronizing.

@@ -130,6 +138,14 @@ Disk space was exhausted while synchronizing.
  .I fd
  is bound to a special file (e.g., a pipe, FIFO, or socket)
  which does not support synchronization.
+.TP
+.BR ENOSPC ", " EDQUOT
+.I fd
+is bound to a file on NFS or another filesystem which does not allocate
+space at the time of a
+.BR write (2)
+system call, and some previous write failed due to insufficient
+storage space.
  .SH CONFORMING TO
  POSIX.1-2001, POSIX.1-2008, 4.3BSD.
  .SH AVAILABILITY

man2/write.2 +25 -3

index 061aa70cf590..b1cc3a2cfb17 100644
@@ -47,7 +47,7 @@ write \- write to a file descriptor
  .BR write ()
  writes up to
  .I count
-bytes from the buffer pointed
+bytes from the buffer starting at
  .I buf
  to the file referred to by the file descriptor
  .IR fd .

@@ -181,6 +181,22 @@ or the file offset is not suitably aligned.
  .TP
  .B EIO
  A low-level I/O error occurred while modifying the inode.
+This error may relate to the write-back of data written by an
+earlier
+.BR write (2),
+which may have been issued to a different file descriptor on
+the same file.  Since Linux 4.13, errors from write-back come
+with a promise that they
+.I may
+be reported by subsequent.
+.BR write (2)
+requests, and
+.I will
+be reported by a subsequent
+.BR fsync (2)
+(whether or not they were also reported by
+.BR write (2)).
+.\" commit 088737f44bbf6378745f5b57b035e57ee3dc4750
  .TP
  .B ENOSPC
  The device containing the file referred to by

@@ -222,8 +238,14 @@ unsigned and signed integer data types specified by POSIX.1.
  A successful return from
  .BR write ()
  does not make any guarantee that data has been committed to disk.
-In fact, on some buggy implementations, it does not even guarantee
-that space has successfully been reserved for the data.
+On some filesystems, including NFS, it does not even guarantee
+that space has successfully been reserved for the data.  In the case,
+some errors might be delayed to a future
+.BR write (2)
+or to
+.BR fsync (2)
+or even
+.BR close (2).
  The only way to be sure is to call
  .BR fsync (2)
  after you are done writing all your data.

Trond Myklebust

2017-09-12 02:24 UTC #26 lore raw

In reply to NeilBrown

Quoted 150 lines · NeilBrown

However, while looking over the code to make sure I really understood it
and all the possible consequences of changing to errseq_t I found a few
anomalies. The patch below addresses them all.

Would you see if they may sense to you?

Thanks,
NeilBrown

From: NeilBrown <[email protected]>
Date: Mon, 11 Sep 2017 13:15:50 +1000
Subject: [PATCH] NFS: various changes relating to reporting IO errors.

1/ remove 'start' and 'end' args from nfs_file_fsync_commit().
They aren't used.

2/ Make nfs_context_set_write_error() a "static inline" in internal.h
so we can...

4/ If filemap_write_and_write_range() reports any error, still
check ctx->error. The value in ctx->error is likely to be more useful. As part of this, NFS_CONTEXT_ERROR_WRITE is cleared slightly earlier, before nfs_file_fsync_commit() is
called,
rather than at the start of that function.

Signed-off-by NeilBrown <[email protected]>

---
 fs/nfs/file.c     | 16 ++++++++++------
 fs/nfs/internal.h |  7 +++++++
 fs/nfs/pagelist.c |  4 ++--
 fs/nfs/write.c    |  7 -------
 4 files changed, 19 insertions(+), 15 deletions(-)

fs/nfs/file.c +2 -4

index af330c31f627..ab324f14081f 100644
@@ -208,21 +208,19 @@ EXPORT_SYMBOL_GPL(nfs_file_mmap);
  * fall back to doing a synchronous write.
  */
 static int
-nfs_file_fsync_commit(struct file *file, loff_t start, loff_t end,
 int datasync)
+nfs_file_fsync_commit(struct file *file, int datasync)
 {
 	struct nfs_open_context *ctx = nfs_file_open_context(file);
 	struct inode *inode = file_inode(file);
-	int have_error, do_resend, status;
+	int do_resend, status;
 	int ret = 0;
 
 	dprintk("NFS: fsync file(%pD2) datasync %d\n", file,
 datasync);
 
 	nfs_inc_stats(inode, NFSIOS_VFSFSYNC);
 	do_resend = test_and_clear_bit(NFS_CONTEXT_RESEND_WRITES,
 &ctx->flags);
-	have_error = test_and_clear_bit(NFS_CONTEXT_ERROR_WRITE,
 &ctx->flags);
 	status = nfs_commit_inode(inode, FLUSH_SYNC);
-	have_error |= test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx-

Quoted 1 line

flags);

fs/nfs/file.c +7 -2

-	if (have_error) {
+	if (test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags)) {
 		ret = xchg(&ctx->error, 0);
 		if (ret)
 			goto out;

@@ -247,10 +245,16 @@ 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;
+		}
 		if (ret != 0)
 			break;
-		ret = nfs_file_fsync_commit(file, start, end,

datasync);

fs/nfs/file.c +1 -0

+		ret = nfs_file_fsync_commit(file, datasync);
 		if (!ret)
 			ret = pnfs_sync_inode(inode, !!datasync);
 		/*

fs/nfs/internal.h +2 -0

index dc456416d2be..44c8962fec91 100644
@@ -769,3 +769,10 @@ static inline bool nfs_error_is_fatal(int err)
 		return false;
 	}
 }
+
+static inline void nfs_context_set_write_error(struct

nfs_open_context *ctx, int error)

fs/nfs/internal.h +5 -0

+{
+	ctx->error = error;
+	smp_wmb();
+	set_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags);
+}

fs/nfs/pagelist.c +2 -2

index de9066a92c0d..0ebd26b9a6bd 100644
@@ -1198,8 +1198,8 @@ out_failed:
 
 		/* remember fatal errors */
 		if (nfs_error_is_fatal(desc->pg_error))
-			mapping_set_error(desc->pg_inode->i_mapping,
-					  desc->pg_error);
+			nfs_context_set_write_error(req->wb_context,
+						    desc->pg_error);
 
 		func = desc->pg_completion_ops->error_cleanup;
 		for (midx = 0; midx < desc->pg_mirror_count; midx++)

{

fs/nfs/write.c +0 -7

index b1af5dee5e0a..f702bf2def79 100644
@@ -147,13 +147,6 @@ static void nfs_io_completion_put(struct
 nfs_io_completion *ioc)
 		kref_put(&ioc->refcount, nfs_io_completion_release);
 }
 
-static 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);
-}
-
 /*

Quoted 3 lines

* nfs_page_find_head_request_locked - find head request associated
with @page
*

That makes sense to me. I'm applying and will send as an update to 4.14...

Signature · 3 lines

Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]

NeilBrown

2017-09-12 05:29 UTC #27 lore raw

Quoted 4 lines · Trond Myklebust

That makes sense to me. I'm applying and will send as an update to 4.14...

Thanks!
NeilBrown