| Message ID | 20220122111054.1126146-11-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: don't drain file system I/O on del_gendisk | expand |
On Sat, Jan 22, 2022 at 07:10:51PM +0800, Ming Lei wrote:
> Add one helper of disk_release_queue for releasing queue data for disk.
Please explain what "queue data for disk" is, and why this helper is
useful. Right now it seems to just move a few things around without a
rationale or explanation of why it is safe.
On Mon, Jan 24, 2022 at 02:16:24PM +0100, Christoph Hellwig wrote: > On Sat, Jan 22, 2022 at 07:10:51PM +0800, Ming Lei wrote: > > Add one helper of disk_release_queue for releasing queue data for disk. > > Please explain what "queue data for disk" is, and why this helper is Here it means elevator, blk-cgroup and rq qos, all actually serve FS IOs, maybe it should be changed to FS IOs. > useful. Right now it seems to just move a few things around without a > rationale or explanation of why it is safe. This patch just moves two function calls into the helper, and there doesn't have functional change, so no need rationale and explanation. Thanks, Ming
On Tue, Jan 25, 2022 at 07:27:37AM +0800, Ming Lei wrote: > > Please explain what "queue data for disk" is, and why this helper is > > Here it means elevator, blk-cgroup and rq qos, all actually serve FS > IOs, maybe it should be changed to FS IOs. > > > useful. Right now it seems to just move a few things around without a > > rationale or explanation of why it is safe. > > This patch just moves two function calls into the helper, and there > doesn't have functional change, so no need rationale and explanation. Yes, it it does. If you move calls into a "helper" that has a single caller and a weird name it better have a good explanation, because the move does not look useful at all.
diff --git a/block/genhd.c b/block/genhd.c index 5bd7bcd6246e..a86027619683 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -1085,6 +1085,20 @@ static const struct attribute_group *disk_attr_groups[] = { NULL }; +static void disk_release_queue(struct gendisk *disk) +{ + struct request_queue *q = disk->queue; + + blk_mq_cancel_work_sync(q); + + /* + * Remove all references to @q from the block cgroup controller before + * restoring @q->queue_lock to avoid that restoring this pointer causes + * e.g. blkcg_print_blkgs() to crash. + */ + blkcg_exit_queue(q); +} + /** * disk_release - releases all allocated resources of the gendisk * @dev: the device representing this disk @@ -1106,19 +1120,12 @@ static void disk_release(struct device *dev) might_sleep(); WARN_ON_ONCE(disk_live(disk)); - blk_mq_cancel_work_sync(disk->queue); + disk_release_queue(disk); disk_release_events(disk); kfree(disk->random); xa_destroy(&disk->part_tbl); - /* - * Remove all references to @q from the block cgroup controller before - * restoring @q->queue_lock to avoid that restoring this pointer causes - * e.g. blkcg_print_blkgs() to crash. - */ - blkcg_exit_queue(disk->queue); - disk->queue->disk = NULL; blk_put_queue(disk->queue); iput(disk->part0->bd_inode); /* frees the disk */
Add one helper of disk_release_queue for releasing queue data for disk. Signed-off-by: Ming Lei <ming.lei@redhat.com> --- block/genhd.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)