Re: [PATCH 10/12] block: move blk_exit_queue into disk_release

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2/22/22 06:14, Christoph Hellwig wrote:
From: Ming Lei <ming.lei@xxxxxxxxxx>

There can't be FS IO in disk_release(), so move blk_exit_queue() there.

We still need to freeze queue here since the request is freed after the
bio is completed and passthrough request rely on scheduler tags as well.

The disk can be released before or after queue is cleaned up, and we have
to free the scheduler request pool before blk_cleanup_queue returns,
while the static request pool has to be freed before exiting the
I/O scheduler.

This patch looks dubious to me because:
- The blk_freeze_queue() call in blk_cleanup_queue() waits for pending
  requests to finish, so why to move blk_exit_queue() from
  blk_cleanup_queue() into disk_release()?
- I'm concerned that this patch will break user space, e.g. scripts that
  try to unload an I/O scheduler kernel module immediately after having
  removed a request queue.

+static void blk_mq_release_queue(struct request_queue *q)
+{
+	blk_mq_cancel_work_sync(q);
+
+	/*
+	 * There can't be any non non-passthrough bios in flight here, but
+	 * requests stay around longer, including passthrough ones so we
+	 * still need to freeze the queue here.
+	 */
+	blk_mq_freeze_queue(q);

The above comment should be elaborated since what matters in this context is not whether or not any bios are still in flight but what happens with the request structures. As you know blk_queue_enter() fails after the DYING flag has been set, a flag that is set by blk_cleanup_queue(). blk_cleanup_queue() already freezes the queue. So why is it necessary to call blk_mq_freeze_queue() from blk_mq_release_queue()?

Bart.



[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux