diff mbox series

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

Message ID 20220304160331.399757-15-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [01/14] blk-mq: do not include passthrough requests in I/O accounting | expand

Commit Message

Christoph Hellwig March 4, 2022, 4:03 p.m. UTC 
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.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/genhd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Bart Van Assche March 6, 2022, 8:51 p.m. UTC | #1 
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.

Thanks,

Bart.
Ming Lei March 7, 2022, 2:50 a.m. UTC | #2 
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
diff mbox series

Patch

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);
 }