[PATCH] dax: set errors in mapping when writeback fails

[PATCH] dax: set errors in mapping when writeback fails

[Jeff Layton]

Jan's description for this patch is much better than mine, so I'm
quoting it verbatim here:

DAX currently doesn't set errors in the mapping when cache flushing
fails in dax_writeback_mapping_range(). Since this function can get
called only from fsync(2) or sync(2), this is actually as good as it can
currently get since we correctly propagate the error up from
dax_writeback_mapping_range() to filemap_fdatawrite(). However in the
future better writeback error handling will enable us to properly report
these errors on fsync(2) even if there are multiple file descriptors
open against the file or if sync(2) gets called before fsync(2). So
convert DAX to using standard error reporting through the mapping.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-and-Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 fs/dax.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/dax.c b/fs/dax.c
index c22eaf162f95..441280e15d5b 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -856,8 +856,10 @@ int dax_writeback_mapping_range(struct address_space *mapping,
 
 			ret = dax_writeback_one(bdev, dax_dev, mapping,
 					indices[i], pvec.pages[i]);
-			if (ret < 0)
+			if (ret < 0) {
+				mapping_set_error(mapping, ret);
 				goto out;
+			}
 		}
 	}
 out:
-- 
2.9.4

Re: [PATCH] dax: set errors in mapping when writeback fails

[Ross Zwisler]

On Thu, May 25, 2017 at 06:47:46AM -0400, Jeff Layton wrote:
> Jan's description for this patch is much better than mine, so I'm
> quoting it verbatim here:
> 
> DAX currently doesn't set errors in the mapping when cache flushing
> fails in dax_writeback_mapping_range(). Since this function can get
> called only from fsync(2) or sync(2), this is actually as good as it can
> currently get since we correctly propagate the error up from
> dax_writeback_mapping_range() to filemap_fdatawrite(). However in the
> future better writeback error handling will enable us to properly report
> these errors on fsync(2) even if there are multiple file descriptors
> open against the file or if sync(2) gets called before fsync(2). So
> convert DAX to using standard error reporting through the mapping.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-and-Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>

Is this still part of a larger series, or are you trying to submit it on its
own?  On it's own this patch still suffers from the issue I reported here:

https://lkml.org/lkml/2017/3/6/976

> ---
>  fs/dax.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index c22eaf162f95..441280e15d5b 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -856,8 +856,10 @@ int dax_writeback_mapping_range(struct address_space *mapping,
>  
>  			ret = dax_writeback_one(bdev, dax_dev, mapping,
>  					indices[i], pvec.pages[i]);
> -			if (ret < 0)
> +			if (ret < 0) {
> +				mapping_set_error(mapping, ret);
>  				goto out;
> +			}
>  		}
>  	}
>  out:
> -- 
> 2.9.4
> 

Re: [PATCH] dax: set errors in mapping when writeback fails

[Jeff Layton]

On Fri, 2017-05-26 at 14:31 -0600, Ross Zwisler wrote:
> On Thu, May 25, 2017 at 06:47:46AM -0400, Jeff Layton wrote:
> > Jan's description for this patch is much better than mine, so I'm
> > quoting it verbatim here:
> > 
> > DAX currently doesn't set errors in the mapping when cache flushing
> > fails in dax_writeback_mapping_range(). Since this function can get
> > called only from fsync(2) or sync(2), this is actually as good as it can
> > currently get since we correctly propagate the error up from
> > dax_writeback_mapping_range() to filemap_fdatawrite(). However in the
> > future better writeback error handling will enable us to properly report
> > these errors on fsync(2) even if there are multiple file descriptors
> > open against the file or if sync(2) gets called before fsync(2). So
> > convert DAX to using standard error reporting through the mapping.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > Reviewed-by: Jan Kara <jack@suse.cz>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-and-Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> 
> Is this still part of a larger series, or are you trying to submit it on its
> own?

I was trying to submit it on its own. Trying to get a series merged that
sprawls over the tree like this is proving difficult, so I was hoping to
get maintainers to pick up individual patches for their subsystems.

>  On it's own this patch still suffers from the issue I reported here:
> 
> https://lkml.org/lkml/2017/3/6/976
> 

My bad. I remember seeing you mail this and then let it slip through the
cracks. Let's see, you said:

> I think maybe the missing piece is that our normal DAX fsync call stack
> doesn't include a call to filemap_check_errors() if we return -EIO.

I think the actual problem is in filemap_write_and_wait_range. When
there is an error from __filemap_fdatawrite_range, we probably ought to
go ahead and clear the AS_EIO/AS_ENOSPC flags as well even though we're
not waiting on writeback.

I think I had a patch to do that in an earlier iteration of this series,
but I dropped it when I started pursuing the errseq_t based error
tracking.

Now that we're going to have to do this in a more piecemeal way, it may
be worth resurrecting that patch as an interim step before merging this 
one. I'll see if I can dust that patch off after the holiday weekend.

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>