[PATCH] nvmet: fix nvmet_execute_write_zeroes function

[PATCH] nvmet: fix nvmet_execute_write_zeroes function

[Rodrigo R. Galvao]

When trying to issue write_zeroes command against TARGET the nr_sector is
being incremented by 1, which ends up hitting the following condition at
__blkdev_issue_zeroout:

     if ((sector | nr_sects) & bs_mask)
                return -EINVAL;

Causing the command to always fail. Removing the increment makes the
command to work properly.

Signed-off-by: Rodrigo R. Galvao <rosattig@linux.vnet.ibm.com>
---
 drivers/nvme/target/io-cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/io-cmd.c b/drivers/nvme/target/io-cmd.c
index 28bbdff..8d72686 100644
--- a/drivers/nvme/target/io-cmd.c
+++ b/drivers/nvme/target/io-cmd.c
@@ -174,7 +174,7 @@ static void nvmet_execute_write_zeroes(struct nvmet_req *req)
 	sector = le64_to_cpu(write_zeroes->slba) <<
 		(req->ns->blksize_shift - 9);
 	nr_sector = (((sector_t)le16_to_cpu(write_zeroes->length)) <<
-		(req->ns->blksize_shift - 9)) + 1;
+		(req->ns->blksize_shift - 9));
 
 	if (__blkdev_issue_zeroout(req->ns->bdev, sector, nr_sector,
 				GFP_KERNEL, &bio, 0))
-- 
2.7.4

[PATCH] nvmet: fix nvmet_execute_write_zeroes function

[Rodrigo R. Galvao]

When trying to issue write_zeroes command against TARGET the nr_sector is
being incremented by 1, which ends up hitting the following condition at
__blkdev_issue_zeroout:

     if ((sector | nr_sects) & bs_mask)
                return -EINVAL;

Causing the command to always fail. Removing the increment makes the
command to work properly.

Signed-off-by: Rodrigo R. Galvao <rosattig at linux.vnet.ibm.com>
---
 drivers/nvme/target/io-cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/io-cmd.c b/drivers/nvme/target/io-cmd.c
index 28bbdff..8d72686 100644
--- a/drivers/nvme/target/io-cmd.c
+++ b/drivers/nvme/target/io-cmd.c
@@ -174,7 +174,7 @@ static void nvmet_execute_write_zeroes(struct nvmet_req *req)
 	sector = le64_to_cpu(write_zeroes->slba) <<
 		(req->ns->blksize_shift - 9);
 	nr_sector = (((sector_t)le16_to_cpu(write_zeroes->length)) <<
-		(req->ns->blksize_shift - 9)) + 1;
+		(req->ns->blksize_shift - 9));
 
 	if (__blkdev_issue_zeroout(req->ns->bdev, sector, nr_sector,
 				GFP_KERNEL, &bio, 0))
-- 
2.7.4

Re: [PATCH] nvmet: fix nvmet_execute_write_zeroes function

[Keith Busch]

On Fri, Mar 30, 2018 at 06:18:50PM -0300, Rodrigo R. Galvao wrote:
> When trying to issue write_zeroes command against TARGET the nr_sector is
> being incremented by 1, which ends up hitting the following condition at
> __blkdev_issue_zeroout:
> 
>      if ((sector | nr_sects) & bs_mask)
>                 return -EINVAL;
> 
> Causing the command to always fail. Removing the increment makes the
> command to work properly.

Doesn't that mean your host is using this command wrong? The NLB is a
0's based value, we're supposed to +1 to get the correct block count.

[PATCH] nvmet: fix nvmet_execute_write_zeroes function

[Keith Busch]

On Fri, Mar 30, 2018@06:18:50PM -0300, Rodrigo R. Galvao wrote:
> When trying to issue write_zeroes command against TARGET the nr_sector is
> being incremented by 1, which ends up hitting the following condition at
> __blkdev_issue_zeroout:
> 
>      if ((sector | nr_sects) & bs_mask)
>                 return -EINVAL;
> 
> Causing the command to always fail. Removing the increment makes the
> command to work properly.

Doesn't that mean your host is using this command wrong? The NLB is a
0's based value, we're supposed to +1 to get the correct block count.

