On Sun, Mar 06, 2022 at 12:51:37PM -0800, Bart Van Assche wrote:
> On 3/4/22 08:03, Christoph Hellwig wrote:
> > From: Ming Lei <ming.lei@xxxxxxxxxx>
> >
> > There can't be FS IO in disk_release(), so it is safe to move rq_qos_exit()
> > there.
>
> The commit message only explains why it is safe to move rq_qos_exit() but
> not why moving that function call is useful. Please add an explanation of
> why moving that function call is useful and/or necessary.
>
> > diff --git a/block/genhd.c b/block/genhd.c
> > index 857e0a54da7dd..56f66c6fee943 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -627,7 +627,6 @@ void del_gendisk(struct gendisk *disk)
> > blk_mq_freeze_queue_wait(q);
> > - rq_qos_exit(q);
> > blk_sync_queue(q);
> > blk_flush_integrity();
> > /*
> > @@ -1119,7 +1118,7 @@ static void disk_release_mq(struct request_queue *q)
> > elevator_exit(q);
> > mutex_unlock(&q->sysfs_lock);
> > }
> > -
> > + rq_qos_exit(q);
> > __blk_mq_unfreeze_queue(q, true);
> > }
>
> Commit 8e141f9eb803 ("block: drain file system I/O on del_gendisk") removed
> the rq_qos_exit() call from blk_cleanup_queue(). This patch series does not
> restore the rq_qos_exit() call in blk_cleanup_queue(). I think that call
> should be restored since rq_qos_add() can be called before add_disk() is
> called. I'm referring to the following call chain:
>
> __alloc_disk_node()
> blkcg_init_queue()
> blk_iolatency_init()
> rq_qos_add()
>
> sd_probe() is one of the functions that can take an error path after
> __alloc_disk_node() has returned and before device_add_disk() is called.
blkcg_exit_queue() is called in disk_release(), so this error handing is
covered since put_disk() should be called once __alloc_disk_node()
is successful, no matter disk is added or not.
We move rq_qos_exit() to disk_release() too, then every FS IO related
resources are released in disk_release().
Thanks,
Ming
