Message ID | 20220517145039.3202184-6-hch@lst.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [01/15] btrfs: introduce a pure data checksum checking helper | expand |
On 17/05/2022 16:51, Christoph Hellwig wrote: > +/* > + * Iterate through a btrfs_bio (@bbio) on a per-sector basis. > + */ > +#define btrfs_bio_for_each_sector(fs_info, bvl, bbio, iter, bio_offset) \ > + for ((iter) = (bbio)->iter, (bio_offset) = 0; \ > + (iter).bi_size && \ > + (((bvl) = bio_iter_iovec((&(bbio)->bio), (iter))), 1); \ > + (bio_offset) += fs_info->sectorsize, \ > + bio_advance_iter_single(&(bbio)->bio, &(iter), \ > + (fs_info)->sectorsize)) > + > + This is first used in patch 12 why not move there?
On Tue, May 17, 2022 at 03:27:35PM +0000, Johannes Thumshirn wrote: > On 17/05/2022 16:51, Christoph Hellwig wrote: > > +/* > > + * Iterate through a btrfs_bio (@bbio) on a per-sector basis. > > + */ > > +#define btrfs_bio_for_each_sector(fs_info, bvl, bbio, iter, bio_offset) \ > > + for ((iter) = (bbio)->iter, (bio_offset) = 0; \ > > + (iter).bi_size && \ > > + (((bvl) = bio_iter_iovec((&(bbio)->bio), (iter))), 1); \ > > + (bio_offset) += fs_info->sectorsize, \ > > + bio_advance_iter_single(&(bbio)->bio, &(iter), \ > > + (fs_info)->sectorsize)) > > + > > + > > This is first used in patch 12 why not move there? Because it is a logically separate change that doesn't really have anything to do with the repair code, except that it happens to be the first user?
On 2022/5/18 16:46, Christoph Hellwig wrote: > On Tue, May 17, 2022 at 03:27:35PM +0000, Johannes Thumshirn wrote: >> On 17/05/2022 16:51, Christoph Hellwig wrote: >>> +/* >>> + * Iterate through a btrfs_bio (@bbio) on a per-sector basis. >>> + */ >>> +#define btrfs_bio_for_each_sector(fs_info, bvl, bbio, iter, bio_offset) \ >>> + for ((iter) = (bbio)->iter, (bio_offset) = 0; \ >>> + (iter).bi_size && \ >>> + (((bvl) = bio_iter_iovec((&(bbio)->bio), (iter))), 1); \ >>> + (bio_offset) += fs_info->sectorsize, \ >>> + bio_advance_iter_single(&(bbio)->bio, &(iter), \ >>> + (fs_info)->sectorsize)) >>> + >>> + >> >> This is first used in patch 12 why not move there? > > Because it is a logically separate change that doesn't really have > anything to do with the repair code, except that it happens to be the > first user? In fact, there are some other call sites can benefit from the helper, like btrfs_csum_one_bio(). Thus if you can convert those call sites, there will be no question on introducing the helper in one patch. Thanks, Qu
On Wed, May 18, 2022 at 06:07:38PM +0800, Qu Wenruo wrote: >> Because it is a logically separate change that doesn't really have >> anything to do with the repair code, except that it happens to be the >> first user? > > In fact, there are some other call sites can benefit from the helper, > like btrfs_csum_one_bio(). > > Thus if you can convert those call sites, there will be no question on > introducing the helper in one patch. Yes, there is a bunch of places where it would be pretty useful. But the series is already large enough as is, so for now I'd rather keep that for another round.
On 2022/5/21 00:27, Christoph Hellwig wrote: > On Wed, May 18, 2022 at 06:07:38PM +0800, Qu Wenruo wrote: >>> Because it is a logically separate change that doesn't really have >>> anything to do with the repair code, except that it happens to be the >>> first user? >> >> In fact, there are some other call sites can benefit from the helper, >> like btrfs_csum_one_bio(). >> >> Thus if you can convert those call sites, there will be no question on >> introducing the helper in one patch. > > Yes, there is a bunch of places where it would be pretty useful. But > the series is already large enough as is, so for now I'd rather > keep that for another round. After more thought (refreshing bike riding), considering there are already quite some independent fixes and cleanups, it looks reasonable to me to split the patchset into two parts: 1. Independent cleanups/fixes/refactors Those can be queued for v5.20. And especially for this helper, I'd like to rework a lot of call sites to use this helpers to simplify the code. 2. Real read-time refactor In fact I also want to revive the old initial RFC version, and compare with different versions. Does this sound OK? Thanks, Qu
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index 12b2af9260e92..6f784d4f54664 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -391,6 +391,18 @@ static inline void btrfs_bio_free_csum(struct btrfs_bio *bbio) } } +/* + * Iterate through a btrfs_bio (@bbio) on a per-sector basis. + */ +#define btrfs_bio_for_each_sector(fs_info, bvl, bbio, iter, bio_offset) \ + for ((iter) = (bbio)->iter, (bio_offset) = 0; \ + (iter).bi_size && \ + (((bvl) = bio_iter_iovec((&(bbio)->bio), (iter))), 1); \ + (bio_offset) += fs_info->sectorsize, \ + bio_advance_iter_single(&(bbio)->bio, &(iter), \ + (fs_info)->sectorsize)) + + struct btrfs_io_stripe { struct btrfs_device *dev; u64 physical;