[PATCH 0/3] Separate zone requests from medium access requests

This series introduces blk_rq_accesses_medium(), which is equivalent to
!blk_rq_is_passthrough() minus the zone request operations REQ_OP_ZONE_REPORT
and REQ_OP_ZONE_RESET. This new helper allows avoiding problems due to the
non-standard nature of these commands (report zones does no operate on logical
block size units while reset zone operates on entire zones only).

Using blk_rq_accesses_medium(), patch 2 excludes the zone commands from
request accounting (these commands are not accessing the device medium).
Exclusion from request scheduling is also added.

Finally, patch 3 uses the blk_rq_accesses_medium() helper to fix improperly
unaligned resid values only and only for medium access commands. This
correctly excludes from the resid correction zone requests as well as
passthrough requests.

ALl 3 patches are originally from Bart.

Bart Van Assche (3):
  block: Introduce blk_rq_accesses_medium()
  block: Separate zone requests from medium access requests
  mpt3sas: Do not check resid for non medium access commands

 block/blk-core.c                     |  2 +-
 block/blk.h                          |  2 +-
 block/elevator.c                     | 12 ++++++++----
 block/mq-deadline.c                  |  2 +-
 drivers/scsi/mpt3sas/mpt3sas_scsih.c |  4 ++--
 include/linux/blk_types.h            | 17 ++++++++++-------
 include/linux/blkdev.h               | 18 +++++++++++++++---
 7 files changed, 38 insertions(+), 19 deletions(-)

-- 
2.9.3

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.

From: Bart Van Assche <redacted>

A medium access request is defined as an internal regular request that
operates on a whole number of logical blocks of the storage medium.
These include REQ_OP_READ, REQ_OP_WRITE, REQ_OP_FLUSH, REQ_OP_DISCARD,
REQ_OP_SECURE_ERASE, REQ_OP_WRITE_SAME and REQ_OP_WRITE_ZEROES.

Zoned block device requests (REQ_OP_ZONE_REPORT and REQ_OP_ZONE_RESET)
as well as SCSI passthrough and driver private commands are not
considered medium access request.

Reshuffle enum req_opf definitions grouping medium access request up
to REQ_OP_MEDIUM_LAST and introduce the helper function
blk_rq_accesses_medium() to test a request.

Signed-off-by: Bart Van Assche <redacted>
---
 include/linux/blk_types.h | 17 ++++++++++-------
 include/linux/blkdev.h    | 12 ++++++++++++
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index d703acb..6420057 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h

@@ -151,16 +151,19 @@ enum req_opf {
 	REQ_OP_FLUSH		= 2,
 	/* discard sectors */
 	REQ_OP_DISCARD		= 3,
-	/* get zone information */
-	REQ_OP_ZONE_REPORT	= 4,
 	/* securely erase sectors */
-	REQ_OP_SECURE_ERASE	= 5,
-	/* seset a zone write pointer */
-	REQ_OP_ZONE_RESET	= 6,
+	REQ_OP_SECURE_ERASE	= 4,
 	/* write the same sector many times */
-	REQ_OP_WRITE_SAME	= 7,
+	REQ_OP_WRITE_SAME	= 5,
 	/* write the zero filled sector many times */
-	REQ_OP_WRITE_ZEROES	= 8,
+	REQ_OP_WRITE_ZEROES	= 6,
+
+	REQ_OP_MEDIUM_LAST = REQ_OP_WRITE_ZEROES,
+
+	/* get zone information */
+	REQ_OP_ZONE_REPORT	= 16,
+	/* reset a zone write pointer */
+	REQ_OP_ZONE_RESET	= 17,
 
 	/* SCSI passthrough using struct scsi_request */
 	REQ_OP_SCSI_IN		= 32,

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index aecca0e..7d1ce2d 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h

@@ -248,6 +248,18 @@ static inline bool blk_rq_is_passthrough(struct request *rq)
 	return blk_rq_is_scsi(rq) || blk_rq_is_private(rq);
 }
 
+/**
+ * blk_rq_accesses_medium - test if a request is a medium access request
+ * @rq: A block layer request.
+ *
+ * A medium access request is a regular internal request that operates on
+ * a whole number of logical blocks of the storage medium.
+ */
+static inline bool blk_rq_accesses_medium(const struct request *rq)
+{
+	return req_op(rq) <= REQ_OP_MEDIUM_LAST;
+}
+
 static inline unsigned short req_get_ioprio(struct request *req)
 {
 	return req->ioprio;

-- 
2.9.3

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.

From: Bart Van Assche <redacted>

Use blk_rq_accesses_medium() instead of !blk_rq_is_passthrough() to
ensure that code that is intended for normal medium access requests,
e.g. DISCARD, READ and WRITE requests, is not applied to
REQ_OP_ZONE_REPORT requests nor to REQ_OP_ZONE_RESET requests.
This allows excluding these zone requests from request accounting
and from request scheduling.

Signed-off-by: Bart Van Assche <redacted>
---
 block/blk-core.c       |  2 +-
 block/blk.h            |  2 +-
 block/elevator.c       | 12 ++++++++----
 block/mq-deadline.c    |  2 +-
 include/linux/blkdev.h |  6 +++---
 5 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index b9e857f..addd8e1 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c

@@ -2608,7 +2608,7 @@ bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)
 	req->__data_len -= total_bytes;
 
 	/* update sector only for requests with clear definition of sector */
