FAILED: patch "[PATCH] target: Fix WRITE_SAME/DISCARD conversion to linux 512b" failed to apply to 4.4-stable tree

PATCH [email protected] 3 participants 4 messages Mar 1, 2016 – Mar 5, 2016

Subsystems (2)

Stable branch SCSI target

lore

Share & tools

lore

[email protected]

author maintainer

2016-03-01 21:52 UTC #0 lore raw

The patch below does not apply to the 4.4-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 8a9ebe717a133ba7bc90b06047f43cc6b8bcb8b3 Mon Sep 17 00:00:00 2001

From: Mike Christie <[email protected]>
Date: Mon, 18 Jan 2016 14:09:27 -0600
Subject: [PATCH] target: Fix WRITE_SAME/DISCARD conversion to linux 512b
sectors

In a couple places we are not converting to/from the Linux block layer 512 bytes sectors.

The request queue values and what we do are a mismatch of things:

max_discard_sectors - This is in linux block layer 512 byte sectors. We are just copying this to max_unmap_lba_count.

discard_granularity - This is in bytes. We are converting it to Linux block layer 512 byte sectors.

discard_alignment - This is in bytes. We are just copying this over.

The problem is that the core LIO code exports these values in spc_emulate_evpd_b0 and we use them to test request arguments in sbc_execute_unmap, but we never convert to the block size we export to the initiator. If we are not using 512 byte sectors then we are exporting the wrong values or are checks are off. And, for the discard_alignment/bytes case we are just plain messed up.

blkdev_issue_discard's start and number of sector arguments are supposed to be in linux block layer 512 byte sectors. We are currently passing in the values we get from the initiator which might be based on some other sector size.

There is a similar problem in iblock_execute_write_same where the bio functions want values in 512 byte sectors but we are passing in what we got from the initiator.

Signed-off-by Mike Christie <[email protected]>

Cc [email protected] # 3.10+

Signed-off-by Nicholas Bellinger <[email protected]>

drivers/target/target_core_device.c +44 -0

index cacd97a8cbd0..da457e25717a 100644
@@ -828,6 +828,50 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
 	return dev;
 }
 
+/*
+ * Check if the underlying struct block_device request_queue supports
+ * the QUEUE_FLAG_DISCARD bit for UNMAP/WRITE_SAME in SCSI + TRIM
+ * in ATA and we need to set TPE=1
+ */
+bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
+				       struct request_queue *q, int block_size)
+{
+	if (!blk_queue_discard(q))
+		return false;
+
+	attrib->max_unmap_lba_count = (q->limits.max_discard_sectors << 9) /
+								block_size;
+	/*
+	 * Currently hardcoded to 1 in Linux/SCSI code..
+	 */
+	attrib->max_unmap_block_desc_count = 1;
+	attrib->unmap_granularity = q->limits.discard_granularity / block_size;
+	attrib->unmap_granularity_alignment = q->limits.discard_alignment /
+								block_size;
+	attrib->unmap_zeroes_data = q->limits.discard_zeroes_data;
+	return true;
+}
+EXPORT_SYMBOL(target_configure_unmap_from_queue);
+
+/*
+ * Convert from blocksize advertised to the initiator to the 512 byte
+ * units unconditionally used by the Linux block layer.
+ */
+sector_t target_to_linux_sector(struct se_device *dev, sector_t lb)
+{
+	switch (dev->dev_attrib.block_size) {
+	case 4096:
+		return lb << 3;
+	case 2048:
+		return lb << 2;
+	case 1024:
+		return lb << 1;
+	default:
+		return lb;
+	}
+}
+EXPORT_SYMBOL(target_to_linux_sector);
+
 int target_configure_device(struct se_device *dev)
 {
 	struct se_hba *hba = dev->se_hba;

drivers/target/target_core_file.c +9 ~~-20~~

index e3195700211a..75f0f08b2a34 100644
@@ -160,25 +160,11 @@ static int fd_configure_device(struct se_device *dev)
 			" block_device blocks: %llu logical_block_size: %d\n",
 			dev_size, div_u64(dev_size, fd_dev->fd_block_size),
 			fd_dev->fd_block_size);