Re: [PATCH] nvmet: fix nvmet_execute_write_zeroes function

[Rodrigo Rosatti Galvão]

> Doesn't that mean your host is using this command wrong? The NLB is a
> 0's based value, we're supposed to +1 to get the correct block count.

Keith, I tested passing different values to the c (number of blocks) and 
s (64-bit LBA of first block to access) parameters, and it was failing. 
When I removed the +1, the command worked fine.

I used a simple script to validate this:

for s in {0..20}
     do
             for c in {0..20}
             do
                     nvme write-zeroes /dev/nvme0 -n 10 -s $s -c $c
             done
     done


Is there some other way to test it?

-- 
Rodrigo R. Galvão
Intern - Linux Technology Center - IBM

[PATCH] nvmet: fix nvmet_execute_write_zeroes function

[Rodrigo Rosatti Galvão]

> Doesn't that mean your host is using this command wrong? The NLB is a
> 0's based value, we're supposed to +1 to get the correct block count.

Keith, I tested passing different values to the c (number of blocks) and 
s (64-bit LBA of first block to access) parameters, and it was failing. 
When I removed the +1, the command worked fine.

I used a simple script to validate this:

for s in {0..20}
 ??? do
 ??? ??? ??? for c in {0..20}
 ??? ??? ??? do
 ??? ??? ??????????? nvme write-zeroes /dev/nvme0 -n 10 -s $s -c $c
 ??? ??? ??? done
 ??? done


Is there some other way to test it?

-- 
Rodrigo R. Galv?o
Intern - Linux Technology Center - IBM

Re: [PATCH] nvmet: fix nvmet_execute_write_zeroes function

[chaitany kulkarni]

Please give me a couple of days, I'll look into this.

On Fri, Mar 30, 2018 at 2:48 PM, Rodrigo Rosatti Galvão
<rosattig@linux.vnet.ibm.com> wrote:
>
>> Doesn't that mean your host is using this command wrong? The NLB is a
>> 0's based value, we're supposed to +1 to get the correct block count.
>
>
> Keith, I tested passing different values to the c (number of blocks) and s
> (64-bit LBA of first block to access) parameters, and it was failing. When I
> removed the +1, the command worked fine.
>
> I used a simple script to validate this:
>
> for s in {0..20}
>     do
>             for c in {0..20}
>             do
>                     nvme write-zeroes /dev/nvme0 -n 10 -s $s -c $c
>             done
>     done
>
>
> Is there some other way to test it?
>
> --
> Rodrigo R. Galvão
> Intern - Linux Technology Center - IBM
>
>
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH] nvmet: fix nvmet_execute_write_zeroes function

[chaitany kulkarni]

Please give me a couple of days, I'll look into this.

On Fri, Mar 30, 2018 at 2:48 PM, Rodrigo Rosatti Galv?o
<rosattig@linux.vnet.ibm.com> wrote:
>
>> Doesn't that mean your host is using this command wrong? The NLB is a
>> 0's based value, we're supposed to +1 to get the correct block count.
>
>
> Keith, I tested passing different values to the c (number of blocks) and s
> (64-bit LBA of first block to access) parameters, and it was failing. When I
> removed the +1, the command worked fine.
>
> I used a simple script to validate this:
>
> for s in {0..20}
>     do
>             for c in {0..20}
>             do
>                     nvme write-zeroes /dev/nvme0 -n 10 -s $s -c $c
>             done
>     done
>
>
> Is there some other way to test it?
>
> --
> Rodrigo R. Galv?o
> Intern - Linux Technology Center - IBM
>
>
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvmet: fix nvmet_execute_write_zeroes function

[Rodrigo Rosatti Galvao]

One thing that I just forgot to explain previously, but I think its 
relevant:

1. The command is failing with 4k logical block size, but works with 512B

2. With the patch, the command is working for both 512B and 4K.


Here are some extra information I could get when executing the command 
with both block sizes:


[without the patch]


# nvme write-zeroes /dev/nvme0n1 -c 1 -s 0

#### 512 B ####

