[PATCH v2] block: document blk-plug

[PATCH v2] block: document blk-plug

[Suresh Jayaraman]

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.

Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
---
 block/blk-core.c       |   14 ++++++++++++++
 include/linux/blkdev.h |   16 +++++++++++-----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 90e1ffd..ea360c8 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -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;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 84b15d5..f45d783 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -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

Re: [PATCH v2] block: document blk-plug

[Andrew Morton]

On Mon, 29 Aug 2011 16:58:21 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -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.

What does the older part of this comment mean?  If a code section is
preemptible then it *will* sleep.  That's what preemption does.

Re: [PATCH v2] block: document blk-plug

[Suresh Jayaraman]

On 08/30/2011 03:18 AM, Andrew Morton wrote:
> On Mon, 29 Aug 2011 16:58:21 +0530
> Suresh Jayaraman <sjayaraman@suse.de> wrote:
> 
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -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.
> 
> 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->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?



-- 
Suresh Jayaraman

Re: [PATCH v2] block: document blk-plug

[Shaohua Li]

On Tue, 2011-08-30 at 13:21 +0800, Suresh Jayaraman wrote:
> On 08/30/2011 03:18 AM, Andrew Morton wrote:
> > On Mon, 29 Aug 2011 16:58:21 +0530
> > Suresh Jayaraman <sjayaraman@suse.de> wrote:
> > 
> >> --- a/include/linux/blkdev.h
> >> +++ b/include/linux/blkdev.h
> >> @@ -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.
> > 
> > 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->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?
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

Re: [PATCH v2] block: document blk-plug

[Suresh Jayaraman]

On 08/30/2011 12:30 PM, Shaohua Li wrote:
> On Tue, 2011-08-30 at 13:21 +0800, Suresh Jayaraman wrote:
>> On 08/30/2011 03:18 AM, Andrew Morton wrote:
>>> On Mon, 29 Aug 2011 16:58:21 +0530
>>> Suresh Jayaraman <sjayaraman@suse.de> wrote:
>>>
>>>> --- a/include/linux/blkdev.h
>>>> +++ b/include/linux/blkdev.h
>>>> @@ -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.
>>>
>>> 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->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?
> 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.
> 

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,

-- 
Suresh Jayaraman

Re: [PATCH v2] block: document blk-plug

[Shaohua Li]

2011/9/5 Suresh Jayaraman <sjayaraman@suse.de>:
> On 08/30/2011 12:30 PM, Shaohua Li wrote:
>> On Tue, 2011-08-30 at 13:21 +0800, Suresh Jayaraman wrote:
>>> On 08/30/2011 03:18 AM, Andrew Morton wrote:
>>>> On Mon, 29 Aug 2011 16:58:21 +0530
>>>> Suresh Jayaraman <sjayaraman@suse.de> wrote:
>>>>
>>>>> --- a/include/linux/blkdev.h
>>>>> +++ b/include/linux/blkdev.h
>>>>> @@ -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.
>>>>
>>>> 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->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?
>> 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.
>>
>
> 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?
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