Re: [PATCH 14/14] block: move rq_qos_exit() into disk_release()

[Ming Lei <ming.lei@redhat.com>]

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@redhat.com>
> > 
> > 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