[PATCH] nvmet: Change error code of discard command if not supported

[PATCH] nvmet: Change error code of discard command if not supported

[Israel Rukshin]

Execute discard command on block device that doesn't support it
should return not supported error.
Returning internal error while using multi-path fails the path.

Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
Signed-off-by: Israel Rukshin <israelr at mellanox.com>
---
 drivers/nvme/target/io-cmd.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/io-cmd.c b/drivers/nvme/target/io-cmd.c
index 0a4372a..00d4607a 100644
--- a/drivers/nvme/target/io-cmd.c
+++ b/drivers/nvme/target/io-cmd.c
@@ -105,10 +105,15 @@ static void nvmet_execute_flush(struct nvmet_req *req)
 static u16 nvmet_discard_range(struct nvmet_ns *ns,
 		struct nvme_dsm_range *range, struct bio **bio)
 {
-	if (__blkdev_issue_discard(ns->bdev,
+	int ret;
+
+	ret = __blkdev_issue_discard(ns->bdev,
 			le64_to_cpu(range->slba) << (ns->blksize_shift - 9),
 			le32_to_cpu(range->nlb) << (ns->blksize_shift - 9),
-			GFP_KERNEL, 0, bio))
+			GFP_KERNEL, 0, bio);
+	if (ret == -EOPNOTSUPP)
+		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+	else if (ret)
 		return NVME_SC_INTERNAL | NVME_SC_DNR;
 	return 0;
 }
-- 
1.8.3.1

[PATCH] nvmet: Change error code of discard command if not supported

[Sagi Grimberg]

> Execute discard command on block device that doesn't support it
> should return not supported error.
> Returning internal error while using multi-path fails the path.
> 
> Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
> Signed-off-by: Israel Rukshin <israelr at mellanox.com>
> ---
>   drivers/nvme/target/io-cmd.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/target/io-cmd.c b/drivers/nvme/target/io-cmd.c
> index 0a4372a..00d4607a 100644
> --- a/drivers/nvme/target/io-cmd.c
> +++ b/drivers/nvme/target/io-cmd.c
> @@ -105,10 +105,15 @@ static void nvmet_execute_flush(struct nvmet_req *req)
>   static u16 nvmet_discard_range(struct nvmet_ns *ns,
>   		struct nvme_dsm_range *range, struct bio **bio)
>   {
> -	if (__blkdev_issue_discard(ns->bdev,
> +	int ret;
> +
> +	ret = __blkdev_issue_discard(ns->bdev,
>   			le64_to_cpu(range->slba) << (ns->blksize_shift - 9),
>   			le32_to_cpu(range->nlb) << (ns->blksize_shift - 9),
> -			GFP_KERNEL, 0, bio))
> +			GFP_KERNEL, 0, bio);
> +	if (ret == -EOPNOTSUPP)
> +		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;

Actually I don't think we are allowed to fail with this status. We claim
we support dsm regardless of the underlying backend device (because we
can expose different devices in the same subsystem), so we can't fail
a valid dsm command on invalid opcode. Either we keep the existing
behavior or we simply return success as dsm is a hint anyways...

> +	else if (ret)
>   		return NVME_SC_INTERNAL | NVME_SC_DNR;
>   	return 0;
>   }
> 

[PATCH] nvmet: Change error code of discard command if not supported

[Max Gurtovoy]

On 1/29/2018 8:30 PM, Sagi Grimberg wrote:
> 
>> Execute discard command on block device that doesn't support it
>> should return not supported error.
>> Returning internal error while using multi-path fails the path.
>>
>> Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
>> Signed-off-by: Israel Rukshin <israelr at mellanox.com>
>> ---
>> ? drivers/nvme/target/io-cmd.c | 9 +++++++--
>> ? 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/target/io-cmd.c b/drivers/nvme/target/io-cmd.c
>> index 0a4372a..00d4607a 100644
>> --- a/drivers/nvme/target/io-cmd.c
>> +++ b/drivers/nvme/target/io-cmd.c
>> @@ -105,10 +105,15 @@ static void nvmet_execute_flush(struct nvmet_req 
>> *req)
>> ? static u16 nvmet_discard_range(struct nvmet_ns *ns,
>> ????????? struct nvme_dsm_range *range, struct bio **bio)
>> ? {
>> -??? if (__blkdev_issue_discard(ns->bdev,
>> +??? int ret;
>> +
>> +??? ret = __blkdev_issue_discard(ns->bdev,
>> ????????????? le64_to_cpu(range->slba) << (ns->blksize_shift - 9),
>> ????????????? le32_to_cpu(range->nlb) << (ns->blksize_shift - 9),
>> -??????????? GFP_KERNEL, 0, bio))
>> +??????????? GFP_KERNEL, 0, bio);
>> +??? if (ret == -EOPNOTSUPP)
>> +??????? return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
> 
> Actually I don't think we are allowed to fail with this status. We claim
> we support dsm regardless of the underlying backend device (because we
> can expose different devices in the same subsystem), so we can't fail
> a valid dsm command on invalid opcode. Either we keep the existing
> behavior or we simply return success as dsm is a hint anyways...

we can't fail a path in case the backend device doesn't support dsm. The 
avarage user that will try to create a fs using mkfs.ext4, for example, 
on top of a multipath device, without the appropriate flags will fail.
I'm fine with returning successful value, we'll test it and send a V2.

> 
>> +??? else if (ret)
>> ????????? return NVME_SC_INTERNAL | NVME_SC_DNR;
>> ????? return 0;
>> ? }
>>

[PATCH] nvmet: Change error code of discard command if not supported

[Christoph Hellwig]

On Mon, Jan 29, 2018@08:30:45PM +0200, Sagi Grimberg wrote:
> Actually I don't think we are allowed to fail with this status. We claim
> we support dsm regardless of the underlying backend device (because we
> can expose different devices in the same subsystem), so we can't fail
> a valid dsm command on invalid opcode. Either we keep the existing
> behavior or we simply return success as dsm is a hint anyways...

Yes, we should return success here.