[PATCH v2 0/2] Enabling ATA Command Priorities

[PATCH v2 0/2] Enabling ATA Command Priorities

[Adam Manzanares]

This patch builds ATA commands with high priority if the iocontext
of a process is set to real time. The goal of the patch is to
improve tail latencies of workloads that use higher queue depths.

This patch has been tested with an Ultrastar HE8 HDD and cuts the 
the p99.99 tail latency of foreground IO from 2s down to 72ms when
using the deadline scheduler. This patch works independently of the
scheduler so it can be used with all of the currently available 
request based schedulers. 

Foreground IO, for the previously described results, is an async fio job 
submitting 4K read requests at a QD of 1 to the HDD. The foreground IO is set 
with the iopriority class of real time. The background workload is another fio
job submitting read requests at a QD of 32 to the same HDD with default 
iopriority.

This feature is enabled by setting a queue flag that is exposed as a sysfs
entry named req_prio. If this feature is enabled the request ioprio comes
from the highest priority between the iocontext and the bio. 

v2:
 - Add queue flag to set iopriority going to the request
 - If queue flag set, send iopriority class to ata_build_rw_tf
 - Remove redundant code in ata_ncq_prio_enabled function.


Adam Manzanares (2):
  ata: Enabling ATA Command Priorities
  block: Add iocontext priority to request

 block/blk-core.c       |  8 +++++++-
 block/blk-sysfs.c      | 32 ++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |  2 ++
 3 files changed, 41 insertions(+), 1 deletion(-)

-- 
2.1.4

[PATCH v2 1/2] ata: Enabling ATA Command Priorities

[Adam Manzanares]

This patch checks to see if an ATA device supports NCQ command priorities.
If so and the user has specified an iocontext that indicates IO_PRIO_CLASS_RT
and also enables request priorities in the block queue then we build a tf
with a high priority command.

This patch depends on patch block-Add-iocontext-priority-to-request

Signed-off-by: Adam Manzanares <adam.manzanares@hgst.com>
---
 drivers/ata/libata-core.c | 35 ++++++++++++++++++++++++++++++++++-
 drivers/ata/libata-scsi.c | 10 +++++++++-
 drivers/ata/libata.h      |  2 +-
 include/linux/ata.h       |  6 ++++++
 include/linux/libata.h    | 18 ++++++++++++++++++
 5 files changed, 68 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 223a770..181b530 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -739,6 +739,7 @@ u64 ata_tf_read_block(const struct ata_taskfile *tf, struct ata_device *dev)
  *	@n_block: Number of blocks
  *	@tf_flags: RW/FUA etc...
  *	@tag: tag
+ *	@class: IO priority class
  *
  *	LOCKING:
  *	None.
@@ -753,7 +754,7 @@ u64 ata_tf_read_block(const struct ata_taskfile *tf, struct ata_device *dev)
  */
 int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
 		    u64 block, u32 n_block, unsigned int tf_flags,
-		    unsigned int tag)
+		    unsigned int tag, int class)
 {
 	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
 	tf->flags |= tf_flags;
@@ -785,6 +786,12 @@ int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
 		tf->device = ATA_LBA;
 		if (tf->flags & ATA_TFLAG_FUA)
 			tf->device |= 1 << 7;
+
+		if (ata_ncq_prio_enabled(dev)) {
+			if (class == IOPRIO_CLASS_RT)
+				tf->hob_nsect |= ATA_PRIO_HIGH <<
+						 ATA_SHIFT_PRIO;
+		}
 	} else if (dev->flags & ATA_DFLAG_LBA) {
 		tf->flags |= ATA_TFLAG_LBA;
 
@@ -2156,6 +2163,30 @@ static void ata_dev_config_ncq_non_data(struct ata_device *dev)
 	}
 }
 
