On Tue, Apr 09, 2019 at 11:48:39AM +0200, Christoph Hellwig wrote:
> On Mon, Apr 08, 2019 at 07:12:04AM -0700, Matthew Wilcox wrote:
> > On Mon, Apr 08, 2019 at 08:07:55AM +0200, Hannes Reinecke wrote:
> > > Oh yes, please do.
> > > The macros are fast becoming unparseable :-)
> >
> > I think something that would help is removing the mandatory 'i' parameter
> > to this iterator. Most of the users don't use it, and the ones that do
> > can implement it themselves. eg:
>
> Yeah, I quickly hacked this up during a meeting yesterday and we have
> exactly two users of the iterator. Patch against the for-linus tree
three in the below patch ...
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index 64def336f053..aa793fef52eb 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
> @@ -429,14 +429,16 @@ static void do_btree_node_write(struct btree *b)
> bset_sector_offset(&b->keys, i));
>
> if (!bch_bio_alloc_pages(b->bio, __GFP_NOWARN|GFP_NOWAIT)) {
> - int j;
> + int j = 0;
> struct bio_vec *bv;
> void *base = (void *) ((unsigned long) i & ~(PAGE_SIZE - 1));
> struct bvec_iter_all iter_all;
>
> - bio_for_each_segment_all(bv, b->bio, j, iter_all)
> + bio_for_each_segment_all(bv, b->bio, iter_all) {
> memcpy(page_address(bv->bv_page),
> base + j * PAGE_SIZE, PAGE_SIZE);
> + j++;
> + }
I think this one works better to replace 'base' with 'addr':
@@ -429,14 +429,14 @@ static void do_btree_node_write(struct btree *b)
bset_sector_offset(&b->keys, i));
if (!bch_bio_alloc_pages(b->bio, __GFP_NOWARN|GFP_NOWAIT)) {
- int j;
struct bio_vec *bv;
- void *base = (void *) ((unsigned long) i & ~(PAGE_SIZE - 1));
+ void *addr = (void *) ((unsigned long) i & ~(PAGE_SIZE - 1));
struct bvec_iter_all iter_all;
- bio_for_each_segment_all(bv, b->bio, j, iter_all)
- memcpy(page_address(bv->bv_page),
- base + j * PAGE_SIZE, PAGE_SIZE);
+ bio_for_each_segment_all(bv, b->bio, iter_all) {
+ memcpy(page_address(bv->bv_page), addr, PAGE_SIZE);
+ addr += PAGE_SIZE;
+ }
bch_submit_bbio(b->bio, b->c, &k.key, 0);
> +++ b/drivers/md/raid1.c
> @@ -2110,7 +2110,7 @@ static void process_checks(struct r1bio *r1_bio)
> }
> r1_bio->read_disk = primary;
> for (i = 0; i < conf->raid_disks * 2; i++) {
> - int j;
> + int j = 0;
> struct bio *pbio = r1_bio->bios[primary];
> struct bio *sbio = r1_bio->bios[i];
> blk_status_t status = sbio->bi_status;
> @@ -2125,8 +2125,8 @@ static void process_checks(struct r1bio *r1_bio)
> /* Now we can 'fixup' the error value */
> sbio->bi_status = 0;
>
> - bio_for_each_segment_all(bi, sbio, j, iter_all)
> - page_len[j] = bi->bv_len;
> + bio_for_each_segment_all(bi, sbio, iter_all)
> + page_len[j++] = bi->bv_len;
Yes.
> +++ b/fs/btrfs/inode.c
> @@ -7919,7 +7918,7 @@ static void btrfs_retry_endio(struct bio *bio)
> struct bio_vec *bvec;
> int uptodate;
> int ret;
> - int i;
> + int i = 0;
> struct bvec_iter_all iter_all;
>
> if (bio->bi_status)
> @@ -7934,7 +7933,7 @@ static void btrfs_retry_endio(struct bio *bio)
> failure_tree = &BTRFS_I(inode)->io_failure_tree;
>
> ASSERT(!bio_flagged(bio, BIO_CLONED));
> - bio_for_each_segment_all(bvec, bio, i, iter_all) {
> + bio_for_each_segment_all(bvec, bio, iter_all) {
> ret = __readpage_endio_check(inode, io_bio, i, bvec->bv_page,
> bvec->bv_offset, done->start,
> bvec->bv_len);
> @@ -7946,6 +7945,7 @@ static void btrfs_retry_endio(struct bio *bio)
> bvec->bv_offset);
> else
> uptodate = 0;
> + i++;
> }
I'd be tempted to instead:
@@ -7935,7 +7935,7 @@ static void btrfs_retry_endio(struct bio *bio)
ASSERT(!bio_flagged(bio, BIO_CLONED));
bio_for_each_segment_all(bvec, bio, i, iter_all) {
- ret = __readpage_endio_check(inode, io_bio, i, bvec->bv_page,
+ ret = __readpage_endio_check(inode, io_bio, i++, bvec->bv_page,
bvec->bv_offset, done->start,
bvec->bv_len);
if (!ret)
(i is used nowhere else in this loop, and it's a mercifully short loop with
no breaks or continues).
Thanks for turning this musing into a patch.
