Re: [PATCH] nfs: track writeback errors with errseq_t

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 3232 bytes --]

On Fri, Aug 25 2017, Jeff Layton wrote:

> On Thu, 2017-07-20 at 15:42 -0400, Jeff Layton wrote:
>> From: Jeff Layton <jlayton@redhat.com>
>> 
>> 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 <jlayton@redhat.com>
>> ---
>>  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);
>> +	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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]