+static void ata_dev_config_ncq_prio(struct ata_device *dev)
+{
+	struct ata_port *ap = dev->link->ap;
+	unsigned int err_mask;
+
+	err_mask = ata_read_log_page(dev,
+				     ATA_LOG_SATA_ID_DEV_DATA,
+				     ATA_LOG_SATA_SETTINGS,
+				     ap->sector_buf,
+				     1);
+	if (err_mask) {
+		ata_dev_dbg(dev,
+			    "failed to get Identify Device data, Emask 0x%x\n",
+			    err_mask);
+		return;
+	}
+
+	if (ap->sector_buf[ATA_LOG_NCQ_PRIO_OFFSET] & BIT(3))
+		dev->flags |= ATA_DFLAG_NCQ_PRIO;
+	else
+		ata_dev_dbg(dev, "SATA page does not support priority\n");
+
+}
+
 static int ata_dev_config_ncq(struct ata_device *dev,
 			       char *desc, size_t desc_sz)
 {
@@ -2205,6 +2236,8 @@ static int ata_dev_config_ncq(struct ata_device *dev,
 			ata_dev_config_ncq_send_recv(dev);
 		if (ata_id_has_ncq_non_data(dev->id))
 			ata_dev_config_ncq_non_data(dev);
+		if (ata_id_has_ncq_prio(dev->id))
+			ata_dev_config_ncq_prio(dev);
 	}
 
 	return 0;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e207b33..65ea15d 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -50,6 +50,7 @@
 #include <linux/uaccess.h>
 #include <linux/suspend.h>
 #include <asm/unaligned.h>
+#include <linux/ioprio.h>
 
 #include "libata.h"
 #include "libata-transport.h"
@@ -1757,6 +1758,8 @@ static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *scmd = qc->scsicmd;
 	const u8 *cdb = scmd->cmnd;
+	struct request *rq = scmd->request;
+	int class = 0;
 	unsigned int tf_flags = 0;
 	u64 block;
 	u32 n_block;
@@ -1822,8 +1825,13 @@ static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc)
 	qc->flags |= ATA_QCFLAG_IO;
 	qc->nbytes = n_block * scmd->device->sector_size;
 
+	/* If queue supports req prio pass it onto the task file */
+	if (blk_queue_req_prio(rq->q))
+		class = IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
+
 	rc = ata_build_rw_tf(&qc->tf, qc->dev, block, n_block, tf_flags,
-			     qc->tag);
+			     qc->tag, class);
+
 	if (likely(rc == 0))
 		return 0;
 
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 3b301a4..8f3a559 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -66,7 +66,7 @@ extern u64 ata_tf_to_lba48(const struct ata_taskfile *tf);
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag);
 extern int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
 			   u64 block, u32 n_block, unsigned int tf_flags,
