On Tue, Dec 08, 2020 at 11:36:58AM +0000, John Garry wrote:

On 03/12/2020 09:26, John Garry wrote:

quoted

On 03/12/2020 00:55, Ming Lei wrote:

Hi Ming,

quoted

quoted

Yeah, so I said that was another problem which you mentioned
there, which
I'm not addressing, but I don't think that I'm making thing worse here.

The thing is that this patch does not fix the issue completely.

quoted

So AFAICS, the blk-mq/sched code doesn't wait for any "readers" to be
finished, such as those running blk_mq_queue_tag_busy_iter or
blk_mq_tagset_busy_iter() in another context.

So how about the idea of introducing some synchronization
primitive, such as
semaphore, which those "readers" must grab and release at start
and end (of
iter), to ensure the requests are not freed during the iteration?

It looks good, however devil is in details, please make into patch for
review.

OK, but another thing to say is that I need to find a somewhat reliable
reproducer for the potential problem you mention. So far this patch
solves the issue I see (in that kasan stops warning). Let me analyze
this a bit further.

Hi Ming,

I am just looking at this again, and have some doubt on your concern [0].

From checking blk_mq_queue_tag_busy_iter() specifically, don't we actually
guard against this with the q->q_usage_counter mechanism? That is, an agent
needs to grab a q counter ref when attempting the iter. This will fail when
the queue IO sched is being changed, as we freeze the queue during this
time, which is when the requests are freed, so no agent can hold a reference
to a freed request then. And same goes for blk_mq_update_nr_requests(),
where we freeze the queue.

blk_mq_queue_tag_busy_iter() can be run on another request queue just
between one driver tag is allocated and updating the request map, so one
extra request reference still can be grabbed.

So looks only holding one queue's usage_counter doesn't help this issue, since
bt_for_each() always iterates on driver tags wide.

But I didn't see such a guard for blk_mq_tagset_busy_iter().

IMO there isn't real difference between the two iteration.


Thanks, 
Ming