[ 3222.049211] __blkdev_issue_zeroout() - sector: 0 -- rrg
[ 3222.049375] __blkdev_issue_zeroout() - nr_sects: 9 -- rrg
[ 3222.049419] __blkdev_issue_zeroout() - bs_mask: 0 -- rrg
[ 3222.049575] __blkdev_issue_zeroout() - ((sector | nr_sects) & 
bs_mask): 0 -- rrg


#### 4K #### (FAILS)

[ 4110.654424] __blkdev_issue_zeroout() - sector: 0 -- rrg
[ 4110.654601] __blkdev_issue_zeroout() - nr_sects: 9 -- rrg
[ 4110.654645] __blkdev_issue_zeroout() - bs_mask: 7 -- rrg
[ 4110.654794] __blkdev_issue_zeroout() - ((sector | nr_sects) & 
bs_mask): 1 -- rrg



[with the patch]

nvme write-zeroes /dev/nvme0n1 -c 1 -s 0

#### 512 B #####

[root@ltc-garrison ~]# nvme write-zeroes /dev/nvme0n1 -c 1 -s 0

[  426.028592] __blkdev_issue_zeroout() - sector: 0 -- rrg
[  426.028779] __blkdev_issue_zeroout() - nr_sects: 8 -- rrg
[  426.028946] __blkdev_issue_zeroout() - bs_mask: 0 -- rrg
[  426.028986] __blkdev_issue_zeroout() - ((sector | nr_sects) & 
bs_mask): 0 -- rrg



#### 4K ####

[  265.489219] __blkdev_issue_zeroout() - sector: 0 -- rrg
[  265.489419] __blkdev_issue_zeroout() - nr_sects: 8 -- rrg
[  265.489587] __blkdev_issue_zeroout() - bs_mask: 7 -- rrg
[  265.489627] __blkdev_issue_zeroout() - ((sector | nr_sects) & 
bs_mask): 0 -- rrg


On 04/02/2018 05:03 AM, chaitany kulkarni wrote:
> Please give me a couple of days, I'll look into this.
>
> On Fri, Mar 30, 2018 at 2:48 PM, Rodrigo Rosatti Galvão
> <rosattig@linux.vnet.ibm.com> wrote:
>>> Doesn't that mean your host is using this command wrong? The NLB is a
>>> 0's based value, we're supposed to +1 to get the correct block count.
>>
>> Keith, I tested passing different values to the c (number of blocks) and s
>> (64-bit LBA of first block to access) parameters, and it was failing. When I
>> removed the +1, the command worked fine.
>>
>> I used a simple script to validate this:
>>
>> for s in {0..20}
>>      do
>>              for c in {0..20}
>>              do
>>                      nvme write-zeroes /dev/nvme0 -n 10 -s $s -c $c
>>              done
>>      done
>>
>>
>> Is there some other way to test it?
>>
>> --
>> Rodrigo R. Galvão
>> Intern - Linux Technology Center - IBM
>>
>>
>>
>> _______________________________________________
>> Linux-nvme mailing list
>> Linux-nvme@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-nvme

-- 
Rodrigo R. Galvão
Intern - Linux Technology Center - IBM

[PATCH] nvmet: fix nvmet_execute_write_zeroes function

[Rodrigo Rosatti Galvao]

One thing that I just forgot to explain previously, but I think its 
relevant:

1. The command is failing with 4k logical block size, but works with 512B

2. With the patch, the command is working for both 512B and 4K.


Here are some extra information I could get when executing the command 
with both block sizes:


[without the patch]


# nvme write-zeroes /dev/nvme0n1 -c 1 -s 0

#### 512 B ####

[ 3222.049211] __blkdev_issue_zeroout() - sector: 0 -- rrg
[ 3222.049375] __blkdev_issue_zeroout() - nr_sects: 9 -- rrg
[ 3222.049419] __blkdev_issue_zeroout() - bs_mask: 0 -- rrg
[ 3222.049575] __blkdev_issue_zeroout() - ((sector | nr_sects) & 
bs_mask): 0 -- rrg


#### 4K #### (FAILS)

