| Message ID | 20250409111055.3640328-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/8] btrfs: remove the alignment checks in end_bbio_data_read | expand |
在 2025/4/9 20:40, Christoph Hellwig 写道: > end_bbio_data_read checks that each iterated folio fragment is aligned > and justifies that with block drivers advancing the bio. But block > driver only advance bi_iter, while end_bbio_data_read uses > bio_for_each_folio_all to iterate the immutable bi_io_vec array that > can't be changed by drivers at all. The old comment indeed is incorrect, but can we still leave an ASSERT() just in case to case any unaligned submission? It shouldn't cause anything different for end users, but should greatly improve the life of quality for developers. Thanks, Qu> > Remove the alignment checking and the misleading comment. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/btrfs/extent_io.c | 27 +++------------------------ > 1 file changed, 3 insertions(+), 24 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 197f5e51c474..193736b07a0b 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -512,43 +512,22 @@ static void end_bbio_data_read(struct btrfs_bio *bbio) > struct btrfs_fs_info *fs_info = bbio->fs_info; > struct bio *bio = &bbio->bio; > struct folio_iter fi; > - const u32 sectorsize = fs_info->sectorsize; > > ASSERT(!bio_flagged(bio, BIO_CLONED)); > bio_for_each_folio_all(fi, &bbio->bio) { > bool uptodate = !bio->bi_status; > struct folio *folio = fi.folio; > struct inode *inode = folio->mapping->host; > - u64 start; > - u64 end; > - u32 len; > + u64 start = folio_pos(folio) + fi.offset; > > btrfs_debug(fs_info, > "%s: bi_sector=%llu, err=%d, mirror=%u", > __func__, bio->bi_iter.bi_sector, bio->bi_status, > bbio->mirror_num); > > - /* > - * We always issue full-sector reads, but if some block in a > - * folio fails to read, blk_update_request() will advance > - * bv_offset and adjust bv_len to compensate. Print a warning > - * for unaligned offsets, and an error if they don't add up to > - * a full sector. > - */ > - if (!IS_ALIGNED(fi.offset, sectorsize)) > - btrfs_err(fs_info, > - "partial page read in btrfs with offset %zu and length %zu", > - fi.offset, fi.length); > - else if (!IS_ALIGNED(fi.offset + fi.length, sectorsize)) > - btrfs_info(fs_info, > - "incomplete page read with offset %zu and length %zu", > - fi.offset, fi.length); > - > - start = folio_pos(folio) + fi.offset; > - end = start + fi.length - 1; > - len = fi.length; > > if (likely(uptodate)) { > + u64 end = start + fi.length - 1; > loff_t i_size = i_size_read(inode); > > /* > @@ -573,7 +552,7 @@ static void end_bbio_data_read(struct btrfs_bio *bbio) > } > > /* Update page status and unlock. */ > - end_folio_read(folio, uptodate, start, len); > + end_folio_read(folio, uptodate, start, fi.length); > } > bio_put(bio); > }
On Thu, Apr 10, 2025 at 07:43:54AM +0930, Qu Wenruo wrote: > The old comment indeed is incorrect, but can we still leave an ASSERT() > just in case to case any unaligned submission? > > It shouldn't cause anything different for end users, but should greatly > improve the life of quality for developers. What is is trying to check for? fi.offset isn't really a meaningful value to check for as it's just an offset into the folio. bi_sector would be useful, but it's not owned by the file system after the bio was submitted. Maybe adding that assert to the submission path would make sense? Remember that the bio completed is exactly the one submitted by the file system, and cloned split bio never reaches back into the file systems.
在 2025/4/10 15:00, Christoph Hellwig 写道: > On Thu, Apr 10, 2025 at 07:43:54AM +0930, Qu Wenruo wrote: >> The old comment indeed is incorrect, but can we still leave an ASSERT() >> just in case to case any unaligned submission? >> >> It shouldn't cause anything different for end users, but should greatly >> improve the life of quality for developers. > > What is is trying to check for? fi.offset isn't really a meaningful value > to check for as it's just an offset into the folio. bi_sector would be > useful, but it's not owned by the file system after the bio was submitted. > Maybe adding that assert to the submission path would make sense? Remember > that the bio completed is exactly the one submitted by the file system, > and cloned split bio never reaches back into the file systems. OK, it makes more sense, and we already have the alignment checks in the write path (submit_one_sector()). Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 197f5e51c474..193736b07a0b 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -512,43 +512,22 @@ static void end_bbio_data_read(struct btrfs_bio *bbio) struct btrfs_fs_info *fs_info = bbio->fs_info; struct bio *bio = &bbio->bio; struct folio_iter fi; - const u32 sectorsize = fs_info->sectorsize; ASSERT(!bio_flagged(bio, BIO_CLONED)); bio_for_each_folio_all(fi, &bbio->bio) { bool uptodate = !bio->bi_status; struct folio *folio = fi.folio; struct inode *inode = folio->mapping->host; - u64 start; - u64 end; - u32 len; + u64 start = folio_pos(folio) + fi.offset; btrfs_debug(fs_info, "%s: bi_sector=%llu, err=%d, mirror=%u", __func__, bio->bi_iter.bi_sector, bio->bi_status, bbio->mirror_num); - /* - * We always issue full-sector reads, but if some block in a - * folio fails to read, blk_update_request() will advance - * bv_offset and adjust bv_len to compensate. Print a warning - * for unaligned offsets, and an error if they don't add up to - * a full sector. - */ - if (!IS_ALIGNED(fi.offset, sectorsize)) - btrfs_err(fs_info, - "partial page read in btrfs with offset %zu and length %zu", - fi.offset, fi.length); - else if (!IS_ALIGNED(fi.offset + fi.length, sectorsize)) - btrfs_info(fs_info, - "incomplete page read with offset %zu and length %zu", - fi.offset, fi.length); - - start = folio_pos(folio) + fi.offset; - end = start + fi.length - 1; - len = fi.length; if (likely(uptodate)) { + u64 end = start + fi.length - 1; loff_t i_size = i_size_read(inode); /* @@ -573,7 +552,7 @@ static void end_bbio_data_read(struct btrfs_bio *bbio) } /* Update page status and unlock. */ - end_folio_read(folio, uptodate, start, len); + end_folio_read(folio, uptodate, start, fi.length); } bio_put(bio); }
end_bbio_data_read checks that each iterated folio fragment is aligned and justifies that with block drivers advancing the bio. But block driver only advance bi_iter, while end_bbio_data_read uses bio_for_each_folio_all to iterate the immutable bi_io_vec array that can't be changed by drivers at all. Remove the alignment checking and the misleading comment. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/extent_io.c | 27 +++------------------------ 1 file changed, 3 insertions(+), 24 deletions(-)