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)))
...
> @@ -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?
> +/**
> + * 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?
> + */
> +void blk_mq_all_tag_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,
> + true);
> + bt_tags_for_each(tags, &tags->bitmap_tags, fn, priv, false, true);
> }
Same comment here: how about inserting /*reserved=*/ and
/*iterate_all=*/ comments in the bt_tags_for_each() call?
Otherwise this proposal looks good to me.
Thanks,
Bart.
