[PATCH] zonefs: Call zonefs_io_error() on any error from filemap_splice_read()

[PATCH] zonefs: Call zonefs_io_error() on any error from filemap_splice_read()

[David Howells]

    
Call zonefs_io_error() after getting any error from filemap_splice_read()
in zonefs_file_splice_read(), including non-fatal errors such as ENOMEM,
EINTR and EAGAIN.

Suggested-by: Damien Le Moal <dlemoal@kernel.org>
Link: https://lore.kernel.org/r/5d327bed-b532-ad3b-a211-52ad0a3e276a@kernel.org/
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Naohiro Aota <naohiro.aota@wdc.com>
cc: Johannes Thumshirn <jth@kernel.org>
cc: Christoph Hellwig <hch@lst.de>
cc: Al Viro <viro@zeniv.linux.org.uk>
cc: Jens Axboe <axboe@kernel.dk>
cc: linux-fsdevel@vger.kernel.org
cc: linux-block@vger.kernel.org
cc: linux-mm@kvack.org
---
 fs/zonefs/file.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/zonefs/file.c b/fs/zonefs/file.c
index 65d4c4fe6364..0660cffc5ed8 100644
--- a/fs/zonefs/file.c
+++ b/fs/zonefs/file.c
@@ -782,7 +782,7 @@ static ssize_t zonefs_file_splice_read(struct file *in, loff_t *ppos,
 
 	if (len > 0) {
 		ret = filemap_splice_read(in, ppos, pipe, len, flags);
-		if (ret == -EIO)
+		if (ret < 0)
 			zonefs_io_error(inode, false);
 	}
 

Re: [PATCH] zonefs: Call zonefs_io_error() on any error from filemap_splice_read()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH] zonefs: Call zonefs_io_error() on any error from filemap_splice_read()

[Matthew Wilcox]

On Thu, May 25, 2023 at 09:38:57AM +0100, David Howells wrote:
>     
> Call zonefs_io_error() after getting any error from filemap_splice_read()
> in zonefs_file_splice_read(), including non-fatal errors such as ENOMEM,
> EINTR and EAGAIN.
> 
> Suggested-by: Damien Le Moal <dlemoal@kernel.org>
> Link: https://lore.kernel.org/r/5d327bed-b532-ad3b-a211-52ad0a3e276a@kernel.org/

This seems like a bizarre thing to do.  Let's suppose you got an
-ENOMEM.  blkdev_report_zones() is also likely to report -ENOMEM in
that case, which will cause a zonefs_err() to be called.  Surely
that can't be the desired outcome from getting -ENOMEM!

Re: [PATCH] zonefs: Call zonefs_io_error() on any error from filemap_splice_read()

[Damien Le Moal]

On 5/26/23 00:21, Matthew Wilcox wrote:
> On Thu, May 25, 2023 at 09:38:57AM +0100, David Howells wrote:
>>     
>> Call zonefs_io_error() after getting any error from filemap_splice_read()
>> in zonefs_file_splice_read(), including non-fatal errors such as ENOMEM,
>> EINTR and EAGAIN.
>>
>> Suggested-by: Damien Le Moal <dlemoal@kernel.org>
>> Link: https://lore.kernel.org/r/5d327bed-b532-ad3b-a211-52ad0a3e276a@kernel.org/
> 
> This seems like a bizarre thing to do.  Let's suppose you got an
> -ENOMEM.  blkdev_report_zones() is also likely to report -ENOMEM in
> that case, which will cause a zonefs_err() to be called.  Surely
> that can't be the desired outcome from getting -ENOMEM!

Right... What I want to make sure here is that the error we get is not the
result of a failed IO. Beside EIO, are there any other cases ?
I can think of at least:
1) -ETIMEDOUT -> the drive is not responding. In this case, calling
zonefs_io_error() may not be useful either.
2) -ETIME: The IO was done with a duration limit (e.g. active time limit) and
was aborted by the drive because it took too long. Calling zonefs_io_error() for
this case is also not useful.

But I am thinking block layer (blk_status_t to errno conversion) here. Does the
folio code *always* return EIO if it could not get a page/folio, regardless of
the actual bio status ?

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] zonefs: Call zonefs_io_error() on any error from filemap_splice_read()

[Damien Le Moal]

On 5/26/23 08:04, Damien Le Moal wrote:
> On 5/26/23 00:21, Matthew Wilcox wrote:
>> On Thu, May 25, 2023 at 09:38:57AM +0100, David Howells wrote:
>>>     
>>> Call zonefs_io_error() after getting any error from filemap_splice_read()
>>> in zonefs_file_splice_read(), including non-fatal errors such as ENOMEM,
>>> EINTR and EAGAIN.
>>>
>>> Suggested-by: Damien Le Moal <dlemoal@kernel.org>
>>> Link: https://lore.kernel.org/r/5d327bed-b532-ad3b-a211-52ad0a3e276a@kernel.org/
>>
>> This seems like a bizarre thing to do.  Let's suppose you got an
>> -ENOMEM.  blkdev_report_zones() is also likely to report -ENOMEM in
>> that case, which will cause a zonefs_err() to be called.  Surely
>> that can't be the desired outcome from getting -ENOMEM!
> 
> Right... What I want to make sure here is that the error we get is not the
> result of a failed IO. Beside EIO, are there any other cases ?
> I can think of at least:
> 1) -ETIMEDOUT -> the drive is not responding. In this case, calling
> zonefs_io_error() may not be useful either.
> 2) -ETIME: The IO was done with a duration limit (e.g. active time limit) and
> was aborted by the drive because it took too long. Calling zonefs_io_error() for
> this case is also not useful.
> 
> But I am thinking block layer (blk_status_t to errno conversion) here. Does the
> folio code *always* return EIO if it could not get a page/folio, regardless of
> the actual bio status ?

Replying to myself :)

iomap_read_folio() or iomap_finish_folio_read() -> folio_set_error(), which sets
PG_error. Then filemap_read_folio() will see !folio_test_uptodate(folio) and end
up returning -EIO. So if there was an IO and it failed, we always get EIO,
regardless of the actual reason for the IO failure. Right ?

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] zonefs: Call zonefs_io_error() on any error from filemap_splice_read()

[Matthew Wilcox]

On Fri, May 26, 2023 at 08:46:44AM +0900, Damien Le Moal wrote:
> iomap_read_folio() or iomap_finish_folio_read() -> folio_set_error(), which sets
> PG_error. Then filemap_read_folio() will see !folio_test_uptodate(folio) and end
> up returning -EIO. So if there was an IO and it failed, we always get EIO,
> regardless of the actual reason for the IO failure. Right ?

Don't rely on that.  I have plans for returning the correct error.

Really we need a function that knows whether an errno is transient or
reportable.