| Message ID | 20230227151704.1224688-7-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/12] btrfs: don't set force_bio_submit in read_extent_buffer_subpage | expand |
On 27.02.23 16:17, Christoph Hellwig wrote: > + if (bio_ctrl->compress_type != this_bio_flag) > + submit_one_bio(bio_ctrl); > + > if (force_bio_submit) > submit_one_bio(bio_ctrl); Sorry for not having noticed this earlier. But this looks odd TBH. if (bio_ctrl->compress_type != this_bio_flag || force_bio_submit) submit_one_bio(bio_ctrl); looks better IMHO. Or as it changes a bit in 8/12 maybe: if (bio_ctrl->compress_type != this_bio_flag) force_bio_submit = true; if (force_bio_submit) submit_one_bio(bio_ctrl);
On Mon, Feb 27, 2023 at 04:20:54PM +0000, Johannes Thumshirn wrote: > On 27.02.23 16:17, Christoph Hellwig wrote: > > + if (bio_ctrl->compress_type != this_bio_flag) > > + submit_one_bio(bio_ctrl); > > + > > if (force_bio_submit) > > submit_one_bio(bio_ctrl); > > > Sorry for not having noticed this earlier. But this looks odd TBH. > > if (bio_ctrl->compress_type != this_bio_flag || > force_bio_submit) > submit_one_bio(bio_ctrl); It does. But with patch 8 it would stop working, as we must uptdate the compress_type field after submitting the bio for that case. Been there, done that and it broke :)
On 28.02.23 03:58, Christoph Hellwig wrote: > On Mon, Feb 27, 2023 at 04:20:54PM +0000, Johannes Thumshirn wrote: >> On 27.02.23 16:17, Christoph Hellwig wrote: >>> + if (bio_ctrl->compress_type != this_bio_flag) >>> + submit_one_bio(bio_ctrl); >>> + >>> if (force_bio_submit) >>> submit_one_bio(bio_ctrl); >> >> >> Sorry for not having noticed this earlier. But this looks odd TBH. >> >> if (bio_ctrl->compress_type != this_bio_flag || >> force_bio_submit) >> submit_one_bio(bio_ctrl); > > It does. But with patch 8 it would stop working, as we must > uptdate the compress_type field after submitting the bio for that case. > > Been there, done that and it broke :) > Sh*t, then at least let's document that with a comment. Otherwise someone will see the code and try to clean it up and possibly break it.
On Tue, Feb 28, 2023 at 09:02:42AM +0000, Johannes Thumshirn wrote: > Sh*t, then at least let's document that with a comment. Otherwise someone > will see the code and try to clean it up and possibly break it. I'll literally blow up on the first compression xfstests.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 1de56abd7308c7..eac31f212069a0 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -875,7 +875,6 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array) * a contiguous page to the previous one * @size: portion of page that we want to write * @pg_offset: starting offset in the page - * @compress_type: compression type of the current bio to see if we can merge them * * Attempt to add a page to bio considering stripe alignment etc. * @@ -886,8 +885,7 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array) static int btrfs_bio_add_page(struct btrfs_bio_ctrl *bio_ctrl, struct page *page, u64 disk_bytenr, unsigned int size, - unsigned int pg_offset, - enum btrfs_compression_type compress_type) + unsigned int pg_offset) { struct bio *bio = bio_ctrl->bio; u32 bio_size = bio->bi_iter.bi_size; @@ -898,9 +896,6 @@ static int btrfs_bio_add_page(struct btrfs_bio_ctrl *bio_ctrl, ASSERT(bio); /* The limit should be calculated when bio_ctrl->bio is allocated */ ASSERT(bio_ctrl->len_to_oe_boundary); - if (bio_ctrl->compress_type != compress_type) - return 0; - if (bio->bi_iter.bi_size == 0) { /* We can always add a page into an empty bio. */ @@ -1049,12 +1044,11 @@ static int submit_extent_page(struct btrfs_bio_ctrl *bio_ctrl, */ if (compress_type != BTRFS_COMPRESS_NONE) added = btrfs_bio_add_page(bio_ctrl, page, disk_bytenr, - size - offset, pg_offset + offset, - compress_type); + size - offset, pg_offset + offset); else added = btrfs_bio_add_page(bio_ctrl, page, disk_bytenr + offset, size - offset, - pg_offset + offset, compress_type); + pg_offset + offset); /* Metadata page range should never be split */ if (!is_data_inode(&inode->vfs_inode)) @@ -1320,6 +1314,9 @@ static int btrfs_do_readpage(struct page *page, struct extent_map **em_cached, continue; } + if (bio_ctrl->compress_type != this_bio_flag) + submit_one_bio(bio_ctrl); + if (force_bio_submit) submit_one_bio(bio_ctrl); ret = submit_extent_page(bio_ctrl, disk_bytenr, page, iosize,
The compress_type can only change on a per-extent basis. So instead of checking it for every page in btrfs_bio_add_page, do the check once in btrfs_do_readpage, which is the only caller of btrfs_bio_add_page and submit_extent_page that deals with compressed extents. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/extent_io.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)