[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20110504090945.GD8007@htj.dyndns.org>
Date: Wed, 4 May 2011 11:09:45 +0200
From: Tejun Heo <htejun@...il.com>
To: shaohua.li@...el.com
Cc: linux-kernel@...r.kernel.org, linux-ide@...r.kernel.org,
jaxboe@...ionio.com, jgarzik@...ox.com, hch@...radead.org,
djwong@...ibm.com
Subject: Re: [patch v2 2/3] block: hold queue if flush is running for
non-queueable flush drive
Hello,
On Wed, May 04, 2011 at 04:17:27PM +0800, shaohua.li@...el.com wrote:
> In some drives, flush requests are non-queueable. When flush request is running,
> normal read/write requests can't run. If block layer dispatches such request,
> driver can't handle it and requeue it.
> Tejun suggested we can hold the queue when flush is running. This can avoid
> unnecessary requeue.
> Also this can improve performance. Say we have requests f1, w1, f2 (f is flush
> request, and w is write request). When f1 is running, queue will be hold, so w1
> will not be added to queue list. Just after f1 is finished, f2 will be
> dispatched. Since f1 already flushs cache out, f2 can be finished very quickly.
> In my test, the queue holding completely solves a regression introduced by
> commit 53d63e6b0dfb9588, which is about 20% regression running a sysbench fileio
> workload.
>
> Signed-off-by: Shaohua Li <shaohua.li@...el.com>
It looks good to me now, but some nitpicks.
> Index: linux/block/blk-flush.c
> ===================================================================
> --- linux.orig/block/blk-flush.c 2011-05-04 14:20:33.000000000 +0800
> +++ linux/block/blk-flush.c 2011-05-04 15:23:50.000000000 +0800
> @@ -199,6 +199,9 @@ static void flush_end_io(struct request
>
> BUG_ON(q->flush_pending_idx == q->flush_running_idx);
>
> + queued |= q->flush_queue_delayed;
> + q->flush_queue_delayed = 0;
> +
> /* account completion of the flush request */
> q->flush_running_idx ^= 1;
> elv_completed_request(q, flush_rq);
Can you please do if (queued || q->flush_queue_delayed) instead of
setting queued? And please also update the comment above the if
statement so that it explains the flush_queue_delayed case too.
> Index: linux/block/blk.h
> ===================================================================
> --- linux.orig/block/blk.h 2011-05-04 14:20:33.000000000 +0800
> +++ linux/block/blk.h 2011-05-04 16:09:42.000000000 +0800
> @@ -61,7 +61,17 @@ static inline struct request *__elv_next
> rq = list_entry_rq(q->queue_head.next);
> return rq;
> }
> -
> + /*
> + * Flush request is running and flush request isn't queeueable
> + * in the drive, we can hold the queue till flush request is
> + * finished. Even we don't do this, driver can't dispatch next
> + * requests and will requeue them.
> + */
Please explain the f1, w1, f2 case here as that's the biggest reason
this optimization is implemented and also explain the use of
flush_queue_delayed (just explain briefly and refer to
flush_end_io()).
Thank you.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists