sd: Implement support for WRITE SAME

v3.7 Regression In review Joseph Salisbury 4 participants 26 messages Dec 14, 2012 – Jul 13, 2015 Device mapper

Share & tools

Joseph Salisbury

author

2012-12-14 20:31 UTC #0 lore raw

Hi Martin,

A bug was opened against the Ubuntu kernel[0]. After a kernel bisect, it was found that reverting the following commit resolved this bug:

commit 5db44863b6ebbb400c5e61d56ebe8f21ef48b1bd

Author: Martin K. Petersen <[email protected]>
Date:   Tue Sep 18 12:19:32 2012 -0400

[SCSI] sd: Implement support for WRITE SAME

The regression was introduced as of v3.7-rc7.

The bug can be reproduced with the following commands, which will operate on a virtual scsi_debug device, so they won't change any data on the test system. However, this will completely crash the system:

sudo modprobe scsi_debug
sudo luksformat -t ext4 /dev/sdb <- Or whatever device gets assigned after inserting scsi_debug. sudo cryptsetup luksOpen /dev/sdb treasure

Everything works fine up to here, but the following will cause the crash:

sudo mount /dev/mapper/treasure /mnt

The bug can be reproduced on bare metal, in a VM and on i386 or amd64.

I see that you are the author of this patch, so I wanted to run this by you. I was thinking of requesting a revert for v3.7, but I wanted to get your feedback first.

Thanks,

Joe

[0] https://bugs.launchpad.net/bugs/1089818

Mike Snitzer

2012-12-14 21:12 UTC #1 lore raw

Quoted 34 lines · Joseph Salisbury

Hi Martin,
A bug was opened against the Ubuntu kernel[0]. After a kernel bisect, it was found that reverting the following commit resolved this bug:
commit 5db44863b6ebbb400c5e61d56ebe8f21ef48b1bd
Author: Martin K. Petersen <[email protected]>
Date:   Tue Sep 18 12:19:32 2012 -0400
[SCSI] sd: Implement support for WRITE SAME
The regression was introduced as of v3.7-rc7.
The bug can be reproduced with the following commands, which will operate on a virtual scsi_debug device, so they won't change any data on the test system. However, this will completely crash the system:
sudo modprobe scsi_debug
sudo luksformat -t ext4 /dev/sdb <- Or whatever device gets assigned after inserting scsi_debug.
sudo cryptsetup luksOpen /dev/sdb treasure
Everything works fine up to here, but the following will cause the crash:
sudo mount /dev/mapper/treasure /mnt
The bug can be reproduced on bare metal, in a VM and on i386 or amd64.
I see that you are the author of this patch, so I wanted to run this by you. I was thinking of requesting a revert for v3.7, but I wanted to get your feedback first.

Quoted 6 lines

Thanks,
Joe
[0] https://bugs.launchpad.net/bugs/1089818

The WRITE SAME change was introduced long before v3.7-rc7. I think your bisect is somehow wrong.

Milan Broz recently pointed out issues he found with luks when using a late 3.7-rc (rc7 afaik). Linus fixed that issue with this commit (which landed in the final v3.7):
http://git.kernel.org/linus/684c9aaebbb0ea3a9954

That may not be _the_ problem though. But have you tried the final v3.7?

Mike

Joseph Salisbury

author

2012-12-15 02:38 UTC #2 lore raw

Quoted 37 lines · Mike Snitzer

Quoted 34 lines · Joseph Salisbury
Hi Martin,
A bug was opened against the Ubuntu kernel[0]. After a kernel bisect, it was found that reverting the following commit resolved this bug:
commit 5db44863b6ebbb400c5e61d56ebe8f21ef48b1bd
Author: Martin K. Petersen <[email protected]>
Date:   Tue Sep 18 12:19:32 2012 -0400
[SCSI] sd: Implement support for WRITE SAME
The regression was introduced as of v3.7-rc7.
The bug can be reproduced with the following commands, which will operate on a virtual scsi_debug device, so they won't change any data on the test system. However, this will completely crash the system:
sudo modprobe scsi_debug
sudo luksformat -t ext4 /dev/sdb <- Or whatever device gets assigned after inserting scsi_debug.
sudo cryptsetup luksOpen /dev/sdb treasure
Everything works fine up to here, but the following will cause the crash:
sudo mount /dev/mapper/treasure /mnt
The bug can be reproduced on bare metal, in a VM and on i386 or amd64.
I see that you are the author of this patch, so I wanted to run this by you. I was thinking of requesting a revert for v3.7, but I wanted to get your feedback first.

Quoted 8 lines

Quoted 6 lines
Thanks,
Joe
[0] https://bugs.launchpad.net/bugs/1089818
The WRITE SAME change was introduced long before v3.7-rc7. I think your bisect is somehow wrong.

From Linus' tree:
git describe --contains 5db4486
v3.7-rc7~19^2

Reverting commit 5db4486 solves the bug previously mentioned.

Quoted 8 lines

Milan Broz recently pointed out issues he found with luks when using a late 3.7-rc (rc7 afaik). Linus fixed that issue with this commit (which landed in the final v3.7):
http://git.kernel.org/linus/684c9aaebbb0ea3a9954
That may not be _the_ problem though. But have you tried the final v3.7?

Yes, v3.7 without reverting 5db4486 exhibits the bug.

Quoted 2 lines

Mike

I'll research further and provide additional data.

Martin K. Petersen

2012-12-14 22:36 UTC #3 lore raw

In reply to Joseph Salisbury

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Joseph" == Joseph Salisbury <[email protected]> writes:

Joseph> I see that you are the author of this patch, so I wanted to run Joseph> this by you. I was thinking of requesting a revert for v3.7, Joseph> but I wanted to get your feedback first.

I copied luksformat from a Debian box so I could try to reproduce on OL6. Everything works fine for me here. I set up a encrypted ext4 device, unpacked a kernel tarball, and did a build.

The oops screenshot in the launchpad bug report is pretty useless. Please provide a full backtrace so we can get a better idea what's going on.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Joseph Salisbury

author

2012-12-15 02:41 UTC #4 lore raw

Quoted 5 lines · Martin K. Petersen

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Joseph" == Joseph Salisbury <[email protected]> writes:
Joseph> I see that you are the author of this patch, so I wanted to run Joseph> this by you. I was thinking of requesting a revert for v3.7, Joseph> but I wanted to get your feedback first.

Quoted 8 lines

I copied luksformat from a Debian box so I could try to reproduce on OL6. Everything works fine for me here. I set up a encrypted ext4 device, unpacked a kernel tarball, and did a build.
The oops screenshot in the launchpad bug report is pretty useless. Please provide a full backtrace so we can get a better idea what's going on.

Thanks for the feedback, Martin.

I will do some additional research and testing. I'll be sure to capture a full backtrace for analysis.

Thanks again,

Joe

Joseph Salisbury

author

2012-12-18 19:53 UTC #5 lore raw

In reply to Martin K. Petersen

Quoted 5 lines · Martin K. Petersen

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Joseph" == Joseph Salisbury <[email protected]> writes:
Joseph> I see that you are the author of this patch, so I wanted to run Joseph> this by you. I was thinking of requesting a revert for v3.7, Joseph> but I wanted to get your feedback first.

Quoted 8 lines

I copied luksformat from a Debian box so I could try to reproduce on OL6. Everything works fine for me here. I set up a encrypted ext4 device, unpacked a kernel tarball, and did a build.
The oops screenshot in the launchpad bug report is pretty useless. Please provide a full backtrace so we can get a better idea what's going on.

Hi Martin,

I captured the netconsole output from boot until I reproduced the bug. The RIP points to kcryptd_crypt_write_io_submit() in ~/drivers/md/dm-crypt.c. The output can be seen at:

