| Message ID | 3788353.1685003937@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | zonefs: Call zonefs_io_error() on any error from filemap_splice_read() | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
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!
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 ?
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 ?
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.
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); }
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(-)