Messages in this thread | | | Subject | Re: [PATCH v2] block: document blk-plug | From | Shaohua Li <> | Date | Tue, 30 Aug 2011 15:00:46 +0800 |
| |
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
| |