FAILED: patch "[PATCH] block: don't deal with discard limit in" failed to apply to 4.14-stable tree

PATCH [email protected] 3 participants 5 messages Nov 9, 2018 – Jan 10, 2019

Subsystems (2)

Stable branch Block layer

lore

Share & tools

lore

[email protected]

author maintainer

2018-11-09 02:59 UTC #0 lore raw

The patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

Quoted 1 line

From 744889b7cbb56a64f957e65ade7cb65fe3f35714 Mon Sep 17 00:00:00 2001

From: Ming Lei <[email protected]>
Date: Fri, 12 Oct 2018 15:53:10 +0800
Subject: [PATCH] block: don't deal with discard limit in
blkdev_issue_discard()

blk_queue_split() does respect this limit via bio splitting, so no need to do that in blkdev_issue_discard(), then we can align to normal bio submit(bio_add_page() & submit_bio()).

More importantly, this patch fixes one issue introduced in a22c4d7e34402cc ("block: re-add discard_granularity and alignment checks"), in which zero discard bio may be generated in case of zero alignment.

Fixes a22c4d7e34402ccdf3 ("block: re-add discard_granularity and alignment checks")

Cc [email protected]

Cc Ming Lin <[email protected]>

Cc Mike Snitzer <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Xiao Ni <[email protected]>

Tested-by Mariusz Dabrowski <[email protected]>

Signed-off-by Ming Lei <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

block/blk-lib.c +2 ~~-26~~

index d1b9dd03da25..bbd44666f2b5 100644
@@ -29,9 +29,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 {
 	struct request_queue *q = bdev_get_queue(bdev);
 	struct bio *bio = *biop;
-	unsigned int granularity;
 	unsigned int op;
-	int alignment;
 	sector_t bs_mask;
 
 	if (!q)

@@ -54,38 +52,16 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	if ((sector | nr_sects) & bs_mask)
 		return -EINVAL;
 
-	/* Zero-sector (unknown) and one-sector granularities are the same.  */
-	granularity = max(q->limits.discard_granularity >> 9, 1U);
-	alignment = (bdev_discard_alignment(bdev) >> 9) % granularity;
-
 	while (nr_sects) {
-		unsigned int req_sects;
-		sector_t end_sect, tmp;
+		unsigned int req_sects = nr_sects;
+		sector_t end_sect;
 
-		/*
-		 * Issue in chunks of the user defined max discard setting,
-		 * ensuring that bi_size doesn't overflow
-		 */
-		req_sects = min_t(sector_t, nr_sects,
-					q->limits.max_discard_sectors);
 		if (!req_sects)
 			goto fail;
 		if (req_sects > UINT_MAX >> 9)
 			req_sects = UINT_MAX >> 9;
 
-		/*
-		 * If splitting a request, and the next starting sector would be
-		 * misaligned, stop the discard at the previous aligned sector.
-		 */
 		end_sect = sector + req_sects;
-		tmp = end_sect;
-		if (req_sects < nr_sects &&
-		    sector_div(tmp, granularity) != alignment) {
-			end_sect = end_sect - alignment;
-			sector_div(end_sect, granularity);
-			end_sect = end_sect * granularity + alignment;
-			req_sects = end_sect - sector;
-		}
 
 		bio = next_bio(bio, 0, gfp_mask);
 		bio->bi_iter.bi_sector = sector;

Sudip Mukherjee

Reviewed-by Darrick J. Wong Reviewed-by Omar Sandoval Tested-by Zdenek Kabelac Tested-by Mariusz Dabrowski

2018-12-20 20:53 UTC #1 lore raw

Hi Greg,

Quoted 5 lines · On Thu, Nov 08, 2018 at 09:23:10AM -0800, [email protected] wrote:

The patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

Instead of backporting it was easier to add the commits it depends on.
1) af097f5d199e ("block: break discard submissions into the user defined size") It is not marked for stable but seems it can be in stable.

2) b88aef36b87c ("block: fix infinite loop if the device loses discard capability") It is marked for stable but is not there in 4.14-stable.

All are attached to this mail for you to apply.

--
Regards
Sudip

Quoted 1 line

From 1be4fbd23b620d384219ba0a1b2ed14a09ac1c01 Mon Sep 17 00:00:00 2001

From: Jens Axboe <[email protected]>
Date: Tue, 8 May 2018 15:09:41 -0600
Subject: [PATCH 1/3] block: break discard submissions into the user defined
size

commit af097f5d199e2aa3ab3ef777f0716e487b8f7b08 upstream

Don't build discards bigger than what the user asked for, if the user decided to limit the size by writing to 'discard_max_bytes'.

Reviewed-by Darrick J. Wong <[email protected]>

Reviewed-by Omar Sandoval <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

Signed-off-by Sudip Mukherjee <[email protected]>

---
 block/blk-lib.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

block/blk-lib.c +9 -3

index 2bc544ce3d2e..53a45663e688 100644
@@ -59,10 +59,16 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		unsigned int req_sects;
 		sector_t end_sect, tmp;
 
-		/* Make sure bi_size doesn't overflow */
-		req_sects = min_t(sector_t, nr_sects, UINT_MAX >> 9);
+		/*
+		 * Issue in chunks of the user defined max discard setting,
+		 * ensuring that bi_size doesn't overflow
+		 */
+		req_sects = min_t(sector_t, nr_sects,
+					q->limits.max_discard_sectors);
+		if (req_sects > UINT_MAX >> 9)
+			req_sects = UINT_MAX >> 9;
 
-		/**
+		/*
 		 * If splitting a request, and the next starting sector would be
 		 * misaligned, stop the discard at the previous aligned sector.
 		 */

Signature · 1 line

2.11.0

Quoted 1 line

From d4a1fe129ccf82570d7add250b77a9b9ee878561 Mon Sep 17 00:00:00 2001

From: Mikulas Patocka <[email protected]>
Date: Tue, 3 Jul 2018 13:34:22 -0400
Subject: [PATCH 2/3] block: fix infinite loop if the device loses discard
capability

commit b88aef36b87c9787a4db724923ec4f57dfd513f3 upstream

If __blkdev_issue_discard is in progress and a device mapper device is reloaded with a table that doesn't support discard, q->limits.max_discard_sectors is set to zero. This results in infinite loop in __blkdev_issue_discard.

This patch checks if max_discard_sectors is zero and aborts with -EOPNOTSUPP.

Signed-off-by Mikulas Patocka <[email protected]>

Tested-by Zdenek Kabelac <[email protected]>

Cc [email protected]

Signed-off-by Jens Axboe <[email protected]>

Signed-off-by Sudip Mukherjee <[email protected]>

---
 block/blk-lib.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

block/blk-lib.c +10 -0

index 53a45663e688..0bdc77888dc5 100644
@@ -65,6 +65,8 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		 */
 		req_sects = min_t(sector_t, nr_sects,
 					q->limits.max_discard_sectors);
+		if (!req_sects)
+			goto fail;
 		if (req_sects > UINT_MAX >> 9)
 			req_sects = UINT_MAX >> 9;

@@ -102,6 +104,14 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 
 	*biop = bio;
 	return 0;
+
+fail:
+	if (bio) {
+		submit_bio_wait(bio);
+		bio_put(bio);
+	}
+	*biop = NULL;
+	return -EOPNOTSUPP;
 }
 EXPORT_SYMBOL(__blkdev_issue_discard);

Signature · 1 line

2.11.0

Quoted 1 line

From 4fe24d18d61cdbf07db6969e506c1751c0fdfc42 Mon Sep 17 00:00:00 2001

From: Ming Lei <[email protected]>
Date: Fri, 12 Oct 2018 15:53:10 +0800
Subject: [PATCH 3/3] block: don't deal with discard limit in
blkdev_issue_discard()

commit 744889b7cbb56a64f957e65ade7cb65fe3f35714 upstream

blk_queue_split() does respect this limit via bio splitting, so no need to do that in blkdev_issue_discard(), then we can align to normal bio submit(bio_add_page() & submit_bio()).

Fixes a22c4d7e34402ccdf3 ("block: re-add discard_granularity and alignment checks")

Cc [email protected]

Cc Ming Lin <[email protected]>

Cc Mike Snitzer <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Xiao Ni <[email protected]>

Tested-by Mariusz Dabrowski <[email protected]>

Signed-off-by Ming Lei <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

Signed-off-by Sudip Mukherjee <[email protected]>

---
 block/blk-lib.c | 28 ++--------------------------
 1 file changed, 2 insertions(+), 26 deletions(-)

block/blk-lib.c +2 ~~-26~~

index 0bdc77888dc5..3e334b4575ab 100644
@@ -29,9 +29,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 {
 	struct request_queue *q = bdev_get_queue(bdev);
 	struct bio *bio = *biop;
-	unsigned int granularity;
 	unsigned int op;
-	int alignment;
 	sector_t bs_mask;
 
 	if (!q)

@@ -51,38 +49,16 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	if ((sector | nr_sects) & bs_mask)
 		return -EINVAL;
 
-	/* Zero-sector (unknown) and one-sector granularities are the same.  */
-	granularity = max(q->limits.discard_granularity >> 9, 1U);
-	alignment = (bdev_discard_alignment(bdev) >> 9) % granularity;
-
 	while (nr_sects) {
-		unsigned int req_sects;
-		sector_t end_sect, tmp;
+		unsigned int req_sects = nr_sects;
+		sector_t end_sect;
 
-		/*
-		 * Issue in chunks of the user defined max discard setting,
-		 * ensuring that bi_size doesn't overflow
-		 */
-		req_sects = min_t(sector_t, nr_sects,
-					q->limits.max_discard_sectors);
 		if (!req_sects)
 			goto fail;
 		if (req_sects > UINT_MAX >> 9)
 			req_sects = UINT_MAX >> 9;
 
-		/*
-		 * If splitting a request, and the next starting sector would be
-		 * misaligned, stop the discard at the previous aligned sector.
-		 */
 		end_sect = sector + req_sects;
-		tmp = end_sect;
-		if (req_sects < nr_sects &&
-		    sector_div(tmp, granularity) != alignment) {
-			end_sect = end_sect - alignment;
-			sector_div(end_sect, granularity);
-			end_sect = end_sect * granularity + alignment;
-			req_sects = end_sect - sector;
-		}
 
 		bio = next_bio(bio, 0, gfp_mask);
 		bio->bi_iter.bi_sector = sector;

Signature · 1 line

2.11.0

Sasha Levin

maintainer

2018-12-21 23:20 UTC #2 lore raw

Quoted 9 lines · Sudip Mukherjee

Hi Greg,
Quoted 5 lines · On Thu, Nov 08, 2018 at 09:23:10AM -0800, [email protected] wrote:
The patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

Quoted 8 lines

Instead of backporting it was easier to add the commits it depends on.
1) af097f5d199e ("block: break discard submissions into the user defined size") It is not marked for stable but seems it can be in stable.
2) b88aef36b87c ("block: fix infinite loop if the device loses discard capability") It is marked for stable but is not there in 4.14-stable.
All are attached to this mail for you to apply.

I've queued both for 4.14 and 4.9. It seems that 4.4 needs something more complex than that.

--
Thanks,
Sasha

Sudip Mukherjee

Reviewed-by Darrick J. Wong Reviewed-by Omar Sandoval Reviewed-by Christoph Hellwig Tested-by Christoph Hellwig Reviewed-by Mike Snitzer

2018-12-22 22:40 UTC #3 lore raw

Quoted 14 lines · Sasha Levin

Quoted 13 lines · Sudip Mukherjee
Hi Greg,
Quoted 5 lines · On Thu, Nov 08, 2018 at 09:23:10AM -0800, [email protected] wrote:
The patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
Instead of backporting it was easier to add the commits it depends on.
1) af097f5d199e ("block: break discard submissions into the user defined size") It is not marked for stable but seems it can be in stable.

Quoted 7 lines

Quoted 4 lines
2) b88aef36b87c ("block: fix infinite loop if the device loses discard capability") It is marked for stable but is not there in 4.14-stable.
All are attached to this mail for you to apply.
I've queued both for 4.14 and 4.9. It seems that 4.4 needs something more complex than that.

Backported patch attached for 4.4-stable tree.

--
Regards
Sudip

Quoted 1 line

From 2377ab657d423978108dfdaacd443adfa2613d2b Mon Sep 17 00:00:00 2001

From: Jens Axboe <[email protected]>
Date: Tue, 8 May 2018 15:09:41 -0600
Subject: [PATCH 1/2] block: break discard submissions into the user defined size

commit af097f5d199e2aa3ab3ef777f0716e487b8f7b08 upstream

Don't build discards bigger than what the user asked for, if the user decided to limit the size by writing to 'discard_max_bytes'.

Reviewed-by Darrick J. Wong <[email protected]>

Reviewed-by Omar Sandoval <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

Signed-off-by Sudip Mukherjee <[email protected]>

---
 block/blk-lib.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

block/blk-lib.c +8 -2

index 9ebf65379556..73ca3ef8ca5a 100644
@@ -81,8 +81,14 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 			break;
 		}
 
-		/* Make sure bi_size doesn't overflow */
-		req_sects = min_t(sector_t, nr_sects, UINT_MAX >> 9);
+		/*
+		 * Issue in chunks of the user defined max discard setting,
+		 * ensuring that bi_size doesn't overflow
+		 */
+		req_sects = min_t(sector_t, nr_sects,
+					q->limits.max_discard_sectors);
+		if (req_sects > UINT_MAX >> 9)
+			req_sects = UINT_MAX >> 9;
 
 		/*
 		 * If splitting a request, and the next starting sector would be

Signature · 1 line

2.11.0

Quoted 1 line

From 2e507841428638b7fac3cea954e675b780a861a3 Mon Sep 17 00:00:00 2001

From: Ming Lin <[email protected]>
Date: Thu, 22 Oct 2015 09:59:42 -0700
Subject: [PATCH 2/2] block: re-add discard_granularity and alignment checks

commit 744889b7cbb56a64f957e65ade7cb65fe3f35714 upstream

In commit b49a087("block: remove split code in blkdev_issue_{discard,write_same}"), discard_granularity and alignment checks were removed. Ideally, with bio late splitting, the upper layers shouldn't need to depend on device's limits.

Christoph reported a discard regression on the HGST Ultrastar SN100 NVMe device when mkfs.xfs. We have not found the root cause yet.

This patch re-adds discard_granularity and alignment checks by reverting the related changes in commit b49a087. The good thing is now we can remove the 2G discard size cap and just use UINT_MAX to avoid bi_size overflow.

Reviewed-by Christoph Hellwig <[email protected]>

Tested-by Christoph Hellwig <[email protected]>

Signed-off-by Ming Lin <[email protected]>

Reviewed-by Mike Snitzer <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

Signed-off-by Sudip Mukherjee <[email protected]>

---
 block/blk-lib.c | 28 ++--------------------------
 1 file changed, 2 insertions(+), 26 deletions(-)

block/blk-lib.c +2 ~~-26~~

index 73ca3ef8ca5a..a42e680ef8f7 100644
@@ -43,8 +43,6 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	DECLARE_COMPLETION_ONSTACK(wait);
 	struct request_queue *q = bdev_get_queue(bdev);
 	int type = REQ_WRITE | REQ_DISCARD;
-	unsigned int granularity;
-	int alignment;
 	struct bio_batch bb;
 	struct bio *bio;
 	int ret = 0;

@@ -56,10 +54,6 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	if (!blk_queue_discard(q))
 		return -EOPNOTSUPP;
 
-	/* Zero-sector (unknown) and one-sector granularities are the same.  */
-	granularity = max(q->limits.discard_granularity >> 9, 1U);
-	alignment = (bdev_discard_alignment(bdev) >> 9) % granularity;
-
 	if (flags & BLKDEV_DISCARD_SECURE) {
 		if (!blk_queue_secdiscard(q))
 			return -EOPNOTSUPP;

@@ -72,8 +66,8 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 
 	blk_start_plug(&plug);
 	while (nr_sects) {
-		unsigned int req_sects;
-		sector_t end_sect, tmp;
+		unsigned int req_sects = nr_sects;
+		sector_t end_sect;
 
 		bio = bio_alloc(gfp_mask, 1);
 		if (!bio) {

@@ -81,28 +75,10 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 			break;
 		}
 
-		/*
-		 * Issue in chunks of the user defined max discard setting,
-		 * ensuring that bi_size doesn't overflow
-		 */
-		req_sects = min_t(sector_t, nr_sects,
-					q->limits.max_discard_sectors);
 		if (req_sects > UINT_MAX >> 9)
 			req_sects = UINT_MAX >> 9;
 
-		/*
-		 * If splitting a request, and the next starting sector would be
-		 * misaligned, stop the discard at the previous aligned sector.
-		 */
 		end_sect = sector + req_sects;
-		tmp = end_sect;
-		if (req_sects < nr_sects &&
-		    sector_div(tmp, granularity) != alignment) {
-			end_sect = end_sect - alignment;
-			sector_div(end_sect, granularity);
-			end_sect = end_sect * granularity + alignment;
-			req_sects = end_sect - sector;
-		}
 
 		bio->bi_iter.bi_sector = sector;
 		bio->bi_end_io = bio_batch_end_io;

Signature · 1 line

2.11.0

Greg KH

author maintainer

2019-01-10 19:21 UTC #4 lore raw

Quoted 18 lines · Sudip Mukherjee

Quoted 17 lines · Sasha Levin
Quoted 16 lines · Sudip Mukherjee
Hi Greg,
Quoted 5 lines · On Thu, Nov 08, 2018 at 09:23:10AM -0800, [email protected] wrote:
The patch below does not apply to the 4.14-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
Instead of backporting it was easier to add the commits it depends on.
1) af097f5d199e ("block: break discard submissions into the user defined size") It is not marked for stable but seems it can be in stable.
2) b88aef36b87c ("block: fix infinite loop if the device loses discard capability") It is marked for stable but is not there in 4.14-stable.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
All are attached to this mail for you to apply.
I've queued both for 4.14 and 4.9. It seems that 4.4 needs something more complex than that.
Backported patch attached for 4.4-stable tree.

Thanks for these, now queued up.

greg k-h