-		/*
-		 * Check if the underlying struct block_device request_queue supports
-		 * the QUEUE_FLAG_DISCARD bit for UNMAP/WRITE_SAME in SCSI + TRIM
-		 * in ATA and we need to set TPE=1
-		 */
-		if (blk_queue_discard(q)) {
-			dev->dev_attrib.max_unmap_lba_count =
-				q->limits.max_discard_sectors;
-			/*
-			 * Currently hardcoded to 1 in Linux/SCSI code..
-			 */
-			dev->dev_attrib.max_unmap_block_desc_count = 1;
-			dev->dev_attrib.unmap_granularity =
-				q->limits.discard_granularity >> 9;
-			dev->dev_attrib.unmap_granularity_alignment =
-				q->limits.discard_alignment;
+
+		if (target_configure_unmap_from_queue(&dev->dev_attrib, q,
+						      fd_dev->fd_block_size))
 			pr_debug("IFILE: BLOCK Discard support available,"
-					" disabled by default\n");
-		}
+				 " disabled by default\n");
 		/*
 		 * Enable write same emulation for IBLOCK and use 0xFFFF as
 		 * the smaller WRITE_SAME(10) only has a two-byte block count.

@@ -490,9 +476,12 @@ fd_execute_unmap(struct se_cmd *cmd, sector_t lba, sector_t nolb)
 	if (S_ISBLK(inode->i_mode)) {
 		/* The backend is block device, use discard */
 		struct block_device *bdev = inode->i_bdev;
+		struct se_device *dev = cmd->se_dev;
 
-		ret = blkdev_issue_discard(bdev, lba,
-				nolb, GFP_KERNEL, 0);
+		ret = blkdev_issue_discard(bdev,
+					   target_to_linux_sector(dev, lba),
+					   target_to_linux_sector(dev,  nolb),
+					   GFP_KERNEL, 0);
 		if (ret < 0) {
 			pr_warn("FILEIO: blkdev_issue_discard() failed: %d\n",
 				ret);

drivers/target/target_core_iblock.c +14 ~~-44~~

96 lines not on this page · raw message

include/target/target_core_backend.h +3 -0

index 56cf8e485ef2..28ee5c2e6bcd 100644
@@ -94,5 +94,8 @@ sense_reason_t passthrough_parse_cdb(struct se_cmd *cmd,
 	sense_reason_t (*exec_cmd)(struct se_cmd *cmd));
 
 bool target_sense_desc_format(struct se_device *dev);
+sector_t target_to_linux_sector(struct se_device *dev, sector_t lb);
+bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
+				       struct request_queue *q, int block_size);
 
 #endif /* TARGET_CORE_BACKEND_H */

Kamal Mostafa

2016-03-04 20:45 UTC #1 lore raw

[4.4-stable backport] target: Fix WRITE_SAME/DISCARD conversion to linux 512b sectors

From: Mike Christie <[email protected]>

commit 8a9ebe717a133ba7bc90b06047f43cc6b8bcb8b3 upstream.

In a couple places we are not converting to/from the Linux block layer 512 bytes sectors.

The request queue values and what we do are a mismatch of things:

max_discard_sectors - This is in linux block layer 512 byte sectors. We are just copying this to max_unmap_lba_count.

discard_granularity - This is in bytes. We are converting it to Linux block layer 512 byte sectors.

discard_alignment - This is in bytes. We are just copying this over.

There is a similar problem in iblock_execute_write_same where the bio functions want values in 512 byte sectors but we are passing in what we got from the initiator.

Signed-off-by Mike Christie <[email protected]>

Cc [email protected] # 3.10+

Signed-off-by Nicholas Bellinger <[email protected]>

[ kamal: backport to 4.4-stable: no unmap_zeroes_data ]

Signed-off-by Kamal Mostafa <[email protected]>

---
 drivers/target/target_core_device.c  | 43 +++++++++++++++++++++++++++
 drivers/target/target_core_file.c    | 29 ++++++-------------
 drivers/target/target_core_iblock.c  | 56 +++++++++---------------------------
 include/target/target_core_backend.h |  3 ++
 4 files changed, 69 insertions(+), 62 deletions(-)

drivers/target/target_core_device.c +43 -0

index 88ea4e4..3436a83 100644
@@ -826,6 +826,49 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
 	return dev;
 }
 
