[1/3] blk-mq: remove redundant call to blk_freeze_queue_start in blk_mq_destroy_queue

  Calling blk_freeze_queue results in a redundant call to
blk_freeze_queue_start as it has been called in blk_queue_start_drain.

Replace blk_freeze_queue with blk_mq_freeze_queue_wait to avoid the
redundant call.

Signed-off-by: Jinlong Chen <nickyc975@zju.edu.cn>
---
 block/blk-mq.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
  

On 10/29/22 03:02, Jinlong Chen wrote:
> Calling blk_freeze_queue results in a redundant call to
> blk_freeze_queue_start as it has been called in blk_queue_start_drain.
> 
> Replace blk_freeze_queue with blk_mq_freeze_queue_wait to avoid the
> redundant call.

blk_mq_destroy_queue() has more callers than blk_queue_start_drain() so 
the above description is at least misleading.

Additionally, the word "cleanup" from the patch series title indicates 
that no patch in this series changes the behavior of the code. This 
patch involves a behavior change.

I think this patch introduces a hang for every caller of 
blk_mq_destroy_queue() other than blk_queue_start_drain().

Bart.
  

Hi, Bart.

> > Calling blk_freeze_queue results in a redundant call to
> > blk_freeze_queue_start as it has been called in blk_queue_start_drain.
> > 
> > Replace blk_freeze_queue with blk_mq_freeze_queue_wait to avoid the
> > redundant call.
> 
> blk_mq_destroy_queue() has more callers than blk_queue_start_drain() so 
> the above description is at least misleading.
> 
> Additionally, the word "cleanup" from the patch series title indicates 
> that no patch in this series changes the behavior of the code. This 
> patch involves a behavior change.

Sorry for my poor description. I'll send a new series with these
description problems resolved.

> I think this patch introduces a hang for every caller of 
> blk_mq_destroy_queue() other than blk_queue_start_drain().
> 
> Bart.

I don't see why the patch introduces a hang. The calling relationship in
blk_mq_destroy_queue is as follows:

blk_mq_destroy_queue()
    ...
    -> blk_queue_start_drain()
        -> blk_freeze_queue_start()  <- called
        ...
    -> blk_freeze_queue()
        -> blk_freeze_queue_start()  <- called again
        -> blk_mq_freeze_queue_wait()
    ...

So I think there is a redundant call to blk_freeze_queue_start(), we
just need to call blk_mq_freeze_queue_wait() after calling
blk_queue_start_drain().

Thanks!
Jinlong Chen
  

On 10/29/22 19:27, Jinlong Chen wrote:
>> I think this patch introduces a hang for every caller of
>> blk_mq_destroy_queue() other than blk_queue_start_drain().
 >> I don't see why the patch introduces a hang. The calling relationship in
> blk_mq_destroy_queue is as follows: [ ... ]

Agreed - what I wrote is wrong.

> So I think there is a redundant call to blk_freeze_queue_start(), we
> just need to call blk_mq_freeze_queue_wait() after calling
> blk_queue_start_drain().

I think it is on purpose that blk_queue_start_drain() freezes the 
request queue and never unfreezes it. So if you want to change this 
behavior it's up to you to motivate why you want to change this behavior 
and also why it is safe to make that change. See also commit 
d3cfb2a0ac0b ("block: block new I/O just after queue is set as dying").

Bart.
  

> > So I think there is a redundant call to blk_freeze_queue_start(), we
> > just need to call blk_mq_freeze_queue_wait() after calling
> > blk_queue_start_drain().
> 
> I think it is on purpose that blk_queue_start_drain() freezes the 
> request queue and never unfreezes it. So if you want to change this 
> behavior it's up to you to motivate why you want to change this behavior 
> and also why it is safe to make that change. See also commit 
> d3cfb2a0ac0b ("block: block new I/O just after queue is set as dying").
> 
> Bart.

I think there might be some misunderstanding. I didn't touch
blk_queue_start_drain(), so its behavior is not changed. What I have done
is just replacing blk_freeze_queue() with blk_mq_freeze_queue_wait() in
blk_mq_destroy_queue().

See:
- https://lore.kernel.org/linux-block/20221030084011.GA5262@lst.de/T/#t

Thanks!
Jinlong Chen
  

On 10/30/22 07:55, Jinlong Chen wrote:
>>> So I think there is a redundant call to blk_freeze_queue_start(), we
>>> just need to call blk_mq_freeze_queue_wait() after calling
>>> blk_queue_start_drain().
>>
>> I think it is on purpose that blk_queue_start_drain() freezes the
>> request queue and never unfreezes it. So if you want to change this
>> behavior it's up to you to motivate why you want to change this behavior
>> and also why it is safe to make that change. See also commit
>> d3cfb2a0ac0b ("block: block new I/O just after queue is set as dying").
> 
> I think there might be some misunderstanding. I didn't touch
> blk_queue_start_drain(), so its behavior is not changed. What I have done
> is just replacing blk_freeze_queue() with blk_mq_freeze_queue_wait() in
> blk_mq_destroy_queue().

Hi Jinlong,

Does this mean that you want me to provide more information about what I 
wrote? Without this patch, blk_mq_destroy_queue() uses two mechanisms to 
block future I/O requests:
1. Set the flag QUEUE_FLAG_DYING.
2. Freeze the request queue and leave it frozen.

Your patch modifies blk_mq_destroy_queue() such that it unfreezes the 
request queue after I/O has been quiesced instead of leaving it frozen. 
I would appreciate it if Ming Lei (Cc-ed) could comment on this change 
since I think that Ming introduced (2) in blk_mq_destroy_queue() 
(formerly called blk_cleanup_queue()).

Thanks,

Bart.
  

> On 10/30/22 07:55, Jinlong Chen wrote:
> >>> So I think there is a redundant call to blk_freeze_queue_start(), we
> >>> just need to call blk_mq_freeze_queue_wait() after calling
> >>> blk_queue_start_drain().
> >>
> >> I think it is on purpose that blk_queue_start_drain() freezes the
> >> request queue and never unfreezes it. So if you want to change this
> >> behavior it's up to you to motivate why you want to change this behavior
> >> and also why it is safe to make that change. See also commit
> >> d3cfb2a0ac0b ("block: block new I/O just after queue is set as dying").
> > 
> > I think there might be some misunderstanding. I didn't touch
> > blk_queue_start_drain(), so its behavior is not changed. What I have done
> > is just replacing blk_freeze_queue() with blk_mq_freeze_queue_wait() in
> > blk_mq_destroy_queue().
> 
> Hi Jinlong,
> 
> Does this mean that you want me to provide more information about what I 
> wrote? Without this patch, blk_mq_destroy_queue() uses two mechanisms to 
> block future I/O requests:
> 1. Set the flag QUEUE_FLAG_DYING.
> 2. Freeze the request queue and leave it frozen.

I agreed.

> Your patch modifies blk_mq_destroy_queue() such that it unfreezes the 
> request queue after I/O has been quiesced instead of leaving it frozen. 

This is what blk_mq_destroy_queue() looks like with the patch (removed
the stupid comment as suggested by Christoph Hellwig):

void blk_mq_destroy_queue(struct request_queue *q)
{
	WARN_ON_ONCE(!queue_is_mq(q));
	WARN_ON_ONCE(blk_queue_registered(q));

	might_sleep();

	blk_queue_flag_set(QUEUE_FLAG_DYING, q);
	blk_queue_start_drain(q);
	blk_mq_freeze_queue_wait(q);

	blk_sync_queue(q);
	blk_mq_cancel_work_sync(q);
	blk_mq_exit_queue(q);
}

I can't see where the unfreezing happens. Did I miss something?

> I would appreciate it if Ming Lei (Cc-ed) could comment on this change 
> since I think that Ming introduced (2) in blk_mq_destroy_queue() 
> (formerly called blk_cleanup_queue()).

I would appreciate it too.

Thanks!
Jinlong Chen