https://launchpadlibrarian.net/126102023/netconsole-lp1089818.log

Thanks again,

Joe

Martin K. Petersen

2012-12-19 16:58 UTC #6 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Joseph" == Joseph Salisbury <[email protected]> writes:

Joseph> I captured the netconsole output from boot until I reproduced Joseph> the bug. The RIP points to kcryptd_crypt_write_io_submit() in Joseph> ~/drivers/md/dm-crypt.c. The output can be seen at:

I'm thinking that dm-crypt should probably set max_write_same_sectors to
0. It doesn't really make much sense for a crypto driver to pass that command through.

Mike, do you want to look into this?

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Mike Snitzer

2012-12-19 19:58 UTC #7 lore raw

Quoted 11 lines · Martin K. Petersen

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Joseph" == Joseph Salisbury <[email protected]> writes:
Joseph> I captured the netconsole output from boot until I reproduced Joseph> the bug. The RIP points to kcryptd_crypt_write_io_submit() in Joseph> ~/drivers/md/dm-crypt.c. The output can be seen at:
I'm thinking that dm-crypt should probably set max_write_same_sectors to
0. It doesn't really make much sense for a crypto driver to pass that command through.
Mike, do you want to look into this?

Milan (cc'd) is more well-versed with dm-crypt. Though we likely need to audit DM relative to WRITE SAME too (will do once I understand what added safety/constraints are needed).

But do we know mount to somwhow be issuing WRITE SAME requests? Martin, why is it you weren't able to reproduce (I haven't attempted yet)?

Joseph Salisbury

author

2012-12-19 20:00 UTC #8 lore raw

Quoted 12 lines · Mike Snitzer

Quoted 9 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Joseph" == Joseph Salisbury <[email protected]> writes:
Joseph> I captured the netconsole output from boot until I reproduced Joseph> the bug. The RIP points to kcryptd_crypt_write_io_submit() in Joseph> ~/drivers/md/dm-crypt.c. The output can be seen at:
I'm thinking that dm-crypt should probably set max_write_same_sectors to
0. It doesn't really make much sense for a crypto driver to pass that command through.

Quoted 7 lines

Quoted 1 line
Mike, do you want to look into this?
Milan (cc'd) is more well-versed with dm-crypt. Though we likely need to audit DM relative to WRITE SAME too (will do once I understand what added safety/constraints are needed).
But do we know mount to somwhow be issuing WRITE SAME requests? Martin, why is it you weren't able to reproduce (I haven't attempted yet)?

Martin, were you using the scsi_debug module, or a real scsi device(s)?

Martin K. Petersen

2012-12-19 20:46 UTC #9 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Joseph" == Joseph Salisbury <[email protected]> writes:

Joseph> Martin, were you using the scsi_debug module, or a real scsi Joseph> device(s)?

I used scsi_debug for the test.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Martin K. Petersen

2012-12-19 20:45 UTC #10 lore raw

In reply to Mike Snitzer

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Mike" == Mike Snitzer <[email protected]> writes:

Mike> But do we know mount to somwhow be issuing WRITE SAME requests?

I'm assuming that ext4 is somehow calling sb_issue_zeroout().

Mike> Martin, why is it you weren't able to reproduce (I haven't Mike> attempted yet)?

Dunno.

But regardless of any bugs, WRITE SAME will still need some special attention in the context of dm-crypt.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Milan Broz

2012-12-19 20:45 UTC #11 lore raw

In reply to Mike Snitzer

Quoted 5 lines · Mike Snitzer

Quoted 2 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Joseph" == Joseph Salisbury <[email protected]> writes:

Quoted 7 lines

Quoted 7 lines
Joseph> I captured the netconsole output from boot until I reproduced Joseph> the bug. The RIP points to kcryptd_crypt_write_io_submit() in Joseph> ~/drivers/md/dm-crypt.c. The output can be seen at:
I'm thinking that dm-crypt should probably set max_write_same_sectors to
0. It doesn't really make much sense for a crypto driver to pass that command through.

