* [patch v2 0/3] block: optimize flush for non-queueable flush drive
@ 2011-05-04 8:17 shaohua.li
2011-05-04 8:17 ` [patch v2 1/3] block: add a non-queueable flush flag shaohua.li
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: shaohua.li @ 2011-05-04 8:17 UTC (permalink / raw)
To: linux-kernel, linux-ide; +Cc: jaxboe, htejun, jgarzik, hch, djwong
Flush request isn't queueable. When it's running, other request
can't. We can optimize flush performance according to this knowledge
In my test, I got about 20% performance boost.
^ permalink raw reply [flat|nested] 9+ messages in thread* [patch v2 1/3] block: add a non-queueable flush flag 2011-05-04 8:17 [patch v2 0/3] block: optimize flush for non-queueable flush drive shaohua.li @ 2011-05-04 8:17 ` shaohua.li 2011-05-04 9:05 ` Tejun Heo 2011-05-04 8:17 ` [patch v2 2/3] block: hold queue if flush is running for non-queueable flush drive shaohua.li 2011-05-04 8:17 ` [patch v2 3/3] SATA: enable non-queueable flush flag shaohua.li 2 siblings, 1 reply; 9+ messages in thread From: shaohua.li @ 2011-05-04 8:17 UTC (permalink / raw) To: linux-kernel, linux-ide; +Cc: jaxboe, htejun, jgarzik, hch, djwong, Shaohua Li [-- Attachment #1: blk-nonqueueable-flush-flag.patch --] [-- Type: text/plain, Size: 1233 bytes --] flush request isn't queueable in some drives. Add a flag to let driver notify block layer about this. We can optimize flush performance with the knowledge. 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-05-04 14:23:42.000000000 +0800 +++ linux/include/linux/blkdev.h 2011-05-04 14:24:40.000000000 +0800 @@ -364,6 +364,7 @@ struct request_queue * for flush operations */ unsigned int flush_flags; + unsigned int flush_not_queueable:1; unsigned int flush_pending_idx:1; unsigned int flush_running_idx:1; unsigned long flush_pending_since; @@ -549,6 +550,16 @@ static inline void blk_clear_queue_full( queue_flag_clear(QUEUE_FLAG_ASYNCFULL, q); } +static inline void blk_set_queue_flush_queueable(struct request_queue *q, + bool queueable) +{ + q->flush_not_queueable = !queueable; +} + +static inline bool blk_queue_flush_queueable(struct request_queue *q) +{ + return !q->flush_not_queueable; +} /* * mergeable request must not have _NOMERGE or _BARRIER bit set, nor may ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch v2 1/3] block: add a non-queueable flush flag 2011-05-04 8:17 ` [patch v2 1/3] block: add a non-queueable flush flag shaohua.li @ 2011-05-04 9:05 ` Tejun Heo 0 siblings, 0 replies; 9+ messages in thread From: Tejun Heo @ 2011-05-04 9:05 UTC (permalink / raw) To: shaohua.li; +Cc: linux-kernel, linux-ide, jaxboe, jgarzik, hch, djwong Hello, On Wed, May 04, 2011 at 04:17:26PM +0800, shaohua.li@intel.com wrote: > flush request isn't queueable in some drives. Add a flag to let driver > notify block layer about this. We can optimize flush performance with the > knowledge. > > 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-05-04 14:23:42.000000000 +0800 > +++ linux/include/linux/blkdev.h 2011-05-04 14:24:40.000000000 +0800 > @@ -364,6 +364,7 @@ struct request_queue > * for flush operations > */ > unsigned int flush_flags; > + unsigned int flush_not_queueable:1; > unsigned int flush_pending_idx:1; > unsigned int flush_running_idx:1; > unsigned long flush_pending_since; > @@ -549,6 +550,16 @@ static inline void blk_clear_queue_full( > queue_flag_clear(QUEUE_FLAG_ASYNCFULL, q); > } > > +static inline void blk_set_queue_flush_queueable(struct request_queue *q, > + bool queueable) > +{ > + q->flush_not_queueable = !queueable; > +} > + > +static inline bool blk_queue_flush_queueable(struct request_queue *q) > +{ > + return !q->flush_not_queueable; > +} In the block layer, these setters live in block/blk-settings.c and usually don't have _set in their names. Also, negation in the interface is slightly bothering but I can't think of better name meaning !queueable either, so unless someone can come up with better name, it should do, I think. Thank you. -- tejun ^ permalink raw reply [flat|nested] 9+ messages in thread
* [patch v2 2/3] block: hold queue if flush is running for non-queueable flush drive 2011-05-04 8:17 [patch v2 0/3] block: optimize flush for non-queueable flush drive shaohua.li 2011-05-04 8:17 ` [patch v2 1/3] block: add a non-queueable flush flag shaohua.li @ 2011-05-04 8:17 ` shaohua.li 2011-05-04 9:09 ` Tejun Heo 2011-05-04 10:42 ` Sergei Shtylyov 2011-05-04 8:17 ` [patch v2 3/3] SATA: enable non-queueable flush flag shaohua.li 2 siblings, 2 replies; 9+ messages in thread From: shaohua.li @ 2011-05-04 8:17 UTC (permalink / raw) To: linux-kernel, linux-ide; +Cc: jaxboe, htejun, jgarzik, hch, djwong, Shaohua Li [-- Attachment #1: blk-hold-queue-for-nonqueueable-flush.patch --] [-- Type: text/plain, Size: 2850 bytes --] 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@intel.com> --- block/blk-flush.c | 3 +++ block/blk.h | 12 +++++++++++- include/linux/blkdev.h | 1 + 3 files changed, 15 insertions(+), 1 deletion(-) 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); Index: linux/include/linux/blkdev.h =================================================================== --- linux.orig/include/linux/blkdev.h 2011-05-04 14:24:40.000000000 +0800 +++ linux/include/linux/blkdev.h 2011-05-04 14:29:29.000000000 +0800 @@ -365,6 +365,7 @@ struct request_queue */ unsigned int flush_flags; unsigned int flush_not_queueable:1; + unsigned int flush_queue_delayed:1; unsigned int flush_pending_idx:1; unsigned int flush_running_idx:1; unsigned long flush_pending_since; 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. + */ + if (q->flush_pending_idx != q->flush_running_idx && + !blk_queue_flush_queueable(q)) { + q->flush_queue_delayed = 1; + return NULL; + } if (!q->elevator->ops->elevator_dispatch_fn(q, 0)) return NULL; } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch v2 2/3] block: hold queue if flush is running for non-queueable flush drive 2011-05-04 8:17 ` [patch v2 2/3] block: hold queue if flush is running for non-queueable flush drive shaohua.li @ 2011-05-04 9:09 ` Tejun Heo 2011-05-04 10:42 ` Sergei Shtylyov 1 sibling, 0 replies; 9+ messages in thread From: Tejun Heo @ 2011-05-04 9:09 UTC (permalink / raw) To: shaohua.li; +Cc: linux-kernel, linux-ide, jaxboe, jgarzik, hch, djwong Hello, On Wed, May 04, 2011 at 04:17:27PM +0800, shaohua.li@intel.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@intel.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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch v2 2/3] block: hold queue if flush is running for non-queueable flush drive 2011-05-04 8:17 ` [patch v2 2/3] block: hold queue if flush is running for non-queueable flush drive shaohua.li 2011-05-04 9:09 ` Tejun Heo @ 2011-05-04 10:42 ` Sergei Shtylyov 1 sibling, 0 replies; 9+ messages in thread From: Sergei Shtylyov @ 2011-05-04 10:42 UTC (permalink / raw) To: shaohua.li; +Cc: linux-kernel, linux-ide, jaxboe, htejun, jgarzik, hch, djwong Hello. On 04-05-2011 12:17, shaohua.li@intel.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 Please specify that commit's summary -- for human readers. The ID is only immediately usable to gitweb. > workload. > Signed-off-by: Shaohua Li <shaohua.li@intel.com> [...] > 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 Queueable. WBR, Sergei ^ permalink raw reply [flat|nested] 9+ messages in thread
* [patch v2 3/3] SATA: enable non-queueable flush flag 2011-05-04 8:17 [patch v2 0/3] block: optimize flush for non-queueable flush drive shaohua.li 2011-05-04 8:17 ` [patch v2 1/3] block: add a non-queueable flush flag shaohua.li 2011-05-04 8:17 ` [patch v2 2/3] block: hold queue if flush is running for non-queueable flush drive shaohua.li @ 2011-05-04 8:17 ` shaohua.li 2011-05-04 8:53 ` Tejun Heo 2 siblings, 1 reply; 9+ messages in thread From: shaohua.li @ 2011-05-04 8:17 UTC (permalink / raw) To: linux-kernel, linux-ide; +Cc: jaxboe, htejun, jgarzik, hch, djwong, Shaohua Li [-- Attachment #1: sata-add-nonqueueable.patch --] [-- Type: text/plain, Size: 770 bytes --] Enable non-queueable flush flag for SATA. Signed-off-by: Shaohua Li <shaohua.li@intel.com> --- drivers/ata/libata-scsi.c | 2 ++ 1 file changed, 2 insertions(+) Index: linux/drivers/ata/libata-scsi.c =================================================================== --- linux.orig/drivers/ata/libata-scsi.c 2011-05-04 14:20:41.000000000 +0800 +++ linux/drivers/ata/libata-scsi.c 2011-05-04 14:28:28.000000000 +0800 @@ -3424,7 +3424,9 @@ void ata_scsi_scan_host(struct ata_port sdev = __scsi_add_device(ap->scsi_host, channel, id, 0, NULL); if (!IS_ERR(sdev)) { + struct request_queue *q = sdev->request_queue; dev->sdev = sdev; + blk_set_queue_flush_queueable(q, false); scsi_device_put(sdev); } else { dev->sdev = NULL; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch v2 3/3] SATA: enable non-queueable flush flag 2011-05-04 8:17 ` [patch v2 3/3] SATA: enable non-queueable flush flag shaohua.li @ 2011-05-04 8:53 ` Tejun Heo 2011-05-04 10:29 ` Jeff Garzik 0 siblings, 1 reply; 9+ messages in thread From: Tejun Heo @ 2011-05-04 8:53 UTC (permalink / raw) To: shaohua.li; +Cc: linux-kernel, linux-ide, jaxboe, jgarzik, hch, djwong On Wed, May 04, 2011 at 04:17:28PM +0800, shaohua.li@intel.com wrote: > Enable non-queueable flush flag for SATA. > > Signed-off-by: Shaohua Li <shaohua.li@intel.com> > --- > drivers/ata/libata-scsi.c | 2 ++ > 1 file changed, 2 insertions(+) > > Index: linux/drivers/ata/libata-scsi.c > =================================================================== > --- linux.orig/drivers/ata/libata-scsi.c 2011-05-04 14:20:41.000000000 +0800 > +++ linux/drivers/ata/libata-scsi.c 2011-05-04 14:28:28.000000000 +0800 > @@ -3424,7 +3424,9 @@ void ata_scsi_scan_host(struct ata_port > sdev = __scsi_add_device(ap->scsi_host, channel, id, 0, > NULL); > if (!IS_ERR(sdev)) { > + struct request_queue *q = sdev->request_queue; > dev->sdev = sdev; > + blk_set_queue_flush_queueable(q, false); > scsi_device_put(sdev); > } else { > dev->sdev = NULL; Isn't ata_scsi_dev_config() better place for this? Thakns. -- tejun ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch v2 3/3] SATA: enable non-queueable flush flag 2011-05-04 8:53 ` Tejun Heo @ 2011-05-04 10:29 ` Jeff Garzik 0 siblings, 0 replies; 9+ messages in thread From: Jeff Garzik @ 2011-05-04 10:29 UTC (permalink / raw) To: Tejun Heo; +Cc: shaohua.li, linux-kernel, linux-ide, jaxboe, hch, djwong On 05/04/2011 04:53 AM, Tejun Heo wrote: > On Wed, May 04, 2011 at 04:17:28PM +0800, shaohua.li@intel.com wrote: >> @@ -3424,7 +3424,9 @@ void ata_scsi_scan_host(struct ata_port >> sdev = __scsi_add_device(ap->scsi_host, channel, id, 0, >> NULL); >> if (!IS_ERR(sdev)) { >> + struct request_queue *q = sdev->request_queue; >> dev->sdev = sdev; >> + blk_set_queue_flush_queueable(q, false); >> scsi_device_put(sdev); >> } else { >> dev->sdev = NULL; > Isn't ata_scsi_dev_config() better place for this? Yes :) ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-05-04 10:43 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-05-04 8:17 [patch v2 0/3] block: optimize flush for non-queueable flush drive shaohua.li 2011-05-04 8:17 ` [patch v2 1/3] block: add a non-queueable flush flag shaohua.li 2011-05-04 9:05 ` Tejun Heo 2011-05-04 8:17 ` [patch v2 2/3] block: hold queue if flush is running for non-queueable flush drive shaohua.li 2011-05-04 9:09 ` Tejun Heo 2011-05-04 10:42 ` Sergei Shtylyov 2011-05-04 8:17 ` [patch v2 3/3] SATA: enable non-queueable flush flag shaohua.li 2011-05-04 8:53 ` Tejun Heo 2011-05-04 10:29 ` Jeff Garzik
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.