* [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