[v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME

[v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME

[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 <martin.petersen@oracle.com>
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

Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME

[Mike Snitzer]

On Fri, Dec 14 2012 at  3:30pm -0500,
Joseph Salisbury <joseph.salisbury@canonical.com> wrote:

> 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 <martin.petersen@oracle.com>
> 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

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

Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME

[Martin K. Petersen]

>>>>> "Joseph" == Joseph Salisbury <joseph.salisbury@canonical.com> 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.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME

[Joseph Salisbury]

On 12/14/2012 04:11 PM, Mike Snitzer wrote:
> On Fri, Dec 14 2012 at  3:30pm -0500,
> Joseph Salisbury <joseph.salisbury@canonical.com> wrote:
>
>> 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 <martin.petersen@oracle.com>
>> 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
> 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.

>
> 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.

>
> Mike
I'll research further and provide additional data.

Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME

[Joseph Salisbury]

On 12/14/2012 05:35 PM, Martin K. Petersen wrote:
>>>>>> "Joseph" == Joseph Salisbury <joseph.salisbury@canonical.com> 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.
>

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

Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME

[Joseph Salisbury]

On 12/14/2012 05:35 PM, Martin K. Petersen wrote:
>>>>>> "Joseph" == Joseph Salisbury <joseph.salisbury@canonical.com> 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.
>
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

Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME

[Martin K. Petersen]

>>>>> "Joseph" == Joseph Salisbury <joseph.salisbury@canonical.com> 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?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME

[Mike Snitzer]

On Wed, Dec 19 2012 at 11:58am -0500,
Martin K. Petersen <martin.petersen@oracle.com> wrote:

> >>>>> "Joseph" == Joseph Salisbury <joseph.salisbury@canonical.com> 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)?

Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME

[Joseph Salisbury]

On 12/19/2012 02:58 PM, Mike Snitzer wrote:
> On Wed, Dec 19 2012 at 11:58am -0500,
> Martin K. Petersen <martin.petersen@oracle.com> wrote:
>
>>>>>>> "Joseph" == Joseph Salisbury <joseph.salisbury@canonical.com> 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)?
Martin, were you using the scsi_debug module, or a real scsi device(s)?

Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME

[Martin K. Petersen]

>>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> 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.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME

[Milan Broz]

On 12/19/2012 08:58 PM, Mike Snitzer wrote:
> On Wed, Dec 19 2012 at 11:58am -0500,
> Martin K. Petersen <martin.petersen@oracle.com> wrote:
> 
>>>>>>> "Joseph" == Joseph Salisbury <joseph.salisbury@canonical.com> 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.

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

Re: [v3.7 Regression] [SCSI] sd: Implement support for WRITE SAME

[Martin K. Petersen]

>>>>> "Joseph" == Joseph Salisbury <joseph.salisbury@canonical.com> writes:

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

I used scsi_debug for the test.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[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 <mbroz@redhat.com>

--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -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)

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

[Joseph Salisbury]

On 12/19/2012 04:07 PM, Milan Broz wrote:
> 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 <mbroz@redhat.com>
>
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -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)
>
I'll give it a spin right now.  Thanks for the quick work!

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

[Joseph Salisbury]

On 12/19/2012 04:07 PM, Milan Broz wrote:
> 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 <mbroz@redhat.com>
>
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -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)
>
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

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

[Milan Broz]

On 12/19/2012 11:20 PM, Joseph Salisbury wrote:
> Great work, Milan.  Your patch fixes the bug, stops the panic and allows 
> dm-crypt to function properly.

Thanks.

> 
> 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

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

[Martin K. Petersen]

>>>>> "Milan" == Milan Broz <mbroz@redhat.com> 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 <mbroz@redhat.com>

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Mike Snitzer]

On Wed, Dec 19 2012 at  7:11pm -0500,
Martin K. Petersen <martin.petersen@oracle.com> wrote:

> >>>>> "Milan" == Milan Broz <mbroz@redhat.com> 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 <mbroz@redhat.com>
> 
> Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

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)

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

[Mike Snitzer]

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 <snitzer@redhat.com>
---
 drivers/md/dm-table.c         |   30 ++++++++++++++++++++++++++++++
 include/linux/device-mapper.h |    6 ++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 4c145fa..1ae337b 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -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);
 
 	/*
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index 2d65c70..072b72f 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -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.
 	 */
-- 
1.7.1

[PATCH 2/4] dm: add WRITE SAME support

[Mike Snitzer]

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 <snitzer@redhat.com>
---
 drivers/md/dm.c |   44 +++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 739fd2b..cee9926 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -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))
-- 
1.7.1

[PATCH 3/4] dm linear: add WRITE SAME support

[Mike Snitzer]

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-linear.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c
index 2f57bd8..f982e9c 100644
--- a/drivers/md/dm-linear.c
+++ b/drivers/md/dm-linear.c
@@ -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;
 
-- 
1.7.1

[PATCH 4/4] dm stripe: add WRITE SAME support

[Mike Snitzer]

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

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-stripe.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-stripe.c b/drivers/md/dm-stripe.c
index 6b0e5ea..c89cde8 100644
--- a/drivers/md/dm-stripe.c
+++ b/drivers/md/dm-stripe.c
@@ -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);
-- 
1.7.1

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

[Joseph Salisbury]

On 12/19/2012 05:20 PM, Joseph Salisbury wrote:
> On 12/19/2012 04:07 PM, Milan Broz wrote:
>> 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 <mbroz@redhat.com>
>>
>> --- a/drivers/md/dm-crypt.c
>> +++ b/drivers/md/dm-crypt.c
>> @@ -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)
>>
> 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
>
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

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

[Milan Broz]

On 07/13/2015 06:33 PM, Joseph Salisbury wrote:
> 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

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

[Milan Broz]

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

On 07/13/2015 06:33 PM, Joseph Salisbury wrote:
> 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

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

[Mike Snitzer]

On Mon, Jul 13 2015 at  1:01pm -0400,
Milan Broz <mbroz@redhat.com> wrote:

> (sorry, resending again, not sure if it was sent correctly)
> 
> On 07/13/2015 06:33 PM, Joseph Salisbury wrote:
> > 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?

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