-			   unsigned int tag);
+			   unsigned int tag, int class);
 extern u64 ata_tf_read_block(const struct ata_taskfile *tf,
 			     struct ata_device *dev);
 extern unsigned ata_exec_internal(struct ata_device *dev,
diff --git a/include/linux/ata.h b/include/linux/ata.h
index adbc812..ebe4c3b 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -347,6 +347,7 @@ enum {
 	ATA_LOG_DEVSLP_DETO	  = 0x01,
 	ATA_LOG_DEVSLP_VALID	  = 0x07,
 	ATA_LOG_DEVSLP_VALID_MASK = 0x80,
+	ATA_LOG_NCQ_PRIO_OFFSET   = 0x09,
 
 	/* NCQ send and receive log */
 	ATA_LOG_NCQ_SEND_RECV_SUBCMDS_OFFSET	= 0x00,
@@ -897,6 +898,11 @@ static inline bool ata_id_has_ncq_non_data(const u16 *id)
 	return id[ATA_ID_SATA_CAPABILITY_2] & BIT(5);
 }
 
+static inline bool ata_id_has_ncq_prio(const u16 *id)
+{
+	return id[ATA_ID_SATA_CAPABILITY] & BIT(12);
+}
+
 static inline bool ata_id_has_trim(const u16 *id)
 {
 	if (ata_id_major_version(id) >= 7 &&
diff --git a/include/linux/libata.h b/include/linux/libata.h
index e37d4f9..244f261 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -165,6 +165,7 @@ enum {
 	ATA_DFLAG_NO_UNLOAD	= (1 << 17), /* device doesn't support unload */
 	ATA_DFLAG_UNLOCK_HPA	= (1 << 18), /* unlock HPA */
 	ATA_DFLAG_NCQ_SEND_RECV = (1 << 19), /* device supports NCQ SEND and RECV */
+	ATA_DFLAG_NCQ_PRIO	= (1 << 20), /* device supports NCQ priority */
 	ATA_DFLAG_INIT_MASK	= (1 << 24) - 1,
 
 	ATA_DFLAG_DETACH	= (1 << 24),
@@ -341,7 +342,9 @@ enum {
 	ATA_SHIFT_PIO		= 0,
 	ATA_SHIFT_MWDMA		= ATA_SHIFT_PIO + ATA_NR_PIO_MODES,
 	ATA_SHIFT_UDMA		= ATA_SHIFT_MWDMA + ATA_NR_MWDMA_MODES,
+	ATA_SHIFT_PRIO		= 6,
 
+	ATA_PRIO_HIGH		= 2,
 	/* size of buffer to pad xfers ending on unaligned boundaries */
 	ATA_DMA_PAD_SZ		= 4,
 
@@ -1609,6 +1612,21 @@ static inline int ata_ncq_enabled(struct ata_device *dev)
 			      ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ;
 }
 
+/**
+ *	ata_ncq_prio_enabled - Test whether NCQ prio is enabled
+ *	@dev: ATA device to test for
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host lock)
+ *
+ *	RETURNS:
+ *	1 if NCQ prio is enabled for @dev, 0 otherwise.
+ */
+static inline int ata_ncq_prio_enabled(struct ata_device *dev)
+{
+	return (dev->flags & ATA_DFLAG_NCQ_PRIO) == ATA_DFLAG_NCQ_PRIO;
+}
+
 static inline bool ata_fpdma_dsm_supported(struct ata_device *dev)
 {
 	return (dev->flags & ATA_DFLAG_NCQ_SEND_RECV) &&
-- 
2.1.4

Re: [PATCH v2 1/2] ata: Enabling ATA Command Priorities

[Hannes Reinecke]

On 10/05/2016 09:00 PM, Adam Manzanares wrote:
> This patch checks to see if an ATA device supports NCQ command priorities.
> If so and the user has specified an iocontext that indicates IO_PRIO_CLASS_RT
> and also enables request priorities in the block queue then we build a tf
> with a high priority command.
>
> This patch depends on patch block-Add-iocontext-priority-to-request
>
> Signed-off-by: Adam Manzanares <adam.manzanares@hgst.com>
> ---
>  drivers/ata/libata-core.c | 35 ++++++++++++++++++++++++++++++++++-
>  drivers/ata/libata-scsi.c | 10 +++++++++-
>  drivers/ata/libata.h      |  2 +-
>  include/linux/ata.h       |  6 ++++++
>  include/linux/libata.h    | 18 ++++++++++++++++++
>  5 files changed, 68 insertions(+), 3 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

[PATCH v2 2/2] block: Add iocontext priority to request

[Adam Manzanares]

Patch adds an association between iocontext ioprio and the ioprio of
a request. This feature is only enabled if a queue flag is set to
indicate that requests should have ioprio associated with them. The
queue flag is exposed as the req_prio queue sysfs entry.

Signed-off-by: Adam Mananzanares <adam.manzanares@hgst.com>
---
 block/blk-core.c       |  8 +++++++-
 block/blk-sysfs.c      | 32 ++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |  2 ++
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 36c7ac3..17c3ce5 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -33,6 +33,7 @@
 #include <linux/ratelimit.h>
 #include <linux/pm_runtime.h>
 #include <linux/blk-cgroup.h>
+#include <linux/ioprio.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/block.h>
@@ -1648,6 +1649,7 @@ out:
 
 void init_request_from_bio(struct request *req, struct bio *bio)
 {
+	struct io_context *ioc = rq_ioc(bio);
 	req->cmd_type = REQ_TYPE_FS;
 
 	req->cmd_flags |= bio->bi_opf & REQ_COMMON_MASK;
@@ -1656,7 +1658,11 @@ void init_request_from_bio(struct request *req, struct bio *bio)
 
 	req->errors = 0;
 	req->__sector = bio->bi_iter.bi_sector;
-	req->ioprio = bio_prio(bio);
+	if (blk_queue_req_prio(req->q))
+		req->ioprio = ioprio_best(bio_prio(bio), ioc->ioprio);
+	else
+		req->ioprio = bio_prio(bio);
+
 	blk_rq_bio_prep(req->q, req, bio);
 }
 
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index f87a7e7..268a71a 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -384,6 +384,31 @@ static ssize_t queue_dax_show(struct request_queue *q, char *page)
 	return queue_var_show(blk_queue_dax(q), page);
 }
 
+static ssize_t queue_req_prio_show(struct request_queue *q, char *page)
+{
+	return queue_var_show(blk_queue_req_prio(q), page);
+}
+
+static ssize_t queue_req_prio_store(struct request_queue *q, const char *page,
+				    size_t count)
+{
+	unsigned long req_prio_on;
+	ssize_t ret;
+
+	ret = queue_var_store(&req_prio_on, page, count);
+	if (ret < 0)
+		return ret;
+
+	spin_lock_irq(q->queue_lock);
+	if (req_prio_on)
+		queue_flag_set(QUEUE_FLAG_REQ_PRIO, q);
+	else
+		queue_flag_clear(QUEUE_FLAG_REQ_PRIO, q);
+	spin_unlock_irq(q->queue_lock);
+
+	return ret;
+}
+
 static struct queue_sysfs_entry queue_requests_entry = {
 	.attr = {.name = "nr_requests", .mode = S_IRUGO | S_IWUSR },
 	.show = queue_requests_show,
@@ -526,6 +551,12 @@ static struct queue_sysfs_entry queue_dax_entry = {
 	.show = queue_dax_show,
 };
 
+static struct queue_sysfs_entry queue_req_prio_entry = {
+	.attr = {.name = "req_prio", .mode = S_IRUGO | S_IWUSR },
+	.show = queue_req_prio_show,
+	.store = queue_req_prio_store,
+};
+
 static struct attribute *default_attrs[] = {
 	&queue_requests_entry.attr,
 	&queue_ra_entry.attr,
@@ -553,6 +584,7 @@ static struct attribute *default_attrs[] = {
 	&queue_poll_entry.attr,
 	&queue_wc_entry.attr,
 	&queue_dax_entry.attr,
+	&queue_req_prio_entry.attr,
 	NULL,
 };
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index e79055c..23e1e2d 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -505,6 +505,7 @@ struct request_queue {
 #define QUEUE_FLAG_FUA	       24	/* device supports FUA writes */
 #define QUEUE_FLAG_FLUSH_NQ    25	/* flush not queueuable */
 #define QUEUE_FLAG_DAX         26	/* device supports DAX */
+#define QUEUE_FLAG_REQ_PRIO    27	/* Use iocontext ioprio */
 
 #define QUEUE_FLAG_DEFAULT	((1 << QUEUE_FLAG_IO_STAT) |		\
 				 (1 << QUEUE_FLAG_STACKABLE)	|	\
@@ -595,6 +596,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
 #define blk_queue_secure_erase(q) \
 	(test_bit(QUEUE_FLAG_SECERASE, &(q)->queue_flags))
 #define blk_queue_dax(q)	test_bit(QUEUE_FLAG_DAX, &(q)->queue_flags)
+#define blk_queue_req_prio(q)	test_bit(QUEUE_FLAG_REQ_PRIO, &(q)->queue_flags)
 
 #define blk_noretry_request(rq) \
 	((rq)->cmd_flags & (REQ_FAILFAST_DEV|REQ_FAILFAST_TRANSPORT| \
-- 
2.1.4

Re: [PATCH v2 2/2] block: Add iocontext priority to request

[Hannes Reinecke]

On 10/05/2016 09:00 PM, Adam Manzanares wrote:
> Patch adds an association between iocontext ioprio and the ioprio of
> a request. This feature is only enabled if a queue flag is set to
> indicate that requests should have ioprio associated with them. The
> queue flag is exposed as the req_prio queue sysfs entry.
>
> Signed-off-by: Adam Mananzanares <adam.manzanares@hgst.com>
> ---
>  block/blk-core.c       |  8 +++++++-
>  block/blk-sysfs.c      | 32 ++++++++++++++++++++++++++++++++
>  include/linux/blkdev.h |  2 ++
>  3 files changed, 41 insertions(+), 1 deletion(-)
>
As the previous patch depends on this one it should actually be the 
first in the series.

But other than that:

Reviewed-by: Hannes Reinecke <hare@xuse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH v2 2/2] block: Add iocontext priority to request

[Jeff Moyer]

Hi, Adam,

Adam Manzanares <adam.manzanares@hgst.com> writes:

> Patch adds an association between iocontext ioprio and the ioprio of
> a request. This feature is only enabled if a queue flag is set to
> indicate that requests should have ioprio associated with them. The
> queue flag is exposed as the req_prio queue sysfs entry.
>
> Signed-off-by: Adam Mananzanares <adam.manzanares@hgst.com>

I like the idea of the patch, but I have a few comments.

First, don't add a tunable, there's no need for it.  (And in the future,
if you do add tunables, document them.)  That should make your patch
much smaller.

> @@ -1648,6 +1649,7 @@ out:
>  
>  void init_request_from_bio(struct request *req, struct bio *bio)
>  {
> +	struct io_context *ioc = rq_ioc(bio);

That can return NULL, and you blindly dereference it later.

> @@ -1656,7 +1658,11 @@ void init_request_from_bio(struct request *req, struct bio *bio)
>  
>  	req->errors = 0;
>  	req->__sector = bio->bi_iter.bi_sector;
> -	req->ioprio = bio_prio(bio);
> +	if (blk_queue_req_prio(req->q))
> +		req->ioprio = ioprio_best(bio_prio(bio), ioc->ioprio);
> +	else
> +		req->ioprio = bio_prio(bio);
> +

If the bio actually has an ioprio (only happens for bcache at this
point), you should use it.  Something like this:

        req->ioprio = bio_prio(bio);
        if (!req->ioprio && ioc)
		req->ioprio = ioc->ioprio;

Finally, please re-order your series as Hannes suggested.

Thanks!
Jeff

Re: [PATCH v2 2/2] block: Add iocontext priority to request

[Adam Manzanares]

Hello Jeff,

The 10/06/2016 15:46, Jeff Moyer wrote:
> Hi, Adam,
> 
> Adam Manzanares <adam.manzanares@hgst.com> writes:
> 
> > Patch adds an association between iocontext ioprio and the ioprio of
> > a request. This feature is only enabled if a queue flag is set to
> > indicate that requests should have ioprio associated with them. The
> > queue flag is exposed as the req_prio queue sysfs entry.
> >
> > Signed-off-by: Adam Mananzanares <adam.manzanares@hgst.com>
> 
> I like the idea of the patch, but I have a few comments.
> 
> First, don't add a tunable, there's no need for it.  (And in the future,
> if you do add tunables, document them.)  That should make your patch
> much smaller.
> 

I have a strong preference for making this a tunable for the following 
reason. I am concerned that this could negatively impact performance if this 
feature is not properly implemented on a device. In addition, this feature 
can make a dramatic difference in the performance of prioritized vs 
non-prioritized IO. Priority IO is improved, but it comes at the cost of 
non-prioritized IO. If someone has tuned a system in such a way that things 
work well as is, I do not want to cause any surprises.

I can see the argument for not having the tunable in the block layer, but 
then we need to add a tunable to all request based drivers that may leverage
the iopriority information. This has the potential to generate a lot more 
code and documentation.  I also would like to use the tunable when the 
iopriority is set on the request so we can preserve the default behavior. 
This can be a concern when we have drivers that use request iopriority 
information, such as the fusion mptsas driver.

I will also document the tunable :) if we agree that it is necessary.

> > @@ -1648,6 +1649,7 @@ out:
> >  
> >  void init_request_from_bio(struct request *req, struct bio *bio)
> >  {
> > +	struct io_context *ioc = rq_ioc(bio);
> 
> That can return NULL, and you blindly dereference it later.
>

Ouch, this will be cleaned up in the next revision.

> > @@ -1656,7 +1658,11 @@ void init_request_from_bio(struct request *req, struct bio *bio)
> >  
> >  	req->errors = 0;
> >  	req->__sector = bio->bi_iter.bi_sector;
> > -	req->ioprio = bio_prio(bio);
> > +	if (blk_queue_req_prio(req->q))
> > +		req->ioprio = ioprio_best(bio_prio(bio), ioc->ioprio);
> > +	else
> > +		req->ioprio = bio_prio(bio);
> > +
> 
> If the bio actually has an ioprio (only happens for bcache at this
> point), you should use it.  Something like this:
> 
>         req->ioprio = bio_prio(bio);
>         if (!req->ioprio && ioc)
> 		req->ioprio = ioc->ioprio;
>

I caught this in the explanation of the first patch I sent out. I am still
assuming that this will be a tunable, but I will have the bio_prio take 
precedence in the next patch.

> Finally, please re-order your series as Hannes suggested.

Will do. 

> 
> Thanks!
> Jeff

Take care,
Adam