Re: [PATCH v9 1/9] nvme: fail cancelled commands with NVME_SC_HOST_PATH_ERROR

Re: [PATCH v9 1/9] nvme: fail cancelled commands with NVME_SC_HOST_PATH_ERROR

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 08/29/2019 03:07 PM, Sagi Grimberg wrote:
> NVME_SC_ABORT_REQ means that the request was aborted due to
> an abort command received. In our case, this is a transport
> cancellation, so host pathing error is much more appropriate.
>
> Also, convert NVME_SC_HOST_PATH_ERROR to BLK_STS_TRANSPORT for
> such that callers can understand that the status is a transport
> related error. This will be used by the ns scanning code to
> understand if it got an error from the controller or that the
> controller happens to be unreachable by the transport.
>
> Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> Reviewed-by: James Smart <james.smart@broadcom.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/core.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 0eb5c1bb2f48..11ef174e8399 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -226,6 +226,8 @@ static blk_status_t nvme_error_status(struct request *req)
>   		return BLK_STS_PROTECTION;
>   	case NVME_SC_RESERVATION_CONFLICT:
>   		return BLK_STS_NEXUS;
> +	case NVME_SC_HOST_PATH_ERROR:
> +		return BLK_STS_TRANSPORT;
>   	default:
>   		return BLK_STS_IOERR;
>   	}
> @@ -294,7 +296,7 @@ bool nvme_cancel_request(struct request *req, void *data, bool reserved)
>   	if (blk_mq_request_completed(req))
>   		return true;
>
> -	nvme_req(req)->status = NVME_SC_ABORT_REQ;
> +	nvme_req(req)->status = NVME_SC_HOST_PATH_ERROR;
>   	blk_mq_complete_request(req);
>   	return true;
>   }
>


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v9 2/9] nvme: make nvme_identify_ns propagate errors back

[Chaitanya Kulkarni]

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 08/29/2019 03:07 PM, Sagi Grimberg wrote:
> right now callers of nvme_identify_ns only know that it failed,
> but don't know why. Make nvme_identify_ns propagate the error back.
> Because nvme_submit_sync_cmd may return a positive status code, we
> make nvme_identify_ns receive the id by reference and return that
> status up the call chain.
>
> Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> Reviewed-by: James Smart <james.smart@broadcom.com>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/core.c | 30 +++++++++++++-----------------
>   1 file changed, 13 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v9 3/9] nvme: make nvme_report_ns_ids propagate error back

[Chaitanya Kulkarni]

Starting commit log with "And" seems little bit odd.

Otherwise, looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 08/29/2019 03:07 PM, Sagi Grimberg wrote:
> And make the callers check the return status and propagate
> back accordingly. Also print the return status.
>
> Reviewed-by: Hannes Reinecke<hare@suse.com>
> Reviewed-by: James Smart<james.smart@broadcom.com>
> Reviewed-by: Christoph Hellwig<hch@lst.de>
> Signed-off-by: Sagi Grimberg<sagi@grimberg.me>
> ---
>   drivers/nvme/host/core.c | 24 ++++++++++++++++++------
>   1 file changed, 18 insertions(+), 6 deletions(-)


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v9 6/9] block_dev: split disk size check out of revalidate_disk

[Chaitanya Kulkarni]

On 08/29/2019 03:07 PM, Sagi Grimberg wrote:
> Allow callers to call separately the ->revalidate_disk()
> callout and the disk size change check separately. This
> will help the caller to take action based on the return
> status of ->revalidate_disk() and run the disk size check
> afterwards.

Commit message looks odd as compare to other commits, please have
a look at following message which uses limit of 72 car per line  :-

Allow callers to call separately the ->revalidate_disk() callout and
the disk size change check separately. This will help the caller to
take action based on the return status of ->revalidate_disk() and
run the disk size check afterwards.

