[PATCH v4 0/4] Enabling ATA Command Priorities

[PATCH v4 0/4] 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 required setting the iocontext ioprio on the request when 
the request is initialized.

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 for ATA devices by setting the ata enable_prio device 
attribute to 1. An ATA device is also checked to see if the device supports per
command priority.

v4:
 - Added blk_rq_set_prio function to associate request prio with ioc prio
 - In init_request_from_bio use bio_prio if it is valid
 - Added ata enable_prio dev attribute to sysfs to enable prioritized commands

v3:
 - Removed null dereference issue in blk-core
 - Renamed queue sysfs entries for clarity
 - Added documentation for sysfs queue entry

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 (4):
  block: Add iocontext priority to request
  fusion: remove iopriority handling
  ata: Enabling ATA Command Priorities
  ata: ATA Command Priority Disabled By Default

 block/blk-core.c                  |  4 ++-
 drivers/ata/libahci.c             |  1 +
 drivers/ata/libata-core.c         | 35 +++++++++++++++++-
 drivers/ata/libata-scsi.c         | 74 ++++++++++++++++++++++++++++++++++++++-
 drivers/ata/libata.h              |  2 +-
 drivers/message/fusion/mptscsih.c |  5 ---
 include/linux/ata.h               |  6 ++++
 include/linux/blkdev.h            | 14 ++++++++
 include/linux/libata.h            | 26 ++++++++++++++
 9 files changed, 158 insertions(+), 9 deletions(-)

-- 
2.1.4

[PATCH v4 1/4] block: Add iocontext priority to request

[Adam Manzanares]

Patch adds an association between iocontext ioprio and the ioprio of a
request. This value is set in blk_rq_set_prio which takes the request and
the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
iopriority of the request is set as the iopriority of the ioc. In
init_request_from_bio a check is made to see if the ioprio of the bio is
valid and if so then the request prio comes from the bio.

Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
---
 block/blk-core.c       |  4 +++-
 include/linux/blkdev.h | 14 ++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 14d7c07..361b1b9 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1153,6 +1153,7 @@ static struct request *__get_request(struct request_list *rl, int op,
 
 	blk_rq_init(q, rq);
 	blk_rq_set_rl(rq, rl);
+	blk_rq_set_prio(rq, ioc);
 	req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
 
 	/* init elvpriv */
@@ -1656,7 +1657,8 @@ 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 (ioprio_valid(bio_prio(bio)))
+		req->ioprio = bio_prio(bio);
 	blk_rq_bio_prep(req->q, req, bio);
 }
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c47c358..9a0ceaa 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -934,6 +934,20 @@ static inline unsigned int blk_rq_count_bios(struct request *rq)
 }
 
 /*
+ * blk_rq_set_prio - associate a request with prio from ioc
+ * @rq: request of interest
+ * @ioc: target iocontext
+ *
+ * Assocate request prio with ioc prio so request based drivers
+ * can leverage priority information.
+ */
+static inline void blk_rq_set_prio(struct request *rq, struct io_context *ioc)
+{
+	if (ioc)
+		rq->ioprio = ioc->ioprio;
+}
+
+/*
  * Request issue related functions.
  */
 extern struct request *blk_peek_request(struct request_queue *q);
-- 
2.1.4

[PATCH v4 2/4] fusion: remove iopriority handling

[Adam Manzanares]

The request priority is now by default coming from the ioc. It was not
clear what this code was trying to do based upon the iopriority class or
data. The driver should check that a device supports priorities and use
them according to the specificiations of ioprio.

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
 drivers/message/fusion/mptscsih.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/message/fusion/mptscsih.c b/drivers/message/fusion/mptscsih.c
index 6c9fc11..4740bb6 100644
--- a/drivers/message/fusion/mptscsih.c
+++ b/drivers/message/fusion/mptscsih.c
@@ -1369,11 +1369,6 @@ mptscsih_qcmd(struct scsi_cmnd *SCpnt)
 	if ((vdevice->vtarget->tflags & MPT_TARGET_FLAGS_Q_YES)
 	    && (SCpnt->device->tagged_supported)) {
 		scsictl = scsidir | MPI_SCSIIO_CONTROL_SIMPLEQ;
-		if (SCpnt->request && SCpnt->request->ioprio) {
-			if (((SCpnt->request->ioprio & 0x7) == 1) ||
-				!(SCpnt->request->ioprio & 0x7))
-				scsictl |= MPI_SCSIIO_CONTROL_HEADOFQ;
-		}
 	} else
 		scsictl = scsidir | MPI_SCSIIO_CONTROL_UNTAGGED;
 
-- 
2.1.4

[PATCH v4 3/4] 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
then we build a tf with a high priority command.

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
 drivers/ata/libata-core.c | 35 ++++++++++++++++++++++++++++++++++-
 drivers/ata/libata-scsi.c |  6 +++++-
 drivers/ata/libata.h      |  2 +-
 include/linux/ata.h       |  6 ++++++
 include/linux/libata.h    | 18 ++++++++++++++++++
 5 files changed, 64 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..18629e8 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 = IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
 	unsigned int tf_flags = 0;
 	u64 block;
 	u32 n_block;
