| Message ID | 20230126202415.1682629-5-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert most of ext4 to folios | expand |
"Matthew Wilcox (Oracle)" <willy@infradead.org> writes: > Prepare ext4 to support large folios in the page writeback path. Sure. I am guessing for ext4 to completely support large folio requires more work like fscrypt bounce page handling doesn't yet support folios right? Could you please give a little background on what all would be required to add large folio support in ext4 buffered I/O path? (I mean ofcourse other than saying move ext4 to iomap ;)) What I was interested in was, what other components in particular for e.g. fscrypt, fsverity, ext4's xyz component needs large folio support? And how should one go about in adding this support? So can we move ext4's read path to have large folio support to get started? Have you already identified what all is missing from this path to convert it? > Also set the actual error in the mapping, not just -EIO. Right. I looked at the history and I think it always just had EIO. I think setting the actual err in mapping_set_error() is the right thing to do here. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> w.r.t this patch series. I reviewed the mechanical changes & error paths which converts ext4 ext4_finish_bio() to use folio. The changes looks good to me from that perspective. Feel free to add - Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > --- > fs/ext4/page-io.c | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c > index 982791050892..fd6c0dca24b9 100644 > --- a/fs/ext4/page-io.c > +++ b/fs/ext4/page-io.c > @@ -99,30 +99,30 @@ static void buffer_io_error(struct buffer_head *bh) > > static void ext4_finish_bio(struct bio *bio) > { > - struct bio_vec *bvec; > - struct bvec_iter_all iter_all; > + struct folio_iter fi; > > - bio_for_each_segment_all(bvec, bio, iter_all) { > - struct page *page = bvec->bv_page; > - struct page *bounce_page = NULL; > + bio_for_each_folio_all(fi, bio) { > + struct folio *folio = fi.folio; > + struct folio *io_folio = NULL; > struct buffer_head *bh, *head; > - unsigned bio_start = bvec->bv_offset; > - unsigned bio_end = bio_start + bvec->bv_len; > + size_t bio_start = fi.offset; > + size_t bio_end = bio_start + fi.length; > unsigned under_io = 0; > unsigned long flags; > > - if (fscrypt_is_bounce_page(page)) { > - bounce_page = page; > - page = fscrypt_pagecache_page(bounce_page); > + if (fscrypt_is_bounce_folio(folio)) { > + io_folio = folio; > + folio = fscrypt_pagecache_folio(folio); > } > > if (bio->bi_status) { > - SetPageError(page); > - mapping_set_error(page->mapping, -EIO); > + int err = blk_status_to_errno(bio->bi_status); > + folio_set_error(folio); > + mapping_set_error(folio->mapping, err); > } > - bh = head = page_buffers(page); > + bh = head = folio_buffers(folio); > /* > - * We check all buffers in the page under b_uptodate_lock > + * We check all buffers in the folio under b_uptodate_lock > * to avoid races with other end io clearing async_write flags > */ > spin_lock_irqsave(&head->b_uptodate_lock, flags); > @@ -141,8 +141,8 @@ static void ext4_finish_bio(struct bio *bio) > } while ((bh = bh->b_this_page) != head); > spin_unlock_irqrestore(&head->b_uptodate_lock, flags); > if (!under_io) { > - fscrypt_free_bounce_page(bounce_page); > - end_page_writeback(page); > + fscrypt_free_bounce_page(&io_folio->page); Could you please help understand what would it take to convert bounce page in fscrypt to folio? Today, we allocate 32 bounce pages of order 0 via mempool in fscrypt_initialize() <...> fscrypt_bounce_page_pool = mempool_create_page_pool(num_prealloc_crypto_pages, 0); <...> And IIUC, we might need to add some support for having higher order pages in the pool so that one can allocate a folio->_folio_order folio from this pool for bounce page to support large folio. Is that understanding correct? Your thoughts on this please? -ritesh
On Thu, Jan 26, 2023 at 08:23:48PM +0000, Matthew Wilcox (Oracle) wrote: > Prepare ext4 to support large folios in the page writeback path. > Also set the actual error in the mapping, not just -EIO. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Reviewed-by: Theodore Ts'o <tytso@mit.edu>
On Mon, Mar 06, 2023 at 02:40:55PM +0530, Ritesh Harjani wrote: > "Matthew Wilcox (Oracle)" <willy@infradead.org> writes: > > > Prepare ext4 to support large folios in the page writeback path. > > Sure. I am guessing for ext4 to completely support large folio > requires more work like fscrypt bounce page handling doesn't > yet support folios right? > > Could you please give a little background on what all would be required > to add large folio support in ext4 buffered I/O path? > (I mean ofcourse other than saying move ext4 to iomap ;)) > > What I was interested in was, what other components in particular for > e.g. fscrypt, fsverity, ext4's xyz component needs large folio support? > > And how should one go about in adding this support? So can we move > ext4's read path to have large folio support to get started? > Have you already identified what all is missing from this path to > convert it? Honestly, I don't know what else needs to be done beyond this patch series. I can point at some stuff and say "This doesn't work", but in general, you have to just enable it and see what breaks. A lot of the buffer_head code is not large-folio safe right now, so that's somewhere to go and look. Or maybe we "just" convert to iomap, and never bother fixing the bufferhead code for large folios. > > Also set the actual error in the mapping, not just -EIO. > > Right. I looked at the history and I think it always just had EIO. > I think setting the actual err in mapping_set_error() is the right thing > to do here. > > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > w.r.t this patch series. I reviewed the mechanical changes & error paths > which converts ext4 ext4_finish_bio() to use folio. > > The changes looks good to me from that perspective. Feel free to add - > Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> Thanks!
On Thu, Mar 23, 2023 at 03:26:43AM +0000, Matthew Wilcox wrote: > On Mon, Mar 06, 2023 at 02:40:55PM +0530, Ritesh Harjani wrote: > > "Matthew Wilcox (Oracle)" <willy@infradead.org> writes: > > > > > Prepare ext4 to support large folios in the page writeback path. > > > > Sure. I am guessing for ext4 to completely support large folio > > requires more work like fscrypt bounce page handling doesn't > > yet support folios right? > > > > Could you please give a little background on what all would be required > > to add large folio support in ext4 buffered I/O path? > > (I mean ofcourse other than saying move ext4 to iomap ;)) > > > > What I was interested in was, what other components in particular for > > e.g. fscrypt, fsverity, ext4's xyz component needs large folio support? > > > > And how should one go about in adding this support? So can we move > > ext4's read path to have large folio support to get started? > > Have you already identified what all is missing from this path to > > convert it? > > Honestly, I don't know what else needs to be done beyond this patch > series. I can point at some stuff and say "This doesn't work", but in > general, you have to just enable it and see what breaks. A lot of the > buffer_head code is not large-folio safe right now, so that's somewhere > to go and look. Or maybe we "just" convert to iomap, and never bother > fixing the bufferhead code for large folios. Yes. Let's leave bufferheads in the legacy doo-doo-dooooo basement instead of wasting more time on them. Ideally we'd someday run all the filesystems through: bufferheads -> iomap with bufferheads -> iomap with folios -> iomap with large folios -> retire to somewhere cheaper than Hawaii --D > > > Also set the actual error in the mapping, not just -EIO. > > > > Right. I looked at the history and I think it always just had EIO. > > I think setting the actual err in mapping_set_error() is the right thing > > to do here. > > > > > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > > > w.r.t this patch series. I reviewed the mechanical changes & error paths > > which converts ext4 ext4_finish_bio() to use folio. > > > > The changes looks good to me from that perspective. Feel free to add - > > Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > > Thanks!
On Thu, Mar 23, 2023 at 07:51:09AM -0700, Darrick J. Wong wrote: > On Thu, Mar 23, 2023 at 03:26:43AM +0000, Matthew Wilcox wrote: > > On Mon, Mar 06, 2023 at 02:40:55PM +0530, Ritesh Harjani wrote: > > > "Matthew Wilcox (Oracle)" <willy@infradead.org> writes: > > > > > > > Prepare ext4 to support large folios in the page writeback path. > > > > > > Sure. I am guessing for ext4 to completely support large folio > > > requires more work like fscrypt bounce page handling doesn't > > > yet support folios right? > > > > > > Could you please give a little background on what all would be required > > > to add large folio support in ext4 buffered I/O path? > > > (I mean ofcourse other than saying move ext4 to iomap ;)) > > > > > > What I was interested in was, what other components in particular for > > > e.g. fscrypt, fsverity, ext4's xyz component needs large folio support? > > > > > > And how should one go about in adding this support? So can we move > > > ext4's read path to have large folio support to get started? > > > Have you already identified what all is missing from this path to > > > convert it? > > > > Honestly, I don't know what else needs to be done beyond this patch > > series. I can point at some stuff and say "This doesn't work", but in > > general, you have to just enable it and see what breaks. A lot of the > > buffer_head code is not large-folio safe right now, so that's somewhere > > to go and look. Or maybe we "just" convert to iomap, and never bother > > fixing the bufferhead code for large folios. > > Yes. Let's leave bufferheads in the legacy doo-doo-dooooo basement > instead of wasting more time on them. Ideally we'd someday run all the > filesystems through: > > bufferheads -> iomap with bufferheads -> iomap with folios -> iomap with > large folios -> retire to somewhere cheaper than Hawaii Places cheaper than Hawaii probably aren't as pretty as Hawaii though :-( XFS is fine because it uses xfs_buf, but if we don't add support for large folios to bufferheads, we can't support LBA size > PAGE_SIZE even to read the superblock. Maybe that's fine ... only filesystems which don't use sb_bread() get to support LBA size > PAGE_SIZE. I really want to see a cheaper abstraction for accessing the block device than BHs. Or xfs_buf for that matter.
On Thu, Mar 23, 2023 at 07:51:09AM -0700, Darrick J. Wong wrote: > Yes. Let's leave bufferheads in the legacy doo-doo-dooooo basement > instead of wasting more time on them. Ideally we'd someday run all the > filesystems through: > > bufferheads -> iomap with bufferheads -> iomap with folios -> iomap with > large folios -> retire to somewhere cheaper than Hawaii For a lot of the legacy stuff (and with that I don't mean ext4) we'd really need volunteers to do any work other than typo fixing and cosmetic cleanups. I suspect just dropping many of them is the only thing we can do long term. But even if we do the above for the data path for all file systems remaining in tree, we still have buffer_heads for metadata. And I think buffered_heads really are more or less the right abstraction there anyway. And for these existing file systems we also do not care about using large folios for metadata caching anyway. The only nasty part is that these buffer_heads use the same mapping as all block device access, so we'll need to find a way to use large folios for the block device mapping for the LBA size > page size case, while supporting buffer heads for those file systems. Nothing unsolvable, but a bit tricky.
On Thu, Mar 23, 2023 at 03:30:38PM +0000, Matthew Wilcox wrote: > I really want to see a cheaper abstraction for accessing the block device > than BHs. Or xfs_buf for that matter. You literally can just use the bdev page cache using the normal page cache helpers. It's not quite what most of these file systems expect, though, especially for the block size < PAGE_SIZE case.
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c index 982791050892..fd6c0dca24b9 100644 --- a/fs/ext4/page-io.c +++ b/fs/ext4/page-io.c @@ -99,30 +99,30 @@ static void buffer_io_error(struct buffer_head *bh) static void ext4_finish_bio(struct bio *bio) { - struct bio_vec *bvec; - struct bvec_iter_all iter_all; + struct folio_iter fi; - bio_for_each_segment_all(bvec, bio, iter_all) { - struct page *page = bvec->bv_page; - struct page *bounce_page = NULL; + bio_for_each_folio_all(fi, bio) { + struct folio *folio = fi.folio; + struct folio *io_folio = NULL; struct buffer_head *bh, *head; - unsigned bio_start = bvec->bv_offset; - unsigned bio_end = bio_start + bvec->bv_len; + size_t bio_start = fi.offset; + size_t bio_end = bio_start + fi.length; unsigned under_io = 0; unsigned long flags; - if (fscrypt_is_bounce_page(page)) { - bounce_page = page; - page = fscrypt_pagecache_page(bounce_page); + if (fscrypt_is_bounce_folio(folio)) { + io_folio = folio; + folio = fscrypt_pagecache_folio(folio); } if (bio->bi_status) { - SetPageError(page); - mapping_set_error(page->mapping, -EIO); + int err = blk_status_to_errno(bio->bi_status); + folio_set_error(folio); + mapping_set_error(folio->mapping, err); } - bh = head = page_buffers(page); + bh = head = folio_buffers(folio); /* - * We check all buffers in the page under b_uptodate_lock + * We check all buffers in the folio under b_uptodate_lock * to avoid races with other end io clearing async_write flags */ spin_lock_irqsave(&head->b_uptodate_lock, flags); @@ -141,8 +141,8 @@ static void ext4_finish_bio(struct bio *bio) } while ((bh = bh->b_this_page) != head); spin_unlock_irqrestore(&head->b_uptodate_lock, flags); if (!under_io) { - fscrypt_free_bounce_page(bounce_page); - end_page_writeback(page); + fscrypt_free_bounce_page(&io_folio->page); + folio_end_writeback(folio); } } }
Prepare ext4 to support large folios in the page writeback path. Also set the actual error in the mapping, not just -EIO. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- fs/ext4/page-io.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)