+/*
+ * Check if the underlying struct block_device request_queue supports
+ * the QUEUE_FLAG_DISCARD bit for UNMAP/WRITE_SAME in SCSI + TRIM
+ * in ATA and we need to set TPE=1
+ */
+bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
+				       struct request_queue *q, int block_size)
+{
+	if (!blk_queue_discard(q))
+		return false;
+
+	attrib->max_unmap_lba_count = (q->limits.max_discard_sectors << 9) /
+								block_size;
+	/*
+	 * Currently hardcoded to 1 in Linux/SCSI code..
+	 */
+	attrib->max_unmap_block_desc_count = 1;
+	attrib->unmap_granularity = q->limits.discard_granularity / block_size;
+	attrib->unmap_granularity_alignment = q->limits.discard_alignment /
+								block_size;
+	return true;
+}
+EXPORT_SYMBOL(target_configure_unmap_from_queue);
+
+/*
+ * Convert from blocksize advertised to the initiator to the 512 byte
+ * units unconditionally used by the Linux block layer.
+ */
+sector_t target_to_linux_sector(struct se_device *dev, sector_t lb)
+{
+	switch (dev->dev_attrib.block_size) {
+	case 4096:
+		return lb << 3;
+	case 2048:
+		return lb << 2;
+	case 1024:
+		return lb << 1;
+	default:
+		return lb;
+	}
+}
+EXPORT_SYMBOL(target_to_linux_sector);
+
 int target_configure_device(struct se_device *dev)
 {
 	struct se_hba *hba = dev->se_hba;

drivers/target/target_core_file.c +9 ~~-20~~

index e319570..75f0f08 100644
@@ -160,25 +160,11 @@ static int fd_configure_device(struct se_device *dev)
 			" block_device blocks: %llu logical_block_size: %d\n",
 			dev_size, div_u64(dev_size, fd_dev->fd_block_size),
 			fd_dev->fd_block_size);
-		/*
-		 * Check if the underlying struct block_device request_queue supports
-		 * the QUEUE_FLAG_DISCARD bit for UNMAP/WRITE_SAME in SCSI + TRIM
-		 * in ATA and we need to set TPE=1
-		 */
-		if (blk_queue_discard(q)) {
-			dev->dev_attrib.max_unmap_lba_count =
-				q->limits.max_discard_sectors;
-			/*
-			 * Currently hardcoded to 1 in Linux/SCSI code..
-			 */
-			dev->dev_attrib.max_unmap_block_desc_count = 1;
-			dev->dev_attrib.unmap_granularity =
-				q->limits.discard_granularity >> 9;
-			dev->dev_attrib.unmap_granularity_alignment =
-				q->limits.discard_alignment;
+
+		if (target_configure_unmap_from_queue(&dev->dev_attrib, q,
+						      fd_dev->fd_block_size))
 			pr_debug("IFILE: BLOCK Discard support available,"
-					" disabled by default\n");
-		}
+				 " disabled by default\n");
 		/*
 		 * Enable write same emulation for IBLOCK and use 0xFFFF as
 		 * the smaller WRITE_SAME(10) only has a two-byte block count.

@@ -490,9 +476,12 @@ fd_execute_unmap(struct se_cmd *cmd, sector_t lba, sector_t nolb)
 	if (S_ISBLK(inode->i_mode)) {
 		/* The backend is block device, use discard */
 		struct block_device *bdev = inode->i_bdev;
+		struct se_device *dev = cmd->se_dev;
 
-		ret = blkdev_issue_discard(bdev, lba,
-				nolb, GFP_KERNEL, 0);
+		ret = blkdev_issue_discard(bdev,
+					   target_to_linux_sector(dev, lba),
+					   target_to_linux_sector(dev,  nolb),
+					   GFP_KERNEL, 0);
 		if (ret < 0) {
 			pr_warn("FILEIO: blkdev_issue_discard() failed: %d\n",
 				ret);

drivers/target/target_core_iblock.c +14 ~~-42~~

94 lines not on this page · raw message

include/target/target_core_backend.h +3 -0

index 56cf8e4..28ee5c2 100644
@@ -94,5 +94,8 @@ sense_reason_t passthrough_parse_cdb(struct se_cmd *cmd,
 	sense_reason_t (*exec_cmd)(struct se_cmd *cmd));
 
 bool target_sense_desc_format(struct se_device *dev);
+sector_t target_to_linux_sector(struct se_device *dev, sector_t lb);
+bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
+				       struct request_queue *q, int block_size);
 
 #endif /* TARGET_CORE_BACKEND_H */

Signature · 1 line

2.7.0

Greg KH

author maintainer

2016-03-05 18:51 UTC #2 lore raw

Re: [4.4-stable backport] target: Fix WRITE_SAME/DISCARD conversion to linux 512b sectors

Quoted 40 lines · Kamal Mostafa

From: Mike Christie <[email protected]>
commit 8a9ebe717a133ba7bc90b06047f43cc6b8bcb8b3 upstream.
In a couple places we are not converting to/from the Linux block layer 512 bytes sectors.
1.
The request queue values and what we do are a mismatch of things:
max_discard_sectors - This is in linux block layer 512 byte sectors. We are just copying this to max_unmap_lba_count.
discard_granularity - This is in bytes. We are converting it to Linux block layer 512 byte sectors.
discard_alignment - This is in bytes. We are just copying this over.
The problem is that the core LIO code exports these values in spc_emulate_evpd_b0 and we use them to test request arguments in sbc_execute_unmap, but we never convert to the block size we export to the initiator. If we are not using 512 byte sectors then we are exporting the wrong values or are checks are off. And, for the discard_alignment/bytes case we are just plain messed up.
2.
blkdev_issue_discard's start and number of sector arguments are supposed to be in linux block layer 512 byte sectors. We are currently passing in the values we get from the initiator which might be based on some other sector size.
There is a similar problem in iblock_execute_write_same where the bio functions want values in 512 byte sectors but we are passing in what we got from the initiator.

Quoted 5 lines

Signed-off-by Mike Christie <[email protected]>
Cc [email protected] # 3.10+
Signed-off-by Nicholas Bellinger <[email protected]>
[ kamal: backport to 4.4-stable: no unmap_zeroes_data ]
Signed-off-by Kamal Mostafa <[email protected]>

Thanks, but what about a 3.14 and 3.10-stable backport as well?

greg k-h

Nicholas A. Bellinger

2016-03-05 21:06 UTC #3 lore raw

Re: [4.4-stable backport] target: Fix WRITE_SAME/DISCARD conversion to linux 512b sectors

Quoted 41 lines · Greg KH

Quoted 40 lines · Kamal Mostafa
From: Mike Christie <[email protected]>
commit 8a9ebe717a133ba7bc90b06047f43cc6b8bcb8b3 upstream.
In a couple places we are not converting to/from the Linux block layer 512 bytes sectors.
1.
The request queue values and what we do are a mismatch of things:
max_discard_sectors - This is in linux block layer 512 byte sectors. We are just copying this to max_unmap_lba_count.
discard_granularity - This is in bytes. We are converting it to Linux block layer 512 byte sectors.
discard_alignment - This is in bytes. We are just copying this over.
The problem is that the core LIO code exports these values in spc_emulate_evpd_b0 and we use them to test request arguments in sbc_execute_unmap, but we never convert to the block size we export to the initiator. If we are not using 512 byte sectors then we are exporting the wrong values or are checks are off. And, for the discard_alignment/bytes case we are just plain messed up.
2.
blkdev_issue_discard's start and number of sector arguments are supposed to be in linux block layer 512 byte sectors. We are currently passing in the values we get from the initiator which might be based on some other sector size.
There is a similar problem in iblock_execute_write_same where the bio functions want values in 512 byte sectors but we are passing in what we got from the initiator.

Quoted 8 lines

Quoted 5 lines
Signed-off-by Mike Christie <[email protected]>
Cc [email protected] # 3.10+
Signed-off-by Nicholas Bellinger <[email protected]>
[ kamal: backport to 4.4-stable: no unmap_zeroes_data ]
Signed-off-by Kamal Mostafa <[email protected]>
Thanks, but what about a 3.14 and 3.10-stable backport as well?

Hi Greg-KH,

I'm working on <= v4.1 stable backports today, and will include Mike's patch to the series.

Thank you,

--nab