Re: Better read bio error granularity?

[Matthew Wilcox <willy@infradead.org>]

On Sun, Mar 13, 2022 at 07:03:39PM +0800, Qu Wenruo wrote:
> > Specifically for the page cache (which I hope is what you meant by
> > "page error status", because we definitely can't use that for DIO),
> 
> Although what I exactly mean is PageError flag.
> 
> For DIO the pages are not mapping to any inode, but it shouldn't prevent
> us from using PageError flag I guess?

For DIO, *it is not your page*.  It belongs to somebody else, and you
can't even look at the flags, or the ->mapping, or anything, really.
You have a refcount on the page to prevent it from going away, but the
only thing you can do to the page is use its address and put your refcount
when you're done with it.  It might be an anonymous page, the ZERO_PAGE,
a pagecache page for another filesystem, a pagecache page for your own
filesystem, or even a pagecache page for the same file you're writing to.
The DIO might only be 512-byte aligned, so different parts of the page can
be being written to by different DIOs, or parts or the entire page might
be being read by multiple DIOs.  It might be registered to an RDMA device.
I haven't been keeping up, but I think it's even possible that the page
of memory might be on a graphics card.

> > the intent is that ->readahead can just fail and not set any of the
> > pages Uptodate.  Then we come along later, notice there's a page in
> > the cache and call ->readpage on it and get the error for that one
> > page.  The other 31 pages should read successfully.
> 
> This comes a small question, what is prevent the fs to submit a large
> bio containing the 32 pages again, other than reading them page by page?
> 
> Just because of that page is there, but not Uptodate?

This would be a bad idea.  Pages can be !Uptodate and Dirty, if they've
been partially written by a block-aligned write.  So mindlessly
expanding a read of a single page to be a read of multiple pages will
result in data loss.  Also we might not have pages in
the page cache for the surrounding pages, and we might be low enough on
memory that we can't allocate them.  And pages have to be locked before
we do I/O.  They have to be locked in order of file index, so you would
need to cope with trylocking the pages before this one and those
trylocks maybe failing.

In short, this would be prohibitively complex.

> > 
> > (There's an awkward queston to ask about large folios here, and what
> > we might be able to do around sub-folio or even sub-page or sub-block
> > reads that happen to not touch the bytes which are in an error region,
> > but let's keep the conversation about pages for now).
> > 
> Yeah, that can go crazy pretty soon.
> 
> Like iomap or btrfs, they all use page::private to store extra bitmaps
> for those cases, thus it really impossible to use PageError flag.
> Thus I intentionally skip them here.

I'm actually looking to get rid of PageError, but I think the other
points stand by themselves even without that intention ;-)

> Thank you very much for the quick and helpful reply,

You're welcome!  Thanks for starting the discussion rather than rushing
off to implement it ;-)