Re: [PATCH V2 2/3] blk-mq: complete request locally if the completion is from tagset iterator

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

On Tue, Apr 27, 2021 at 03:06:51PM +0800, Ming Lei wrote:
> On Mon, Apr 26, 2021 at 07:30:51PM -0700, Bart Van Assche wrote:
> > On 4/26/21 6:45 PM, Ming Lei wrote:
> > > diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
> > > index 100fa44d52a6..773aea4db90c 100644
> > > --- a/block/blk-mq-tag.c
> > > +++ b/block/blk-mq-tag.c
> > > @@ -284,8 +284,11 @@ static bool bt_tags_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
> > >  	if ((iter_data->flags & BT_TAG_ITER_STARTED) &&
> > >  	    !blk_mq_request_started(rq))
> > >  		ret = true;
> > > -	else
> > > +	else {
> > > +		rq->rq_flags |= RQF_ITERATING;
> > >  		ret = iter_data->fn(rq, iter_data->data, reserved);
> > > +		rq->rq_flags &= ~RQF_ITERATING;
> > > +	}
> > >  	if (!iter_static_rqs)
> > >  		blk_mq_put_rq_ref(rq);
> > >  	return ret;
> > 
> > All existing rq->rq_flags modifications are serialized. The above change
> > adds code that may change rq_flags concurrently with regular request
> > processing. I think that counts as a race condition.
> 
> Good catch, but we still can change .rq_flags via atomic op, such as:
> 
> 	do {
> 		old = rq->rq_flags;
> 		new = old | RQF_ITERATING;
> 	} while (cmpxchg(&rq->rq_flags, old, new) != old);

oops, this way can't work because other .rq_flags modification still may
clear RQF_ITERATING.

As I mentioned in another thread, blk-mq needn't to consider the double
completion[1] any more, which is covered by driver. blk-mq just needs to
make sure that valid request is passed to ->fn(), and it is driver's
responsibility to avoid double completion.

[1] https://lore.kernel.org/linux-block/YIdWz8C5eklPvEiV@T590/T/#u


Thanks,
Ming