Re: [patch]block: document blk_plug

[Jens Axboe <axboe@kernel.dk>]

On 2011-08-27 00:25, Andrew Morton wrote:
> On Tue, 02 Aug 2011 08:46:10 +0800
> Shaohua Li <shaohua.li@intel.com> wrote:
> 
>> On Fri, 2011-07-29 at 20:14 +0800, Suresh Jayaraman wrote:
>>> On 07/29/2011 08:43 AM, Shaohua Li wrote:
>>>> Andrew Morton is asking to document blk_plug, so here is my attempt.
>>>>
>>>> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
>>>> ---
>>>>  include/linux/blkdev.h |   11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> Index: linux/include/linux/blkdev.h
>>>> ===================================================================
>>>> --- linux.orig/include/linux/blkdev.h	2011-07-29 10:51:29.000000000 +0800
>>>> +++ linux/include/linux/blkdev.h	2011-07-29 11:07:49.000000000 +0800
>>>> @@ -858,6 +858,17 @@ struct request_queue *blk_alloc_queue_no
>>>>  extern void blk_put_queue(struct request_queue *);
>>>>  
>>>>  /*
>>>> + * blk_plug gives each task a request list. Since blk_start_plug() called,
>>>> + * requests from the task will be added to the per-task list and then moved
>>>> + * to global request_queue in a batch way at appropriate time(either
>>>> + * blk_finish_plug() is called or task goes to sleep). blk_plug has some
>>>> + * advantages:
>>>> + * 1. Better request merge. The assumption here is requests from a task have
>>>> + *    better chances to be merged.
>>>> + * 2. Better scalability. Requests are moved from per-task list to global
>>>> + *    request_queue in a batch way, so the total times grabing global
>>>> + *    request_queue lock are reduced.
>>>> + *
>>>
>>> Hi Shaohua,
>>>
>>> This seems too brief atleast for someone like me who has not spent much
>>> time with the code and also is not in kerneldoc format. Here's my attempt:
>> Hi Suresh,
>> I like the blk_start_plug part below. The blk_plug part needs more
>> description to explain why we need it.
>>
> 
> I'm getting all excited about getting some blk_plug documentation!
> 
>>> From: Suresh Jayaraman <sjayaraman@suse.de>
>>> Subject: [PATCH] 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?"
>>>
>>> Shaohua Li attempted to document it. But, I think it was too brief and
>>> was not in kerneldoc format. Here's my attempt to document it. 
>>>
>>> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
>>> ---
>>>
>>>  block/blk-core.c       |   10 ++++++++++
>>>  include/linux/blkdev.h |   13 ++++++++-----
>>>  2 files changed, 18 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>> index b850bed..355aa2c 100644
>>> --- a/block/blk-core.c
>>> +++ b/block/blk-core.c
>>> @@ -2620,6 +2620,16 @@ 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 flushing the
>>> + *   pending I/O should the task end up blocking between blk_start_plug() and
>>> + *   blk_finish_plug() and is important for deadlock avoidance and for the
>>> + *   performance.
>>> + */
>> I'm not aware blk_plug is to avoid deadlock. It's most for performance
>> to me. Jens, any idea?
> 
> Only we can't document it because we don't understand it.  Great.
> 
> c'mon Jens.   Talk to us?

The pointer in the task_struct is to be able to auto-flush the plug if
the task inadvertently ends up scheduling with requests plugged. For
instance, if we end up doing a wait for a page that is already plugged,
then we must ensure that everything up to that point has been flushed
out.

-- 
Jens Axboe