On Fri, Aug 25 2017, Jeff Layton wrote:
> On Thu, 2017-07-20 at 15:42 -0400, Jeff Layton wrote:
>> From: Jeff Layton <jlayton@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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:
signature.asc
Description: PGP signature