IMHO WRITE_SAME doesn't not make sense in encrypted storage. The same sectors in plaintext are _not_ the same in ciphertext. NEVER.
(If so, you have a security problem.)

It must be disabled in dmcrypt explicitly then (max_write_same_sectors?).

Is it only dmcrypt, or encrypted fs are affected too?

Milan

Milan Broz

2012-12-19 21:07 UTC #12 lore raw

PATCH 1/1 dm-crypt: never use write same (was Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME)

Does this help?

dm-crypt: never use write same

Ciphertext device is not compatible with WRITE SAME, disable it for all dmcrypt devices.

Signed-off-by Milan Broz <[email protected]>

drivers/md/dm-crypt.c +7 -0

@@ -1844,6 +1844,12 @@ static int crypt_iterate_devices(struct dm_target *ti,
 	return fn(ti, cc->dev, cc->start, ti->len, data);
 }
 
+static void crypt_io_hints(struct dm_target *ti,
+			    struct queue_limits *limits)
+{
+	limits->max_write_same_sectors = 0;
+}
+
 static struct target_type crypt_target = {
 	.name   = "crypt",
 	.version = {1, 11, 0},

@@ -1858,6 +1864,7 @@ static struct target_type crypt_target = {
 	.message = crypt_message,
 	.merge  = crypt_merge,
 	.iterate_devices = crypt_iterate_devices,
+	.io_hints = crypt_io_hints,
 };
 
 static int __init dm_crypt_init(void)

Joseph Salisbury

author

2012-12-19 21:08 UTC #13 lore raw

Re: [PATCH] dm-crypt: never use write same (was Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME)

Quoted 26 lines · Milan Broz

Does this help?

dm-crypt: never use write same

Ciphertext device is not compatible with WRITE SAME, disable it for all dmcrypt devices.

Signed-off-by Milan Broz <[email protected]>

drivers/md/dm-crypt.c +6 -0

@@ -1844,6 +1844,12 @@ static int crypt_iterate_devices(struct dm_target *ti,
  	return fn(ti, cc->dev, cc->start, ti->len, data);
  }
 
+static void crypt_io_hints(struct dm_target *ti,
+			    struct queue_limits *limits)
+{
+	limits->max_write_same_sectors = 0;
+}
+
  static struct target_type crypt_target = {
  	.name   = "crypt",
  	.version = {1, 11, 0},

drivers/md/dm-crypt.c static struct target_type crypt_target = { · line 1870

Quoted 7 lines

drivers/md/dm-crypt.c +1 -0

@@ -1858,6 +1864,7 @@ static struct target_type crypt_target = {
  	.message = crypt_message,
  	.merge  = crypt_merge,
  	.iterate_devices = crypt_iterate_devices,
+	.io_hints = crypt_io_hints,
  };
 
  static int __init dm_crypt_init(void)

I'll give it a spin right now. Thanks for the quick work!

Joseph Salisbury

author

2012-12-19 22:21 UTC #14 lore raw

Re: [PATCH] dm-crypt: never use write same (was Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME)

In reply to Milan Broz

Quoted 26 lines · Milan Broz

Does this help?

dm-crypt: never use write same

Ciphertext device is not compatible with WRITE SAME, disable it for all dmcrypt devices.

Signed-off-by Milan Broz <[email protected]>

drivers/md/dm-crypt.c +6 -0

@@ -1844,6 +1844,12 @@ static int crypt_iterate_devices(struct dm_target *ti,
  	return fn(ti, cc->dev, cc->start, ti->len, data);
  }
 
+static void crypt_io_hints(struct dm_target *ti,
+			    struct queue_limits *limits)
+{
+	limits->max_write_same_sectors = 0;
+}
+
  static struct target_type crypt_target = {
  	.name   = "crypt",
  	.version = {1, 11, 0},

drivers/md/dm-crypt.c static struct target_type crypt_target = { · line 1870

Quoted 7 lines

drivers/md/dm-crypt.c +1 -0

@@ -1858,6 +1864,7 @@ static struct target_type crypt_target = {
  	.message = crypt_message,
  	.merge  = crypt_merge,
  	.iterate_devices = crypt_iterate_devices,
+	.io_hints = crypt_io_hints,
  };
 
  static int __init dm_crypt_init(void)

Great work, Milan. Your patch fixes the bug, stops the panic and allows dm-crypt to function properly.

Will you be requesting this in v3.8 ?

Thanks again,

Joe

Milan Broz

2012-12-19 22:23 UTC #15 lore raw

Re: [PATCH] dm-crypt: never use write same (was Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME)

Quoted 2 lines · Joseph Salisbury

Great work, Milan. Your patch fixes the bug, stops the panic and allows dm-crypt to function properly.

Thanks.

Quoted 2 lines

Will you be requesting this in v3.8 ?

This should go into 3.7 stable as well, I am talking with Alasdair already how to handle it.

Milan

Joseph Salisbury

author

2015-07-13 16:33 UTC #16 lore raw

Re: [PATCH] dm-crypt: never use write same (was Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME)

In reply to Joseph Salisbury

Quoted 36 lines · Joseph Salisbury

Quoted 25 lines · Milan Broz
Does this help?
dm-crypt: never use write same
Ciphertext device is not compatible with WRITE SAME, disable it for all dmcrypt devices.
Signed-off-by Milan Broz <[email protected]>
drivers/md/dm-crypt.c +5 -0
@@ -1844,6 +1844,12 @@ static int crypt_iterate_devices(struct
 dm_target *ti,
      return fn(ti, cc->dev, cc->start, ti->len, data);
  }
  +static void crypt_io_hints(struct dm_target *ti,
+                struct queue_limits *limits)
+{
+    limits->max_write_same_sectors = 0;
+}
+
  static struct target_type crypt_target = {
      .name   = "crypt",
      .version = {1, 11, 0},
drivers/md/dm-crypt.c static struct target_type crypt_target = { · line 1870
Quoted 7 lines
drivers/md/dm-crypt.c +1 -0
@@ -1858,6 +1864,7 @@ static struct target_type crypt_target = {
      .message = crypt_message,
      .merge  = crypt_merge,
      .iterate_devices = crypt_iterate_devices,
+    .io_hints = crypt_io_hints,
  };
    static int __init dm_crypt_init(void)
 
Great work, Milan. Your patch fixes the bug, stops the panic and allows dm-crypt to function properly.

Quoted 6 lines

Will you be requesting this in v3.8 ?
Thanks again,
Joe

Hi Milan,

The Ubuntu kernel has been carrying this patch since the discussion[0] we were having about the bug. I don't see that patch was ever included in mainline. Do you happen to know if this patch is still needed or was the bug we were seeing fixed in some other way?

Thanks,

Joe

[0] https://lists.ubuntu.com/archives/kernel-team/2012-December/023524.html

Milan Broz

2015-07-13 16:59 UTC #17 lore raw

Re: [PATCH] dm-crypt: never use write same (was Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME)

Quoted 6 lines · Joseph Salisbury

Hi Milan,
The Ubuntu kernel has been carrying this patch since the discussion[0] we were having about the bug. I don't see that patch was ever included in mainline. Do you happen to know if this patch is still needed or was the bug we were seeing fixed in some other way?

I think it was superseded by later Mike's approach to reverse the logic
- disable write same for all targets and require to explicitly allow it.

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/md/dm-table.c?id=c1a94672a830e01d58c7c7e8de530c3f136d6ff2
(+later patches)

So I think we do no need this patch upstream anymore.

Mike, am I right here?

Milan

Milan Broz

2015-07-13 17:01 UTC #18 lore raw

Re: [PATCH] dm-crypt: never use write same (was Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME)

In reply to Joseph Salisbury

(sorry, resending again, not sure if it was sent correctly)

Quoted 6 lines · Joseph Salisbury

Hi Milan,
The Ubuntu kernel has been carrying this patch since the discussion[0] we were having about the bug. I don't see that patch was ever included in mainline. Do you happen to know if this patch is still needed or was the bug we were seeing fixed in some other way?

I think it was superseded by later Mike's approach to reverse the logic
- disable write same for all targets and require to explicitly allow it.

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/md/dm-table.c?id=c1a94672a830e01d58c7c7e8de530c3f136d6ff2
(+later patches)

So I think we do no need this patch upstream anymore.

Mike, am I right here?

Milan

Mike Snitzer

2015-07-13 18:01 UTC #19 lore raw

Re: dm-crypt: never use write same (was Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME)

Quoted 13 lines · Milan Broz

(sorry, resending again, not sure if it was sent correctly)
Quoted 6 lines · Joseph Salisbury
Hi Milan,
The Ubuntu kernel has been carrying this patch since the discussion[0] we were having about the bug. I don't see that patch was ever included in mainline. Do you happen to know if this patch is still needed or was the bug we were seeing fixed in some other way?
I think it was superseded by later Mike's approach to reverse the logic
- disable write same for all targets and require to explicitly allow it.

Quoted 6 lines

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/md/dm-table.c?id=c1a94672a830e01d58c7c7e8de530c3f136d6ff2
(+later patches)
So I think we do no need this patch upstream anymore.
Mike, am I right here?

Yeah, a DM target needs to opt-in by setting ti->num_write_same_bios (only linear, stripe and mpath do at this time).

Martin K. Petersen

Acked-by on v1 1/1

2012-12-20 00:11 UTC #20 lore raw

Re: [PATCH] dm-crypt: never use write same (was Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME)

In reply to Milan Broz

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Milan" == Milan Broz <[email protected]> writes:

Milan> dm-crypt: never use write same

Milan> Ciphertext device is not compatible with WRITE SAME, disable it Milan> for all dmcrypt devices.

Milan> Signed-off-by: Milan Broz <[email protected]>

Acked-by Martin K. Petersen <[email protected]>

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Mike Snitzer

2012-12-20 05:47 UTC #21 lore raw

Re: dm-crypt: never use write same (was Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME)

Quoted 10 lines · Martin K. Petersen

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Milan" == Milan Broz <[email protected]> writes:
Milan> dm-crypt: never use write same
Milan> Ciphertext device is not compatible with WRITE SAME, disable it Milan> for all dmcrypt devices.
Milan> Signed-off-by: Milan Broz <[email protected]>
Acked-by Martin K. Petersen <[email protected]>

I've developed more comprehensive WRITE SAME support for DM. It can be used in conjunction with Milan's patch (which enables Milan's patch to go in too and be more easily tagged for v3.7 stable).

Anyway, my changes are available in the 'dm-write-same' branch of my github tree: git://github.com/snitm/linux.git

See the top 5 commits: https://github.com/snitm/linux/commits/dm-write-same
(topmost needs to be folded, I'll do that when posting to dm-devel in reply to this mail)

Mike Snitzer

2012-12-20 05:57 UTC #22 lore raw

PATCH 1/4 dm: default to disabling WRITE SAME support for all targets

Best to disable WRITE SAME support for all targets and then require each target to opt-in by setting 'num_write_same_requests' in the dm_target structure.

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

---
 drivers/md/dm-table.c         |   30 ++++++++++++++++++++++++++++++
 include/linux/device-mapper.h |    6 ++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

drivers/md/dm-table.c +30 -0

index 4c145fa..1ae337b 100644
@@ -1423,6 +1423,33 @@ static bool dm_table_all_devices_attribute(struct dm_table *t,
 	return 1;
 }
 
+static int device_write_same_capable(struct dm_target *ti, struct dm_dev *dev,
+				     sector_t start, sector_t len, void *data)
+{
+	struct request_queue *q = bdev_get_queue(dev->bdev);
+
+	return q && q->limits.max_write_same_sectors;
+}
+
+static bool dm_table_supports_write_same(struct dm_table *t)
+{
+	struct dm_target *ti;
+	unsigned i = 0;
+
+	while (i < dm_table_get_num_targets(t)) {
+		ti = dm_table_get_target(t, i++);
+
+		if (!ti->num_write_same_requests)
+			continue;
+
+		if (!ti->type->iterate_devices ||
+		    !ti->type->iterate_devices(ti, device_write_same_capable, NULL))
+			return 1;
+	}
+
+	return 0;
+}
+
 void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 			       struct queue_limits *limits)
 {

@@ -1454,6 +1481,9 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 	else
 		queue_flag_clear_unlocked(QUEUE_FLAG_NONROT, q);
 
+	if (!dm_table_supports_write_same(t))
+		q->limits.max_write_same_sectors = 0;
+
 	dm_table_set_integrity(t);
 
 	/*

include/linux/device-mapper.h +6 -0

index 2d65c70..072b72f 100644
@@ -211,6 +211,12 @@ struct dm_target {
 	unsigned num_discard_requests;
 
 	/*
+	 * The number of WRITE SAME requests that will be submitted to the target.
+	 * The request number can be accessed with dm_bio_get_target_request_nr.
+	 */
+	unsigned num_write_same_requests;
+
+	/*
 	 * The minimum number of extra bytes allocated in each bio for the
 	 * target to use.  dm_per_bio_data returns the data location.
 	 */

Signature · 1 line

1.7.1

Mike Snitzer

2012-12-20 05:57 UTC #23 lore raw

PATCH 2/4 dm: add WRITE SAME support

WRITE SAME bios have a payload that contain a single page. When cloning WRITE SAME bios DM has no need to modify the bi_io_vec attributes (doing so would be detrimental). DM need only alter the start and end of the WRITE SAME bio accordingly.

Rather than duplicate __clone_and_map_discard, factor out a common function that is also used by __clone_and_map_write_same.

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

---
 drivers/md/dm.c |   44 +++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 39 insertions(+), 5 deletions(-)

drivers/md/dm.c +39 -5

index 739fd2b..cee9926 100644
@@ -1218,7 +1218,28 @@ static void __clone_and_map_simple(struct clone_info *ci, struct dm_target *ti)
 	ci->sector_count = 0;
 }
 
-static int __clone_and_map_discard(struct clone_info *ci)
+typedef unsigned (*get_num_requests_fn)(struct dm_target *ti);
+
+static unsigned get_num_discard_requests(struct dm_target *ti)
+{
+	return ti->num_discard_requests;
+}
+
+static unsigned get_num_write_same_requests(struct dm_target *ti)
+{
+	return ti->num_write_same_requests;
+}
+
+typedef bool (*is_split_required_fn)(struct dm_target *ti);
+
+static bool is_split_required_for_discard(struct dm_target *ti)
+{
+	return ti->split_discard_requests;
+}
+
+static int __clone_and_map_changing_extent_only(struct clone_info *ci,
+						get_num_requests_fn get_num_requests,
+						is_split_required_fn is_split_required)
 {
 	struct dm_target *ti;
 	sector_t len;

@@ -1229,15 +1250,15 @@ static int __clone_and_map_discard(struct clone_info *ci)
 			return -EIO;
 
 		/*
-		 * Even though the device advertised discard support,
-		 * that does not mean every target supports it, and
+		 * Even though the device advertised support for this type of
+		 * request, that does not mean every target supports it, and
 		 * reconfiguration might also have changed that since the
 		 * check was performed.
 		 */
-		if (!ti->num_discard_requests)
+		if (!get_num_requests || !get_num_requests(ti))
 			return -EOPNOTSUPP;
 
-		if (!ti->split_discard_requests)
+		if (is_split_required && !is_split_required(ti))
 			len = min(ci->sector_count, max_io_len_target_boundary(ci->sector, ti));
 		else
 			len = min(ci->sector_count, max_io_len(ci->sector, ti));

@@ -1250,6 +1271,17 @@ static int __clone_and_map_discard(struct clone_info *ci)
 	return 0;
 }
 
+static int __clone_and_map_discard(struct clone_info *ci)
+{
+	return __clone_and_map_changing_extent_only(ci, get_num_discard_requests,
+						    is_split_required_for_discard);
+}
+
+static int __clone_and_map_write_same(struct clone_info *ci)
+{
+	return __clone_and_map_changing_extent_only(ci, get_num_write_same_requests, NULL);
+}
+
 static int __clone_and_map(struct clone_info *ci)
 {
 	struct bio *bio = ci->bio;

@@ -1258,6 +1290,8 @@ static int __clone_and_map(struct clone_info *ci)
 
 	if (unlikely(bio->bi_rw & REQ_DISCARD))
 		return __clone_and_map_discard(ci);
+	else if (unlikely(bio->bi_rw & REQ_WRITE_SAME))
+		return __clone_and_map_write_same(ci);
 
 	ti = dm_table_find_target(ci->map, ci->sector);
 	if (!dm_target_is_valid(ti))

Signature · 1 line

1.7.1

Mike Snitzer

2012-12-20 05:57 UTC #24 lore raw

PATCH 3/4 dm linear: add WRITE SAME support

In reply to Mike Snitzer

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

---
 drivers/md/dm-linear.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/md/dm-linear.c +1 -0

index 2f57bd8..f982e9c 100644
@@ -55,6 +55,7 @@ static int linear_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 
 	ti->num_flush_requests = 1;
 	ti->num_discard_requests = 1;
+	ti->num_write_same_requests = 1;
 	ti->private = lc;
 	return 0;

Signature · 1 line

1.7.1

Mike Snitzer

2012-12-20 05:57 UTC #25 lore raw

PATCH 4/4 dm stripe: add WRITE SAME support

In reply to Mike Snitzer

Rename stripe_map_discard to stripe_map_range and reuse it for WRITE SAME bio processing.

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

---
 drivers/md/dm-stripe.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

drivers/md/dm-stripe.c +6 -4

index 6b0e5ea..c89cde8 100644
@@ -162,6 +162,7 @@ static int stripe_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 
 	ti->num_flush_requests = stripes;
 	ti->num_discard_requests = stripes;
+	ti->num_write_same_requests = stripes;
 
 	sc->chunk_size = chunk_size;
 	if (chunk_size & (chunk_size - 1))

@@ -251,8 +252,8 @@ static void stripe_map_range_sector(struct stripe_c *sc, sector_t sector,
 		*result += sc->chunk_size;		/* next chunk */
 }
 
-static int stripe_map_discard(struct stripe_c *sc, struct bio *bio,
-			      uint32_t target_stripe)
+static int stripe_map_range(struct stripe_c *sc, struct bio *bio,
+			    uint32_t target_stripe)
 {
 	sector_t begin, end;

@@ -283,10 +284,11 @@ static int stripe_map(struct dm_target *ti, struct bio *bio)
 		bio->bi_bdev = sc->stripe[target_request_nr].dev->bdev;
 		return DM_MAPIO_REMAPPED;
 	}
-	if (unlikely(bio->bi_rw & REQ_DISCARD)) {
+	if (unlikely(bio->bi_rw & REQ_DISCARD) ||
+	    unlikely(bio->bi_rw & REQ_WRITE_SAME)) {
 		target_request_nr = dm_bio_get_target_request_nr(bio);
 		BUG_ON(target_request_nr >= sc->stripes);
-		return stripe_map_discard(sc, bio, target_request_nr);
+		return stripe_map_range(sc, bio, target_request_nr);
 	}
 
 	stripe_map_sector(sc, bio->bi_sector, &stripe, &bio->bi_sector);

Signature · 1 line

1.7.1