@@ -1823,7 +1826,8 @@ static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc)
 	qc->nbytes = n_block * scmd->device->sector_size;
 
 	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

[PATCH v4 4/4] ata: ATA Command Priority Disabled By Default

[Adam Manzanares]

Add a sysfs entry to turn on priority information being passed
to a ATA device. By default this feature is turned off.

This patch depends on ata: Enabling ATA Command Priorities

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
 drivers/ata/libahci.c     |  1 +
 drivers/ata/libata-core.c |  2 +-
 drivers/ata/libata-scsi.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/libata.h    |  8 ++++++
 4 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index dcf2c72..383adf7 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -140,6 +140,7 @@ EXPORT_SYMBOL_GPL(ahci_shost_attrs);
 struct device_attribute *ahci_sdev_attrs[] = {
 	&dev_attr_sw_activity,
 	&dev_attr_unload_heads,
+	&dev_attr_enable_prio,
 	NULL
 };
 EXPORT_SYMBOL_GPL(ahci_sdev_attrs);
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 181b530..d0cf987 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -787,7 +787,7 @@ int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
 		if (tf->flags & ATA_TFLAG_FUA)
 			tf->device |= 1 << 7;
 
-		if (ata_ncq_prio_enabled(dev)) {
+		if (ata_ncq_prio_enabled(dev) && ata_prio_enabled(dev)) {
 			if (class == IOPRIO_CLASS_RT)
 				tf->hob_nsect |= ATA_PRIO_HIGH <<
 						 ATA_SHIFT_PRIO;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 18629e8..10ba118 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -271,6 +271,73 @@ DEVICE_ATTR(unload_heads, S_IRUGO | S_IWUSR,
 	    ata_scsi_park_show, ata_scsi_park_store);
 EXPORT_SYMBOL_GPL(dev_attr_unload_heads);
 
+static ssize_t ata_enable_prio_show(struct device *device,
+				    struct device_attribute *attr, char *buf)
+{
+	struct scsi_device *sdev = to_scsi_device(device);
+	struct ata_port *ap;
+	struct ata_device *dev;
+	int rc = 0;
+	int enable_prio;
+
+	ap = ata_shost_to_port(sdev->host);
+
+	spin_lock_irq(ap->lock);
+	dev = ata_scsi_find_dev(ap, sdev);
+	if (!dev) {
+		rc = -ENODEV;
+		goto unlock;
+	}
+
+	enable_prio = ata_prio_enabled(dev);
+
+unlock:
+	spin_unlock_irq(ap->lock);
+
+	return rc ? rc : snprintf(buf, 20, "%u\n", enable_prio);
+}
+
+static ssize_t ata_enable_prio_store(struct device *device,
+				     struct device_attribute *attr,
+				     const char *buf, size_t len)
+{
+	struct scsi_device *sdev = to_scsi_device(device);
+	struct ata_port *ap;
+	struct ata_device *dev;
+	long int input;
+	unsigned long flags;
+	int rc;
+
+	rc = kstrtol(buf, 10, &input);
+	if (rc)
+		return rc;
+	if ((input < 0) || (input > 1))
+		return -EINVAL;
+
+	ap = ata_shost_to_port(sdev->host);
+
+	spin_lock_irqsave(ap->lock, flags);
+	dev = ata_scsi_find_dev(ap, sdev);
+	if (unlikely(!dev)) {
+		rc = -ENODEV;
+		goto unlock;
+	}
+
+	if (input)
+		dev->flags |= ATA_DFLAG_ENABLE_PRIO;
+	else
+		dev->flags &= ~ATA_DFLAG_ENABLE_PRIO;
+
+unlock:
+	spin_unlock_irqrestore(ap->lock, flags);
+
+	return rc ? rc : len;
+}
+
+DEVICE_ATTR(enable_prio, S_IRUGO | S_IWUSR,
+	    ata_enable_prio_show, ata_enable_prio_store);
+EXPORT_SYMBOL_GPL(dev_attr_enable_prio);
+
 void ata_scsi_set_sense(struct ata_device *dev, struct scsi_cmnd *cmd,
 			u8 sk, u8 asc, u8 ascq)
 {
@@ -402,6 +469,7 @@ EXPORT_SYMBOL_GPL(dev_attr_sw_activity);
 
 struct device_attribute *ata_common_sdev_attrs[] = {
 	&dev_attr_unload_heads,
+	&dev_attr_enable_prio,
 	NULL
 };
 EXPORT_SYMBOL_GPL(ata_common_sdev_attrs);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 244f261..c8acb16 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -166,6 +166,7 @@ enum {
 	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_ENABLE_PRIO	= (1 << 21), /* User enable device priority */
 	ATA_DFLAG_INIT_MASK	= (1 << 24) - 1,
 
 	ATA_DFLAG_DETACH	= (1 << 24),
@@ -544,6 +545,7 @@ typedef void (*ata_postreset_fn_t)(struct ata_link *link, unsigned int *classes)
 
 extern struct device_attribute dev_attr_link_power_management_policy;
 extern struct device_attribute dev_attr_unload_heads;
+extern struct device_attribute dev_attr_enable_prio;
 extern struct device_attribute dev_attr_em_message_type;
 extern struct device_attribute dev_attr_em_message;
 extern struct device_attribute dev_attr_sw_activity;
@@ -1627,6 +1629,12 @@ static inline int ata_ncq_prio_enabled(struct ata_device *dev)
 	return (dev->flags & ATA_DFLAG_NCQ_PRIO) == ATA_DFLAG_NCQ_PRIO;
 }
 
+static inline int ata_prio_enabled(struct ata_device *dev)
+{
+	return ((dev->flags & ATA_DFLAG_ENABLE_PRIO) ==
+		 ATA_DFLAG_ENABLE_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 v4 1/4] block: Add iocontext priority to request

[Dan Williams]

On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
<adam.manzanares@hgst.com> wrote:
> Patch adds an association between iocontext ioprio and the ioprio of a
> request. This value is set in blk_rq_set_prio which takes the request and
> the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
> iopriority of the request is set as the iopriority of the ioc. In
> init_request_from_bio a check is made to see if the ioprio of the bio is
> valid and if so then the request prio comes from the bio.
>
> Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
> ---
>  block/blk-core.c       |  4 +++-
>  include/linux/blkdev.h | 14 ++++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 14d7c07..361b1b9 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1153,6 +1153,7 @@ static struct request *__get_request(struct request_list *rl, int op,
>
>         blk_rq_init(q, rq);
>         blk_rq_set_rl(rq, rl);
> +       blk_rq_set_prio(rq, ioc);
>         req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
>
>         /* init elvpriv */
> @@ -1656,7 +1657,8 @@ 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 (ioprio_valid(bio_prio(bio)))
> +               req->ioprio = bio_prio(bio);

Should we use ioprio_best() here?  If req->ioprio and bio_prio()
disagree one side has explicitly asked for a higher priority.

Re: [PATCH v4 1/4] block: Add iocontext priority to request

[Jens Axboe]

On 10/13/2016 02:06 PM, Dan Williams wrote:
> On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
> <adam.manzanares@hgst.com> wrote:
>> Patch adds an association between iocontext ioprio and the ioprio of a
>> request. This value is set in blk_rq_set_prio which takes the request and
>> the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
>> iopriority of the request is set as the iopriority of the ioc. In
>> init_request_from_bio a check is made to see if the ioprio of the bio is
>> valid and if so then the request prio comes from the bio.
>>
>> Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
>> ---
>>  block/blk-core.c       |  4 +++-
>>  include/linux/blkdev.h | 14 ++++++++++++++
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 14d7c07..361b1b9 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -1153,6 +1153,7 @@ static struct request *__get_request(struct request_list *rl, int op,
>>
>>         blk_rq_init(q, rq);
>>         blk_rq_set_rl(rq, rl);
>> +       blk_rq_set_prio(rq, ioc);
>>         req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
>>
>>         /* init elvpriv */
>> @@ -1656,7 +1657,8 @@ 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 (ioprio_valid(bio_prio(bio)))
>> +               req->ioprio = bio_prio(bio);
>
> Should we use ioprio_best() here?  If req->ioprio and bio_prio()
> disagree one side has explicitly asked for a higher priority.

It's a good question - but if priority has been set in the bio, it makes
sense that that would take priority over the general setting for the
task/io context. So I think the patch is correct as-is.

Adam, you'll want to rewrite the commit message though. A good commit
message should explain WHY the change is made, not detail the code
implementation of it.

-- 
Jens Axboe

Re: [PATCH v4 1/4] block: Add iocontext priority to request

[Dan Williams]

On Thu, Oct 13, 2016 at 1:09 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 10/13/2016 02:06 PM, Dan Williams wrote:
>>
>> On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
>> <adam.manzanares@hgst.com> wrote:
>>>
>>> Patch adds an association between iocontext ioprio and the ioprio of a
>>> request. This value is set in blk_rq_set_prio which takes the request and
>>> the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
>>> iopriority of the request is set as the iopriority of the ioc. In
>>> init_request_from_bio a check is made to see if the ioprio of the bio is
>>> valid and if so then the request prio comes from the bio.
>>>
>>> Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
>>> ---
>>>  block/blk-core.c       |  4 +++-
>>>  include/linux/blkdev.h | 14 ++++++++++++++
>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>> index 14d7c07..361b1b9 100644
>>> --- a/block/blk-core.c
>>> +++ b/block/blk-core.c
>>> @@ -1153,6 +1153,7 @@ static struct request *__get_request(struct
>>> request_list *rl, int op,
>>>
>>>         blk_rq_init(q, rq);
>>>         blk_rq_set_rl(rq, rl);
>>> +       blk_rq_set_prio(rq, ioc);
>>>         req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
>>>
>>>         /* init elvpriv */
>>> @@ -1656,7 +1657,8 @@ 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 (ioprio_valid(bio_prio(bio)))
>>> +               req->ioprio = bio_prio(bio);
>>
>>
>> Should we use ioprio_best() here?  If req->ioprio and bio_prio()
>> disagree one side has explicitly asked for a higher priority.
>
>
> It's a good question - but if priority has been set in the bio, it makes
> sense that that would take priority over the general setting for the
> task/io context. So I think the patch is correct as-is.

Assuming you always trust the kernel to know the right priority...

Re: [PATCH v4 1/4] block: Add iocontext priority to request

[Jens Axboe]

On 10/13/2016 02:19 PM, Dan Williams wrote:
> On Thu, Oct 13, 2016 at 1:09 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 10/13/2016 02:06 PM, Dan Williams wrote:
>>>
>>> On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
>>> <adam.manzanares@hgst.com> wrote:
>>>>
>>>> Patch adds an association between iocontext ioprio and the ioprio of a
>>>> request. This value is set in blk_rq_set_prio which takes the request and
>>>> the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
>>>> iopriority of the request is set as the iopriority of the ioc. In
>>>> init_request_from_bio a check is made to see if the ioprio of the bio is
>>>> valid and if so then the request prio comes from the bio.
>>>>
>>>> Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
>>>> ---
>>>>  block/blk-core.c       |  4 +++-
>>>>  include/linux/blkdev.h | 14 ++++++++++++++
>>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>> index 14d7c07..361b1b9 100644
>>>> --- a/block/blk-core.c
>>>> +++ b/block/blk-core.c
>>>> @@ -1153,6 +1153,7 @@ static struct request *__get_request(struct
>>>> request_list *rl, int op,
>>>>
>>>>         blk_rq_init(q, rq);
>>>>         blk_rq_set_rl(rq, rl);
>>>> +       blk_rq_set_prio(rq, ioc);
>>>>         req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
>>>>
>>>>         /* init elvpriv */
>>>> @@ -1656,7 +1657,8 @@ 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 (ioprio_valid(bio_prio(bio)))
>>>> +               req->ioprio = bio_prio(bio);
>>>
>>>
>>> Should we use ioprio_best() here?  If req->ioprio and bio_prio()
>>> disagree one side has explicitly asked for a higher priority.
>>
>>
>> It's a good question - but if priority has been set in the bio, it makes
>> sense that that would take priority over the general setting for the
>> task/io context. So I think the patch is correct as-is.
>
> Assuming you always trust the kernel to know the right priority...

If it set it in the bio, it better know what it's doing. Besides,
there's nothing stopping the caller from checking the task priority when
it sets it. If we do ioprio_best(), then we are effectively preventing
anyone from submitting a bio with a lower priority than the task has
generally set.

Now, this depends on the priority being set in req->ioprio is
exclusively inherited from ioc->ioprio. That's the case for file system
requests, but if someone allocated a request and set the priority
otherwise, then ioprio_best() would be correct.

-- 
Jens Axboe

Re: [PATCH v4 1/4] block: Add iocontext priority to request

[Dan Williams]

On Thu, Oct 13, 2016 at 1:24 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 10/13/2016 02:19 PM, Dan Williams wrote:
>>
>> On Thu, Oct 13, 2016 at 1:09 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> On 10/13/2016 02:06 PM, Dan Williams wrote:
>>>>
>>>>
>>>> On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
>>>> <adam.manzanares@hgst.com> wrote:
>>>>>
>>>>>
>>>>> Patch adds an association between iocontext ioprio and the ioprio of a
>>>>> request. This value is set in blk_rq_set_prio which takes the request
>>>>> and
>>>>> the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
>>>>> iopriority of the request is set as the iopriority of the ioc. In
>>>>> init_request_from_bio a check is made to see if the ioprio of the bio
>>>>> is
>>>>> valid and if so then the request prio comes from the bio.
>>>>>
>>>>> Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
>>>>> ---
>>>>>  block/blk-core.c       |  4 +++-
>>>>>  include/linux/blkdev.h | 14 ++++++++++++++
>>>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>>> index 14d7c07..361b1b9 100644
>>>>> --- a/block/blk-core.c
>>>>> +++ b/block/blk-core.c
>>>>> @@ -1153,6 +1153,7 @@ static struct request *__get_request(struct
>>>>> request_list *rl, int op,
>>>>>
>>>>>         blk_rq_init(q, rq);
>>>>>         blk_rq_set_rl(rq, rl);
>>>>> +       blk_rq_set_prio(rq, ioc);
>>>>>         req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
>>>>>
>>>>>         /* init elvpriv */
>>>>> @@ -1656,7 +1657,8 @@ 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 (ioprio_valid(bio_prio(bio)))
>>>>> +               req->ioprio = bio_prio(bio);
>>>>
>>>>
>>>>
>>>> Should we use ioprio_best() here?  If req->ioprio and bio_prio()
>>>> disagree one side has explicitly asked for a higher priority.
>>>
>>>
>>>
>>> It's a good question - but if priority has been set in the bio, it makes
>>> sense that that would take priority over the general setting for the
>>> task/io context. So I think the patch is correct as-is.
>>
>>
>> Assuming you always trust the kernel to know the right priority...
>
>
> If it set it in the bio, it better know what it's doing. Besides,
> there's nothing stopping the caller from checking the task priority when
> it sets it. If we do ioprio_best(), then we are effectively preventing
> anyone from submitting a bio with a lower priority than the task has
> generally set.

Ah, that makes sense.  Move the ioprio_best() decision out to whatever
code is setting bio_prio() to allow for cases where the kernel knows
best.

RE: [PATCH v4 2/4] fusion: remove iopriority handling

[Sathya Prakash Veerichetty]

By removing the code below, we put all the commands for all the types of
devices (SAS/SATA) as simple-Q (requeue as the device require) and I am
not sure whether it is the intention of this change.

-----Original Message-----
From: Adam Manzanares [mailto:adam.manzanares@hgst.com]
Sent: Thursday, October 13, 2016 1:54 PM
To: axboe@kernel.dk; tj@kernel.org; dan.j.williams@intel.com;
hare@suse.de; martin.petersen@oracle.com; mchristi@redhat.com;
toshi.kani@hpe.com; ming.lei@canonical.com; sathya.prakash@broadcom.com;
chaitra.basappa@broadcom.com; suganath-prabu.subramani@broadcom.com
Cc: linux-block@vger.kernel.org; linux-ide@vger.kernel.org;
linux-kernel@vger.kernel.org; MPT-FusionLinux.pdl@broadcom.com;
linux-scsi@vger.kernel.org; Adam Manzanares; Adam Manzanares
Subject: [PATCH v4 2/4] fusion: remove iopriority handling

The request priority is now by default coming from the ioc. It was not
clear what this code was trying to do based upon the iopriority class or
data. The driver should check that a device supports priorities and use
them according to the specificiations of ioprio.

Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
---
 drivers/message/fusion/mptscsih.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/message/fusion/mptscsih.c
b/drivers/message/fusion/mptscsih.c
index 6c9fc11..4740bb6 100644
--- a/drivers/message/fusion/mptscsih.c
+++ b/drivers/message/fusion/mptscsih.c
@@ -1369,11 +1369,6 @@ mptscsih_qcmd(struct scsi_cmnd *SCpnt)
 	if ((vdevice->vtarget->tflags & MPT_TARGET_FLAGS_Q_YES)
 	    && (SCpnt->device->tagged_supported)) {
 		scsictl = scsidir | MPI_SCSIIO_CONTROL_SIMPLEQ;
-		if (SCpnt->request && SCpnt->request->ioprio) {
-			if (((SCpnt->request->ioprio & 0x7) == 1) ||
-				!(SCpnt->request->ioprio & 0x7))
-				scsictl |= MPI_SCSIIO_CONTROL_HEADOFQ;
-		}
 	} else
 		scsictl = scsidir | MPI_SCSIIO_CONTROL_UNTAGGED;

--
2.1.4

Western Digital Corporation (and its subsidiaries) E-mail Confidentiality
Notice & Disclaimer:

This e-mail and any files transmitted with it may contain confidential or
legally privileged information of WDC and/or its affiliates, and are
intended solely for the use of the individual or entity to which they are
addressed. If you are not the intended recipient, any disclosure, copying,
distribution or any action taken or omitted to be taken in reliance on it,
is prohibited. If you have received this e-mail in error, please notify
the sender immediately and delete the e-mail in its entirety from your
system.

Re: [PATCH v4 1/4] block: Add iocontext priority to request

[Jens Axboe]

On 10/13/2016 02:59 PM, Dan Williams wrote:
> On Thu, Oct 13, 2016 at 1:24 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 10/13/2016 02:19 PM, Dan Williams wrote:
>>>
>>> On Thu, Oct 13, 2016 at 1:09 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>
>>>> On 10/13/2016 02:06 PM, Dan Williams wrote:
>>>>>
>>>>>
>>>>> On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
>>>>> <adam.manzanares@hgst.com> wrote:
>>>>>>
>>>>>>
>>>>>> Patch adds an association between iocontext ioprio and the ioprio of a
>>>>>> request. This value is set in blk_rq_set_prio which takes the request
>>>>>> and
>>>>>> the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
>>>>>> iopriority of the request is set as the iopriority of the ioc. In
>>>>>> init_request_from_bio a check is made to see if the ioprio of the bio
>>>>>> is
>>>>>> valid and if so then the request prio comes from the bio.
>>>>>>
>>>>>> Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
>>>>>> ---
>>>>>>  block/blk-core.c       |  4 +++-
>>>>>>  include/linux/blkdev.h | 14 ++++++++++++++
>>>>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>>>> index 14d7c07..361b1b9 100644
>>>>>> --- a/block/blk-core.c
>>>>>> +++ b/block/blk-core.c
>>>>>> @@ -1153,6 +1153,7 @@ static struct request *__get_request(struct
>>>>>> request_list *rl, int op,
>>>>>>
>>>>>>         blk_rq_init(q, rq);
>>>>>>         blk_rq_set_rl(rq, rl);
>>>>>> +       blk_rq_set_prio(rq, ioc);
>>>>>>         req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
>>>>>>
>>>>>>         /* init elvpriv */
>>>>>> @@ -1656,7 +1657,8 @@ 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 (ioprio_valid(bio_prio(bio)))
>>>>>> +               req->ioprio = bio_prio(bio);
>>>>>
>>>>>
>>>>>
>>>>> Should we use ioprio_best() here?  If req->ioprio and bio_prio()
>>>>> disagree one side has explicitly asked for a higher priority.
>>>>
>>>>
>>>>
>>>> It's a good question - but if priority has been set in the bio, it makes
>>>> sense that that would take priority over the general setting for the
>>>> task/io context. So I think the patch is correct as-is.
>>>
>>>
>>> Assuming you always trust the kernel to know the right priority...
>>
>>
>> If it set it in the bio, it better know what it's doing. Besides,
>> there's nothing stopping the caller from checking the task priority when
>> it sets it. If we do ioprio_best(), then we are effectively preventing
>> anyone from submitting a bio with a lower priority than the task has
>> generally set.
>
> Ah, that makes sense.  Move the ioprio_best() decision out to whatever
> code is setting bio_prio() to allow for cases where the kernel knows
> best.

Yes, precisely.

-- 
Jens Axboe

Re: [PATCH v4 1/4] block: Add iocontext priority to request

[Adam Manzananares]

The 10/13/2016 14:09, Jens Axboe wrote:
> On 10/13/2016 02:06 PM, Dan Williams wrote:
> >On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
> ><adam.manzanares@hgst.com> wrote:
> >>Patch adds an association between iocontext ioprio and the ioprio of a
> >>request. This value is set in blk_rq_set_prio which takes the request and
> >>the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
> >>iopriority of the request is set as the iopriority of the ioc. In
> >>init_request_from_bio a check is made to see if the ioprio of the bio is
> >>valid and if so then the request prio comes from the bio.
> >>
> >>Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
> >>---
> >> block/blk-core.c       |  4 +++-
> >> include/linux/blkdev.h | 14 ++++++++++++++
> >> 2 files changed, 17 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/block/blk-core.c b/block/blk-core.c
> >>index 14d7c07..361b1b9 100644
> >>--- a/block/blk-core.c
> >>+++ b/block/blk-core.c
> >>@@ -1153,6 +1153,7 @@ static struct request *__get_request(struct request_list *rl, int op,
> >>
> >>        blk_rq_init(q, rq);
> >>        blk_rq_set_rl(rq, rl);
> >>+       blk_rq_set_prio(rq, ioc);
> >>        req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
> >>
> >>        /* init elvpriv */
> >>@@ -1656,7 +1657,8 @@ 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 (ioprio_valid(bio_prio(bio)))
> >>+               req->ioprio = bio_prio(bio);
> >
> >Should we use ioprio_best() here?  If req->ioprio and bio_prio()
> >disagree one side has explicitly asked for a higher priority.
> 
> It's a good question - but if priority has been set in the bio, it makes
> sense that that would take priority over the general setting for the
> task/io context. So I think the patch is correct as-is.
> 
> Adam, you'll want to rewrite the commit message though. A good commit
> message should explain WHY the change is made, not detail the code
> implementation of it.

Got it I'll send something out soon.

> 
> -- 
> Jens Axboe
> 

Take care,
Adam

Re: [PATCH v4 2/4] fusion: remove iopriority handling

[Adam Manzanares]

The 10/13/2016 15:05, Sathya Prakash Veerichetty wrote:
> By removing the code below, we put all the commands for all the types of
> devices (SAS/SATA) as simple-Q (requeue as the device require) and I am
> not sure whether it is the intention of this change.
> 

This is the intention of the change. I don't think the iopriority of the
request is being used correctly. What does it mean to use 0x7 as an 
indicator that a command should be put at the head of the queue? This 
would be clearer if it was using some of the macros from ioprio. If 
0x7 means something special I think this should be some #define in the 
includes of the fusion driver with some documentation.

> -----Original Message-----
> From: Adam Manzanares [mailto:adam.manzanares@hgst.com]
> Sent: Thursday, October 13, 2016 1:54 PM
> To: axboe@kernel.dk; tj@kernel.org; dan.j.williams@intel.com;
> hare@suse.de; martin.petersen@oracle.com; mchristi@redhat.com;
> toshi.kani@hpe.com; ming.lei@canonical.com; sathya.prakash@broadcom.com;
> chaitra.basappa@broadcom.com; suganath-prabu.subramani@broadcom.com
> Cc: linux-block@vger.kernel.org; linux-ide@vger.kernel.org;
> linux-kernel@vger.kernel.org; MPT-FusionLinux.pdl@broadcom.com;
> linux-scsi@vger.kernel.org; Adam Manzanares; Adam Manzanares
> Subject: [PATCH v4 2/4] fusion: remove iopriority handling
> 
> The request priority is now by default coming from the ioc. It was not
> clear what this code was trying to do based upon the iopriority class or
> data. The driver should check that a device supports priorities and use
> them according to the specificiations of ioprio.
> 
> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
> ---
>  drivers/message/fusion/mptscsih.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/message/fusion/mptscsih.c
> b/drivers/message/fusion/mptscsih.c
> index 6c9fc11..4740bb6 100644
> --- a/drivers/message/fusion/mptscsih.c
> +++ b/drivers/message/fusion/mptscsih.c
> @@ -1369,11 +1369,6 @@ mptscsih_qcmd(struct scsi_cmnd *SCpnt)
>  	if ((vdevice->vtarget->tflags & MPT_TARGET_FLAGS_Q_YES)
>  	    && (SCpnt->device->tagged_supported)) {
>  		scsictl = scsidir | MPI_SCSIIO_CONTROL_SIMPLEQ;
> -		if (SCpnt->request && SCpnt->request->ioprio) {
> -			if (((SCpnt->request->ioprio & 0x7) == 1) ||
> -				!(SCpnt->request->ioprio & 0x7))
> -				scsictl |= MPI_SCSIIO_CONTROL_HEADOFQ;
> -		}
>  	} else
>  		scsictl = scsidir | MPI_SCSIIO_CONTROL_UNTAGGED;
> 
> --
> 2.1.4
> 
> Western Digital Corporation (and its subsidiaries) E-mail Confidentiality
> Notice & Disclaimer:
> 
> This e-mail and any files transmitted with it may contain confidential or
> legally privileged information of WDC and/or its affiliates, and are
> intended solely for the use of the individual or entity to which they are
> addressed. If you are not the intended recipient, any disclosure, copying,
> distribution or any action taken or omitted to be taken in reliance on it,
> is prohibited. If you have received this e-mail in error, please notify
> the sender immediately and delete the e-mail in its entirety from your
> system.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Take care,
Adam

Re: [PATCH v4 1/4] block: Add iocontext priority to request

[Hannes Reinecke]

On 10/13/2016 09:53 PM, Adam Manzanares wrote:
> Patch adds an association between iocontext ioprio and the ioprio of a
> request. This value is set in blk_rq_set_prio which takes the request and
> the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
> iopriority of the request is set as the iopriority of the ioc. In
> init_request_from_bio a check is made to see if the ioprio of the bio is
> valid and if so then the request prio comes from the bio.
> 
> Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
> ---
>  block/blk-core.c       |  4 +++-
>  include/linux/blkdev.h | 14 ++++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 14d7c07..361b1b9 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1153,6 +1153,7 @@ static struct request *__get_request(struct request_list *rl, int op,
>  
>  	blk_rq_init(q, rq);
>  	blk_rq_set_rl(rq, rl);
> +	blk_rq_set_prio(rq, ioc);
>  	req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
>  
>  	/* init elvpriv */
> @@ -1656,7 +1657,8 @@ 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 (ioprio_valid(bio_prio(bio)))
> +		req->ioprio = bio_prio(bio);
>  	blk_rq_bio_prep(req->q, req, bio);
>  }
>  
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index c47c358..9a0ceaa 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -934,6 +934,20 @@ static inline unsigned int blk_rq_count_bios(struct request *rq)
>  }
>  
>  /*
> + * blk_rq_set_prio - associate a request with prio from ioc
> + * @rq: request of interest
> + * @ioc: target iocontext
> + *
> + * Assocate request prio with ioc prio so request based drivers
> + * can leverage priority information.
> + */
> +static inline void blk_rq_set_prio(struct request *rq, struct io_context *ioc)
> +{
> +	if (ioc)
> +		rq->ioprio = ioc->ioprio;
> +}
> +
> +/*
>   * Request issue related functions.
>   */
>  extern struct request *blk_peek_request(struct request_queue *q);
> 
Don't you need to check for 'ioprio_valid()' here, too?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH v4 2/4] fusion: remove iopriority handling

[Hannes Reinecke]

On 10/13/2016 09:53 PM, Adam Manzanares wrote:
> The request priority is now by default coming from the ioc. It was not
> clear what this code was trying to do based upon the iopriority class or
> data. The driver should check that a device supports priorities and use
> them according to the specificiations of ioprio.
> 
> Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
> ---
>  drivers/message/fusion/mptscsih.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/message/fusion/mptscsih.c b/drivers/message/fusion/mptscsih.c
> index 6c9fc11..4740bb6 100644
> --- a/drivers/message/fusion/mptscsih.c
> +++ b/drivers/message/fusion/mptscsih.c
> @@ -1369,11 +1369,6 @@ mptscsih_qcmd(struct scsi_cmnd *SCpnt)
>  	if ((vdevice->vtarget->tflags & MPT_TARGET_FLAGS_Q_YES)
>  	    && (SCpnt->device->tagged_supported)) {
>  		scsictl = scsidir | MPI_SCSIIO_CONTROL_SIMPLEQ;
> -		if (SCpnt->request && SCpnt->request->ioprio) {
> -			if (((SCpnt->request->ioprio & 0x7) == 1) ||
> -				!(SCpnt->request->ioprio & 0x7))
> -				scsictl |= MPI_SCSIIO_CONTROL_HEADOFQ;
> -		}
>  	} else
>  		scsictl = scsidir | MPI_SCSIIO_CONTROL_UNTAGGED;
>  
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH v4 1/4] block: Add iocontext priority to request

[Adam Manzananares]

The 10/14/2016 07:54, Hannes Reinecke wrote:
> On 10/13/2016 09:53 PM, Adam Manzanares wrote:
> > Patch adds an association between iocontext ioprio and the ioprio of a
> > request. This value is set in blk_rq_set_prio which takes the request and
> > the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
> > iopriority of the request is set as the iopriority of the ioc. In
> > init_request_from_bio a check is made to see if the ioprio of the bio is
> > valid and if so then the request prio comes from the bio.
> > 
> > Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
> > ---
> >  block/blk-core.c       |  4 +++-
> >  include/linux/blkdev.h | 14 ++++++++++++++
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 14d7c07..361b1b9 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -1153,6 +1153,7 @@ static struct request *__get_request(struct request_list *rl, int op,
> >  
> >  	blk_rq_init(q, rq);
> >  	blk_rq_set_rl(rq, rl);
> > +	blk_rq_set_prio(rq, ioc);
> >  	req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
> >  
> >  	/* init elvpriv */
> > @@ -1656,7 +1657,8 @@ 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 (ioprio_valid(bio_prio(bio)))
> > +		req->ioprio = bio_prio(bio);
> >  	blk_rq_bio_prep(req->q, req, bio);
> >  }
> >  
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index c47c358..9a0ceaa 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -934,6 +934,20 @@ static inline unsigned int blk_rq_count_bios(struct request *rq)
> >  }
> >  
> >  /*
> > + * blk_rq_set_prio - associate a request with prio from ioc
> > + * @rq: request of interest
> > + * @ioc: target iocontext
> > + *
> > + * Assocate request prio with ioc prio so request based drivers
> > + * can leverage priority information.
> > + */
> > +static inline void blk_rq_set_prio(struct request *rq, struct io_context *ioc)
> > +{
> > +	if (ioc)
> > +		rq->ioprio = ioc->ioprio;
> > +}
> > +
> > +/*
> >   * Request issue related functions.
> >   */
> >  extern struct request *blk_peek_request(struct request_queue *q);
> > 
> Don't you need to check for 'ioprio_valid()' here, too?

I poked around and it should be safe to not check for ioprio valid
at this point. ioprio_valid only checks to see if the ioprio is 
not IOPRIO_CLASS_NONE. The request by default has a ioprio of none
so if the ioc has ioprio of none we are not changing anything. 

The locations in the code that I found where the ioc prio is set are 
either filtered through the syscall handler, which checks for invalid 
priority combinations, or have valid priority values. 

Take care,
Adam

> 
> Cheers,
> 
> Hannes
> -- 
> Dr. Hannes Reinecke		   Teamlead Storage & Networking
> hare@suse.de			               +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)

Re: [PATCH v4 1/4] block: Add iocontext priority to request

[Hannes Reinecke]

On 10/14/2016 08:35 PM, Adam Manzananares wrote:
> The 10/14/2016 07:54, Hannes Reinecke wrote:
>> On 10/13/2016 09:53 PM, Adam Manzanares wrote:
>>> Patch adds an association between iocontext ioprio and the ioprio of a
>>> request. This value is set in blk_rq_set_prio which takes the request and
>>> the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
>>> iopriority of the request is set as the iopriority of the ioc. In
>>> init_request_from_bio a check is made to see if the ioprio of the bio is
>>> valid and if so then the request prio comes from the bio.
>>>
>>> Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
>>> ---
>>>  block/blk-core.c       |  4 +++-
>>>  include/linux/blkdev.h | 14 ++++++++++++++
>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>> index 14d7c07..361b1b9 100644
>>> --- a/block/blk-core.c
>>> +++ b/block/blk-core.c
>>> @@ -1153,6 +1153,7 @@ static struct request *__get_request(struct request_list *rl, int op,
>>>
>>>  	blk_rq_init(q, rq);
>>>  	blk_rq_set_rl(rq, rl);
>>> +	blk_rq_set_prio(rq, ioc);
>>>  	req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
>>>
>>>  	/* init elvpriv */
>>> @@ -1656,7 +1657,8 @@ 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 (ioprio_valid(bio_prio(bio)))
>>> +		req->ioprio = bio_prio(bio);
>>>  	blk_rq_bio_prep(req->q, req, bio);
>>>  }
>>>
>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>> index c47c358..9a0ceaa 100644
>>> --- a/include/linux/blkdev.h
>>> +++ b/include/linux/blkdev.h
>>> @@ -934,6 +934,20 @@ static inline unsigned int blk_rq_count_bios(struct request *rq)
>>>  }
>>>
>>>  /*
>>> + * blk_rq_set_prio - associate a request with prio from ioc
>>> + * @rq: request of interest
>>> + * @ioc: target iocontext
>>> + *
>>> + * Assocate request prio with ioc prio so request based drivers
>>> + * can leverage priority information.
>>> + */
>>> +static inline void blk_rq_set_prio(struct request *rq, struct io_context *ioc)
>>> +{
>>> +	if (ioc)
>>> +		rq->ioprio = ioc->ioprio;
>>> +}
>>> +
>>> +/*
>>>   * Request issue related functions.
>>>   */
>>>  extern struct request *blk_peek_request(struct request_queue *q);
>>>
>> Don't you need to check for 'ioprio_valid()' here, too?
>
> I poked around and it should be safe to not check for ioprio valid
> at this point. ioprio_valid only checks to see if the ioprio is
> not IOPRIO_CLASS_NONE. The request by default has a ioprio of none
> so if the ioc has ioprio of none we are not changing anything.
>
> The locations in the code that I found where the ioc prio is set are
> either filtered through the syscall handler, which checks for invalid
> priority combinations, or have valid priority values.
>
Thanks for the confirmation, that'll clarifies things.

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)