Re: [PATCH] block: don't use for-inside-for in bio_for_each_segment_all

[Matthew Wilcox <willy@infradead.org>]

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.