| Message ID | 20230301134244.1378533-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] btrfs: remove unused members from struct btrfs_encoded_read_private | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On 2023/3/1 21:42, Christoph Hellwig wrote: > btrfs_encoded_read_regular_fill_pages has a pretty odd control flow. > Unwind it so that there is a single loop over the pages array. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/btrfs/inode.c | 51 ++++++++++++++++++++++-------------------------- > 1 file changed, 23 insertions(+), 28 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 9ad0d181c7082a..431b6082ab3d83 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -9959,39 +9959,34 @@ int btrfs_encoded_read_regular_fill_pages(struct btrfs_inode *inode, > .pending = ATOMIC_INIT(1), > }; > unsigned long i = 0; > - u64 cur = 0; > + struct bio *bio; > > init_waitqueue_head(&priv.wait); > - /* Submit bios for the extent, splitting due to bio limits as necessary. */ > - while (cur < disk_io_size) { > - struct bio *bio = NULL; > - u64 remaining = disk_io_size - cur; > - > - while (bio || remaining) { > - size_t bytes = min_t(u64, remaining, PAGE_SIZE); > - > - if (!bio) { > - bio = btrfs_bio_alloc(BIO_MAX_VECS, REQ_OP_READ, > - inode, > - btrfs_encoded_read_endio, > - &priv); > - bio->bi_iter.bi_sector = > - (disk_bytenr + cur) >> SECTOR_SHIFT; > - } > > - if (!bytes || > - bio_add_page(bio, pages[i], bytes, 0) < bytes) { > - atomic_inc(&priv.pending); > - btrfs_submit_bio(bio, 0); > - bio = NULL; > - continue; > - } > + bio = btrfs_bio_alloc(BIO_MAX_VECS, REQ_OP_READ, inode, > + btrfs_encoded_read_endio, &priv); > + bio->bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT; Can't we even merge the bio allocation into the main loop? Maybe something like this instead? struct bio *bio = NULL; while (cur < len) { if (!bio) { bio = btrfs_io_alloc(); bio->bi_iter.bi_sector = (cur + orig_disk_bytenr) >> SECTOR_SHIFT; } if (bio_add_page() < bytes) { btrfs_submit_bio(); bio = NULL; } cur += bytes; } Thanks, Qu > > - i++; > - cur += bytes; > - remaining -= bytes; > + do { > + size_t bytes = min_t(u64, disk_io_size, PAGE_SIZE); > + > + if (bio_add_page(bio, pages[i], bytes, 0) < bytes) { > + atomic_inc(&priv.pending); > + btrfs_submit_bio(bio, 0); > + > + bio = btrfs_bio_alloc(BIO_MAX_VECS, REQ_OP_READ, inode, > + btrfs_encoded_read_endio, &priv); > + bio->bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT; > + continue; > } > - } > + > + i++; > + disk_bytenr += bytes; > + disk_io_size -= bytes; > + } while (disk_io_size); > + > + atomic_inc(&priv.pending); > + btrfs_submit_bio(bio, 0); > > if (atomic_dec_return(&priv.pending)) > io_wait_event(priv.wait, !atomic_read(&priv.pending));
On 01/03/2023 21:42, Christoph Hellwig wrote: > btrfs_encoded_read_regular_fill_pages has a pretty odd control flow. > Unwind it so that there is a single loop over the pages array. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Anand Jain <anand.jain@oracle.com>
On Fri, Mar 03, 2023 at 07:24:29AM +0800, Qu Wenruo wrote: > Can't we even merge the bio allocation into the main loop? > > Maybe something like this instead? That should work. I personally prefer the version that I sent out, though.
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 9ad0d181c7082a..431b6082ab3d83 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -9959,39 +9959,34 @@ int btrfs_encoded_read_regular_fill_pages(struct btrfs_inode *inode, .pending = ATOMIC_INIT(1), }; unsigned long i = 0; - u64 cur = 0; + struct bio *bio; init_waitqueue_head(&priv.wait); - /* Submit bios for the extent, splitting due to bio limits as necessary. */ - while (cur < disk_io_size) { - struct bio *bio = NULL; - u64 remaining = disk_io_size - cur; - - while (bio || remaining) { - size_t bytes = min_t(u64, remaining, PAGE_SIZE); - - if (!bio) { - bio = btrfs_bio_alloc(BIO_MAX_VECS, REQ_OP_READ, - inode, - btrfs_encoded_read_endio, - &priv); - bio->bi_iter.bi_sector = - (disk_bytenr + cur) >> SECTOR_SHIFT; - } - if (!bytes || - bio_add_page(bio, pages[i], bytes, 0) < bytes) { - atomic_inc(&priv.pending); - btrfs_submit_bio(bio, 0); - bio = NULL; - continue; - } + bio = btrfs_bio_alloc(BIO_MAX_VECS, REQ_OP_READ, inode, + btrfs_encoded_read_endio, &priv); + bio->bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT; - i++; - cur += bytes; - remaining -= bytes; + do { + size_t bytes = min_t(u64, disk_io_size, PAGE_SIZE); + + if (bio_add_page(bio, pages[i], bytes, 0) < bytes) { + atomic_inc(&priv.pending); + btrfs_submit_bio(bio, 0); + + bio = btrfs_bio_alloc(BIO_MAX_VECS, REQ_OP_READ, inode, + btrfs_encoded_read_endio, &priv); + bio->bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT; + continue; } - } + + i++; + disk_bytenr += bytes; + disk_io_size -= bytes; + } while (disk_io_size); + + atomic_inc(&priv.pending); + btrfs_submit_bio(bio, 0); if (atomic_dec_return(&priv.pending)) io_wait_event(priv.wait, !atomic_read(&priv.pending));
btrfs_encoded_read_regular_fill_pages has a pretty odd control flow. Unwind it so that there is a single loop over the pages array. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/inode.c | 51 ++++++++++++++++++++++-------------------------- 1 file changed, 23 insertions(+), 28 deletions(-)