[ 4110.654424] __blkdev_issue_zeroout() - sector: 0 -- rrg
[ 4110.654601] __blkdev_issue_zeroout() - nr_sects: 9 -- rrg
[ 4110.654645] __blkdev_issue_zeroout() - bs_mask: 7 -- rrg
[ 4110.654794] __blkdev_issue_zeroout() - ((sector | nr_sects) & 
bs_mask): 1 -- rrg



[with the patch]

nvme write-zeroes /dev/nvme0n1 -c 1 -s 0

#### 512 B #####

[root at ltc-garrison ~]# nvme write-zeroes /dev/nvme0n1 -c 1 -s 0

[? 426.028592] __blkdev_issue_zeroout() - sector: 0 -- rrg
[? 426.028779] __blkdev_issue_zeroout() - nr_sects: 8 -- rrg
[? 426.028946] __blkdev_issue_zeroout() - bs_mask: 0 -- rrg
[? 426.028986] __blkdev_issue_zeroout() - ((sector | nr_sects) & 
bs_mask): 0 -- rrg



#### 4K ####

[? 265.489219] __blkdev_issue_zeroout() - sector: 0 -- rrg
[? 265.489419] __blkdev_issue_zeroout() - nr_sects: 8 -- rrg
[? 265.489587] __blkdev_issue_zeroout() - bs_mask: 7 -- rrg
[? 265.489627] __blkdev_issue_zeroout() - ((sector | nr_sects) & 
bs_mask): 0 -- rrg


On 04/02/2018 05:03 AM, chaitany kulkarni wrote:
> Please give me a couple of days, I'll look into this.
>
> On Fri, Mar 30, 2018 at 2:48 PM, Rodrigo Rosatti Galv?o
> <rosattig@linux.vnet.ibm.com> wrote:
>>> Doesn't that mean your host is using this command wrong? The NLB is a
>>> 0's based value, we're supposed to +1 to get the correct block count.
>>
>> Keith, I tested passing different values to the c (number of blocks) and s
>> (64-bit LBA of first block to access) parameters, and it was failing. When I
>> removed the +1, the command worked fine.
>>
>> I used a simple script to validate this:
>>
>> for s in {0..20}
>>      do
>>              for c in {0..20}
>>              do
>>                      nvme write-zeroes /dev/nvme0 -n 10 -s $s -c $c
>>              done
>>      done
>>
>>
>> Is there some other way to test it?
>>
>> --
>> Rodrigo R. Galv?o
>> Intern - Linux Technology Center - IBM
>>
>>
>>
>> _______________________________________________
>> Linux-nvme mailing list
>> Linux-nvme at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-nvme

-- 
Rodrigo R. Galv?o
Intern - Linux Technology Center - IBM

Re: [PATCH] nvmet: fix nvmet_execute_write_zeroes function

[Keith Busch]

On Fri, Mar 30, 2018 at 06:18:50PM -0300, Rodrigo R. Galvao wrote:
>  	sector = le64_to_cpu(write_zeroes->slba) <<
>  		(req->ns->blksize_shift - 9);
>  	nr_sector = (((sector_t)le16_to_cpu(write_zeroes->length)) <<
> -		(req->ns->blksize_shift - 9)) + 1;
> +		(req->ns->blksize_shift - 9));

I see what's wrong here. The +1 needs to be on the native format prior
to converting to 512b, so the fix needs to be:

---
 	sector = le64_to_cpu(write_zeroes->slba) <<
 		(req->ns->blksize_shift - 9);
-	nr_sector = (((sector_t)le16_to_cpu(write_zeroes->length)) <<
-		(req->ns->blksize_shift - 9)) + 1;
+	nr_sector = (((sector_t)le16_to_cpu(write_zeroes->length + 1)) <<
+		(req->ns->blksize_shift - 9));
 
 	if (__blkdev_issue_zeroout(req->ns->bdev, sector, nr_sector,
 				GFP_KERNEL, &bio, 0))
--

[PATCH] nvmet: fix nvmet_execute_write_zeroes function

[Keith Busch]

On Fri, Mar 30, 2018@06:18:50PM -0300, Rodrigo R. Galvao wrote:
>  	sector = le64_to_cpu(write_zeroes->slba) <<
>  		(req->ns->blksize_shift - 9);
>  	nr_sector = (((sector_t)le16_to_cpu(write_zeroes->length)) <<
> -		(req->ns->blksize_shift - 9)) + 1;
> +		(req->ns->blksize_shift - 9));

