[REGRESSION] Secure discard is broken

[REGRESSION] Secure discard is broken

[Adrian Hunter]

Hi

I noticed some changes in the mmc block driver that did not go
through the mmc tree and they looked wrong so I gave them a test.
And it does seem like secure discard is broken, not just for mmc
but in general too.  The commit was:
288dab8a35a0 ("block: add a separate operation type for secure erase")
When I tried it, it gets stuck in blk_queue_bio:

# Performing secure-erase from 5120000000 count 4096 (from sector 10000000 sector count 8)
[  942.771946] INFO: rcu_sched self-detected stall on CPU
[  942.777800]  2-...: (20998 ticks this GP) idle=3ed/140000000000001/0 softirq=2568/2568 fqs=5250 
[  942.787765]   (t=21000 jiffies g=1729 c=1728 q=202)
[  942.793307] Task dump for CPU 2:
[  942.796956] mmc-erase4.stat R  running task    14512  1936   1605 0x00000008
[  942.804984]  0000000000000087 ffff880173c83dd8 ffffffff81080eef 0000000000000002
[  942.813414]  0000000000000087 ffff880173c83df0 ffffffff810833e4 0000000000000002
[  942.821850]  ffff880173c83e20 ffffffff81123635 ffff880173c988c0 ffffffff81e431c0
[  942.830292] Call Trace:
[  942.833056]  <IRQ>  [<ffffffff81080eef>] sched_show_task+0xbf/0x120
[  942.840179]  [<ffffffff810833e4>] dump_cpu_task+0x34/0x40
[  942.846307]  [<ffffffff81123635>] rcu_dump_cpu_stacks+0x79/0xb4
[  942.853023]  [<ffffffff810aff07>] rcu_check_callbacks+0x717/0x870
[  942.859937]  [<ffffffff810f0bfc>] ? __acct_update_integrals+0x2c/0xb0
[  942.867244]  [<ffffffff810c3980>] ? tick_sched_do_timer+0x30/0x30
[  942.874158]  [<ffffffff810b4e8a>] update_process_times+0x2a/0x50
[  942.880971]  [<ffffffff810c33e1>] tick_sched_handle.isra.13+0x31/0x40
[  942.888273]  [<ffffffff810c39b8>] tick_sched_timer+0x38/0x70
[  942.894688]  [<ffffffff810b5b0a>] __hrtimer_run_queues+0xda/0x250
[  942.901602]  [<ffffffff810b5f53>] hrtimer_interrupt+0xa3/0x190
[  942.908219]  [<ffffffff8103e643>] local_apic_timer_interrupt+0x33/0x60
[  942.915627]  [<ffffffff8103f148>] smp_apic_timer_interrupt+0x38/0x50
[  942.922837]  [<ffffffff8199cfcf>] apic_timer_interrupt+0x7f/0x90
[  942.929651]  <EOI>  [<ffffffff81329501>] ? blk_queue_split+0x251/0x520
[  942.937074]  [<ffffffff8132891c>] ? create_task_io_context+0x1c/0xf0
[  942.944282]  [<ffffffff81325490>] blk_queue_bio+0x40/0x350
[  942.950506]  [<ffffffff81323a3b>] generic_make_request+0xcb/0x1a0
[  942.957403]  [<ffffffff81323b76>] submit_bio+0x66/0x120
[  942.963328]  [<ffffffff8132b608>] ? next_bio+0x18/0x40
[  942.969159]  [<ffffffff8132b783>] ? __blkdev_issue_discard+0x153/0x1b0
[  942.976564]  [<ffffffff8131aeb9>] submit_bio_wait+0x49/0x60
[  942.982886]  [<ffffffff8132b975>] blkdev_issue_discard+0x65/0xa0
[  942.989701]  [<ffffffff81092b0f>] ? __wake_up+0x3f/0x50
[  942.995632]  [<ffffffff813fd241>] ? tty_ldisc_deref+0x11/0x20
[  943.002151]  [<ffffffff81331848>] blk_ioctl_discard+0x78/0x90
[  943.008658]  [<ffffffff81332454>] blkdev_ioctl+0x714/0x8a0
[  943.014880]  [<ffffffff811b62d8>] block_ioctl+0x38/0x40
[  943.020801]  [<ffffffff8119265b>] do_vfs_ioctl+0x8b/0x590
[  943.026925]  [<ffffffff81192bd4>] SyS_ioctl+0x74/0x80
[  943.032656]  [<ffffffff8199c41b>] entry_SYSCALL_64_fastpath+0x13/0x8f

I made a few hacks that seemed to make it go but I expect there are more
places to change:

diff --git a/block/bio.c b/block/bio.c
index f39477538fef..8a69af062b9c 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -667,7 +667,7 @@ struct bio *bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
 	bio->bi_iter.bi_sector	= bio_src->bi_iter.bi_sector;
 	bio->bi_iter.bi_size	= bio_src->bi_iter.bi_size;
 
