On Thu, 2017-08-03 at 14:40 -0600, Jens Axboe wrote:
> On 08/03/2017 02:35 PM, Jens Axboe wrote:
> > > I agree with what you wrote in the description of this patch.
> > > However, since I have not yet found the code that clears tags->rqs[],
> > > would it be possible to show me that code?
> >
> > Since it's been a month since I wrote this code, I went and looked
> > too. My memory was that we set/clear it dynamically since we added
> > scheduling, but looks like we don't clear it. The race is still valid
> > for when someone runs a tag check in parallel with someone allocating
> > a tag, since there's a window of time where the tag bit is set, but
> > ->rqs[tag] isn't set yet. That's probably the race I hit, not the
> > completion race mentioned in the change log.
>
> Rewrote the commit message:
>
> http://git.kernel.dk/cgit/linux-block/commit/?h=mq-inflight&id=1908e43118e688e41ac8656edcf3e7a150f3f5081
Hello Jens,
This is what I found in the updated commit:
blk-mq-tag: check for NULL rq when iterating tags
Since we introduced blk-mq-sched, the tags->rqs[] array has been
dynamically assigned. So we need to check for NULL when iterating,
since there's a window of time where the bit is set, but we haven't
dynamically assigned the tags->rqs[] array position yet.
This is perfectly safe, since the memory backing of the request is
never going away while the device is alive.
Does this mean that blk_mq_tagset_busy_iter() can skip requests that it
shouldn't skip and also that blk_mq_tagset_busy_iter() can pass a pointer to
the previous request that was associated with a tag instead of the current
request to its busy_tag_iter_fn argument? Shouldn't these races be fixed,
e.g. by swapping the order in which the tag are set and tags->rqs[] are
assigned such that the correct request pointer is passed to the
busy_tag_iter_fn argument?
Thanks,
Bart.