>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   fs/block_dev.c     | 29 +++++++++++++++++------------
>   include/linux/fs.h |  1 +
>   2 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index a6f7c892cb4a..4939a093e2f8 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1482,6 +1482,21 @@ void check_disk_size_change(struct gendisk *disk, struct block_device *bdev,
>   	}
>   }
>
> +void check_disk_size(struct gendisk *disk, bool verbose)
> +{
> +	struct block_device *bdev = bdget_disk(disk, 0);
> +
> +	if (!bdev)
> +		return;
> +
> +	mutex_lock(&bdev->bd_mutex);
> +	check_disk_size_change(disk, bdev, verbose);
> +	bdev->bd_invalidated = 0;
> +	mutex_unlock(&bdev->bd_mutex);
> +	bdput(bdev);
> +}
> +EXPORT_SYMBOL_GPL(check_disk_size);
> +
>   /**
>    * revalidate_disk - wrapper for lower-level driver's revalidate_disk call-back
>    * @disk: struct gendisk to be revalidated
> @@ -1501,18 +1516,8 @@ int revalidate_disk(struct gendisk *disk)
>   	 * Hidden disks don't have associated bdev so there's no point in
>   	 * revalidating it.
>   	 */
> -	if (!(disk->flags & GENHD_FL_HIDDEN)) {
> -		struct block_device *bdev = bdget_disk(disk, 0);
> -
> -		if (!bdev)
> -			return ret;
> -
> -		mutex_lock(&bdev->bd_mutex);
> -		check_disk_size_change(disk, bdev, ret == 0);
> -		bdev->bd_invalidated = 0;
> -		mutex_unlock(&bdev->bd_mutex);
> -		bdput(bdev);
> -	}
> +	if (!(disk->flags & GENHD_FL_HIDDEN))
> +		check_disk_size(disk, ret == 0);
>   	return ret;
>   }
>   EXPORT_SYMBOL(revalidate_disk);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 997a530ff4e9..6649ad157317 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2679,6 +2679,7 @@ extern bool is_bad_inode(struct inode *);
>   extern void check_disk_size_change(struct gendisk *disk,
>   		struct block_device *bdev, bool verbose);
>   extern int revalidate_disk(struct gendisk *);
> +extern void check_disk_size(struct gendisk *disk, bool verbose);
>   extern int check_disk_change(struct block_device *);
>   extern int __invalidate_device(struct block_device *, bool);
>   extern int invalidate_partition(struct gendisk *, int);
>


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v9 7/9] nvme: pass status to nvme_error_status

[Chaitanya Kulkarni]

Do you find any issue with passing request structure ? or it is
more of a normal cleanup ?

On 08/29/2019 03:08 PM, Sagi Grimberg wrote:
> No need for the full blown request structure.
>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/core.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 0916c0e8d572..8f0881dec907 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -197,9 +197,9 @@ static inline bool nvme_ns_has_pi(struct nvme_ns *ns)
>   	return ns->pi_type && ns->ms == sizeof(struct t10_pi_tuple);
>   }
>
> -static blk_status_t nvme_error_status(struct request *req)
> +static blk_status_t nvme_error_status(u16 status)
>   {
> -	switch (nvme_req(req)->status & 0x7ff) {
> +	switch (status & 0x7ff) {
>   	case NVME_SC_SUCCESS:
>   		return BLK_STS_OK;
>   	case NVME_SC_CAP_EXCEEDED:
> @@ -262,7 +262,7 @@ static void nvme_retry_req(struct request *req)
>
>   void nvme_complete_rq(struct request *req)
>   {
> -	blk_status_t status = nvme_error_status(req);
> +	blk_status_t status = nvme_error_status(nvme_req(req)->status);
>
>   	trace_nvme_complete_rq(req);
>
>


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v9 7/9] nvme: pass status to nvme_error_status

[Sagi Grimberg]

> Do you find any issue with passing request structure ? or it is
> more of a normal cleanup ?

Its used later on...

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v9 6/9] block_dev: split disk size check out of revalidate_disk

[Hannes Reinecke]

On 8/30/19 12:06 AM, Sagi Grimberg wrote:
> Allow callers to call separately the ->revalidate_disk()
> callout and the disk size change check separately. This
> will help the caller to take action based on the return
> status of ->revalidate_disk() and run the disk size check
> afterwards.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>  fs/block_dev.c     | 29 +++++++++++++++++------------
>  include/linux/fs.h |  1 +
>  2 files changed, 18 insertions(+), 12 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v9 7/9] nvme: pass status to nvme_error_status

[Hannes Reinecke]

On 8/30/19 12:06 AM, Sagi Grimberg wrote:
> No need for the full blown request structure.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>  drivers/nvme/host/core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 0916c0e8d572..8f0881dec907 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -197,9 +197,9 @@ static inline bool nvme_ns_has_pi(struct nvme_ns *ns)
>  	return ns->pi_type && ns->ms == sizeof(struct t10_pi_tuple);
>  }
>  
> -static blk_status_t nvme_error_status(struct request *req)
> +static blk_status_t nvme_error_status(u16 status)
>  {
> -	switch (nvme_req(req)->status & 0x7ff) {
> +	switch (status & 0x7ff) {
>  	case NVME_SC_SUCCESS:
>  		return BLK_STS_OK;
>  	case NVME_SC_CAP_EXCEEDED:
> @@ -262,7 +262,7 @@ static void nvme_retry_req(struct request *req)
>  
>  void nvme_complete_rq(struct request *req)
>  {
> -	blk_status_t status = nvme_error_status(req);
> +	blk_status_t status = nvme_error_status(nvme_req(req)->status);
>  
>  	trace_nvme_complete_rq(req);
>  
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme