Re: [PATCH V10 05/11] blk-mq: support rq filter callback when iterating rqs

[Ming Lei <ming.lei@redhat.com>]

On Fri, May 08, 2020 at 08:08:23PM -0700, Bart Van Assche wrote:
> On 2020-05-08 19:05, Ming Lei wrote:
> > Fine, then we can save one callback, how about the following way?
> > 
> > diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
> > index 586c9d6e904a..5e9c743d887b 100644
> > --- a/block/blk-mq-tag.c
> > +++ b/block/blk-mq-tag.c
> > @@ -257,6 +257,7 @@ struct bt_tags_iter_data {
> >  	busy_tag_iter_fn *fn;
> >  	void *data;
> >  	bool reserved;
> > +	bool iterate_all;
> >  };
> >  
> >  static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
> > @@ -274,8 +275,10 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
> >  	 * test and set the bit before assining ->rqs[].
> >  	 */
> >  	rq = tags->rqs[bitnr];
> > -	if (rq && blk_mq_request_started(rq))
> > -		return iter_data->fn(rq, iter_data->data, reserved);
> > +	if (rq) {
> > +		if (iter_data->iterate_all || blk_mq_request_started(rq))
> > +			return iter_data->fn(rq, iter_data->data, reserved);
> > +	}
> 
> How about combining the two if-statements above in the following single
> if-statement:
> 
> if (rq && (iter_data->iterate_all || blk_mq_request_started(rq)))
> 	...

OK.

> 
> > @@ -321,8 +326,30 @@ static void blk_mq_all_tag_busy_iter(struct blk_mq_tags *tags,
> >  		busy_tag_iter_fn *fn, void *priv)
> >  {
> >  	if (tags->nr_reserved_tags)
> > -		bt_tags_for_each(tags, &tags->breserved_tags, fn, priv, true);
> > -	bt_tags_for_each(tags, &tags->bitmap_tags, fn, priv, false);
> > +		bt_tags_for_each(tags, &tags->breserved_tags, fn, priv, true,
> > +				 false);
> > +	bt_tags_for_each(tags, &tags->bitmap_tags, fn, priv, false, false);
> > +}
> 
> How about inserting comments like /*reserved=*/ and /*iterate_all=*/ in
> the bt_tags_for_each() call in front of "false" to make these calls
> easier to read?

I think it isn't necessary, given both two are self-documented from
the name of bt_tags_for_each's parameters.

> 
> > +/**
> > + * blk_mq_all_tag_iter - iterate over all requests in a tag map
> > + * @tags:	Tag map to iterate over.
> > + * @fn:		Pointer to the function that will be called for each
> > + *		request. @fn will be called as follows: @fn(rq, @priv,
> > + *		reserved) where rq is a pointer to a request. 'reserved'
> > + *		indicates whether or not @rq is a reserved request. Return
> > + *		true to continue iterating tags, false to stop.
> > + * @priv:	Will be passed as second argument to @fn.
> > + *
> > + * It is the caller's responsility to check rq's state in @fn.
>                          ^^^^^^^^^^^^
>                          responsibility?

OK.

 
Thanks,
Ming