Make blk_cleanup_queue() finish after request_fn
Subsystems (3)
v2 cover Make blk_cleanup_queue() finish after request_fn
Request processing functions like scsi_request_fn() that unlock the queue lock internally may cause blk_cleanup_queue() to finish while the request_fn is in progress. This two-patch series makes sure that blk_cleanup_queue() can't finish before all active request_fn calls have finished.
Changes compared to v1:
- Included a patch to rename QUEUE_FLAG_DEAD.
- Refined the descriptions of the __blk_run_queue_uncond() and blk_cleanup_queue() functions.
PATCH v2 1/2 block: Rename queue dead flag
This patch is the result of running the command below and manually fixing up indentation:
git grep -lE 'blk_queue_dead|QUEUE_FLAG_DEAD' |
xargs sed -i .tmp -e 's/blk_queue_dead/blk_queue_dying/g' \
-e 's/QUEUE_FLAG_DEAD/QUEUE_FLAG_DYING/g'
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Chanho Min <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
--- block/blk-cgroup.c | 2 +- block/blk-core.c | 16 ++++++++-------- block/blk-exec.c | 2 +- block/blk-sysfs.c | 4 ++-- block/blk-throttle.c | 2 +- block/blk.h | 2 +- drivers/block/ub.c | 2 +- drivers/scsi/scsi_lib.c | 2 +- include/linux/blkdev.h | 4 ++-- 9 files changed, 18 insertions(+), 18 deletions(-)
block/blk-cgroup.c +1 -1
@@ -231,7 +231,7 @@ struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,* we shouldn't allow anything to go through for a bypassing queue. */ if (unlikely (blk_queue_bypass (q))) -return ERR_PTR (blk_queue_dead (q) ? -EINVAL : -EBUSY ); +return ERR_PTR (blk_queue_dying (q) ? -EINVAL : -EBUSY );return __blkg_lookup_create (blkcg, q,NULL ); }EXPORT_SYMBOL_GPL (blkg_lookup_create);
block/blk-core.c +8 -8
@@ -484,7 +484,7 @@ void blk_cleanup_queue(struct request_queue *q)/* mark @q DEAD, no new request or merges will be allowed afterwards */ mutex_lock (&q->sysfs_lock); -queue_flag_set_unlocked (QUEUE_FLAG_DEAD , q); +queue_flag_set_unlocked (QUEUE_FLAG_DYING , q);spin_lock_irq (lock);/*
@@ -501,7 +501,7 @@ void blk_cleanup_queue(struct request_queue *q)queue_flag_set (QUEUE_FLAG_NOMERGES , q);queue_flag_set (QUEUE_FLAG_NOXMERGES , q); -queue_flag_set (QUEUE_FLAG_DEAD , q); +queue_flag_set (QUEUE_FLAG_DYING , q);spin_unlock_irq (lock);mutex_unlock (&q->sysfs_lock);
@@ -723,7 +723,7 @@ EXPORT_SYMBOL(blk_init_allocated_queue);bool blk_get_queue (struct request_queue *q) { -if (likely (!blk_queue_dead (q))) { +if (likely (!blk_queue_dying (q))) {__blk_get_queue (q);return true; }
@@ -877,7 +877,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags,const bool is_sync =rw_is_sync (rw_flags) !=0 ;int may_queue; -if (unlikely (blk_queue_dead (q))) +if (unlikely (blk_queue_dying (q)))return NULL ; may_queue =elv_may_queue (q, rw_flags);
@@ -1057,7 +1057,7 @@ retry:if (rq)return rq; -if (!(gfp_mask & __GFP_WAIT) ||unlikely (blk_queue_dead (q))) { +if (!(gfp_mask & __GFP_WAIT) ||unlikely (blk_queue_dying (q))) {blk_put_rl (rl);return NULL ; }
@@ -1909,7 +1909,7 @@ int blk_insert_cloned_request(struct request_queue *q, struct request *rq)return -EIO ;spin_lock_irqsave (q->queue_lock, flags); -if (unlikely (blk_queue_dead (q))) { +if (unlikely (blk_queue_dying (q))) {spin_unlock_irqrestore (q->queue_lock, flags);return -ENODEV ; }
@@ -2891,7 +2891,7 @@ static void queue_unplugged(struct request_queue *q, unsigned int depth,/* * Don't mess with dead queue. */ -if (unlikely (blk_queue_dead (q))) { +if (unlikely (blk_queue_dying (q))) {spin_unlock (q->queue_lock);return ; }
@@ -3000,7 +3000,7 @@ void blk_flush_plug_list(struct blk_plug *plug, bool from_schedule)/* * Short-circuit if @q is dead */ -if (unlikely (blk_queue_dead (q))) { +if (unlikely (blk_queue_dying (q))) {__blk_end_request_all (rq, -ENODEV );continue ; }
block/blk-exec.c +1 -1
@@ -60,7 +60,7 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,spin_lock_irq (q->queue_lock); -if (unlikely (blk_queue_dead (q))) { +if (unlikely (blk_queue_dying (q))) { rq->errors = -ENXIO ;if (rq->end_io) rq->end_io (rq, rq->errors);
block/blk-sysfs.c +2 -2
@@ -432,7 +432,7 @@ queue_attr_show(struct kobject *kobj, struct attribute *attr, char *page)if (!entry->show)return -EIO ;mutex_lock (&q->sysfs_lock); -if (blk_queue_dead (q)) { +if (blk_queue_dying (q)) {mutex_unlock (&q->sysfs_lock);return -ENOENT ; }
@@ -454,7 +454,7 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr, q = container_of(kobj, struct request_queue, kobj);mutex_lock (&q->sysfs_lock); -if (blk_queue_dead (q)) { +if (blk_queue_dying (q)) {mutex_unlock (&q->sysfs_lock);return -ENOENT ; }
block/blk-throttle.c +1 -1
@@ -303,7 +303,7 @@ static struct throtl_grp *throtl_lookup_create_tg(struct throtl_data *td,/* if %NULL and @q is alive, fall back to root_tg */ if (!IS_ERR (blkg)) tg =blkg_to_tg (blkg); -else if (!blk_queue_dead (q)) +else if (!blk_queue_dying (q)) tg =td_root_tg (td); }
block/blk.h +1 -1
@@ -96,7 +96,7 @@ static inline struct request *__elv_next_request(struct request_queue *q) q->flush_queue_delayed =1 ;return NULL ; } -if (unlikely (blk_queue_dead (q)) || +if (unlikely (blk_queue_dying (q)) || !q->elevator->type->ops.elevator_dispatch_fn (q,0 ))return NULL ; }
drivers/block/ub.c +1 -1
@@ -2394,7 +2394,7 @@ static void ub_disconnect(struct usb_interface *intf)del_gendisk (lun->disk);/* * I wish I could do: -* queue_flag_set(QUEUE_FLAG_DEAD, q); +* queue_flag_set(QUEUE_FLAG_DYING, q); * As it is, we rely on our internal poisoning and let * the upper levels to spin furiously failing all the I/O. */
drivers/scsi/scsi_lib.c +1 -1
@@ -1406,7 +1406,7 @@ static int scsi_lld_busy(struct request_queue *q)struct scsi_device *sdev = q->queuedata;struct Scsi_Host *shost; -if (blk_queue_dead (q)) +if (blk_queue_dying (q))return 0 ; shost = sdev->host;
include/linux/blkdev.h +2 -2
@@ -436,7 +436,7 @@ struct request_queue {#define QUEUE_FLAG_STOPPED 2/* queue is stopped */ #define QUEUE_FLAG_SYNCFULL 3/* read queue has been filled */ #define QUEUE_FLAG_ASYNCFULL 4/* write queue has been filled */ -#define QUEUE_FLAG_DEAD 5/* queue being torn down */ +#define QUEUE_FLAG_DYING 5/* queue being torn down */ #define QUEUE_FLAG_BYPASS 6/* act as dumb FIFO queue */ #define QUEUE_FLAG_BIDI 7/* queue supports bidi requests */ #define QUEUE_FLAG_NOMERGES 8/* disable merge attempts */
@@ -520,7 +520,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)#define blk_queue_tagged (q) test_bit(QUEUE_FLAG_QUEUED, &(q)->queue_flags)#define blk_queue_stopped (q) test_bit(QUEUE_FLAG_STOPPED, &(q)->queue_flags) -#define blk_queue_dead (q) test_bit(QUEUE_FLAG_DEAD, &(q)->queue_flags) +#define blk_queue_dying (q) test_bit(QUEUE_FLAG_DYING, &(q)->queue_flags)#define blk_queue_bypass (q) test_bit(QUEUE_FLAG_BYPASS, &(q)->queue_flags)#define blk_queue_nomerges (q) test_bit(QUEUE_FLAG_NOMERGES, &(q)->queue_flags)#define blk_queue_noxmerges (q) \
Signature · 1 line
1.7.10.4
Tejun Heo
maintainerRe: [PATCH 1/2] block: Rename queue dead flag
Hello,
Quoted 6 lines · Bart Van Assche
This patch is the result of running the command below and manually fixing up indentation:
git grep -lE 'blk_queue_dead|QUEUE_FLAG_DEAD' |
xargs sed -i .tmp -e 's/blk_queue_dead/blk_queue_dying/g' \
-e 's/QUEUE_FLAG_DEAD/QUEUE_FLAG_DYING/g'
Can you please explain why this change is being made in the patch description?
Thanks.
Signature · 1 line
tejun
PATCH v2 2/2 Make blk_cleanup_queue() wait until request_fn finished
In reply to Bart Van AsscheSome request_fn implementations, e.g. scsi_request_fn(), unlock the queue lock. Make sure that blk_cleanup_queue() waits until all active request_fn invocations have finished. This fixes a potential use-after-free at the end of scsi_request_fn(). Also, make sure that the block layer doesn't invoke request_fn after blk_cleanup_queue() finished.
Reported-by Chanho Min <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Tejun Heo <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
--- block/blk-core.c | 32 +++++++++++++++++++++++++++++--- block/blk-exec.c | 2 +- block/blk.h | 2 ++ drivers/scsi/scsi_lib.c | 10 +--------- include/linux/blkdev.h | 7 +++++++ 5 files changed, 40 insertions(+), 13 deletions(-)
block/blk-core.c +29 -3
@@ -293,6 +293,27 @@ void blk_sync_queue(struct request_queue *q)EXPORT_SYMBOL (blk_sync_queue);/** +* __blk_run_queue_uncond - run a queue whether or not it has been stopped +* @q: The queue to run +* +* Description: +* Invoke request handling on a queue if there are any pending requests. +* May be used to restart request handling after a request has completed. +* This variant runs the queue whether or not the queue has been +* stopped. Must be called with the queue lock held and interrupts +* disabled. See also @blk_run_queue. +*/ +void __blk_run_queue_uncond (struct request_queue *q) +{ +if (unlikely (blk_queue_dead (q))) +return ; + + q->request_fn_active++; + q->request_fn (q); + q->request_fn_active--; +} + +/** * __blk_run_queue - run a single device queue * @q: The queue to run *
@@ -305,7 +326,7 @@ void __blk_run_queue(struct request_queue *q)if (unlikely (blk_queue_stopped (q)))return ; - q->request_fn (q); +__blk_run_queue_uncond (q); }EXPORT_SYMBOL (__blk_run_queue);
@@ -388,6 +409,7 @@ void blk_drain_queue(struct request_queue *q, bool drain_all)__blk_run_queue (q); drain |= q->nr_rqs_elvpriv; + drain |= q->request_fn_active;/* * Unfortunately, requests are queued at and tracked from
@@ -475,8 +497,8 @@ EXPORT_SYMBOL_GPL(blk_queue_bypass_end);* blk_cleanup_queue - shutdown a request queue * @q: request queue to shutdown * -* Mark @q DEAD, drain all pending requests, destroy and put it. All -* future requests will be failed immediately with -ENODEV. +* Mark @q as dying, drain all pending requests, mark @q as dead, destroy and +* put it. All future requests will be failed immediately with -ENODEV. */ void blk_cleanup_queue (struct request_queue *q) {
@@ -508,6 +530,10 @@ void blk_cleanup_queue(struct request_queue *q)/* drain all requests queued before DEAD marking */ blk_drain_queue (q, true); +spin_lock_irq (lock); +queue_flag_set (QUEUE_FLAG_DEAD , q); +spin_unlock_irq (lock); +/* @q won't process any more request, flush async actions */ del_timer_sync (&q->backing_dev_info.laptop_mode_wb_timer);blk_sync_queue (q);
block/blk-exec.c +1 -1
@@ -72,7 +72,7 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,__blk_run_queue (q);/* the queue is stopped so it won't be run */ if (rq->cmd_type ==REQ_TYPE_PM_RESUME ) - q->request_fn (q); +__blk_run_queue_uncond (q);spin_unlock_irq (q->queue_lock); }EXPORT_SYMBOL_GPL (blk_execute_rq_nowait);
block/blk.h +2 -0
@@ -145,6 +145,8 @@ int blk_try_merge(struct request *rq, struct bio *bio);void blk_queue_congestion_threshold (struct request_queue *q); +void __blk_run_queue_uncond (struct request_queue *q); +int blk_dev_init (void );
drivers/scsi/scsi_lib.c +1 -9
@@ -1517,10 +1517,6 @@ static void scsi_request_fn(struct request_queue *q)struct scsi_cmnd *cmd;struct request *req; -if (!get_device (&sdev->sdev_gendev)) -/* We must be tearing the block queue down already */ -return ; -/* * To start with, we keep looping until the queue is empty, or until * the host is no longer able to accept any more requests.
@@ -1629,11 +1625,7 @@ out_delay:if (sdev->device_busy ==0 )blk_delay_queue (q,SCSI_QUEUE_DELAY ); out: -/* must be careful here...if we trigger the ->remove() function -* we cannot be holding the q lock */ -spin_unlock_irq (q->queue_lock); -put_device (&sdev->sdev_gendev); -spin_lock_irq (q->queue_lock); + ; }u64 scsi_calculate_bounce_limit (struct Scsi_Host *shost)
include/linux/blkdev.h +7 -0
@@ -377,6 +377,11 @@ struct request_queue {unsigned int nr_sorted;unsigned int in_flight[2 ]; +/* +* Number of active request_fn() calls for those request_fn() +* implementations that unlock the queue_lock, e.g. scsi_request_fn(). +*/ +unsigned int request_fn_active;unsigned int rq_timeout;struct timer_list timeout;
@@ -451,6 +456,7 @@ struct request_queue {#define QUEUE_FLAG_ADD_RANDOM 16/* Contributes to random pool */ #define QUEUE_FLAG_SECDISCARD 17/* supports SECDISCARD */ #define QUEUE_FLAG_SAME_FORCE 18/* force complete on same CPU */ +#define QUEUE_FLAG_DEAD 19/* queue tear-down finished */ #define QUEUE_FLAG_DEFAULT ((1 << QUEUE_FLAG_IO_STAT) | \ (1 << QUEUE_FLAG_STACKABLE) | \
@@ -521,6 +527,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)#define blk_queue_tagged (q) test_bit(QUEUE_FLAG_QUEUED, &(q)->queue_flags)#define blk_queue_stopped (q) test_bit(QUEUE_FLAG_STOPPED, &(q)->queue_flags)#define blk_queue_dying (q) test_bit(QUEUE_FLAG_DYING, &(q)->queue_flags) +#define blk_queue_dead (q) test_bit(QUEUE_FLAG_DEAD, &(q)->queue_flags)#define blk_queue_bypass (q) test_bit(QUEUE_FLAG_BYPASS, &(q)->queue_flags)#define blk_queue_nomerges (q) test_bit(QUEUE_FLAG_NOMERGES, &(q)->queue_flags)#define blk_queue_noxmerges (q) \
Signature · 1 line
1.7.10.4
Tejun Heo
maintainerRe: [PATCH 2/2] Make blk_cleanup_queue() wait until request_fn finished
Hello, Bart.
Quoted 6 lines · Bart Van Assche
Some
request_fnimplementations, e.g.scsi_request_fn(), unlock the queue lock. Make sure thatblk_cleanup_queue()waits until all activerequest_fninvocations have finished. This fixes a potential use-after-free at the end ofscsi_request_fn(). Also, make sure that the block layer doesn't invokerequest_fnafterblk_cleanup_queue()finished.
I think the patch is correct but would prefer if the two changes - request_fn_active and QUEUE_DEAD - are put into separate patches with more detailed explanation what they're protecting how.
Thanks.
Signature · 1 line
tejun