-	if (!blk_rq_is_passthrough(req))
+	if (blk_rq_accesses_medium(req))
 		req->__sector += total_bytes >> 9;
 
 	/* mixed attributes always follow the first bio */

diff --git a/block/blk.h b/block/blk.h
index d1ea4bd9..9b63db7 100644
--- a/block/blk.h
+++ b/block/blk.h

@@ -255,7 +255,7 @@ static inline int blk_do_io_stat(struct request *rq)
 {
 	return rq->rq_disk &&
 	       (rq->rq_flags & RQF_IO_STAT) &&
-		!blk_rq_is_passthrough(rq);
+		blk_rq_accesses_medium(rq);
 }
 
 static inline void req_set_nomerge(struct request_queue *q, struct request *req)

diff --git a/block/elevator.c b/block/elevator.c
index 699d10f..cbf81c6 100644
--- a/block/elevator.c
+++ b/block/elevator.c

@@ -635,16 +635,20 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
 
 	rq->q = q;
 
-	if (rq->rq_flags & RQF_SOFTBARRIER) {
+	if (!blk_rq_accesses_medium(rq)) {
+		/* Do not schedule zone requests */
+		where = ELEVATOR_INSERT_FRONT;
+	} if (rq->rq_flags & RQF_SOFTBARRIER) {
 		/* barriers are scheduling boundary, update end_sector */
-		if (!blk_rq_is_passthrough(rq)) {
+		if (blk_rq_accesses_medium(rq)) {
 			q->end_sector = rq_end_sector(rq);
 			q->boundary_rq = rq;
 		}
 	} else if (!(rq->rq_flags & RQF_ELVPRIV) &&
 		    (where == ELEVATOR_INSERT_SORT ||
-		     where == ELEVATOR_INSERT_SORT_MERGE))
+		     where == ELEVATOR_INSERT_SORT_MERGE)) {
 		where = ELEVATOR_INSERT_BACK;
+	}
 
 	switch (where) {
 	case ELEVATOR_INSERT_REQUEUE:

@@ -679,7 +683,7 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
 		if (elv_attempt_insert_merge(q, rq))
 			break;
 	case ELEVATOR_INSERT_SORT:
-		BUG_ON(blk_rq_is_passthrough(rq));
+		BUG_ON(!blk_rq_accesses_medium(rq));
 		rq->rq_flags |= RQF_SORTED;
 		q->nr_sorted++;
 		if (rq_mergeable(rq)) {

diff --git a/block/mq-deadline.c b/block/mq-deadline.c
index 23612163..389c1af 100644
--- a/block/mq-deadline.c
+++ b/block/mq-deadline.c

@@ -399,7 +399,7 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
 
 	blk_mq_sched_request_inserted(rq);
 
-	if (at_head || blk_rq_is_passthrough(rq)) {
+	if (at_head || !blk_rq_accesses_medium(rq)) {
 		if (at_head)
 			list_add(&rq->queuelist, &dd->dispatch);
 		else

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 7d1ce2d..dcf926d 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h

@@ -720,7 +720,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
 
 static inline bool blk_account_rq(struct request *rq)
 {
-	return (rq->rq_flags & RQF_STARTED) && !blk_rq_is_passthrough(rq);
+	return (rq->rq_flags & RQF_STARTED) && blk_rq_accesses_medium(rq);
 }
 
 #define blk_rq_cpu_valid(rq)	((rq)->cpu != -1)

@@ -796,7 +796,7 @@ static inline void blk_clear_rl_full(struct request_list *rl, bool sync)
 
 static inline bool rq_mergeable(struct request *rq)
 {
-	if (blk_rq_is_passthrough(rq))
+	if (!blk_rq_accesses_medium(rq))
 		return false;
 
 	if (req_op(rq) == REQ_OP_FLUSH)

@@ -1070,7 +1070,7 @@ static inline unsigned int blk_rq_get_max_sectors(struct request *rq,
 {
 	struct request_queue *q = rq->q;
 
-	if (blk_rq_is_passthrough(rq))
+	if (!blk_rq_accesses_medium(rq))
 		return q->limits.max_hw_sectors;
 
 	if (!q->limits.chunk_sectors ||

-- 
2.9.3

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.

From: Bart Van Assche <redacted>

Commit f2e767bb5d6e ("mpt3sas: Force request partial completion
alignment") introduced a forced alignment of resid to the device
logical block size to fix bogus HBA firmware sometimes returning an
unaligned value. This fix however did not consider the case of
commands not operating on logical block size units
(e.g. REQ_OP_ZONE_REPORT and its 64B aligned partial replies). This
could result is incorrectly aligning resid for these commands, which
for REQ_OP_REPORT_ZONES result in the inability to determine the
number of zone descriptors returned.

Fix the resid alignment check to exclude all requests that are not
medium access requests using blk_rq_access_medium(). This will exclude
from the resid forced fix all passthrough requests as well as zone
command requests.

Fixes: f2e767bb5d6e ("mpt3sas: Force request partial completion alignment")
Signed-off-by: Bart Van Assche <redacted>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 46e866c..405dc84 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c

@@ -4748,8 +4748,8 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
 	 * then scsi-ml does not need to handle this misbehavior.
 	 */
 	sector_sz = scmd->device->sector_size;
-	if (unlikely(!blk_rq_is_passthrough(scmd->request) && sector_sz &&
-		     xfer_cnt % sector_sz)) {
+	if (unlikely(sector_sz && (xfer_cnt & (sector_sz - 1)) &&
+		     blk_rq_accesses_medium(scmd->request))) {
 		sdev_printk(KERN_INFO, scmd->device,
 		    "unaligned partial completion avoided (xfer_cnt=%u, sector_sz=%u)\n",
 			    xfer_cnt, sector_sz);

-- 
2.9.3

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.

I don't really like this too much - this is too many SCSI specifics
for the block layer to care.  Maybe using bios for the zone ops was a
mistake after all, and we should just have operations in struct block_device
instead..

On Tue, 2017-02-28 at 17:02 +0100, Christoph Hellwig wrote:

I don't really like this too much - this is too many SCSI specifics
for the block layer to care.  Maybe using bios for the zone ops was a
mistake after all, and we should just have operations in struct block_dev=

ice

instead..

blk_rq_accesses_medium() has nothing to do with SCSI. It is a reintroductio=
n
of what REQ_TYPE_FS stood for before ZBC was introduced but with another na=
me.
BTW, I think that all blk_rq_is_passthrough() callers have to be reviewed t=
o
see whether or not !blk_rq_accesses_medium() is perhaps what was intended.

Bart.=

On Tue, 2017-02-28 at 19:25 +0900, Damien Le Moal wrote:

From: Bart Van Assche <redacted>
=20
Use blk_rq_accesses_medium() instead of !blk_rq_is_passthrough() to
ensure that code that is intended for normal medium access requests,
e.g. DISCARD, READ and WRITE requests, is not applied to
REQ_OP_ZONE_REPORT requests nor to REQ_OP_ZONE_RESET requests.
This allows excluding these zone requests from request accounting
and from request scheduling.
=20
Signed-off-by: Bart Van Assche <redacted>

Hello Damien,

Since you posted this patch you should have added your Signed-off-by. And
since you have edited this patch, you should have documented what you have
changed.

 static inline void req_set_nomerge(struct request_queue *q, struct reque=

st *req)

uct request *rq, int where)

=20
 	rq->q =3D q;
=20
-	if (rq->rq_flags & RQF_SOFTBARRIER) {
+	if (!blk_rq_accesses_medium(rq)) {
+		/* Do not schedule zone requests */
+		where =3D ELEVATOR_INSERT_FRONT;
+	} if (rq->rq_flags & RQF_SOFTBARRIER) {

This change was not in the patch I sent to you. Additionally, this change
doesn't look properly formatted. Please make sure that the second "if" star=
ts
on a new line.

 		}
 	} else if (!(rq->rq_flags & RQF_ELVPRIV) &&
 		    (where =3D=3D ELEVATOR_INSERT_SORT ||
-		     where =3D=3D ELEVATOR_INSERT_SORT_MERGE))
+		     where =3D=3D ELEVATOR_INSERT_SORT_MERGE)) {
 		where =3D ELEVATOR_INSERT_BACK;
+	}

This change wasn't in my patch either. Since this change only adds a pair o=
f
braces, can it be left out?

Thanks,

Bart.=

FYI, we noticed the following commit:

commit: 670fcc25d73abbc113aef9e556a8fb972ec0b56a ("block: Separate zone requests from medium access requests")
url: https://github.com/0day-ci/linux/commits/Damien-Le-Moal/block-Introduce-blk_rq_accesses_medium/20170301-022334


in testcase: boot

on test machine: qemu-system-x86_64 -enable-kvm -m 512M

caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):


+--------------------------------------------------------------------+------------+------------+
|                                                                    | 3aa5b02e7e | 670fcc25d7 |
+--------------------------------------------------------------------+------------+------------+
| boot_successes                                                     | 0          | 0          |
| boot_failures                                                      | 12         | 12         |
| BUG:KASAN:global-out-of-bounds_in__of_match_node_at_addr           | 12         | 12         |
| BUG:KASAN:global-out-of-bounds_in__of_device_is_compatible_at_addr | 12         | 12         |
| kernel_BUG_at_block/blk-core.c                                     | 0          | 8          |
| invalid_opcode:#[##]PREEMPT_SMP_KASAN                              | 0          | 8          |
| Kernel_panic-not_syncing:Fatal_exception                           | 0          | 8          |
+--------------------------------------------------------------------+------------+------------+



[   56.690745] kernel BUG at block/blk-core.c:2426!
[   56.691613] invalid opcode: 0000 [#1] PREEMPT SMP KASAN
[   56.692344] CPU: 0 PID: 146 Comm: kworker/0:1H Tainted: G    B           4.10.0-10533-g670fcc2 #1
[   56.693338] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-20161025_171302-gandalf 04/01/2014
[   56.693338] Workqueue: kblockd blk_delay_work
[   56.693338] task: ffff880017478000 task.stack: ffff880017ba8000
[   56.693338] RIP: 0010:blk_dequeue_request+0x1b/0xd2
[   56.693338] RSP: 0018:ffff880017bafb40 EFLAGS: 00010046
[   56.693338] RAX: ffff880018c12f00 RBX: ffff880018c12f00 RCX: ffffffff87a13267
[   56.693338] RDX: 0000000000000002 RSI: dffffc0000000000 RDI: ffff880018c12f00
[   56.693338] RBP: ffff880017bafb60 R08: 0000000000000003 R09: 0000000000000001
[   56.693338] R10: ffff880017bafd90 R11: ffff8800187cf6ef R12: 0000000000000000
[   56.693338] R13: ffff880018c12f30 R14: ffff8800173fdd80 R15: ffff880017ab9200
[   56.693338] FS:  0000000000000000(0000) GS:ffff88001a400000(0000) knlGS:0000000000000000
[   56.693338] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   56.693338] CR2: 00007f81e2aed670 CR3: 0000000018ab7000 CR4: 00000000000006b0
[   56.693338] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   56.693338] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   56.693338] Call Trace:
[   56.693338]  blk_start_request+0x15/0x9e
[   56.693338]  blk_fetch_request+0x1b/0x21
[   56.693338]  do_ide_request+0x3df/0xae7
[   56.693338]  ? ide_intr+0x370/0x370
[   56.693338]  ? trace_hardirqs_off_caller+0x1f/0x188
[   56.693338]  ? __lock_acquire+0x38/0x90c
[   56.693338]  ? _raw_spin_unlock_irqrestore+0x3c/0x60
[   56.693338]  ? lock_acquire+0xa2/0xd2
[   56.693338]  ? check_flags+0x14/0x251
[   56.693338]  ? blk_delay_work+0x2e/0x51
[   56.693338]  __blk_run_queue+0x63/0x76
[   56.693338]  blk_delay_work+0x36/0x51
[   56.693338]  process_one_work+0x3e5/0x658
[   56.693338]  ? process_one_work+0x352/0x658
[   56.693338]  ? pwq_dec_nr_in_flight+0x106/0x106
[   56.693338]  ? lock_acquire+0xa2/0xd2
[   56.693338]  worker_thread+0x34c/0x549
[   56.693338]  kthread+0x1d5/0x1e5
[   56.693338]  ? process_scheduled_works+0x3e/0x3e
[   56.693338]  ? __kthread_create_on_node+0x26a/0x26a
[   56.693338]  ret_from_fork+0x31/0x40
[   56.693338] Code: c3 00 31 c0 5a 5b 41 5c 41 5d 41 5e 41 5f 5d c3 55 48 89 e5 41 56 41 55 41 54 53 48 89 fb e8 ad b9 c4 ff 48 8b 03 48 39 c3 75 02 <0f> 0b 48 8d 7b 48 e8 bd b8 c4 ff f6 43 4a 01 74 02 0f 0b 48 8d 
[   56.693338] RIP: blk_dequeue_request+0x1b/0xd2 RSP: ffff880017bafb40
[   56.693338] ---[ end trace 9cc9c5310ec16eb6 ]---


To reproduce:

        git clone git://git.kernel.org/pub/scm/linux/kernel/git/wfg/lkp-tests.git
        cd lkp-tests
        bin/lkp qemu -k <bzImage> job-script  # job-script is attached in this email



Thanks,
Xiaolong