I see what's wrong here. The +1 needs to be on the native format prior
to converting to 512b, so the fix needs to be:

---
 	sector = le64_to_cpu(write_zeroes->slba) <<
 		(req->ns->blksize_shift - 9);
-	nr_sector = (((sector_t)le16_to_cpu(write_zeroes->length)) <<
-		(req->ns->blksize_shift - 9)) + 1;
+	nr_sector = (((sector_t)le16_to_cpu(write_zeroes->length + 1)) <<
+		(req->ns->blksize_shift - 9));
 
 	if (__blkdev_issue_zeroout(req->ns->bdev, sector, nr_sector,
 				GFP_KERNEL, &bio, 0))
--

Re: [PATCH] nvmet: fix nvmet_execute_write_zeroes function

[Keith Busch]

On Mon, Apr 02, 2018 at 10:47:10AM -0300, Rodrigo Rosatti Galvao wrote:
> One thing that I just forgot to explain previously, but I think its
> relevant:
> 
> 1. The command is failing with 4k logical block size, but works with 512B
> 
> 2. With the patch, the command is working for both 512B and 4K.

While you're not getting errors with your patch, you're not zeroing out
the requested blocks, so that's a data corruption.

The issue is the +1 is in the wrong place. It needs to be added to the
native format prior to converting it to a 512b sector count. Do you want
to resend with that change?

[PATCH] nvmet: fix nvmet_execute_write_zeroes function

[Keith Busch]

On Mon, Apr 02, 2018@10:47:10AM -0300, Rodrigo Rosatti Galvao wrote:
> One thing that I just forgot to explain previously, but I think its
> relevant:
> 
> 1. The command is failing with 4k logical block size, but works with 512B
> 
> 2. With the patch, the command is working for both 512B and 4K.

While you're not getting errors with your patch, you're not zeroing out
the requested blocks, so that's a data corruption.

The issue is the +1 is in the wrong place. It needs to be added to the
native format prior to converting it to a 512b sector count. Do you want
to resend with that change?

Re: [PATCH] nvmet: fix nvmet_execute_write_zeroes function

[Rodrigo Rosatti Galvao]

Thanks Keith. The patch you proposed worked fine!!

I just sent a V2 with that change!


On 04/02/2018 11:21 AM, Keith Busch wrote:
> On Mon, Apr 02, 2018 at 10:47:10AM -0300, Rodrigo Rosatti Galvao wrote:
>> One thing that I just forgot to explain previously, but I think its
>> relevant:
>>
>> 1. The command is failing with 4k logical block size, but works with 512B
>>
>> 2. With the patch, the command is working for both 512B and 4K.
> While you're not getting errors with your patch, you're not zeroing out
> the requested blocks, so that's a data corruption.
>
> The issue is the +1 is in the wrong place. It needs to be added to the
> native format prior to converting it to a 512b sector count. Do you want
> to resend with that change?
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
>

-- 
Rodrigo R. Galvão
Intern - Linux Technology Center - IBM

[PATCH] nvmet: fix nvmet_execute_write_zeroes function

[Rodrigo Rosatti Galvao]

Thanks Keith. The patch you proposed worked fine!!

I just sent a V2 with that change!


On 04/02/2018 11:21 AM, Keith Busch wrote:
> On Mon, Apr 02, 2018@10:47:10AM -0300, Rodrigo Rosatti Galvao wrote:
>> One thing that I just forgot to explain previously, but I think its
>> relevant:
>>
>> 1. The command is failing with 4k logical block size, but works with 512B
>>
>> 2. With the patch, the command is working for both 512B and 4K.
> While you're not getting errors with your patch, you're not zeroing out
> the requested blocks, so that's a data corruption.
>
> The issue is the +1 is in the wrong place. It needs to be added to the
> native format prior to converting it to a 512b sector count. Do you want
> to resend with that change?
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
>

-- 
Rodrigo R. Galv?o
Intern - Linux Technology Center - IBM