| Message ID | 20230309090526.332550-14-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/20] btrfs: mark extent_buffer_under_io static | expand |
On 09.03.23 10:08, Christoph Hellwig wrote: > + btrfs_subpage_clear_writeback(fs_info, page, eb->start, > + eb->len); > + else > + end_page_writeback(page); > > - end_extent_buffer_writeback(eb); > } > + end_extent_buffer_writeback(eb); > > - bio_put(bio); > + bio_put(&bbio->bio); > } > Can we merge end_extent_buffer_writeback() here? It's a 3 line function that get's only called once after this change.
On Thu, Mar 09, 2023 at 02:10:51PM +0000, Johannes Thumshirn wrote: > > + end_extent_buffer_writeback(eb); > > > > - bio_put(bio); > > + bio_put(&bbio->bio); > > } > > > > > Can we merge end_extent_buffer_writeback() here? It's a 3 line function > that get's only called once after this change. I can add a patch for that.
On 2023/3/9 17:05, Christoph Hellwig wrote: > Now that we always use a single bio to write an extent_buffer, the buffer > can be passed to the end_io handler as private data. This allows > to simplify the metadata write end I/O handler, and merge the subpage > version into the main one with just a single conditional. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/btrfs/extent_io.c | 112 +++++++++---------------------------------- > 1 file changed, 23 insertions(+), 89 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 1d7f48190ee9b9..16522bcf5d4a10 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -1663,13 +1663,11 @@ static bool lock_extent_buffer_for_io(struct extent_buffer *eb, > return ret; > } > > -static void set_btree_ioerr(struct page *page, struct extent_buffer *eb) > +static void set_btree_ioerr(struct extent_buffer *eb) > { > struct btrfs_fs_info *fs_info = eb->fs_info; > > - btrfs_page_set_error(fs_info, page, eb->start, eb->len); > - if (test_and_set_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags)) > - return; > + set_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags); > > /* > * A read may stumble upon this buffer later, make sure that it gets an > @@ -1683,7 +1681,7 @@ static void set_btree_ioerr(struct page *page, struct extent_buffer *eb) > * return a 0 because we are readonly if we don't modify the err seq for > * the superblock. > */ > - mapping_set_error(page->mapping, -EIO); > + mapping_set_error(eb->fs_info->btree_inode->i_mapping, -EIO); > > /* > * If writeback for a btree extent that doesn't belong to a log tree > @@ -1758,101 +1756,37 @@ static struct extent_buffer *find_extent_buffer_nolock( > return NULL; > } > > -/* > - * The endio function for subpage extent buffer write. > - * > - * Unlike end_bio_extent_buffer_writepage(), we only call end_page_writeback() > - * after all extent buffers in the page has finished their writeback. > - */ > -static void end_bio_subpage_eb_writepage(struct btrfs_bio *bbio) > +static void extent_buffer_write_end_io(struct btrfs_bio *bbio) > { > - struct bio *bio = &bbio->bio; > - struct btrfs_fs_info *fs_info; > - struct bio_vec *bvec; > + struct extent_buffer *eb = bbio->private; > + struct btrfs_fs_info *fs_info = eb->fs_info; > + bool uptodate = !bbio->bio.bi_status; > struct bvec_iter_all iter_all; > - > - fs_info = btrfs_sb(bio_first_page_all(bio)->mapping->host->i_sb); > - ASSERT(fs_info->nodesize < PAGE_SIZE); > - > - ASSERT(!bio_flagged(bio, BIO_CLONED)); > - bio_for_each_segment_all(bvec, bio, iter_all) { > - struct page *page = bvec->bv_page; > - u64 bvec_start = page_offset(page) + bvec->bv_offset; > - u64 bvec_end = bvec_start + bvec->bv_len - 1; > - u64 cur_bytenr = bvec_start; > - > - ASSERT(IS_ALIGNED(bvec->bv_len, fs_info->nodesize)); > - > - /* Iterate through all extent buffers in the range */ > - while (cur_bytenr <= bvec_end) { > - struct extent_buffer *eb; > - int done; > - > - /* > - * Here we can't use find_extent_buffer(), as it may > - * try to lock eb->refs_lock, which is not safe in endio > - * context. > - */ > - eb = find_extent_buffer_nolock(fs_info, cur_bytenr); > - ASSERT(eb); > - > - cur_bytenr = eb->start + eb->len; > - > - ASSERT(test_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags)); > - done = atomic_dec_and_test(&eb->io_pages); > - ASSERT(done); > - > - if (bio->bi_status || > - test_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags)) { > - ClearPageUptodate(page); > - set_btree_ioerr(page, eb); > - } > - > - btrfs_subpage_clear_writeback(fs_info, page, eb->start, > - eb->len); > - end_extent_buffer_writeback(eb); > - /* > - * free_extent_buffer() will grab spinlock which is not > - * safe in endio context. Thus here we manually dec > - * the ref. > - */ > - atomic_dec(&eb->refs); > - } > - } > - bio_put(bio); > -} > - > -static void end_bio_extent_buffer_writepage(struct btrfs_bio *bbio) > -{ > - struct bio *bio = &bbio->bio; > struct bio_vec *bvec; > - struct extent_buffer *eb; > - int done; > - struct bvec_iter_all iter_all; > > - ASSERT(!bio_flagged(bio, BIO_CLONED)); > - bio_for_each_segment_all(bvec, bio, iter_all) { > + if (!uptodate) > + set_btree_ioerr(eb); > + > + bio_for_each_segment_all(bvec, &bbio->bio, iter_all) { > struct page *page = bvec->bv_page; > > - eb = (struct extent_buffer *)page->private; > - BUG_ON(!eb); > - done = atomic_dec_and_test(&eb->io_pages); > + atomic_dec(&eb->io_pages); > > - if (bio->bi_status || > - test_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags)) { > + if (!uptodate) { > ClearPageUptodate(page); There seems to be an existing bug in the endio function for subpage. We should call btrfs_page_clear_uptodate() helper. Or we subpage uptodate bitmap and the page uptodate flag would get out of sync. > - set_btree_ioerr(page, eb); > + btrfs_page_set_error(fs_info, page, eb->start, eb->len); > } > > - end_page_writeback(page); > - > - if (!done) > - continue; > + if (fs_info->nodesize < PAGE_SIZE) > + btrfs_subpage_clear_writeback(fs_info, page, eb->start, > + eb->len); > + else > + end_page_writeback(page); Here we can just call btrfs_clear_writeback(), which can handle both subpage and regular cases. Thanks, Qu > > - end_extent_buffer_writeback(eb); > } > + end_extent_buffer_writeback(eb); > > - bio_put(bio); > + bio_put(&bbio->bio); > } > > static void prepare_eb_write(struct extent_buffer *eb) > @@ -1911,7 +1845,7 @@ static void write_one_subpage_eb(struct extent_buffer *eb, > bbio = btrfs_bio_alloc(INLINE_EXTENT_BUFFER_PAGES, > REQ_OP_WRITE | REQ_META | wbc_to_write_flags(wbc), > BTRFS_I(eb->fs_info->btree_inode), > - end_bio_subpage_eb_writepage, NULL); > + extent_buffer_write_end_io, eb); > bbio->bio.bi_iter.bi_sector = eb->start >> SECTOR_SHIFT; > bbio->file_offset = eb->start; > __bio_add_page(&bbio->bio, page, eb->len, eb->start - page_offset(page)); > @@ -1937,7 +1871,7 @@ static noinline_for_stack void write_one_eb(struct extent_buffer *eb, > bbio = btrfs_bio_alloc(INLINE_EXTENT_BUFFER_PAGES, > REQ_OP_WRITE | REQ_META | wbc_to_write_flags(wbc), > BTRFS_I(eb->fs_info->btree_inode), > - end_bio_extent_buffer_writepage, NULL); > + extent_buffer_write_end_io, eb); > bbio->bio.bi_iter.bi_sector = eb->start >> SECTOR_SHIFT; > bbio->file_offset = eb->start; >
On Fri, Mar 10, 2023 at 04:44:37PM +0800, Qu Wenruo wrote: >> - BUG_ON(!eb); >> - done = atomic_dec_and_test(&eb->io_pages); >> + atomic_dec(&eb->io_pages); >> - if (bio->bi_status || >> - test_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags)) { >> + if (!uptodate) { >> ClearPageUptodate(page); > > There seems to be an existing bug in the endio function for subpage. > > We should call btrfs_page_clear_uptodate() helper. > Or we subpage uptodate bitmap and the page uptodate flag would get out of > sync. Indeed. I'll add a patch to fix this to the beginning of the series for the next round. >> + if (fs_info->nodesize < PAGE_SIZE) >> + btrfs_subpage_clear_writeback(fs_info, page, eb->start, >> + eb->len); >> + else >> + end_page_writeback(page); > > > Here we can just call btrfs_clear_writeback(), which can handle both > subpage and regular cases. Ok.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 1d7f48190ee9b9..16522bcf5d4a10 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -1663,13 +1663,11 @@ static bool lock_extent_buffer_for_io(struct extent_buffer *eb, return ret; } -static void set_btree_ioerr(struct page *page, struct extent_buffer *eb) +static void set_btree_ioerr(struct extent_buffer *eb) { struct btrfs_fs_info *fs_info = eb->fs_info; - btrfs_page_set_error(fs_info, page, eb->start, eb->len); - if (test_and_set_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags)) - return; + set_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags); /* * A read may stumble upon this buffer later, make sure that it gets an @@ -1683,7 +1681,7 @@ static void set_btree_ioerr(struct page *page, struct extent_buffer *eb) * return a 0 because we are readonly if we don't modify the err seq for * the superblock. */ - mapping_set_error(page->mapping, -EIO); + mapping_set_error(eb->fs_info->btree_inode->i_mapping, -EIO); /* * If writeback for a btree extent that doesn't belong to a log tree @@ -1758,101 +1756,37 @@ static struct extent_buffer *find_extent_buffer_nolock( return NULL; } -/* - * The endio function for subpage extent buffer write. - * - * Unlike end_bio_extent_buffer_writepage(), we only call end_page_writeback() - * after all extent buffers in the page has finished their writeback. - */ -static void end_bio_subpage_eb_writepage(struct btrfs_bio *bbio) +static void extent_buffer_write_end_io(struct btrfs_bio *bbio) { - struct bio *bio = &bbio->bio; - struct btrfs_fs_info *fs_info; - struct bio_vec *bvec; + struct extent_buffer *eb = bbio->private; + struct btrfs_fs_info *fs_info = eb->fs_info; + bool uptodate = !bbio->bio.bi_status; struct bvec_iter_all iter_all; - - fs_info = btrfs_sb(bio_first_page_all(bio)->mapping->host->i_sb); - ASSERT(fs_info->nodesize < PAGE_SIZE); - - ASSERT(!bio_flagged(bio, BIO_CLONED)); - bio_for_each_segment_all(bvec, bio, iter_all) { - struct page *page = bvec->bv_page; - u64 bvec_start = page_offset(page) + bvec->bv_offset; - u64 bvec_end = bvec_start + bvec->bv_len - 1; - u64 cur_bytenr = bvec_start; - - ASSERT(IS_ALIGNED(bvec->bv_len, fs_info->nodesize)); - - /* Iterate through all extent buffers in the range */ - while (cur_bytenr <= bvec_end) { - struct extent_buffer *eb; - int done; - - /* - * Here we can't use find_extent_buffer(), as it may - * try to lock eb->refs_lock, which is not safe in endio - * context. - */ - eb = find_extent_buffer_nolock(fs_info, cur_bytenr); - ASSERT(eb); - - cur_bytenr = eb->start + eb->len; - - ASSERT(test_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags)); - done = atomic_dec_and_test(&eb->io_pages); - ASSERT(done); - - if (bio->bi_status || - test_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags)) { - ClearPageUptodate(page); - set_btree_ioerr(page, eb); - } - - btrfs_subpage_clear_writeback(fs_info, page, eb->start, - eb->len); - end_extent_buffer_writeback(eb); - /* - * free_extent_buffer() will grab spinlock which is not - * safe in endio context. Thus here we manually dec - * the ref. - */ - atomic_dec(&eb->refs); - } - } - bio_put(bio); -} - -static void end_bio_extent_buffer_writepage(struct btrfs_bio *bbio) -{ - struct bio *bio = &bbio->bio; struct bio_vec *bvec; - struct extent_buffer *eb; - int done; - struct bvec_iter_all iter_all; - ASSERT(!bio_flagged(bio, BIO_CLONED)); - bio_for_each_segment_all(bvec, bio, iter_all) { + if (!uptodate) + set_btree_ioerr(eb); + + bio_for_each_segment_all(bvec, &bbio->bio, iter_all) { struct page *page = bvec->bv_page; - eb = (struct extent_buffer *)page->private; - BUG_ON(!eb); - done = atomic_dec_and_test(&eb->io_pages); + atomic_dec(&eb->io_pages); - if (bio->bi_status || - test_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags)) { + if (!uptodate) { ClearPageUptodate(page); - set_btree_ioerr(page, eb); + btrfs_page_set_error(fs_info, page, eb->start, eb->len); } - end_page_writeback(page); - - if (!done) - continue; + if (fs_info->nodesize < PAGE_SIZE) + btrfs_subpage_clear_writeback(fs_info, page, eb->start, + eb->len); + else + end_page_writeback(page); - end_extent_buffer_writeback(eb); } + end_extent_buffer_writeback(eb); - bio_put(bio); + bio_put(&bbio->bio); } static void prepare_eb_write(struct extent_buffer *eb) @@ -1911,7 +1845,7 @@ static void write_one_subpage_eb(struct extent_buffer *eb, bbio = btrfs_bio_alloc(INLINE_EXTENT_BUFFER_PAGES, REQ_OP_WRITE | REQ_META | wbc_to_write_flags(wbc), BTRFS_I(eb->fs_info->btree_inode), - end_bio_subpage_eb_writepage, NULL); + extent_buffer_write_end_io, eb); bbio->bio.bi_iter.bi_sector = eb->start >> SECTOR_SHIFT; bbio->file_offset = eb->start; __bio_add_page(&bbio->bio, page, eb->len, eb->start - page_offset(page)); @@ -1937,7 +1871,7 @@ static noinline_for_stack void write_one_eb(struct extent_buffer *eb, bbio = btrfs_bio_alloc(INLINE_EXTENT_BUFFER_PAGES, REQ_OP_WRITE | REQ_META | wbc_to_write_flags(wbc), BTRFS_I(eb->fs_info->btree_inode), - end_bio_extent_buffer_writepage, NULL); + extent_buffer_write_end_io, eb); bbio->bio.bi_iter.bi_sector = eb->start >> SECTOR_SHIFT; bbio->file_offset = eb->start;
Now that we always use a single bio to write an extent_buffer, the buffer can be passed to the end_io handler as private data. This allows to simplify the metadata write end I/O handler, and merge the subpage version into the main one with just a single conditional. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/extent_io.c | 112 +++++++++---------------------------------- 1 file changed, 23 insertions(+), 89 deletions(-)