| Message ID | 20211201051756.53742-12-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: split bio at btrfs_map_bio() time | expand |
On Wed, Dec 01, 2021 at 01:17:50PM +0800, Qu Wenruo wrote: > For compression read write endio functions, they all rely on > dec_and_test_compressed_bio() to determine if they are the last bio. > > So here we only need to convert the bio_for_each_segment_all() call into > __bio_for_each_segment() so that compression read/write endio functions > will handle both split and unsplit bios well. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/compression.c | 14 +++++--------- > 1 file changed, 5 insertions(+), 9 deletions(-) > > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c > index 8668c5190805..8b4b84b59b0c 100644 > --- a/fs/btrfs/compression.c > +++ b/fs/btrfs/compression.c > @@ -205,18 +205,14 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio, > static bool dec_and_test_compressed_bio(struct compressed_bio *cb, struct bio *bio) > { > struct btrfs_fs_info *fs_info = btrfs_sb(cb->inode->i_sb); > + struct bio_vec bvec; > + struct bvec_iter iter; > unsigned int bi_size = 0; > bool last_io = false; > - struct bio_vec *bvec; > - struct bvec_iter_all iter_all; > > - /* > - * At endio time, bi_iter.bi_size doesn't represent the real bio size. > - * Thus here we have to iterate through all segments to grab correct > - * bio size. > - */ > - bio_for_each_segment_all(bvec, bio, iter_all) > - bi_size += bvec->bv_len; > + ASSERT(btrfs_bio(bio)->iter.bi_size); We're tripping this assert with generic/476 with -o compress, so I assume there's some error condition that isn't being handled properly. Thanks, Josef
On 2022/3/17 03:46, Josef Bacik wrote: > On Wed, Dec 01, 2021 at 01:17:50PM +0800, Qu Wenruo wrote: >> For compression read write endio functions, they all rely on >> dec_and_test_compressed_bio() to determine if they are the last bio. >> >> So here we only need to convert the bio_for_each_segment_all() call into >> __bio_for_each_segment() so that compression read/write endio functions >> will handle both split and unsplit bios well. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/compression.c | 14 +++++--------- >> 1 file changed, 5 insertions(+), 9 deletions(-) >> >> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c >> index 8668c5190805..8b4b84b59b0c 100644 >> --- a/fs/btrfs/compression.c >> +++ b/fs/btrfs/compression.c >> @@ -205,18 +205,14 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio, >> static bool dec_and_test_compressed_bio(struct compressed_bio *cb, struct bio *bio) >> { >> struct btrfs_fs_info *fs_info = btrfs_sb(cb->inode->i_sb); >> + struct bio_vec bvec; >> + struct bvec_iter iter; >> unsigned int bi_size = 0; >> bool last_io = false; >> - struct bio_vec *bvec; >> - struct bvec_iter_all iter_all; >> >> - /* >> - * At endio time, bi_iter.bi_size doesn't represent the real bio size. >> - * Thus here we have to iterate through all segments to grab correct >> - * bio size. >> - */ >> - bio_for_each_segment_all(bvec, bio, iter_all) >> - bi_size += bvec->bv_len; >> + ASSERT(btrfs_bio(bio)->iter.bi_size); > > We're tripping this assert with generic/476 with -o compress, so I assume > there's some error condition that isn't being handled properly. Thanks, Thank you very much for catching it. It turns out the ASSERT() is really helpful to detect uninitialized btrfs_bio::iter. The problem is related to two call sites: - btrfs_submit_compressed_read() - btrfs_submit_compressed_write() The compressed bio doesn't have its iter properly initialized for error path, thus it's causing the problem. Just two new lines and the problem can be fixed. I'll refresh the patchset. Thank you again for catching this, Qu > > Josef
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c index 8668c5190805..8b4b84b59b0c 100644 --- a/fs/btrfs/compression.c +++ b/fs/btrfs/compression.c @@ -205,18 +205,14 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio, static bool dec_and_test_compressed_bio(struct compressed_bio *cb, struct bio *bio) { struct btrfs_fs_info *fs_info = btrfs_sb(cb->inode->i_sb); + struct bio_vec bvec; + struct bvec_iter iter; unsigned int bi_size = 0; bool last_io = false; - struct bio_vec *bvec; - struct bvec_iter_all iter_all; - /* - * At endio time, bi_iter.bi_size doesn't represent the real bio size. - * Thus here we have to iterate through all segments to grab correct - * bio size. - */ - bio_for_each_segment_all(bvec, bio, iter_all) - bi_size += bvec->bv_len; + ASSERT(btrfs_bio(bio)->iter.bi_size); + __bio_for_each_segment(bvec, bio, iter, btrfs_bio(bio)->iter) + bi_size += bvec.bv_len; if (bio->bi_status) cb->errors = 1;
For compression read write endio functions, they all rely on dec_and_test_compressed_bio() to determine if they are the last bio. So here we only need to convert the bio_for_each_segment_all() call into __bio_for_each_segment() so that compression read/write endio functions will handle both split and unsplit bios well. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/compression.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)