* [patch]block: document blk_plug
@ 2011-07-29 3:13 Shaohua Li
2011-07-29 12:14 ` Suresh Jayaraman
0 siblings, 1 reply; 6+ messages in thread
From: Shaohua Li @ 2011-07-29 3:13 UTC (permalink / raw)
To: Jens Axboe; +Cc: Andrew Morton, lkml
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.
+ *
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch]block: document blk_plug
2011-07-29 3:13 [patch]block: document blk_plug Shaohua Li
@ 2011-07-29 12:14 ` Suresh Jayaraman
2011-08-02 0:46 ` Shaohua Li
0 siblings, 1 reply; 6+ messages in thread
From: Suresh Jayaraman @ 2011-07-29 12:14 UTC (permalink / raw)
To: Shaohua Li; +Cc: Jens Axboe, Andrew Morton, lkml
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:
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.
+ */
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 0e67c45..810ad41 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -858,17 +858,20 @@ struct request_queue *blk_alloc_queue_node(gfp_t, int);
extern void blk_put_queue(struct request_queue *);
/*
+ * blk_plug allows for build up of queue of related requests by holding the I/O
+ * fragments for a short period.
+ *
* 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 cases */
+ struct list_head list; /* requests */
+ struct list_head cb_list; /* support callbacks */
+ 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [patch]block: document blk_plug
2011-07-29 12:14 ` Suresh Jayaraman
@ 2011-08-02 0:46 ` Shaohua Li
2011-08-26 22:25 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Shaohua Li @ 2011-08-02 0:46 UTC (permalink / raw)
To: Suresh Jayaraman; +Cc: Jens Axboe, Andrew Morton, lkml
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.
> 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?
Thanks,
Shaohua
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch]block: document blk_plug
2011-08-02 0:46 ` Shaohua Li
@ 2011-08-26 22:25 ` Andrew Morton
2011-08-27 1:22 ` Jonathan Corbet
2011-08-27 6:36 ` Jens Axboe
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Morton @ 2011-08-26 22:25 UTC (permalink / raw)
To: Shaohua Li; +Cc: Suresh Jayaraman, lkml, Jens Axboe
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch]block: document blk_plug
2011-08-26 22:25 ` Andrew Morton
@ 2011-08-27 1:22 ` Jonathan Corbet
2011-08-27 6:36 ` Jens Axboe
1 sibling, 0 replies; 6+ messages in thread
From: Jonathan Corbet @ 2011-08-27 1:22 UTC (permalink / raw)
To: Andrew Morton; +Cc: Shaohua Li, Suresh Jayaraman, lkml, Jens Axboe
On Fri, 26 Aug 2011 15:25:18 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:
> I'm getting all excited about getting some blk_plug documentation!
> [...]
> Only we can't document it because we don't understand it. Great.
>
> c'mon Jens. Talk to us?
Jens did give us a bit of documentation here:
http://lwn.net/Articles/438256/
Not complete perhaps, but better than nothing...
jon
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch]block: document blk_plug
2011-08-26 22:25 ` Andrew Morton
2011-08-27 1:22 ` Jonathan Corbet
@ 2011-08-27 6:36 ` Jens Axboe
1 sibling, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2011-08-27 6:36 UTC (permalink / raw)
To: Andrew Morton; +Cc: Shaohua Li, Suresh Jayaraman, lkml
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-08-27 6:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-29 3:13 [patch]block: document blk_plug Shaohua Li
2011-07-29 12:14 ` Suresh Jayaraman
2011-08-02 0:46 ` Shaohua Li
2011-08-26 22:25 ` Andrew Morton
2011-08-27 1:22 ` Jonathan Corbet
2011-08-27 6:36 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox