[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

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.

[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 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

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.

[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 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

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.

[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 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

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.

[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 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

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.

[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

[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 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]

VGhlIDEwLzEzLzIwMTYgMTQ6MDksIEplbnMgQXhib2Ugd3JvdGU6Cj4gT24gMTAvMTMvMjAxNiAw
MjowNiBQTSwgRGFuIFdpbGxpYW1zIHdyb3RlOgo+ID5PbiBUaHUsIE9jdCAxMywgMjAxNiBhdCAx
Mjo1MyBQTSwgQWRhbSBNYW56YW5hcmVzCj4gPjxhZGFtLm1hbnphbmFyZXNAaGdzdC5jb20+IHdy
b3RlOgo+ID4+UGF0Y2ggYWRkcyBhbiBhc3NvY2lhdGlvbiBiZXR3ZWVuIGlvY29udGV4dCBpb3By
aW8gYW5kIHRoZSBpb3ByaW8gb2YgYQo+ID4+cmVxdWVzdC4gVGhpcyB2YWx1ZSBpcyBzZXQgaW4g
YmxrX3JxX3NldF9wcmlvIHdoaWNoIHRha2VzIHRoZSByZXF1ZXN0IGFuZAo+ID4+dGhlIGlvYyBh
cyBhcmd1bWVudHMuIElmIHRoZSBpb2MgaXMgdmFsaWQgaW4gYmxrX3JxX3NldF9wcmlvIHRoZW4g
dGhlCj4gPj5pb3ByaW9yaXR5IG9mIHRoZSByZXF1ZXN0IGlzIHNldCBhcyB0aGUgaW9wcmlvcml0
eSBvZiB0aGUgaW9jLiBJbgo+ID4+aW5pdF9yZXF1ZXN0X2Zyb21fYmlvIGEgY2hlY2sgaXMgbWFk
ZSB0byBzZWUgaWYgdGhlIGlvcHJpbyBvZiB0aGUgYmlvIGlzCj4gPj52YWxpZCBhbmQgaWYgc28g
dGhlbiB0aGUgcmVxdWVzdCBwcmlvIGNvbWVzIGZyb20gdGhlIGJpby4KPiA+Pgo+ID4+U2lnbmVk
LW9mZi1ieTogQWRhbSBNYW56YW5hbmFyZXMgPGFkYW0ubWFuemFuYXJlc0B3ZGMuY29tPgo+ID4+
LS0tCj4gPj4gYmxvY2svYmxrLWNvcmUuYyAgICAgICB8ICA0ICsrKy0KPiA+PiBpbmNsdWRlL2xp
bnV4L2Jsa2Rldi5oIHwgMTQgKysrKysrKysrKysrKysKPiA+PiAyIGZpbGVzIGNoYW5nZWQsIDE3
IGluc2VydGlvbnMoKyksIDEgZGVsZXRpb24oLSkKPiA+Pgo+ID4+ZGlmZiAtLWdpdCBhL2Jsb2Nr
L2Jsay1jb3JlLmMgYi9ibG9jay9ibGstY29yZS5jCj4gPj5pbmRleCAxNGQ3YzA3Li4zNjFiMWI5
IDEwMDY0NAo+ID4+LS0tIGEvYmxvY2svYmxrLWNvcmUuYwo+ID4+KysrIGIvYmxvY2svYmxrLWNv
cmUuYwo+ID4+QEAgLTExNTMsNiArMTE1Myw3IEBAIHN0YXRpYyBzdHJ1Y3QgcmVxdWVzdCAqX19n
ZXRfcmVxdWVzdChzdHJ1Y3QgcmVxdWVzdF9saXN0ICpybCwgaW50IG9wLAo+ID4+Cj4gPj4gICAg
ICAgIGJsa19ycV9pbml0KHEsIHJxKTsKPiA+PiAgICAgICAgYmxrX3JxX3NldF9ybChycSwgcmwp
Owo+ID4+KyAgICAgICBibGtfcnFfc2V0X3ByaW8ocnEsIGlvYyk7Cj4gPj4gICAgICAgIHJlcV9z
ZXRfb3BfYXR0cnMocnEsIG9wLCBvcF9mbGFncyB8IFJFUV9BTExPQ0VEKTsKPiA+Pgo+ID4+ICAg
ICAgICAvKiBpbml0IGVsdnByaXYgKi8KPiA+PkBAIC0xNjU2LDcgKzE2NTcsOCBAQCB2b2lkIGlu
aXRfcmVxdWVzdF9mcm9tX2JpbyhzdHJ1Y3QgcmVxdWVzdCAqcmVxLCBzdHJ1Y3QgYmlvICpiaW8p
Cj4gPj4KPiA+PiAgICAgICAgcmVxLT5lcnJvcnMgPSAwOwo+ID4+ICAgICAgICByZXEtPl9fc2Vj
dG9yID0gYmlvLT5iaV9pdGVyLmJpX3NlY3RvcjsKPiA+Pi0gICAgICAgcmVxLT5pb3ByaW8gPSBi
aW9fcHJpbyhiaW8pOwo+ID4+KyAgICAgICBpZiAoaW9wcmlvX3ZhbGlkKGJpb19wcmlvKGJpbykp
KQo+ID4+KyAgICAgICAgICAgICAgIHJlcS0+aW9wcmlvID0gYmlvX3ByaW8oYmlvKTsKPiA+Cj4g
PlNob3VsZCB3ZSB1c2UgaW9wcmlvX2Jlc3QoKSBoZXJlPyAgSWYgcmVxLT5pb3ByaW8gYW5kIGJp
b19wcmlvKCkKPiA+ZGlzYWdyZWUgb25lIHNpZGUgaGFzIGV4cGxpY2l0bHkgYXNrZWQgZm9yIGEg
aGlnaGVyIHByaW9yaXR5Lgo+IAo+IEl0J3MgYSBnb29kIHF1ZXN0aW9uIC0gYnV0IGlmIHByaW9y
aXR5IGhhcyBiZWVuIHNldCBpbiB0aGUgYmlvLCBpdCBtYWtlcwo+IHNlbnNlIHRoYXQgdGhhdCB3
b3VsZCB0YWtlIHByaW9yaXR5IG92ZXIgdGhlIGdlbmVyYWwgc2V0dGluZyBmb3IgdGhlCj4gdGFz
ay9pbyBjb250ZXh0LiBTbyBJIHRoaW5rIHRoZSBwYXRjaCBpcyBjb3JyZWN0IGFzLWlzLgo+IAo+
IEFkYW0sIHlvdSdsbCB3YW50IHRvIHJld3JpdGUgdGhlIGNvbW1pdCBtZXNzYWdlIHRob3VnaC4g
QSBnb29kIGNvbW1pdAo+IG1lc3NhZ2Ugc2hvdWxkIGV4cGxhaW4gV0hZIHRoZSBjaGFuZ2UgaXMg
bWFkZSwgbm90IGRldGFpbCB0aGUgY29kZQo+IGltcGxlbWVudGF0aW9uIG9mIGl0LgoKR290IGl0
IEknbGwgc2VuZCBzb21ldGhpbmcgb3V0IHNvb24uCgo+IAo+IC0tIAo+IEplbnMgQXhib2UKPiAK
ClRha2UgY2FyZSwKQWRhbQpXZXN0ZXJuIERpZ2l0YWwgQ29ycG9yYXRpb24gKGFuZCBpdHMgc3Vi
c2lkaWFyaWVzKSBFLW1haWwgQ29uZmlkZW50aWFsaXR5IE5vdGljZSAmIERpc2NsYWltZXI6CgpU
aGlzIGUtbWFpbCBhbmQgYW55IGZpbGVzIHRyYW5zbWl0dGVkIHdpdGggaXQgbWF5IGNvbnRhaW4g
Y29uZmlkZW50aWFsIG9yIGxlZ2FsbHkgcHJpdmlsZWdlZCBpbmZvcm1hdGlvbiBvZiBXREMgYW5k
L29yIGl0cyBhZmZpbGlhdGVzLCBhbmQgYXJlIGludGVuZGVkIHNvbGVseSBmb3IgdGhlIHVzZSBv
ZiB0aGUgaW5kaXZpZHVhbCBvciBlbnRpdHkgdG8gd2hpY2ggdGhleSBhcmUgYWRkcmVzc2VkLiBJ
ZiB5b3UgYXJlIG5vdCB0aGUgaW50ZW5kZWQgcmVjaXBpZW50LCBhbnkgZGlzY2xvc3VyZSwgY29w
eWluZywgZGlzdHJpYnV0aW9uIG9yIGFueSBhY3Rpb24gdGFrZW4gb3Igb21pdHRlZCB0byBiZSB0
YWtlbiBpbiByZWxpYW5jZSBvbiBpdCwgaXMgcHJvaGliaXRlZC4gSWYgeW91IGhhdmUgcmVjZWl2
ZWQgdGhpcyBlLW1haWwgaW4gZXJyb3IsIHBsZWFzZSBub3RpZnkgdGhlIHNlbmRlciBpbW1lZGlh
dGVseSBhbmQgZGVsZXRlIHRoZSBlLW1haWwgaW4gaXRzIGVudGlyZXR5IGZyb20geW91ciBzeXN0
ZW0uCg==

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 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]

VGhlIDEwLzEzLzIwMTYgMTU6MDUsIFNhdGh5YSBQcmFrYXNoIFZlZXJpY2hldHR5IHdyb3RlOgo+
IEJ5IHJlbW92aW5nIHRoZSBjb2RlIGJlbG93LCB3ZSBwdXQgYWxsIHRoZSBjb21tYW5kcyBmb3Ig
YWxsIHRoZSB0eXBlcyBvZgo+IGRldmljZXMgKFNBUy9TQVRBKSBhcyBzaW1wbGUtUSAocmVxdWV1
ZSBhcyB0aGUgZGV2aWNlIHJlcXVpcmUpIGFuZCBJIGFtCj4gbm90IHN1cmUgd2hldGhlciBpdCBp
cyB0aGUgaW50ZW50aW9uIG9mIHRoaXMgY2hhbmdlLgo+IAoKVGhpcyBpcyB0aGUgaW50ZW50aW9u
IG9mIHRoZSBjaGFuZ2UuIEkgZG9uJ3QgdGhpbmsgdGhlIGlvcHJpb3JpdHkgb2YgdGhlCnJlcXVl
c3QgaXMgYmVpbmcgdXNlZCBjb3JyZWN0bHkuIFdoYXQgZG9lcyBpdCBtZWFuIHRvIHVzZSAweDcg
YXMgYW4gCmluZGljYXRvciB0aGF0IGEgY29tbWFuZCBzaG91bGQgYmUgcHV0IGF0IHRoZSBoZWFk
IG9mIHRoZSBxdWV1ZT8gVGhpcyAKd291bGQgYmUgY2xlYXJlciBpZiBpdCB3YXMgdXNpbmcgc29t
ZSBvZiB0aGUgbWFjcm9zIGZyb20gaW9wcmlvLiBJZiAKMHg3IG1lYW5zIHNvbWV0aGluZyBzcGVj
aWFsIEkgdGhpbmsgdGhpcyBzaG91bGQgYmUgc29tZSAjZGVmaW5lIGluIHRoZSAKaW5jbHVkZXMg
b2YgdGhlIGZ1c2lvbiBkcml2ZXIgd2l0aCBzb21lIGRvY3VtZW50YXRpb24uCgo+IC0tLS0tT3Jp
Z2luYWwgTWVzc2FnZS0tLS0tCj4gRnJvbTogQWRhbSBNYW56YW5hcmVzIFttYWlsdG86YWRhbS5t
YW56YW5hcmVzQGhnc3QuY29tXQo+IFNlbnQ6IFRodXJzZGF5LCBPY3RvYmVyIDEzLCAyMDE2IDE6
NTQgUE0KPiBUbzogYXhib2VAa2VybmVsLmRrOyB0akBrZXJuZWwub3JnOyBkYW4uai53aWxsaWFt
c0BpbnRlbC5jb207Cj4gaGFyZUBzdXNlLmRlOyBtYXJ0aW4ucGV0ZXJzZW5Ab3JhY2xlLmNvbTsg
bWNocmlzdGlAcmVkaGF0LmNvbTsKPiB0b3NoaS5rYW5pQGhwZS5jb207IG1pbmcubGVpQGNhbm9u
aWNhbC5jb207IHNhdGh5YS5wcmFrYXNoQGJyb2FkY29tLmNvbTsKPiBjaGFpdHJhLmJhc2FwcGFA
YnJvYWRjb20uY29tOyBzdWdhbmF0aC1wcmFidS5zdWJyYW1hbmlAYnJvYWRjb20uY29tCj4gQ2M6
IGxpbnV4LWJsb2NrQHZnZXIua2VybmVsLm9yZzsgbGludXgtaWRlQHZnZXIua2VybmVsLm9yZzsK
PiBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnOyBNUFQtRnVzaW9uTGludXgucGRsQGJyb2Fk
Y29tLmNvbTsKPiBsaW51eC1zY3NpQHZnZXIua2VybmVsLm9yZzsgQWRhbSBNYW56YW5hcmVzOyBB
ZGFtIE1hbnphbmFyZXMKPiBTdWJqZWN0OiBbUEFUQ0ggdjQgMi80XSBmdXNpb246IHJlbW92ZSBp
b3ByaW9yaXR5IGhhbmRsaW5nCj4gCj4gVGhlIHJlcXVlc3QgcHJpb3JpdHkgaXMgbm93IGJ5IGRl
ZmF1bHQgY29taW5nIGZyb20gdGhlIGlvYy4gSXQgd2FzIG5vdAo+IGNsZWFyIHdoYXQgdGhpcyBj
b2RlIHdhcyB0cnlpbmcgdG8gZG8gYmFzZWQgdXBvbiB0aGUgaW9wcmlvcml0eSBjbGFzcyBvcgo+
IGRhdGEuIFRoZSBkcml2ZXIgc2hvdWxkIGNoZWNrIHRoYXQgYSBkZXZpY2Ugc3VwcG9ydHMgcHJp
b3JpdGllcyBhbmQgdXNlCj4gdGhlbSBhY2NvcmRpbmcgdG8gdGhlIHNwZWNpZmljaWF0aW9ucyBv
ZiBpb3ByaW8uCj4gCj4gU2lnbmVkLW9mZi1ieTogQWRhbSBNYW56YW5hcmVzIDxhZGFtLm1hbnph
bmFyZXNAd2RjLmNvbT4KPiAtLS0KPiAgZHJpdmVycy9tZXNzYWdlL2Z1c2lvbi9tcHRzY3NpaC5j
IHwgNSAtLS0tLQo+ICAxIGZpbGUgY2hhbmdlZCwgNSBkZWxldGlvbnMoLSkKPiAKPiBkaWZmIC0t
Z2l0IGEvZHJpdmVycy9tZXNzYWdlL2Z1c2lvbi9tcHRzY3NpaC5jCj4gYi9kcml2ZXJzL21lc3Nh
Z2UvZnVzaW9uL21wdHNjc2loLmMKPiBpbmRleCA2YzlmYzExLi40NzQwYmI2IDEwMDY0NAo+IC0t
LSBhL2RyaXZlcnMvbWVzc2FnZS9mdXNpb24vbXB0c2NzaWguYwo+ICsrKyBiL2RyaXZlcnMvbWVz
c2FnZS9mdXNpb24vbXB0c2NzaWguYwo+IEBAIC0xMzY5LDExICsxMzY5LDYgQEAgbXB0c2NzaWhf
cWNtZChzdHJ1Y3Qgc2NzaV9jbW5kICpTQ3BudCkKPiAgCWlmICgodmRldmljZS0+dnRhcmdldC0+
dGZsYWdzICYgTVBUX1RBUkdFVF9GTEFHU19RX1lFUykKPiAgCSAgICAmJiAoU0NwbnQtPmRldmlj
ZS0+dGFnZ2VkX3N1cHBvcnRlZCkpIHsKPiAgCQlzY3NpY3RsID0gc2NzaWRpciB8IE1QSV9TQ1NJ
SU9fQ09OVFJPTF9TSU1QTEVROwo+IC0JCWlmIChTQ3BudC0+cmVxdWVzdCAmJiBTQ3BudC0+cmVx
dWVzdC0+aW9wcmlvKSB7Cj4gLQkJCWlmICgoKFNDcG50LT5yZXF1ZXN0LT5pb3ByaW8gJiAweDcp
ID09IDEpIHx8Cj4gLQkJCQkhKFNDcG50LT5yZXF1ZXN0LT5pb3ByaW8gJiAweDcpKQo+IC0JCQkJ
c2NzaWN0bCB8PSBNUElfU0NTSUlPX0NPTlRST0xfSEVBRE9GUTsKPiAtCQl9Cj4gIAl9IGVsc2UK
PiAgCQlzY3NpY3RsID0gc2NzaWRpciB8IE1QSV9TQ1NJSU9fQ09OVFJPTF9VTlRBR0dFRDsKPiAK
PiAtLQo+IDIuMS40Cj4gCj4gV2VzdGVybiBEaWdpdGFsIENvcnBvcmF0aW9uIChhbmQgaXRzIHN1
YnNpZGlhcmllcykgRS1tYWlsIENvbmZpZGVudGlhbGl0eQo+IE5vdGljZSAmIERpc2NsYWltZXI6
Cj4gCj4gVGhpcyBlLW1haWwgYW5kIGFueSBmaWxlcyB0cmFuc21pdHRlZCB3aXRoIGl0IG1heSBj
b250YWluIGNvbmZpZGVudGlhbCBvcgo+IGxlZ2FsbHkgcHJpdmlsZWdlZCBpbmZvcm1hdGlvbiBv
ZiBXREMgYW5kL29yIGl0cyBhZmZpbGlhdGVzLCBhbmQgYXJlCj4gaW50ZW5kZWQgc29sZWx5IGZv
ciB0aGUgdXNlIG9mIHRoZSBpbmRpdmlkdWFsIG9yIGVudGl0eSB0byB3aGljaCB0aGV5IGFyZQo+
IGFkZHJlc3NlZC4gSWYgeW91IGFyZSBub3QgdGhlIGludGVuZGVkIHJlY2lwaWVudCwgYW55IGRp
c2Nsb3N1cmUsIGNvcHlpbmcsCj4gZGlzdHJpYnV0aW9uIG9yIGFueSBhY3Rpb24gdGFrZW4gb3Ig
b21pdHRlZCB0byBiZSB0YWtlbiBpbiByZWxpYW5jZSBvbiBpdCwKPiBpcyBwcm9oaWJpdGVkLiBJ
ZiB5b3UgaGF2ZSByZWNlaXZlZCB0aGlzIGUtbWFpbCBpbiBlcnJvciwgcGxlYXNlIG5vdGlmeQo+
IHRoZSBzZW5kZXIgaW1tZWRpYXRlbHkgYW5kIGRlbGV0ZSB0aGUgZS1tYWlsIGluIGl0cyBlbnRp
cmV0eSBmcm9tIHlvdXIKPiBzeXN0ZW0uCj4gLS0KPiBUbyB1bnN1YnNjcmliZSBmcm9tIHRoaXMg
bGlzdDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUgbGludXgtaWRlIiBpbgo+IHRoZSBib2R5
IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9Admdlci5rZXJuZWwub3JnCj4gTW9yZSBtYWpvcmRv
bW8gaW5mbyBhdCAgaHR0cDovL3ZnZXIua2VybmVsLm9yZy9tYWpvcmRvbW8taW5mby5odG1sCgpU
YWtlIGNhcmUsCkFkYW0KV2VzdGVybiBEaWdpdGFsIENvcnBvcmF0aW9uIChhbmQgaXRzIHN1YnNp
ZGlhcmllcykgRS1tYWlsIENvbmZpZGVudGlhbGl0eSBOb3RpY2UgJiBEaXNjbGFpbWVyOgoKVGhp
cyBlLW1haWwgYW5kIGFueSBmaWxlcyB0cmFuc21pdHRlZCB3aXRoIGl0IG1heSBjb250YWluIGNv
bmZpZGVudGlhbCBvciBsZWdhbGx5IHByaXZpbGVnZWQgaW5mb3JtYXRpb24gb2YgV0RDIGFuZC9v
ciBpdHMgYWZmaWxpYXRlcywgYW5kIGFyZSBpbnRlbmRlZCBzb2xlbHkgZm9yIHRoZSB1c2Ugb2Yg
dGhlIGluZGl2aWR1YWwgb3IgZW50aXR5IHRvIHdoaWNoIHRoZXkgYXJlIGFkZHJlc3NlZC4gSWYg
eW91IGFyZSBub3QgdGhlIGludGVuZGVkIHJlY2lwaWVudCwgYW55IGRpc2Nsb3N1cmUsIGNvcHlp
bmcsIGRpc3RyaWJ1dGlvbiBvciBhbnkgYWN0aW9uIHRha2VuIG9yIG9taXR0ZWQgdG8gYmUgdGFr
ZW4gaW4gcmVsaWFuY2Ugb24gaXQsIGlzIHByb2hpYml0ZWQuIElmIHlvdSBoYXZlIHJlY2VpdmVk
IHRoaXMgZS1tYWlsIGluIGVycm9yLCBwbGVhc2Ugbm90aWZ5IHRoZSBzZW5kZXIgaW1tZWRpYXRl
bHkgYW5kIGRlbGV0ZSB0aGUgZS1tYWlsIGluIGl0cyBlbnRpcmV0eSBmcm9tIHlvdXIgc3lzdGVt
Lgo=

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 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 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 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]

VGhlIDEwLzE0LzIwMTYgMDc6NTQsIEhhbm5lcyBSZWluZWNrZSB3cm90ZToKPiBPbiAxMC8xMy8y
MDE2IDA5OjUzIFBNLCBBZGFtIE1hbnphbmFyZXMgd3JvdGU6Cj4gPiBQYXRjaCBhZGRzIGFuIGFz
c29jaWF0aW9uIGJldHdlZW4gaW9jb250ZXh0IGlvcHJpbyBhbmQgdGhlIGlvcHJpbyBvZiBhCj4g
PiByZXF1ZXN0LiBUaGlzIHZhbHVlIGlzIHNldCBpbiBibGtfcnFfc2V0X3ByaW8gd2hpY2ggdGFr
ZXMgdGhlIHJlcXVlc3QgYW5kCj4gPiB0aGUgaW9jIGFzIGFyZ3VtZW50cy4gSWYgdGhlIGlvYyBp
cyB2YWxpZCBpbiBibGtfcnFfc2V0X3ByaW8gdGhlbiB0aGUKPiA+IGlvcHJpb3JpdHkgb2YgdGhl
IHJlcXVlc3QgaXMgc2V0IGFzIHRoZSBpb3ByaW9yaXR5IG9mIHRoZSBpb2MuIEluCj4gPiBpbml0
X3JlcXVlc3RfZnJvbV9iaW8gYSBjaGVjayBpcyBtYWRlIHRvIHNlZSBpZiB0aGUgaW9wcmlvIG9m
IHRoZSBiaW8gaXMKPiA+IHZhbGlkIGFuZCBpZiBzbyB0aGVuIHRoZSByZXF1ZXN0IHByaW8gY29t
ZXMgZnJvbSB0aGUgYmlvLgo+ID4gCj4gPiBTaWduZWQtb2ZmLWJ5OiBBZGFtIE1hbnphbmFuYXJl
cyA8YWRhbS5tYW56YW5hcmVzQHdkYy5jb20+Cj4gPiAtLS0KPiA+ICBibG9jay9ibGstY29yZS5j
ICAgICAgIHwgIDQgKysrLQo+ID4gIGluY2x1ZGUvbGludXgvYmxrZGV2LmggfCAxNCArKysrKysr
KysrKysrKwo+ID4gIDIgZmlsZXMgY2hhbmdlZCwgMTcgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlv
bigtKQo+ID4gCj4gPiBkaWZmIC0tZ2l0IGEvYmxvY2svYmxrLWNvcmUuYyBiL2Jsb2NrL2Jsay1j
b3JlLmMKPiA+IGluZGV4IDE0ZDdjMDcuLjM2MWIxYjkgMTAwNjQ0Cj4gPiAtLS0gYS9ibG9jay9i
bGstY29yZS5jCj4gPiArKysgYi9ibG9jay9ibGstY29yZS5jCj4gPiBAQCAtMTE1Myw2ICsxMTUz
LDcgQEAgc3RhdGljIHN0cnVjdCByZXF1ZXN0ICpfX2dldF9yZXF1ZXN0KHN0cnVjdCByZXF1ZXN0
X2xpc3QgKnJsLCBpbnQgb3AsCj4gPiAgCj4gPiAgCWJsa19ycV9pbml0KHEsIHJxKTsKPiA+ICAJ
YmxrX3JxX3NldF9ybChycSwgcmwpOwo+ID4gKwlibGtfcnFfc2V0X3ByaW8ocnEsIGlvYyk7Cj4g
PiAgCXJlcV9zZXRfb3BfYXR0cnMocnEsIG9wLCBvcF9mbGFncyB8IFJFUV9BTExPQ0VEKTsKPiA+
ICAKPiA+ICAJLyogaW5pdCBlbHZwcml2ICovCj4gPiBAQCAtMTY1Niw3ICsxNjU3LDggQEAgdm9p
ZCBpbml0X3JlcXVlc3RfZnJvbV9iaW8oc3RydWN0IHJlcXVlc3QgKnJlcSwgc3RydWN0IGJpbyAq
YmlvKQo+ID4gIAo+ID4gIAlyZXEtPmVycm9ycyA9IDA7Cj4gPiAgCXJlcS0+X19zZWN0b3IgPSBi
aW8tPmJpX2l0ZXIuYmlfc2VjdG9yOwo+ID4gLQlyZXEtPmlvcHJpbyA9IGJpb19wcmlvKGJpbyk7
Cj4gPiArCWlmIChpb3ByaW9fdmFsaWQoYmlvX3ByaW8oYmlvKSkpCj4gPiArCQlyZXEtPmlvcHJp
byA9IGJpb19wcmlvKGJpbyk7Cj4gPiAgCWJsa19ycV9iaW9fcHJlcChyZXEtPnEsIHJlcSwgYmlv
KTsKPiA+ICB9Cj4gPiAgCj4gPiBkaWZmIC0tZ2l0IGEvaW5jbHVkZS9saW51eC9ibGtkZXYuaCBi
L2luY2x1ZGUvbGludXgvYmxrZGV2LmgKPiA+IGluZGV4IGM0N2MzNTguLjlhMGNlYWEgMTAwNjQ0
Cj4gPiAtLS0gYS9pbmNsdWRlL2xpbnV4L2Jsa2Rldi5oCj4gPiArKysgYi9pbmNsdWRlL2xpbnV4
L2Jsa2Rldi5oCj4gPiBAQCAtOTM0LDYgKzkzNCwyMCBAQCBzdGF0aWMgaW5saW5lIHVuc2lnbmVk
IGludCBibGtfcnFfY291bnRfYmlvcyhzdHJ1Y3QgcmVxdWVzdCAqcnEpCj4gPiAgfQo+ID4gIAo+
ID4gIC8qCj4gPiArICogYmxrX3JxX3NldF9wcmlvIC0gYXNzb2NpYXRlIGEgcmVxdWVzdCB3aXRo
IHByaW8gZnJvbSBpb2MKPiA+ICsgKiBAcnE6IHJlcXVlc3Qgb2YgaW50ZXJlc3QKPiA+ICsgKiBA
aW9jOiB0YXJnZXQgaW9jb250ZXh0Cj4gPiArICoKPiA+ICsgKiBBc3NvY2F0ZSByZXF1ZXN0IHBy
aW8gd2l0aCBpb2MgcHJpbyBzbyByZXF1ZXN0IGJhc2VkIGRyaXZlcnMKPiA+ICsgKiBjYW4gbGV2
ZXJhZ2UgcHJpb3JpdHkgaW5mb3JtYXRpb24uCj4gPiArICovCj4gPiArc3RhdGljIGlubGluZSB2
b2lkIGJsa19ycV9zZXRfcHJpbyhzdHJ1Y3QgcmVxdWVzdCAqcnEsIHN0cnVjdCBpb19jb250ZXh0
ICppb2MpCj4gPiArewo+ID4gKwlpZiAoaW9jKQo+ID4gKwkJcnEtPmlvcHJpbyA9IGlvYy0+aW9w
cmlvOwo+ID4gK30KPiA+ICsKPiA+ICsvKgo+ID4gICAqIFJlcXVlc3QgaXNzdWUgcmVsYXRlZCBm
dW5jdGlvbnMuCj4gPiAgICovCj4gPiAgZXh0ZXJuIHN0cnVjdCByZXF1ZXN0ICpibGtfcGVla19y
ZXF1ZXN0KHN0cnVjdCByZXF1ZXN0X3F1ZXVlICpxKTsKPiA+IAo+IERvbid0IHlvdSBuZWVkIHRv
IGNoZWNrIGZvciAnaW9wcmlvX3ZhbGlkKCknIGhlcmUsIHRvbz8KCkkgcG9rZWQgYXJvdW5kIGFu
ZCBpdCBzaG91bGQgYmUgc2FmZSB0byBub3QgY2hlY2sgZm9yIGlvcHJpbyB2YWxpZAphdCB0aGlz
IHBvaW50LiBpb3ByaW9fdmFsaWQgb25seSBjaGVja3MgdG8gc2VlIGlmIHRoZSBpb3ByaW8gaXMg
Cm5vdCBJT1BSSU9fQ0xBU1NfTk9ORS4gVGhlIHJlcXVlc3QgYnkgZGVmYXVsdCBoYXMgYSBpb3By
aW8gb2Ygbm9uZQpzbyBpZiB0aGUgaW9jIGhhcyBpb3ByaW8gb2Ygbm9uZSB3ZSBhcmUgbm90IGNo
YW5naW5nIGFueXRoaW5nLiAKClRoZSBsb2NhdGlvbnMgaW4gdGhlIGNvZGUgdGhhdCBJIGZvdW5k
IHdoZXJlIHRoZSBpb2MgcHJpbyBpcyBzZXQgYXJlIAplaXRoZXIgZmlsdGVyZWQgdGhyb3VnaCB0
aGUgc3lzY2FsbCBoYW5kbGVyLCB3aGljaCBjaGVja3MgZm9yIGludmFsaWQgCnByaW9yaXR5IGNv
bWJpbmF0aW9ucywgb3IgaGF2ZSB2YWxpZCBwcmlvcml0eSB2YWx1ZXMuIAoKVGFrZSBjYXJlLApB
ZGFtCgo+IAo+IENoZWVycywKPiAKPiBIYW5uZXMKPiAtLSAKPiBEci4gSGFubmVzIFJlaW5lY2tl
CQkgICBUZWFtbGVhZCBTdG9yYWdlICYgTmV0d29ya2luZwo+IGhhcmVAc3VzZS5kZQkJCSAgICAg
ICAgICAgICAgICs0OSA5MTEgNzQwNTMgNjg4Cj4gU1VTRSBMSU5VWCBHbWJILCBNYXhmZWxkc3Ry
LiA1LCA5MDQwOSBOw7xybmJlcmcKPiBHRjogRi4gSW1lbmTDtnJmZmVyLCBKLiBTbWl0aGFyZCwg
Si4gR3VpbGQsIEQuIFVwbWFueXUsIEcuIE5vcnRvbgo+IEhSQiAyMTI4NCAoQUcgTsO8cm5iZXJn
KQpXZXN0ZXJuIERpZ2l0YWwgQ29ycG9yYXRpb24gKGFuZCBpdHMgc3Vic2lkaWFyaWVzKSBFLW1h
aWwgQ29uZmlkZW50aWFsaXR5IE5vdGljZSAmIERpc2NsYWltZXI6CgpUaGlzIGUtbWFpbCBhbmQg
YW55IGZpbGVzIHRyYW5zbWl0dGVkIHdpdGggaXQgbWF5IGNvbnRhaW4gY29uZmlkZW50aWFsIG9y
IGxlZ2FsbHkgcHJpdmlsZWdlZCBpbmZvcm1hdGlvbiBvZiBXREMgYW5kL29yIGl0cyBhZmZpbGlh
dGVzLCBhbmQgYXJlIGludGVuZGVkIHNvbGVseSBmb3IgdGhlIHVzZSBvZiB0aGUgaW5kaXZpZHVh
bCBvciBlbnRpdHkgdG8gd2hpY2ggdGhleSBhcmUgYWRkcmVzc2VkLiBJZiB5b3UgYXJlIG5vdCB0
aGUgaW50ZW5kZWQgcmVjaXBpZW50LCBhbnkgZGlzY2xvc3VyZSwgY29weWluZywgZGlzdHJpYnV0
aW9uIG9yIGFueSBhY3Rpb24gdGFrZW4gb3Igb21pdHRlZCB0byBiZSB0YWtlbiBpbiByZWxpYW5j
ZSBvbiBpdCwgaXMgcHJvaGliaXRlZC4gSWYgeW91IGhhdmUgcmVjZWl2ZWQgdGhpcyBlLW1haWwg
aW4gZXJyb3IsIHBsZWFzZSBub3RpZnkgdGhlIHNlbmRlciBpbW1lZGlhdGVseSBhbmQgZGVsZXRl
IHRoZSBlLW1haWwgaW4gaXRzIGVudGlyZXR5IGZyb20geW91ciBzeXN0ZW0uCg==

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

[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)