-	if (bio_op(bio) == REQ_OP_DISCARD)
+	if (bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_SECURE_ERASE)
 		goto integrity_clone;
 
 	if (bio_op(bio) == REQ_OP_WRITE_SAME) {
@@ -1788,7 +1788,7 @@ struct bio *bio_split(struct bio *bio, int sectors,
 	 * Discards need a mutable bio_vec to accommodate the payload
 	 * required by the DSM TRIM and UNMAP commands.
 	 */
-	if (bio_op(bio) == REQ_OP_DISCARD)
+	if (bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_SECURE_ERASE)
 		split = bio_clone_bioset(bio, gfp, bs);
 	else
 		split = bio_clone_fast(bio, gfp, bs);
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 3eec75a9e91d..b995ab078755 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -172,7 +172,8 @@ void blk_queue_split(struct request_queue *q, struct bio **bio,
 	struct bio *split, *res;
 	unsigned nsegs;
 
-	if (bio_op(*bio) == REQ_OP_DISCARD)
+	if (bio_op(*bio) == REQ_OP_DISCARD ||
+	    bio_op(*bio) == REQ_OP_SECURE_ERASE)
 		split = blk_bio_discard_split(q, *bio, bs, &nsegs);
 	else if (bio_op(*bio) == REQ_OP_WRITE_SAME)
 		split = blk_bio_write_same_split(q, *bio, bs, &nsegs);
@@ -213,7 +214,7 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
 	 * This should probably be returning 0, but blk_add_request_payload()
 	 * (Christoph!!!!)
 	 */
-	if (bio_op(bio) == REQ_OP_DISCARD)
+	if (bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_SECURE_ERASE)
 		return 1;
 
 	if (bio_op(bio) == REQ_OP_WRITE_SAME)
@@ -385,7 +386,8 @@ static int __blk_bios_map_sg(struct request_queue *q, struct bio *bio,
 	nsegs = 0;
 	cluster = blk_queue_cluster(q);
 
-	if (bio_op(bio) == REQ_OP_DISCARD) {
+	if (bio_op(bio) == REQ_OP_DISCARD ||
+	    bio_op(bio) == REQ_OP_SECURE_ERASE) {
 		/*
 		 * This is a hack - drivers should be neither modifying the
 		 * biovec, nor relying on bi_vcnt - but because of
diff --git a/block/elevator.c b/block/elevator.c
index 7096c22041e7..f5e2cec71f15 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -368,6 +368,8 @@ void elv_dispatch_sort(struct request_queue *q, struct request *rq)
 
 		if ((req_op(rq) == REQ_OP_DISCARD) != (req_op(pos) == REQ_OP_DISCARD))
 			break;
+		if ((req_op(rq) == REQ_OP_SECURE_ERASE) != (req_op(pos) == REQ_OP_SECURE_ERASE))
+			break;
 		if (rq_data_dir(rq) != rq_data_dir(pos))
 			break;
 		if (pos->cmd_flags & stop_flags)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 48a5dd740f3b..82503e6f04b3 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1726,6 +1726,7 @@ static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request *req)
 			break;
 
 		if (req_op(next) == REQ_OP_DISCARD ||
+		    req_op(next) == REQ_OP_SECURE_ERASE ||
 		    req_op(next) == REQ_OP_FLUSH)
 			break;
 
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index bf14642a576a..29578e98603d 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -33,7 +33,8 @@ static int mmc_prep_request(struct request_queue *q, struct request *req)
 	/*
 	 * We only like normal block requests and discards.
 	 */
-	if (req->cmd_type != REQ_TYPE_FS && req_op(req) != REQ_OP_DISCARD) {
+	if (req->cmd_type != REQ_TYPE_FS && req_op(req) != REQ_OP_DISCARD &&
+	    req_op(req) != REQ_OP_SECURE_ERASE) {
 		blk_dump_rq_flags(req, "MMC bad request");
 		return BLKPREP_KILL;
 	}
diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
index d62531124d54..fee5e1271465 100644
--- a/drivers/mmc/card/queue.h
+++ b/drivers/mmc/card/queue.h
@@ -4,7 +4,9 @@
 static inline bool mmc_req_is_special(struct request *req)
 {
 	return req &&
-		(req_op(req) == REQ_OP_FLUSH || req_op(req) == REQ_OP_DISCARD);
+		(req_op(req) == REQ_OP_FLUSH ||
+		 req_op(req) == REQ_OP_DISCARD ||
+		 req_op(req) == REQ_OP_SECURE_ERASE);
 }
 
 struct request;


Regards
Adrian

Re: [REGRESSION] Secure discard is broken

[Christoph Hellwig]

> I made a few hacks that seemed to make it go but I expect there are more
> places to change:

That should be about it, and I did indeed forget about these places.
For the place where we hceck for multiple op types it would be nice
to use switch statements, though.

[PATCH] block: Fix secure erase

[Adrian Hunter]

Commit 288dab8a35a0 ("block: add a separate operation type for secure
erase") split REQ_OP_SECURE_ERASE from REQ_OP_DISCARD without considering
all the places REQ_OP_DISCARD was being used to mean either. Fix those.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Fixes: 288dab8a35a0 ("block: add a separate operation type for secure erase")
---
 block/bio.c              | 21 +++++++++++----------
 block/blk-merge.c        | 33 +++++++++++++++++++--------------
 block/elevator.c         |  5 ++++-
 drivers/mmc/card/block.c |  1 +
 drivers/mmc/card/queue.c |  3 ++-
 drivers/mmc/card/queue.h |  4 +++-
 include/linux/bio.h      | 10 ++++++++--
 include/linux/blkdev.h   |  6 ++++--
 kernel/trace/blktrace.c  |  2 +-
 9 files changed, 53 insertions(+), 32 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index f39477538fef..aa7354088008 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -667,18 +667,19 @@ struct bio *bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
 	bio->bi_iter.bi_sector	= bio_src->bi_iter.bi_sector;
 	bio->bi_iter.bi_size	= bio_src->bi_iter.bi_size;
 
-	if (bio_op(bio) == REQ_OP_DISCARD)
-		goto integrity_clone;
-
-	if (bio_op(bio) == REQ_OP_WRITE_SAME) {
+	switch (bio_op(bio)) {
+	case REQ_OP_DISCARD:
+	case REQ_OP_SECURE_ERASE:
+		break;
+	case REQ_OP_WRITE_SAME:
 		bio->bi_io_vec[bio->bi_vcnt++] = bio_src->bi_io_vec[0];
-		goto integrity_clone;
+		break;
+	default:
+		bio_for_each_segment(bv, bio_src, iter)
+			bio->bi_io_vec[bio->bi_vcnt++] = bv;
+		break;
 	}
 
-	bio_for_each_segment(bv, bio_src, iter)
-		bio->bi_io_vec[bio->bi_vcnt++] = bv;
-
-integrity_clone:
 	if (bio_integrity(bio_src)) {
 		int ret;
 
@@ -1788,7 +1789,7 @@ struct bio *bio_split(struct bio *bio, int sectors,
 	 * Discards need a mutable bio_vec to accommodate the payload
 	 * required by the DSM TRIM and UNMAP commands.
 	 */
-	if (bio_op(bio) == REQ_OP_DISCARD)
+	if (bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_SECURE_ERASE)
 		split = bio_clone_bioset(bio, gfp, bs);
 	else
 		split = bio_clone_fast(bio, gfp, bs);
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 3eec75a9e91d..72627e3cf91e 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -172,12 +172,18 @@ void blk_queue_split(struct request_queue *q, struct bio **bio,
 	struct bio *split, *res;
 	unsigned nsegs;
 
-	if (bio_op(*bio) == REQ_OP_DISCARD)
+	switch (bio_op(*bio)) {
+	case REQ_OP_DISCARD:
+	case REQ_OP_SECURE_ERASE:
 		split = blk_bio_discard_split(q, *bio, bs, &nsegs);
-	else if (bio_op(*bio) == REQ_OP_WRITE_SAME)
+		break;
+	case REQ_OP_WRITE_SAME:
 		split = blk_bio_write_same_split(q, *bio, bs, &nsegs);
-	else
+		break;
+	default:
 		split = blk_bio_segment_split(q, *bio, q->bio_split, &nsegs);
+		break;
+	}
 
 	/* physical segments can be figured out during splitting */
 	res = split ? split : *bio;
@@ -213,7 +219,7 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
 	 * This should probably be returning 0, but blk_add_request_payload()
 	 * (Christoph!!!!)
 	 */
-	if (bio_op(bio) == REQ_OP_DISCARD)
+	if (bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_SECURE_ERASE)
 		return 1;
 
 	if (bio_op(bio) == REQ_OP_WRITE_SAME)
@@ -385,7 +391,9 @@ static int __blk_bios_map_sg(struct request_queue *q, struct bio *bio,
 	nsegs = 0;
 	cluster = blk_queue_cluster(q);
 
-	if (bio_op(bio) == REQ_OP_DISCARD) {
+	switch (bio_op(bio)) {
+	case REQ_OP_DISCARD:
+	case REQ_OP_SECURE_ERASE:
 		/*
 		 * This is a hack - drivers should be neither modifying the
 		 * biovec, nor relying on bi_vcnt - but because of
@@ -393,19 +401,16 @@ static int __blk_bios_map_sg(struct request_queue *q, struct bio *bio,
 		 * a payload we need to set up here (thank you Christoph) and
 		 * bi_vcnt is really the only way of telling if we need to.
 		 */
-
-		if (bio->bi_vcnt)
-			goto single_segment;
-
-		return 0;
-	}
-
-	if (bio_op(bio) == REQ_OP_WRITE_SAME) {
-single_segment:
+		if (!bio->bi_vcnt)
+			return 0;
+		/* Fall through */
+	case REQ_OP_WRITE_SAME:
 		*sg = sglist;
 		bvec = bio_iovec(bio);
 		sg_set_page(*sg, bvec.bv_page, bvec.bv_len, bvec.bv_offset);
 		return 1;
+	default:
+		break;
 	}
 
 	for_each_bio(bio)
diff --git a/block/elevator.c b/block/elevator.c
index 7096c22041e7..e5924504aff6 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -366,7 +366,10 @@ void elv_dispatch_sort(struct request_queue *q, struct request *rq)
 	list_for_each_prev(entry, &q->queue_head) {
 		struct request *pos = list_entry_rq(entry);
 
-		if ((req_op(rq) == REQ_OP_DISCARD) != (req_op(pos) == REQ_OP_DISCARD))
+		if ((req_op(rq) == REQ_OP_DISCARD ||
+		     req_op(rq) == REQ_OP_SECURE_ERASE) !=
+		    (req_op(pos) == REQ_OP_DISCARD ||
+		     req_op(pos) == REQ_OP_SECURE_ERASE))
 			break;
 		if (rq_data_dir(rq) != rq_data_dir(pos))
 			break;
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 48a5dd740f3b..82503e6f04b3 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1726,6 +1726,7 @@ static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request *req)
 			break;
 
 		if (req_op(next) == REQ_OP_DISCARD ||
+		    req_op(next) == REQ_OP_SECURE_ERASE ||
 		    req_op(next) == REQ_OP_FLUSH)
 			break;
 
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index bf14642a576a..29578e98603d 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -33,7 +33,8 @@ static int mmc_prep_request(struct request_queue *q, struct request *req)
 	/*
 	 * We only like normal block requests and discards.
 	 */
-	if (req->cmd_type != REQ_TYPE_FS && req_op(req) != REQ_OP_DISCARD) {
+	if (req->cmd_type != REQ_TYPE_FS && req_op(req) != REQ_OP_DISCARD &&
+	    req_op(req) != REQ_OP_SECURE_ERASE) {
 		blk_dump_rq_flags(req, "MMC bad request");
 		return BLKPREP_KILL;
 	}
diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
index d62531124d54..fee5e1271465 100644
--- a/drivers/mmc/card/queue.h
+++ b/drivers/mmc/card/queue.h
@@ -4,7 +4,9 @@
 static inline bool mmc_req_is_special(struct request *req)
 {
 	return req &&
-		(req_op(req) == REQ_OP_FLUSH || req_op(req) == REQ_OP_DISCARD);
+		(req_op(req) == REQ_OP_FLUSH ||
+		 req_op(req) == REQ_OP_DISCARD ||
+		 req_op(req) == REQ_OP_SECURE_ERASE);
 }
 
 struct request;
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 59ffaa68b11b..23ddf4b46a9b 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -71,7 +71,8 @@ static inline bool bio_has_data(struct bio *bio)
 {
 	if (bio &&
 	    bio->bi_iter.bi_size &&
-	    bio_op(bio) != REQ_OP_DISCARD)
+	    bio_op(bio) != REQ_OP_DISCARD &&
+	    bio_op(bio) != REQ_OP_SECURE_ERASE)
 		return true;
 
 	return false;
@@ -79,7 +80,9 @@ static inline bool bio_has_data(struct bio *bio)
 
 static inline bool bio_no_advance_iter(struct bio *bio)
 {
-	return bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_WRITE_SAME;
+	return bio_op(bio) == REQ_OP_DISCARD ||
+	       bio_op(bio) == REQ_OP_SECURE_ERASE ||
+	       bio_op(bio) == REQ_OP_WRITE_SAME;
 }
 
 static inline bool bio_is_rw(struct bio *bio)
@@ -199,6 +202,9 @@ static inline unsigned bio_segments(struct bio *bio)
 	if (bio_op(bio) == REQ_OP_DISCARD)
 		return 1;
 
+	if (bio_op(bio) == REQ_OP_SECURE_ERASE)
+		return 1;
+
 	if (bio_op(bio) == REQ_OP_WRITE_SAME)
 		return 1;
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 2c210b6a7bcf..e79055c8b577 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -882,7 +882,7 @@ static inline unsigned int blk_rq_cur_sectors(const struct request *rq)
 static inline unsigned int blk_queue_get_max_sectors(struct request_queue *q,
 						     int op)
 {
-	if (unlikely(op == REQ_OP_DISCARD))
+	if (unlikely(op == REQ_OP_DISCARD || op == REQ_OP_SECURE_ERASE))
 		return min(q->limits.max_discard_sectors, UINT_MAX >> 9);
 
 	if (unlikely(op == REQ_OP_WRITE_SAME))
@@ -913,7 +913,9 @@ static inline unsigned int blk_rq_get_max_sectors(struct request *rq,
 	if (unlikely(rq->cmd_type != REQ_TYPE_FS))
 		return q->limits.max_hw_sectors;
 
-	if (!q->limits.chunk_sectors || (req_op(rq) == REQ_OP_DISCARD))
+	if (!q->limits.chunk_sectors ||
+	    req_op(rq) == REQ_OP_DISCARD ||
+	    req_op(rq) == REQ_OP_SECURE_ERASE)
 		return blk_queue_get_max_sectors(q, req_op(rq));
 
 	return min(blk_max_size_offset(q, offset),
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 7598e6ca817a..dbafc5df03f3 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -223,7 +223,7 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
 	what |= MASK_TC_BIT(op_flags, META);
 	what |= MASK_TC_BIT(op_flags, PREFLUSH);
 	what |= MASK_TC_BIT(op_flags, FUA);
-	if (op == REQ_OP_DISCARD)
+	if (op == REQ_OP_DISCARD || op == REQ_OP_SECURE_ERASE)
 		what |= BLK_TC_ACT(BLK_TC_DISCARD);
 	if (op == REQ_OP_FLUSH)
 		what |= BLK_TC_ACT(BLK_TC_FLUSH);
-- 
1.9.1

Re: [PATCH] block: Fix secure erase

[Shaun Tancheff]

On Mon, Aug 15, 2016 at 9:07 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Commit 288dab8a35a0 ("block: add a separate operation type for secure
> erase") split REQ_OP_SECURE_ERASE from REQ_OP_DISCARD without considering
> all the places REQ_OP_DISCARD was being used to mean either. Fix those.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> Fixes: 288dab8a35a0 ("block: add a separate operation type for secure erase")
> ---
>  block/bio.c              | 21 +++++++++++----------
>  block/blk-merge.c        | 33 +++++++++++++++++++--------------
>  block/elevator.c         |  5 ++++-
>  drivers/mmc/card/block.c |  1 +
>  drivers/mmc/card/queue.c |  3 ++-
>  drivers/mmc/card/queue.h |  4 +++-
>  include/linux/bio.h      | 10 ++++++++--
>  include/linux/blkdev.h   |  6 ++++--
>  kernel/trace/blktrace.c  |  2 +-
>  9 files changed, 53 insertions(+), 32 deletions(-)

Hmm ... Since REQ_SECURE implied REQ_DISCARD doesn't this
mean that we should include REQ_OP_SECURE_ERASE checking
wherever REQ_OP_DISCARD is being checked now in drivers/scsi/sd.c ?

(It's only in 3 spots so it's a quickie patch)

> diff --git a/block/bio.c b/block/bio.c
> index f39477538fef..aa7354088008 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -667,18 +667,19 @@ struct bio *bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
>         bio->bi_iter.bi_sector  = bio_src->bi_iter.bi_sector;
>         bio->bi_iter.bi_size    = bio_src->bi_iter.bi_size;
>
> -       if (bio_op(bio) == REQ_OP_DISCARD)
> -               goto integrity_clone;
> -
> -       if (bio_op(bio) == REQ_OP_WRITE_SAME) {
> +       switch (bio_op(bio)) {
> +       case REQ_OP_DISCARD:
> +       case REQ_OP_SECURE_ERASE:
> +               break;
> +       case REQ_OP_WRITE_SAME:
>                 bio->bi_io_vec[bio->bi_vcnt++] = bio_src->bi_io_vec[0];
> -               goto integrity_clone;
> +               break;
> +       default:
> +               bio_for_each_segment(bv, bio_src, iter)
> +                       bio->bi_io_vec[bio->bi_vcnt++] = bv;
> +               break;
>         }
>
> -       bio_for_each_segment(bv, bio_src, iter)
> -               bio->bi_io_vec[bio->bi_vcnt++] = bv;
> -
> -integrity_clone:
>         if (bio_integrity(bio_src)) {
>                 int ret;
>
> @@ -1788,7 +1789,7 @@ struct bio *bio_split(struct bio *bio, int sectors,
>          * Discards need a mutable bio_vec to accommodate the payload
>          * required by the DSM TRIM and UNMAP commands.
>          */
> -       if (bio_op(bio) == REQ_OP_DISCARD)
> +       if (bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_SECURE_ERASE)
>                 split = bio_clone_bioset(bio, gfp, bs);
>         else
>                 split = bio_clone_fast(bio, gfp, bs);
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 3eec75a9e91d..72627e3cf91e 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -172,12 +172,18 @@ void blk_queue_split(struct request_queue *q, struct bio **bio,
>         struct bio *split, *res;
>         unsigned nsegs;
>
> -       if (bio_op(*bio) == REQ_OP_DISCARD)
> +       switch (bio_op(*bio)) {
> +       case REQ_OP_DISCARD:
> +       case REQ_OP_SECURE_ERASE:
>                 split = blk_bio_discard_split(q, *bio, bs, &nsegs);
> -       else if (bio_op(*bio) == REQ_OP_WRITE_SAME)
> +               break;
> +       case REQ_OP_WRITE_SAME:
>                 split = blk_bio_write_same_split(q, *bio, bs, &nsegs);
> -       else
> +               break;
> +       default:
>                 split = blk_bio_segment_split(q, *bio, q->bio_split, &nsegs);
> +               break;
> +       }
>
>         /* physical segments can be figured out during splitting */
>         res = split ? split : *bio;
> @@ -213,7 +219,7 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
>          * This should probably be returning 0, but blk_add_request_payload()
>          * (Christoph!!!!)
>          */
> -       if (bio_op(bio) == REQ_OP_DISCARD)
> +       if (bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_SECURE_ERASE)
>                 return 1;
>
>         if (bio_op(bio) == REQ_OP_WRITE_SAME)
> @@ -385,7 +391,9 @@ static int __blk_bios_map_sg(struct request_queue *q, struct bio *bio,
>         nsegs = 0;
>         cluster = blk_queue_cluster(q);
>
> -       if (bio_op(bio) == REQ_OP_DISCARD) {
> +       switch (bio_op(bio)) {
> +       case REQ_OP_DISCARD:
> +       case REQ_OP_SECURE_ERASE:
>                 /*
>                  * This is a hack - drivers should be neither modifying the
>                  * biovec, nor relying on bi_vcnt - but because of
> @@ -393,19 +401,16 @@ static int __blk_bios_map_sg(struct request_queue *q, struct bio *bio,
>                  * a payload we need to set up here (thank you Christoph) and
>                  * bi_vcnt is really the only way of telling if we need to.
>                  */
> -
> -               if (bio->bi_vcnt)
> -                       goto single_segment;
> -
> -               return 0;
> -       }
> -
> -       if (bio_op(bio) == REQ_OP_WRITE_SAME) {
> -single_segment:
> +               if (!bio->bi_vcnt)
> +                       return 0;
> +               /* Fall through */
> +       case REQ_OP_WRITE_SAME:
>                 *sg = sglist;
>                 bvec = bio_iovec(bio);
>                 sg_set_page(*sg, bvec.bv_page, bvec.bv_len, bvec.bv_offset);
>                 return 1;
> +       default:
> +               break;
>         }
>
>         for_each_bio(bio)
> diff --git a/block/elevator.c b/block/elevator.c
> index 7096c22041e7..e5924504aff6 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -366,7 +366,10 @@ void elv_dispatch_sort(struct request_queue *q, struct request *rq)
>         list_for_each_prev(entry, &q->queue_head) {
>                 struct request *pos = list_entry_rq(entry);
>
> -               if ((req_op(rq) == REQ_OP_DISCARD) != (req_op(pos) == REQ_OP_DISCARD))
> +               if ((req_op(rq) == REQ_OP_DISCARD ||
> +                    req_op(rq) == REQ_OP_SECURE_ERASE) !=
> +                   (req_op(pos) == REQ_OP_DISCARD ||
> +                    req_op(pos) == REQ_OP_SECURE_ERASE))
>                         break;
>                 if (rq_data_dir(rq) != rq_data_dir(pos))
>                         break;
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 48a5dd740f3b..82503e6f04b3 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -1726,6 +1726,7 @@ static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request *req)
>                         break;
>
>                 if (req_op(next) == REQ_OP_DISCARD ||
> +                   req_op(next) == REQ_OP_SECURE_ERASE ||
>                     req_op(next) == REQ_OP_FLUSH)
>                         break;
>
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index bf14642a576a..29578e98603d 100644
> --- a/drivers/mmc/card/queue.c
> +++ b/drivers/mmc/card/queue.c
> @@ -33,7 +33,8 @@ static int mmc_prep_request(struct request_queue *q, struct request *req)
>         /*
>          * We only like normal block requests and discards.
>          */
> -       if (req->cmd_type != REQ_TYPE_FS && req_op(req) != REQ_OP_DISCARD) {
> +       if (req->cmd_type != REQ_TYPE_FS && req_op(req) != REQ_OP_DISCARD &&
> +           req_op(req) != REQ_OP_SECURE_ERASE) {
>                 blk_dump_rq_flags(req, "MMC bad request");
>                 return BLKPREP_KILL;
>         }
> diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
> index d62531124d54..fee5e1271465 100644
> --- a/drivers/mmc/card/queue.h
> +++ b/drivers/mmc/card/queue.h
> @@ -4,7 +4,9 @@
>  static inline bool mmc_req_is_special(struct request *req)
>  {
>         return req &&
> -               (req_op(req) == REQ_OP_FLUSH || req_op(req) == REQ_OP_DISCARD);
> +               (req_op(req) == REQ_OP_FLUSH ||
> +                req_op(req) == REQ_OP_DISCARD ||
> +                req_op(req) == REQ_OP_SECURE_ERASE);
>  }
>
>  struct request;
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 59ffaa68b11b..23ddf4b46a9b 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -71,7 +71,8 @@ static inline bool bio_has_data(struct bio *bio)
>  {
>         if (bio &&
>             bio->bi_iter.bi_size &&
> -           bio_op(bio) != REQ_OP_DISCARD)
> +           bio_op(bio) != REQ_OP_DISCARD &&
> +           bio_op(bio) != REQ_OP_SECURE_ERASE)
>                 return true;
>
>         return false;
> @@ -79,7 +80,9 @@ static inline bool bio_has_data(struct bio *bio)
>
>  static inline bool bio_no_advance_iter(struct bio *bio)
>  {
> -       return bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_WRITE_SAME;
> +       return bio_op(bio) == REQ_OP_DISCARD ||
> +              bio_op(bio) == REQ_OP_SECURE_ERASE ||
> +              bio_op(bio) == REQ_OP_WRITE_SAME;
>  }
>
>  static inline bool bio_is_rw(struct bio *bio)
> @@ -199,6 +202,9 @@ static inline unsigned bio_segments(struct bio *bio)
>         if (bio_op(bio) == REQ_OP_DISCARD)
>                 return 1;
>
> +       if (bio_op(bio) == REQ_OP_SECURE_ERASE)
> +               return 1;
> +
>         if (bio_op(bio) == REQ_OP_WRITE_SAME)
>                 return 1;
>
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 2c210b6a7bcf..e79055c8b577 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -882,7 +882,7 @@ static inline unsigned int blk_rq_cur_sectors(const struct request *rq)
>  static inline unsigned int blk_queue_get_max_sectors(struct request_queue *q,
>                                                      int op)
>  {
> -       if (unlikely(op == REQ_OP_DISCARD))
> +       if (unlikely(op == REQ_OP_DISCARD || op == REQ_OP_SECURE_ERASE))
>                 return min(q->limits.max_discard_sectors, UINT_MAX >> 9);
>
>         if (unlikely(op == REQ_OP_WRITE_SAME))
> @@ -913,7 +913,9 @@ static inline unsigned int blk_rq_get_max_sectors(struct request *rq,
>         if (unlikely(rq->cmd_type != REQ_TYPE_FS))
>                 return q->limits.max_hw_sectors;
>
> -       if (!q->limits.chunk_sectors || (req_op(rq) == REQ_OP_DISCARD))
> +       if (!q->limits.chunk_sectors ||
> +           req_op(rq) == REQ_OP_DISCARD ||
> +           req_op(rq) == REQ_OP_SECURE_ERASE)
>                 return blk_queue_get_max_sectors(q, req_op(rq));
>
>         return min(blk_max_size_offset(q, offset),
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 7598e6ca817a..dbafc5df03f3 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -223,7 +223,7 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
>         what |= MASK_TC_BIT(op_flags, META);
>         what |= MASK_TC_BIT(op_flags, PREFLUSH);
>         what |= MASK_TC_BIT(op_flags, FUA);
> -       if (op == REQ_OP_DISCARD)
> +       if (op == REQ_OP_DISCARD || op == REQ_OP_SECURE_ERASE)
>                 what |= BLK_TC_ACT(BLK_TC_DISCARD);
>         if (op == REQ_OP_FLUSH)
>                 what |= BLK_TC_ACT(BLK_TC_FLUSH);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DQIBAg&c=IGDlg0lD0b-nebmJJ0Kp8A&r=Wg5NqlNlVTT7Ugl8V50qIHLe856QW0qfG3WVYGOrWzA&m=Y15JQ82w_sRbTpLbwWZih05XQoGHNqYfhx0M_42AepQ&s=LMrzTEwBuBaQO9U9V-bZnGTGnpBfUXCdpjig4yyXvDM&e=



-- 
Shaun Tancheff

Re: [PATCH] block: Fix secure erase

[Christoph Hellwig]

> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -366,7 +366,10 @@ void elv_dispatch_sort(struct request_queue *q, struct request *rq)
>  	list_for_each_prev(entry, &q->queue_head) {
>  		struct request *pos = list_entry_rq(entry);
>  
> -		if ((req_op(rq) == REQ_OP_DISCARD) != (req_op(pos) == REQ_OP_DISCARD))
> +		if ((req_op(rq) == REQ_OP_DISCARD ||
> +		     req_op(rq) == REQ_OP_SECURE_ERASE) !=
> +		    (req_op(pos) == REQ_OP_DISCARD ||
> +		     req_op(pos) == REQ_OP_SECURE_ERASE))
>  			break;

This really should be a:

	if (req_op(rq) != req_op(pos))

I'l lleave it up to Jens if he wants that in this patch or not, otherwise
I'll send an incremental patch.

Otherwise this looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] block: Fix secure erase

[Christoph Hellwig]

On Mon, Aug 15, 2016 at 11:43:12AM -0500, Shaun Tancheff wrote:
> Hmm ... Since REQ_SECURE implied REQ_DISCARD doesn't this
> mean that we should include REQ_OP_SECURE_ERASE checking
> wherever REQ_OP_DISCARD is being checked now in drivers/scsi/sd.c ?
> 
> (It's only in 3 spots so it's a quickie patch)

SCSI doesn't support secure erase operations.  Only MMC really
supports it, plus the usual cargo culting in Xen blkfront that's
probably never been tested..

Re: [PATCH] block: Fix secure erase

[Jens Axboe]

On 08/15/2016 12:13 PM, Christoph Hellwig wrote:
>> --- a/block/elevator.c
>> +++ b/block/elevator.c
>> @@ -366,7 +366,10 @@ void elv_dispatch_sort(struct request_queue *q, struct request *rq)
>>  	list_for_each_prev(entry, &q->queue_head) {
>>  		struct request *pos = list_entry_rq(entry);
>>
>> -		if ((req_op(rq) == REQ_OP_DISCARD) != (req_op(pos) == REQ_OP_DISCARD))
>> +		if ((req_op(rq) == REQ_OP_DISCARD ||
>> +		     req_op(rq) == REQ_OP_SECURE_ERASE) !=
>> +		    (req_op(pos) == REQ_OP_DISCARD ||
>> +		     req_op(pos) == REQ_OP_SECURE_ERASE))
>>  			break;
>
> This really should be a:
>
> 	if (req_op(rq) != req_op(pos))
>
> I'l lleave it up to Jens if he wants that in this patch or not, otherwise
> I'll send an incremental patch.

Let's get a v2 with that fixed up, it makes a big readability
difference.

-- 
Jens Axboe

Re: [PATCH] block: Fix secure erase

[Christoph Hellwig]

On Mon, Aug 15, 2016 at 12:16:30PM -0600, Jens Axboe wrote:
>> This really should be a:
>>
>> 	if (req_op(rq) != req_op(pos))
>>
>> I'l lleave it up to Jens if he wants that in this patch or not, otherwise
>> I'll send an incremental patch.
>
> Let's get a v2 with that fixed up, it makes a big readability
> difference.

It's not just readbility, it's also a potential correctness issue.
Not sure if we'd hit it for other request types at the moment, but
we'd surely hit if people add more..

Re: [PATCH] block: Fix secure erase

[Adrian Hunter]

On 15/08/16 21:14, Christoph Hellwig wrote:
> On Mon, Aug 15, 2016 at 11:43:12AM -0500, Shaun Tancheff wrote:
>> Hmm ... Since REQ_SECURE implied REQ_DISCARD doesn't this
>> mean that we should include REQ_OP_SECURE_ERASE checking
>> wherever REQ_OP_DISCARD is being checked now in drivers/scsi/sd.c ?
>>
>> (It's only in 3 spots so it's a quickie patch)
> 
> SCSI doesn't support secure erase operations.  Only MMC really
> supports it, plus the usual cargo culting in Xen blkfront that's
> probably never been tested..
> 

I left SCSI out because support does not exist at the moment.
However there is UFS which is seen as the replacement for eMMC.
And there is a patch to add support for BLKSECDISCARD:

	http://marc.info/?l=linux-scsi&m=146953519016056

So SCSI will need updating if that is to go in.

[PATCH V2] block: Fix secure erase

[Adrian Hunter]

Commit 288dab8a35a0 ("block: add a separate operation type for secure
erase") split REQ_OP_SECURE_ERASE from REQ_OP_DISCARD without considering
all the places REQ_OP_DISCARD was being used to mean either. Fix those.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Fixes: 288dab8a35a0 ("block: add a separate operation type for secure erase")
---


Changes in V2:
	In elv_dispatch_sort() don't allow requests with different ops to pass
	one another.


 block/bio.c              | 21 +++++++++++----------
 block/blk-merge.c        | 33 +++++++++++++++++++--------------
 block/elevator.c         |  2 +-
 drivers/mmc/card/block.c |  1 +
 drivers/mmc/card/queue.c |  3 ++-
 drivers/mmc/card/queue.h |  4 +++-
 include/linux/bio.h      | 10 ++++++++--
 include/linux/blkdev.h   |  6 ++++--
 kernel/trace/blktrace.c  |  2 +-
 9 files changed, 50 insertions(+), 32 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index f39477538fef..aa7354088008 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -667,18 +667,19 @@ struct bio *bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
 	bio->bi_iter.bi_sector	= bio_src->bi_iter.bi_sector;
 	bio->bi_iter.bi_size	= bio_src->bi_iter.bi_size;
 
-	if (bio_op(bio) == REQ_OP_DISCARD)
-		goto integrity_clone;
-
-	if (bio_op(bio) == REQ_OP_WRITE_SAME) {
+	switch (bio_op(bio)) {
+	case REQ_OP_DISCARD:
+	case REQ_OP_SECURE_ERASE:
+		break;
+	case REQ_OP_WRITE_SAME:
 		bio->bi_io_vec[bio->bi_vcnt++] = bio_src->bi_io_vec[0];
-		goto integrity_clone;
+		break;
+	default:
+		bio_for_each_segment(bv, bio_src, iter)
+			bio->bi_io_vec[bio->bi_vcnt++] = bv;
+		break;
 	}
 
-	bio_for_each_segment(bv, bio_src, iter)
-		bio->bi_io_vec[bio->bi_vcnt++] = bv;
-
-integrity_clone:
 	if (bio_integrity(bio_src)) {
 		int ret;
 
@@ -1788,7 +1789,7 @@ struct bio *bio_split(struct bio *bio, int sectors,
 	 * Discards need a mutable bio_vec to accommodate the payload
 	 * required by the DSM TRIM and UNMAP commands.
 	 */
-	if (bio_op(bio) == REQ_OP_DISCARD)
+	if (bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_SECURE_ERASE)
 		split = bio_clone_bioset(bio, gfp, bs);
 	else
 		split = bio_clone_fast(bio, gfp, bs);
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 3eec75a9e91d..72627e3cf91e 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -172,12 +172,18 @@ void blk_queue_split(struct request_queue *q, struct bio **bio,
 	struct bio *split, *res;
 	unsigned nsegs;
 
-	if (bio_op(*bio) == REQ_OP_DISCARD)
+	switch (bio_op(*bio)) {
+	case REQ_OP_DISCARD:
+	case REQ_OP_SECURE_ERASE:
 		split = blk_bio_discard_split(q, *bio, bs, &nsegs);
-	else if (bio_op(*bio) == REQ_OP_WRITE_SAME)
+		break;
+	case REQ_OP_WRITE_SAME:
 		split = blk_bio_write_same_split(q, *bio, bs, &nsegs);
-	else
+		break;
+	default:
 		split = blk_bio_segment_split(q, *bio, q->bio_split, &nsegs);
+		break;
+	}
 
 	/* physical segments can be figured out during splitting */
 	res = split ? split : *bio;
@@ -213,7 +219,7 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
 	 * This should probably be returning 0, but blk_add_request_payload()
 	 * (Christoph!!!!)
 	 */
-	if (bio_op(bio) == REQ_OP_DISCARD)
+	if (bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_SECURE_ERASE)
 		return 1;
 
 	if (bio_op(bio) == REQ_OP_WRITE_SAME)
@@ -385,7 +391,9 @@ static int __blk_bios_map_sg(struct request_queue *q, struct bio *bio,
 	nsegs = 0;
 	cluster = blk_queue_cluster(q);
 
-	if (bio_op(bio) == REQ_OP_DISCARD) {
+	switch (bio_op(bio)) {
+	case REQ_OP_DISCARD:
+	case REQ_OP_SECURE_ERASE:
 		/*
 		 * This is a hack - drivers should be neither modifying the
 		 * biovec, nor relying on bi_vcnt - but because of
@@ -393,19 +401,16 @@ static int __blk_bios_map_sg(struct request_queue *q, struct bio *bio,
 		 * a payload we need to set up here (thank you Christoph) and
 		 * bi_vcnt is really the only way of telling if we need to.
 		 */
-
-		if (bio->bi_vcnt)
-			goto single_segment;
-
-		return 0;
-	}
-
-	if (bio_op(bio) == REQ_OP_WRITE_SAME) {
-single_segment:
+		if (!bio->bi_vcnt)
+			return 0;
+		/* Fall through */
+	case REQ_OP_WRITE_SAME:
 		*sg = sglist;
 		bvec = bio_iovec(bio);
 		sg_set_page(*sg, bvec.bv_page, bvec.bv_len, bvec.bv_offset);
 		return 1;
+	default:
+		break;
 	}
 
 	for_each_bio(bio)
diff --git a/block/elevator.c b/block/elevator.c
index 7096c22041e7..f7d973a56fd7 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -366,7 +366,7 @@ void elv_dispatch_sort(struct request_queue *q, struct request *rq)
 	list_for_each_prev(entry, &q->queue_head) {
 		struct request *pos = list_entry_rq(entry);
 
-		if ((req_op(rq) == REQ_OP_DISCARD) != (req_op(pos) == REQ_OP_DISCARD))
+		if (req_op(rq) != req_op(pos))
 			break;
 		if (rq_data_dir(rq) != rq_data_dir(pos))
 			break;
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 48a5dd740f3b..82503e6f04b3 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1726,6 +1726,7 @@ static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request *req)
 			break;
 
 		if (req_op(next) == REQ_OP_DISCARD ||
+		    req_op(next) == REQ_OP_SECURE_ERASE ||
 		    req_op(next) == REQ_OP_FLUSH)
 			break;
 
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index bf14642a576a..29578e98603d 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -33,7 +33,8 @@ static int mmc_prep_request(struct request_queue *q, struct request *req)
 	/*
 	 * We only like normal block requests and discards.
 	 */
-	if (req->cmd_type != REQ_TYPE_FS && req_op(req) != REQ_OP_DISCARD) {
+	if (req->cmd_type != REQ_TYPE_FS && req_op(req) != REQ_OP_DISCARD &&
+	    req_op(req) != REQ_OP_SECURE_ERASE) {
 		blk_dump_rq_flags(req, "MMC bad request");
 		return BLKPREP_KILL;
 	}
diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
index d62531124d54..fee5e1271465 100644
--- a/drivers/mmc/card/queue.h
+++ b/drivers/mmc/card/queue.h
@@ -4,7 +4,9 @@
 static inline bool mmc_req_is_special(struct request *req)
 {
 	return req &&
-		(req_op(req) == REQ_OP_FLUSH || req_op(req) == REQ_OP_DISCARD);
+		(req_op(req) == REQ_OP_FLUSH ||
+		 req_op(req) == REQ_OP_DISCARD ||
+		 req_op(req) == REQ_OP_SECURE_ERASE);
 }
 
 struct request;
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 59ffaa68b11b..23ddf4b46a9b 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -71,7 +71,8 @@ static inline bool bio_has_data(struct bio *bio)
 {
 	if (bio &&
 	    bio->bi_iter.bi_size &&
-	    bio_op(bio) != REQ_OP_DISCARD)
+	    bio_op(bio) != REQ_OP_DISCARD &&
+	    bio_op(bio) != REQ_OP_SECURE_ERASE)
 		return true;
 
 	return false;
@@ -79,7 +80,9 @@ static inline bool bio_has_data(struct bio *bio)
 
 static inline bool bio_no_advance_iter(struct bio *bio)
 {
-	return bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_WRITE_SAME;
+	return bio_op(bio) == REQ_OP_DISCARD ||
+	       bio_op(bio) == REQ_OP_SECURE_ERASE ||
+	       bio_op(bio) == REQ_OP_WRITE_SAME;
 }
 
 static inline bool bio_is_rw(struct bio *bio)
@@ -199,6 +202,9 @@ static inline unsigned bio_segments(struct bio *bio)
 	if (bio_op(bio) == REQ_OP_DISCARD)
 		return 1;
 
+	if (bio_op(bio) == REQ_OP_SECURE_ERASE)
+		return 1;
+
 	if (bio_op(bio) == REQ_OP_WRITE_SAME)
 		return 1;
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 2c210b6a7bcf..e79055c8b577 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -882,7 +882,7 @@ static inline unsigned int blk_rq_cur_sectors(const struct request *rq)
 static inline unsigned int blk_queue_get_max_sectors(struct request_queue *q,
 						     int op)
 {
-	if (unlikely(op == REQ_OP_DISCARD))
+	if (unlikely(op == REQ_OP_DISCARD || op == REQ_OP_SECURE_ERASE))
 		return min(q->limits.max_discard_sectors, UINT_MAX >> 9);
 
 	if (unlikely(op == REQ_OP_WRITE_SAME))
@@ -913,7 +913,9 @@ static inline unsigned int blk_rq_get_max_sectors(struct request *rq,
 	if (unlikely(rq->cmd_type != REQ_TYPE_FS))
 		return q->limits.max_hw_sectors;
 
-	if (!q->limits.chunk_sectors || (req_op(rq) == REQ_OP_DISCARD))
+	if (!q->limits.chunk_sectors ||
+	    req_op(rq) == REQ_OP_DISCARD ||
+	    req_op(rq) == REQ_OP_SECURE_ERASE)
 		return blk_queue_get_max_sectors(q, req_op(rq));
 
 	return min(blk_max_size_offset(q, offset),
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 7598e6ca817a..dbafc5df03f3 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -223,7 +223,7 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
 	what |= MASK_TC_BIT(op_flags, META);
 	what |= MASK_TC_BIT(op_flags, PREFLUSH);
 	what |= MASK_TC_BIT(op_flags, FUA);
-	if (op == REQ_OP_DISCARD)
+	if (op == REQ_OP_DISCARD || op == REQ_OP_SECURE_ERASE)
 		what |= BLK_TC_ACT(BLK_TC_DISCARD);
 	if (op == REQ_OP_FLUSH)
 		what |= BLK_TC_ACT(BLK_TC_FLUSH);
-- 
1.9.1

Re: [PATCH V2] block: Fix secure erase

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] block: Fix secure erase

[Christoph Hellwig]

On Tue, Aug 16, 2016 at 10:20:25AM +0300, Adrian Hunter wrote:
> On 15/08/16 21:14, Christoph Hellwig wrote:
> > On Mon, Aug 15, 2016 at 11:43:12AM -0500, Shaun Tancheff wrote:
> >> Hmm ... Since REQ_SECURE implied REQ_DISCARD doesn't this
> >> mean that we should include REQ_OP_SECURE_ERASE checking
> >> wherever REQ_OP_DISCARD is being checked now in drivers/scsi/sd.c ?
> >>
> >> (It's only in 3 spots so it's a quickie patch)
> > 
> > SCSI doesn't support secure erase operations.  Only MMC really
> > supports it, plus the usual cargo culting in Xen blkfront that's
> > probably never been tested..
> > 
> 
> I left SCSI out because support does not exist at the moment.
> However there is UFS which is seen as the replacement for eMMC.
> And there is a patch to add support for BLKSECDISCARD:
> 
> 	http://marc.info/?l=linux-scsi&m=146953519016056
> 
> So SCSI will need updating if that is to go in.

That patch is complete crap and if anyone thinks they'd get shit like
that in they are on the same crack that apparently the authors of the
UFS spec are on.

If you want secure discard supported in UFS get a command for into
SBC instead of bypassing the command set.

Re: [PATCH V2] block: Fix secure erase

[Ulf Hansson]

On 16 August 2016 at 09:59, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Commit 288dab8a35a0 ("block: add a separate operation type for secure
> erase") split REQ_OP_SECURE_ERASE from REQ_OP_DISCARD without considering
> all the places REQ_OP_DISCARD was being used to mean either. Fix those.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> Fixes: 288dab8a35a0 ("block: add a separate operation type for secure erase")

For the mmc parts:

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Jens, will you pick this up via your tree?

Kind regards
Uffe

> ---
>
>
> Changes in V2:
>         In elv_dispatch_sort() don't allow requests with different ops to pass
>         one another.
>
>
>  block/bio.c              | 21 +++++++++++----------
>  block/blk-merge.c        | 33 +++++++++++++++++++--------------
>  block/elevator.c         |  2 +-
>  drivers/mmc/card/block.c |  1 +
>  drivers/mmc/card/queue.c |  3 ++-
>  drivers/mmc/card/queue.h |  4 +++-
>  include/linux/bio.h      | 10 ++++++++--
>  include/linux/blkdev.h   |  6 ++++--
>  kernel/trace/blktrace.c  |  2 +-
>  9 files changed, 50 insertions(+), 32 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index f39477538fef..aa7354088008 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -667,18 +667,19 @@ struct bio *bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
>         bio->bi_iter.bi_sector  = bio_src->bi_iter.bi_sector;
>         bio->bi_iter.bi_size    = bio_src->bi_iter.bi_size;
>
> -       if (bio_op(bio) == REQ_OP_DISCARD)
> -               goto integrity_clone;
> -
> -       if (bio_op(bio) == REQ_OP_WRITE_SAME) {
> +       switch (bio_op(bio)) {
> +       case REQ_OP_DISCARD:
> +       case REQ_OP_SECURE_ERASE:
> +               break;
> +       case REQ_OP_WRITE_SAME:
>                 bio->bi_io_vec[bio->bi_vcnt++] = bio_src->bi_io_vec[0];
> -               goto integrity_clone;
> +               break;
> +       default:
> +               bio_for_each_segment(bv, bio_src, iter)
> +                       bio->bi_io_vec[bio->bi_vcnt++] = bv;
> +               break;
>         }
>
> -       bio_for_each_segment(bv, bio_src, iter)
> -               bio->bi_io_vec[bio->bi_vcnt++] = bv;
> -
> -integrity_clone:
>         if (bio_integrity(bio_src)) {
>                 int ret;
>
> @@ -1788,7 +1789,7 @@ struct bio *bio_split(struct bio *bio, int sectors,
>          * Discards need a mutable bio_vec to accommodate the payload
>          * required by the DSM TRIM and UNMAP commands.
>          */
> -       if (bio_op(bio) == REQ_OP_DISCARD)
> +       if (bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_SECURE_ERASE)
>                 split = bio_clone_bioset(bio, gfp, bs);
>         else
>                 split = bio_clone_fast(bio, gfp, bs);
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 3eec75a9e91d..72627e3cf91e 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -172,12 +172,18 @@ void blk_queue_split(struct request_queue *q, struct bio **bio,
>         struct bio *split, *res;
>         unsigned nsegs;
>
> -       if (bio_op(*bio) == REQ_OP_DISCARD)
> +       switch (bio_op(*bio)) {
> +       case REQ_OP_DISCARD:
> +       case REQ_OP_SECURE_ERASE:
>                 split = blk_bio_discard_split(q, *bio, bs, &nsegs);
> -       else if (bio_op(*bio) == REQ_OP_WRITE_SAME)
> +               break;
> +       case REQ_OP_WRITE_SAME:
>                 split = blk_bio_write_same_split(q, *bio, bs, &nsegs);
> -       else
> +               break;
> +       default:
>                 split = blk_bio_segment_split(q, *bio, q->bio_split, &nsegs);
> +               break;
> +       }
>
>         /* physical segments can be figured out during splitting */
>         res = split ? split : *bio;
> @@ -213,7 +219,7 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
>          * This should probably be returning 0, but blk_add_request_payload()
>          * (Christoph!!!!)
>          */
> -       if (bio_op(bio) == REQ_OP_DISCARD)
> +       if (bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_SECURE_ERASE)
>                 return 1;
>
>         if (bio_op(bio) == REQ_OP_WRITE_SAME)
> @@ -385,7 +391,9 @@ static int __blk_bios_map_sg(struct request_queue *q, struct bio *bio,
>         nsegs = 0;
>         cluster = blk_queue_cluster(q);
>
> -       if (bio_op(bio) == REQ_OP_DISCARD) {
> +       switch (bio_op(bio)) {
> +       case REQ_OP_DISCARD:
> +       case REQ_OP_SECURE_ERASE:
>                 /*
>                  * This is a hack - drivers should be neither modifying the
>                  * biovec, nor relying on bi_vcnt - but because of
> @@ -393,19 +401,16 @@ static int __blk_bios_map_sg(struct request_queue *q, struct bio *bio,
>                  * a payload we need to set up here (thank you Christoph) and
>                  * bi_vcnt is really the only way of telling if we need to.
>                  */
> -
> -               if (bio->bi_vcnt)
> -                       goto single_segment;
> -
> -               return 0;
> -       }
> -
> -       if (bio_op(bio) == REQ_OP_WRITE_SAME) {
> -single_segment:
> +               if (!bio->bi_vcnt)
> +                       return 0;
> +               /* Fall through */
> +       case REQ_OP_WRITE_SAME:
>                 *sg = sglist;
>                 bvec = bio_iovec(bio);
>                 sg_set_page(*sg, bvec.bv_page, bvec.bv_len, bvec.bv_offset);
>                 return 1;
> +       default:
> +               break;
>         }
>
>         for_each_bio(bio)
> diff --git a/block/elevator.c b/block/elevator.c
> index 7096c22041e7..f7d973a56fd7 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -366,7 +366,7 @@ void elv_dispatch_sort(struct request_queue *q, struct request *rq)
>         list_for_each_prev(entry, &q->queue_head) {
>                 struct request *pos = list_entry_rq(entry);
>
> -               if ((req_op(rq) == REQ_OP_DISCARD) != (req_op(pos) == REQ_OP_DISCARD))
> +               if (req_op(rq) != req_op(pos))
>                         break;
>                 if (rq_data_dir(rq) != rq_data_dir(pos))
>                         break;
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 48a5dd740f3b..82503e6f04b3 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -1726,6 +1726,7 @@ static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request *req)
>                         break;
>
>                 if (req_op(next) == REQ_OP_DISCARD ||
> +                   req_op(next) == REQ_OP_SECURE_ERASE ||
>                     req_op(next) == REQ_OP_FLUSH)
>                         break;
>
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index bf14642a576a..29578e98603d 100644
> --- a/drivers/mmc/card/queue.c
> +++ b/drivers/mmc/card/queue.c
> @@ -33,7 +33,8 @@ static int mmc_prep_request(struct request_queue *q, struct request *req)
>         /*
>          * We only like normal block requests and discards.
>          */
> -       if (req->cmd_type != REQ_TYPE_FS && req_op(req) != REQ_OP_DISCARD) {
> +       if (req->cmd_type != REQ_TYPE_FS && req_op(req) != REQ_OP_DISCARD &&
> +           req_op(req) != REQ_OP_SECURE_ERASE) {
>                 blk_dump_rq_flags(req, "MMC bad request");
>                 return BLKPREP_KILL;
>         }
> diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
> index d62531124d54..fee5e1271465 100644
> --- a/drivers/mmc/card/queue.h
> +++ b/drivers/mmc/card/queue.h
> @@ -4,7 +4,9 @@
>  static inline bool mmc_req_is_special(struct request *req)
>  {
>         return req &&
> -               (req_op(req) == REQ_OP_FLUSH || req_op(req) == REQ_OP_DISCARD);
> +               (req_op(req) == REQ_OP_FLUSH ||
> +                req_op(req) == REQ_OP_DISCARD ||
> +                req_op(req) == REQ_OP_SECURE_ERASE);
>  }
>
>  struct request;
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 59ffaa68b11b..23ddf4b46a9b 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -71,7 +71,8 @@ static inline bool bio_has_data(struct bio *bio)
>  {
>         if (bio &&
>             bio->bi_iter.bi_size &&
> -           bio_op(bio) != REQ_OP_DISCARD)
> +           bio_op(bio) != REQ_OP_DISCARD &&
> +           bio_op(bio) != REQ_OP_SECURE_ERASE)
>                 return true;
>
>         return false;
> @@ -79,7 +80,9 @@ static inline bool bio_has_data(struct bio *bio)
>
>  static inline bool bio_no_advance_iter(struct bio *bio)
>  {
> -       return bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_WRITE_SAME;
> +       return bio_op(bio) == REQ_OP_DISCARD ||
> +              bio_op(bio) == REQ_OP_SECURE_ERASE ||
> +              bio_op(bio) == REQ_OP_WRITE_SAME;
>  }
>
>  static inline bool bio_is_rw(struct bio *bio)
> @@ -199,6 +202,9 @@ static inline unsigned bio_segments(struct bio *bio)
>         if (bio_op(bio) == REQ_OP_DISCARD)
>                 return 1;
>
> +       if (bio_op(bio) == REQ_OP_SECURE_ERASE)
> +               return 1;
> +
>         if (bio_op(bio) == REQ_OP_WRITE_SAME)
>                 return 1;
>
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 2c210b6a7bcf..e79055c8b577 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -882,7 +882,7 @@ static inline unsigned int blk_rq_cur_sectors(const struct request *rq)
>  static inline unsigned int blk_queue_get_max_sectors(struct request_queue *q,
>                                                      int op)
>  {
> -       if (unlikely(op == REQ_OP_DISCARD))
> +       if (unlikely(op == REQ_OP_DISCARD || op == REQ_OP_SECURE_ERASE))
>                 return min(q->limits.max_discard_sectors, UINT_MAX >> 9);
>
>         if (unlikely(op == REQ_OP_WRITE_SAME))
> @@ -913,7 +913,9 @@ static inline unsigned int blk_rq_get_max_sectors(struct request *rq,
>         if (unlikely(rq->cmd_type != REQ_TYPE_FS))
>                 return q->limits.max_hw_sectors;
>
> -       if (!q->limits.chunk_sectors || (req_op(rq) == REQ_OP_DISCARD))
> +       if (!q->limits.chunk_sectors ||
> +           req_op(rq) == REQ_OP_DISCARD ||
> +           req_op(rq) == REQ_OP_SECURE_ERASE)
>                 return blk_queue_get_max_sectors(q, req_op(rq));
>
>         return min(blk_max_size_offset(q, offset),
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 7598e6ca817a..dbafc5df03f3 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -223,7 +223,7 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
>         what |= MASK_TC_BIT(op_flags, META);
>         what |= MASK_TC_BIT(op_flags, PREFLUSH);
>         what |= MASK_TC_BIT(op_flags, FUA);
> -       if (op == REQ_OP_DISCARD)
> +       if (op == REQ_OP_DISCARD || op == REQ_OP_SECURE_ERASE)
>                 what |= BLK_TC_ACT(BLK_TC_DISCARD);
>         if (op == REQ_OP_FLUSH)
>                 what |= BLK_TC_ACT(BLK_TC_FLUSH);
> --
> 1.9.1
>

Re: [PATCH V2] block: Fix secure erase

[Jens Axboe]

On 08/19/2016 05:52 AM, Ulf Hansson wrote:
> On 16 August 2016 at 09:59, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Commit 288dab8a35a0 ("block: add a separate operation type for secure
>> erase") split REQ_OP_SECURE_ERASE from REQ_OP_DISCARD without considering
>> all the places REQ_OP_DISCARD was being used to mean either. Fix those.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> Fixes: 288dab8a35a0 ("block: add a separate operation type for secure erase")
>
> For the mmc parts:
>
> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Jens, will you pick this up via your tree?

Already did, was applied 3 days ago.

-- 
Jens Axboe