linux-kernel - Re: Re: [PATCH 1/3] blk-mq: remove redundant call to blk_freeze_queue_start in blk_mq_destroy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 31 Oct 2022 10:02:57 +0800 (GMT+08:00)
From:   "Jinlong Chen" <nickyc975@....edu.cn>
To:     "Bart Van Assche" <bvanassche@....org>
Cc:     axboe@...nel.dk, hch@....de, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org, "Ming Lei" <ming.lei@...hat.com>
Subject: Re: Re: [PATCH 1/3] blk-mq: remove redundant call to
 blk_freeze_queue_start in blk_mq_destroy_queue

> 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