block: document blk-plug
PATCH v2 1/1 block: document blk-plug
Thus spake Andrew Morton:
"And I have the usual maintainability whine. If someone comes up to vmscan.c and sees it calling blk_start_plug(), how are they supposed to work out why that call is there? They go look at the blk_start_plug() definition and it is undocumented. I think we can do better than this?"
Adapted from the LWN article - http://lwn.net/Articles/438256/ by Jens Axboe and from an earlier attempt by Shaohua Li to document blk-plug.
Changes since -v1:
* explain how blk_plug helps with potential deadlock avoidance.
* explain why we need blk-plug.
* add a note that cb_list is required by md.
--- block/blk-core.c | 14 ++++++++++++++ include/linux/blkdev.h | 16 +++++++++++----- 2 files changed, 25 insertions(+), 5 deletions(-)
block/blk-core.c +14 -0
@@ -2626,6 +2626,20 @@ EXPORT_SYMBOL(kblockd_schedule_delayed_work);#define PLUG_MAGIC 0x91827364 +/** +* blk_start_plug - initialize blk_plug and track it inside the task_struct +* @plug: The &struct blk_plug that needs to be initialized +* +* Description: +* Tracking blk_plug inside the task_struct will help with auto-flushing the +* pending I/O should the task end up blocking between blk_start_plug() and +* blk_finish_plug(). This is important from a performance perspective, but +* also ensures that we don't deadlock. For instance, if the task is blocking +* for a memory allocation, memory reclaim could end up wanting to free a +* page belonging to that request that is currently residing in our private +* plug. By flushing the pending I/O when the process goes to sleep, we avoid +* this kind of deadlocks. +*/ void blk_start_plug (struct blk_plug *plug) {struct task_struct *tsk = current;
include/linux/blkdev.h +11 -5
@@ -863,17 +863,23 @@ struct request_queue *blk_alloc_queue_node(gfp_t, int);extern void blk_put_queue (struct request_queue *);/* +* blk_plug allows to build up a queue of related requests by holding the I/O +* fragments for a short period. This allows merging of sequential requests +* into single larger request. As the requests are moved from per-task list to +* the device's request_queue in a batch, this results in improved +* scalability as the lock contention for request_queue lock is reduced. +* * Note: Code in between changing the blk_plug list/cb_list or element of such * lists is preemptable, but such code can't do sleep (or be very careful), * otherwise data is corrupted. For details, please check schedule() where * blk_schedule_flush_plug() is called. */ struct blk_plug { -unsigned long magic; -struct list_head list; -struct list_head cb_list; -unsigned int should_sort; -unsigned int count; +unsigned long magic;/* detect uninitialized use-cases */ +struct list_head list;/* requests */ +struct list_head cb_list;/* md requires an unplug callback */ +unsigned int should_sort;/*list to be sorted before flushing? */ +unsigned int count;/* request count to avoid list getting too big */ };#define BLK_MAX_REQUEST_COUNT 16
Quoted 6 lines · On Mon, 29 Aug 2011 16:58:21 +0530 Suresh Jayaraman <[email protected]> wrote:
include/linux/blkdev.h +0
-0@@ -863,17 +863,23 @@ struct request_queue *blk_alloc_queue_node(gfp_t, int);extern void blk_put_queue (struct request_queue *);/*
include/linux/blkdev.h struct request_queue *blk_alloc_queue_node(gfp_t, int); · line 875
Quoted 10 lines
include/linux/blkdev.h +6
-0+* blk_plug allows to build up a queue of related requests by holding the I/O +* fragments for a short period. This allows merging of sequential requests +* into single larger request. As the requests are moved from per-task list to +* the device's request_queue in a batch, this results in improved +* scalability as the lock contention for request_queue lock is reduced. +* * Note: Code in between changing the blk_plug list/cb_list or element of such * lists is preemptable, but such code can't do sleep (or be very careful), * otherwise data is corrupted. For details, please check schedule() where * blk_schedule_flush_plug() is called.
What does the older part of this comment mean? If a code section is preemptible then it *will* sleep. That's what preemption does.
Quoted 24 lines · Andrew Morton
Quoted 6 lines · On Mon, 29 Aug 2011 16:58:21 +0530 Suresh Jayaraman <[email protected]> wrote:
include/linux/blkdev.h +0
-0@@ -863,17 +863,23 @@ struct request_queue *blk_alloc_queue_node(gfp_t, int);extern void blk_put_queue (struct request_queue *);/* include/linux/blkdev.h struct request_queue *blk_alloc_queue_node(gfp_t, int); · line 875
Quoted 10 lines
include/linux/blkdev.h +6
-0+* blk_plug allows to build up a queue of related requests by holding the I/O +* fragments for a short period. This allows merging of sequential requests +* into single larger request. As the requests are moved from per-task list to +* the device's request_queue in a batch, this results in improved +* scalability as the lock contention for request_queue lock is reduced. +* * Note: Code in between changing the blk_plug list/cb_list or element of such * lists is preemptable, but such code can't do sleep (or be very careful), * otherwise data is corrupted. For details, please check schedule() where * blk_schedule_flush_plug() is called. What does the older part of this comment mean? If a code section is preemptible then it *will* sleep. That's what preemption does.
Quoted 1 line
From what I can understand, we don't need to explicitly disable preemption
when modifying the blk_plug->list because interrupts are disabled when we are there.
void blk_flush_plug_list(struct blk_plug *plug, bool from_schedule) {
..
/*
* Save and disable interrupts here, to avoid doing it for every
* queue lock we have to take.
*/
local_irq_save(flags);
while (!list_empty(&list)) {
rq = list_entry_rq(list.next);
list_del_init(&rq->queuelist);
BUG_ON(!rq->q);
if (rq->q != q) {
/*
* This drops the queue lock
*/
if (q)
queue_unplugged(q, depth, from_schedule);
q = rq->q;
depth = 0;
spin_lock(q->queue_lock);
}..
}
When blk_flush_plug_list() is called from schedule() via blk_schedule_flush_plug() we must be very careful to not cause need_resched set and thus result in a preemption check?
Does that what your comment intend to mean? Shaohua?
Signature · 1 line
Suresh Jayaraman
Quoted 57 lines · Suresh Jayaraman
Quoted 8 lines · Andrew Morton
Quoted 5 lines · On Mon, 29 Aug 2011 16:58:21 +0530 Suresh Jayaraman <[email protected]> wrote:
include/linux/blkdev.h +0
-0@@ -863,17 +863,23 @@ struct request_queue *blk_alloc_queue_node(gfp_t, int);extern void blk_put_queue (struct request_queue *);Quoted 15 lines
include/linux/blkdev.h struct request_queue *blk_alloc_queue_node(gfp_t, int); · line 875
Quoted 11 lines
include/linux/blkdev.h +6
-0/* +* blk_plug allows to build up a queue of related requests by holding the I/O +* fragments for a short period. This allows merging of sequential requests +* into single larger request. As the requests are moved from per-task list to +* the device's request_queue in a batch, this results in improved +* scalability as the lock contention for request_queue lock is reduced. +* * Note: Code in between changing the blk_plug list/cb_list or element of such * lists is preemptable, but such code can't do sleep (or be very careful), * otherwise data is corrupted. For details, please check schedule() where * blk_schedule_flush_plug() is called. What does the older part of this comment mean? If a code section is preemptible then it *will* sleep. That's what preemption does.
From what I can understand, we don't need to explicitly disable preemption when modifying the
blk_plug->listbecause interrupts are disabled when we are there.void
blk_flush_plug_list(struct blk_plug *plug, bool from_schedule){..
/* * Save and disable interrupts here, to avoid doing it for every * queue lock we have to take. */ local_irq_save(flags); while (!list_empty(&list)) { rq = list_entry_rq(list.next); list_del_init(&rq->queuelist); BUG_ON(!rq->q); if (rq->q != q) { /* * This drops the queue lock */ if (q) queue_unplugged(q, depth, from_schedule); q = rq->q; depth = 0; spin_lock(q->queue_lock); }..
Quoted 7 lines
}
When
blk_flush_plug_list()is called fromschedule()viablk_schedule_flush_plug()we must be very careful to not causeneed_reschedset and thus result in a preemption check?Does that what your comment intend to mean? Shaohua?
the code adding request to the plug list and doing merge doesn't disable preempt. That is ok because blk_schedule_flush_plug() only flush the list when the task truly enters sleep (setting task->state non-running and call schedule()). That's why I mean the code can be preempted but can't do sleep.
Thanks,
Shaohua
Quoted 64 lines · Shaohua Li
Quoted 63 lines · Suresh Jayaraman
Quoted 8 lines · Andrew Morton
Quoted 5 lines · On Mon, 29 Aug 2011 16:58:21 +0530 Suresh Jayaraman <[email protected]> wrote:
include/linux/blkdev.h +0
-0@@ -863,17 +863,23 @@ struct request_queue *blk_alloc_queue_node(gfp_t, int);extern void blk_put_queue (struct request_queue *);Quoted 15 lines
include/linux/blkdev.h struct request_queue *blk_alloc_queue_node(gfp_t, int); · line 875
Quoted 11 lines
include/linux/blkdev.h +6
-0/* +* blk_plug allows to build up a queue of related requests by holding the I/O +* fragments for a short period. This allows merging of sequential requests +* into single larger request. As the requests are moved from per-task list to +* the device's request_queue in a batch, this results in improved +* scalability as the lock contention for request_queue lock is reduced. +* * Note: Code in between changing the blk_plug list/cb_list or element of such * lists is preemptable, but such code can't do sleep (or be very careful), * otherwise data is corrupted. For details, please check schedule() where * blk_schedule_flush_plug() is called. What does the older part of this comment mean? If a code section is preemptible then it *will* sleep. That's what preemption does.
From what I can understand, we don't need to explicitly disable preemption when modifying the
blk_plug->listbecause interrupts are disabled when we are there.void
blk_flush_plug_list(struct blk_plug *plug, bool from_schedule){..
/* * Save and disable interrupts here, to avoid doing it for every * queue lock we have to take. */ local_irq_save(flags); while (!list_empty(&list)) { rq = list_entry_rq(list.next); list_del_init(&rq->queuelist); BUG_ON(!rq->q); if (rq->q != q) { /* * This drops the queue lock */ if (q) queue_unplugged(q, depth, from_schedule); q = rq->q; depth = 0; spin_lock(q->queue_lock); }..
}
When
blk_flush_plug_list()is called fromschedule()viablk_schedule_flush_plug()we must be very careful to not causeneed_reschedset and thus result in a preemption check?
Quoted 7 lines
Quoted 1 line
Does that what your comment intend to mean? Shaohua?
the code adding request to the plug list and doing merge doesn't disable preempt. That is ok because
blk_schedule_flush_plug()only flush the list when the task truly enters sleep (settingtask->statenon-running and callschedule()). That's why I mean the code can be preempted but can't do sleep.
Sorry, I'm not sure I understood the above after reading it multiple times.
Yes, preemption is not disabled when adding request to the plug list and doing merge. But, still there is a possibility of corruption for instance - when we are doing list_add_tail() in __make_request(), we get preempted and then go to a sleep. Before we go to sleep, blk_scheduler_flush_plug() via schedule() tries to flush the list leading to corruption, no? What am I missing?
[PS. Sorry about the delay in following it up, back from short vacation]
Thanks,
Signature · 1 line
Suresh Jayaraman
2011/9/5 Suresh Jayaraman <[email protected]>:
Quoted 73 lines · Shaohua Li
Quoted 64 lines · Suresh Jayaraman
Quoted 63 lines · Andrew Morton
Quoted 8 lines
Quoted 5 lines · On Mon, 29 Aug 2011 16:58:21 +0530 Suresh Jayaraman <[email protected]> wrote:
include/linux/blkdev.h +0
-0@@ -863,17 +863,23 @@ struct request_queue *blk_alloc_queue_node(gfp_t, int); extern void blk_put_queue(struct request_queue *);Quoted 15 lines
include/linux/blkdev.h struct request_queue *blk_alloc_queue_node(gfp_t, int); · line 875
Quoted 11 lines
include/linux/blkdev.h +6
-0/* +* blk_plug allows to build up a queue of related requests by holding the I/O +* fragments for a short period. This allows merging of sequential requests +* into single larger request. As the requests are moved from per-task list to +* the device's request_queue in a batch, this results in improved +* scalability as the lock contention for request_queue lock is reduced. +* * Note: Code in between changing the blk_plug list/cb_list or element of such * lists is preemptable, but such code can't do sleep (or be very careful), * otherwise data is corrupted. For details, please check schedule() where * blk_schedule_flush_plug() is called. What does the older part of this comment mean? If a code section is preemptible then it *will* sleep. That's what preemption does.
From what I can understand, we don't need to explicitly disable preemption when modifying the
blk_plug->listbecause interrupts are disabled when we are there.void
blk_flush_plug_list(struct blk_plug *plug, bool from_schedule){..
/*
* Save and disable interrupts here, to avoid doing it for every
* queue lock we have to take.
*/
local_irq_save(flags);
while (!list_empty(&list)) {
rq =list_entry_rq(list.next);list_del_init(&rq->queuelist);BUG_ON(!rq->q);
if (rq->q!= q) {
/*
* This drops the queue lock
*/
if (q)
queue_unplugged(q, depth, from_schedule); q =rq->q;
depth = 0;
spin_lock(q->queue_lock);
}..
}
When
blk_flush_plug_list()is called fromschedule()viablk_schedule_flush_plug()we must be very careful to not causeneed_reschedset and thus result in a preemption check?Quoted 7 lines
Quoted 1 line
Does that what your comment intend to mean? Shaohua?
the code adding request to the plug list and doing merge doesn't disable preempt. That is ok because
blk_schedule_flush_plug()only flush the list when the task truly enters sleep (settingtask->statenon-running and callschedule()). That's why I mean the code can be preempted but can't do sleep.
Quoted 8 lines
Sorry, I'm not sure I understood the above after reading it multiple times.
Yes, preemption is not disabled when adding request to the plug list and doing merge. But, still there is a possibility of corruption for instance - when we are doing
list_add_tail()in__make_request(), we get preempted and then go to a sleep. Before we go to sleep,blk_scheduler_flush_plug()viaschedule()tries to flush the list leading to corruption, no? What am I missing?
Not possible. blk_scheduler_flush_plug() is called with prev->state && !(preempt_count() & PREEMPT_ACTIVE if it's a preempt, prev->state is TASK_RUNNING, blk_scheduler_flush_plug isn't called.
Thanks,
Shaohua