On Mon, 2017-09-11 at 13:24 +1000, NeilBrown wrote:
> 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 <neilb@xxxxxxxx>
> 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 <neilb@xxxxxxxx>
> ---
> 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(-)
>
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index af330c31f627..ab324f14081f 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -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);
> /*
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index dc456416d2be..44c8962fec91 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -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);
> +}
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index de9066a92c0d..0ebd26b9a6bd 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -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++)
> {
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index b1af5dee5e0a..f702bf2def79 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -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
> *
That makes sense to me. I'm applying and will send as an update to
4.14...
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@xxxxxxxxxxxxxxx
