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

[Hannes Reinecke <hare@suse.de>]

On 4/7/19 8:52 AM, Christoph Hellwig wrote:
> The change itself looks fine to be, but a few comments on semingly
> unrelated changes:
> 
>> +#define bio_for_each_segment_all(bvl, bio, i, iter_all)			\
>> +	for (i = 0, bvl = bvec_init_iter_all(&iter_all);		\
>> +		iter_all.idx < (bio)->bi_vcnt &&			\
>> +		(mp_bvec_advance(&((bio)->bi_io_vec[iter_all.idx]),	\
>> +				 &iter_all), 1); i++)
> 
> Instead of the complicated expression inside the for loop test
> expression can't we move the index check into mp_bvec_advance and let
> it return a bool?
> 
>> diff --git a/include/linux/bvec.h b/include/linux/bvec.h
>> index f6275c4da13a..6e4996dfc847 100644
>> --- a/include/linux/bvec.h
>> +++ b/include/linux/bvec.h
>> @@ -48,7 +48,7 @@ struct bvec_iter {
>>   struct bvec_iter_all {
>>   	struct bio_vec	bv;
>>   	int		idx;
>> -	unsigned	done;
>> +	unsigned	bv_done;
> 
> Why the rename here?
> 
>> +static inline void mp_bvec_advance(const struct bio_vec *bvec,
>> +				   struct bvec_iter_all *iter_all)
> 
> If we rename this we should probably drop the mp_ prefix..
> 
> Also not for this patch, but we should really consider moving this
> function out of line given how big it is.
> 
> 
> Here is how I'd do this with my slight changes:
> 
> 
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index bb915591557b..fd7629ac9f11 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -120,19 +120,42 @@ static inline bool bio_full(struct bio *bio)
>   	return bio->bi_vcnt >= bio->bi_max_vecs;
>   }
>   
> -#define mp_bvec_for_each_segment(bv, bvl, i, iter_all)			\
> -	for (bv = bvec_init_iter_all(&iter_all);			\
> -		(iter_all.done < (bvl)->bv_len) &&			\
> -		(mp_bvec_next_segment((bvl), &iter_all), 1);		\
> -		iter_all.done += bv->bv_len, i += 1)
> +static inline bool __bio_next_segment_all(struct bio *bio,
> +		struct bvec_iter_all *iter_all)
> +{
> +	struct bio_vec *bvec = &bio->bi_io_vec[iter_all->idx];
> +	struct bio_vec *bv = &iter_all->bv;
> +
> +	if (iter_all->idx >= bio->bi_vcnt)
> +		return false;
> +
> +	if (iter_all->done) {
> +		bv->bv_page = nth_page(bv->bv_page, 1);
> +		bv->bv_offset = 0;
> +	} else {
> +		bv->bv_page = bvec->bv_page;
> +		bv->bv_offset = bvec->bv_offset;
> +	}
> +	bv->bv_len = min_t(unsigned int, PAGE_SIZE - bv->bv_offset,
> +			   bvec->bv_len - iter_all->done);
> +	iter_all->done += bv->bv_len;
> +
> +	if (iter_all->done == bvec->bv_len) {
> +		iter_all->idx++;
> +		iter_all->done = 0;
> +	}
> +
> +	return true;
> +}
>   
>   /*
>    * drivers should _never_ use the all version - the bio may have been split
>    * before it got to the driver and the driver won't own all of it
>    */
> -#define bio_for_each_segment_all(bvl, bio, i, iter_all)		\
> -	for (i = 0, iter_all.idx = 0; iter_all.idx < (bio)->bi_vcnt; iter_all.idx++)	\
> -		mp_bvec_for_each_segment(bvl, &((bio)->bi_io_vec[iter_all.idx]), i, iter_all)
> +#define bio_for_each_segment_all(bvl, bio, i, iter_all)			\
> +	for (i = 0, bvl = bvec_init_iter_all(&iter_all);		\
> +	     __bio_next_segment_all(bio, &iter_all);			\
> +	     i++)
>   
>   static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
>   				    unsigned bytes)
> diff --git a/include/linux/bvec.h b/include/linux/bvec.h
> index f6275c4da13a..bcebfc522498 100644
> --- a/include/linux/bvec.h
> +++ b/include/linux/bvec.h
> @@ -145,28 +145,12 @@ static inline bool bvec_iter_advance(const struct bio_vec *bv,
>   
>   static inline struct bio_vec *bvec_init_iter_all(struct bvec_iter_all *iter_all)
>   {
> -	iter_all->bv.bv_page = NULL;
>   	iter_all->done = 0;
> +	iter_all->idx = 0;
>   
>   	return &iter_all->bv;
>   }
>   
> -static inline void mp_bvec_next_segment(const struct bio_vec *bvec,
> -					struct bvec_iter_all *iter_all)
> -{
> -	struct bio_vec *bv = &iter_all->bv;
> -
> -	if (bv->bv_page) {
> -		bv->bv_page = nth_page(bv->bv_page, 1);
> -		bv->bv_offset = 0;
> -	} else {
> -		bv->bv_page = bvec->bv_page;
> -		bv->bv_offset = bvec->bv_offset;
> -	}
> -	bv->bv_len = min_t(unsigned int, PAGE_SIZE - bv->bv_offset,
> -			   bvec->bv_len - iter_all->done);
> -}
> -
>   /*
>    * Get the last single-page segment from the multi-page bvec and store it
>    * in @seg
> 
Oh yes, please do.
The macros are fast becoming unparseable :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)