[3/4] nvme: Add consistency check for zone count
diff mbox series

Message ID 20200702065438.46350-4-javier@javigon.com
State New
Headers show
Series
  • ZNS: Extra features for current patche
Related show

Commit Message

Javier González July 2, 2020, 6:54 a.m. UTC
From: Javier González <javier.gonz@samsung.com>

Since the number of zones is calculated through the reported device
capacity and the ZNS specification allows to report the total number of
zones in the device, add an extra check to guarantee consistency between
the device and the kernel.

Also, fix a checkpatch warning: unsigned -> unsigned int in the process

Signed-off-by: Javier González <javier.gonz@samsung.com>
Signed-off-by: SelvaKumar S <selvakuma.s1@samsung.com>
Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
---
 drivers/nvme/host/core.c |  2 +-
 drivers/nvme/host/nvme.h |  6 ++++--
 drivers/nvme/host/zns.c  | 39 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 43 insertions(+), 4 deletions(-)

Comments

Damien Le Moal July 2, 2020, 8:16 a.m. UTC | #1
On 2020/07/02 15:55, Javier González wrote:
> From: Javier González <javier.gonz@samsung.com>
> 
> Since the number of zones is calculated through the reported device
> capacity and the ZNS specification allows to report the total number of
> zones in the device, add an extra check to guarantee consistency between
> the device and the kernel.
> 
> Also, fix a checkpatch warning: unsigned -> unsigned int in the process
> 
> Signed-off-by: Javier González <javier.gonz@samsung.com>
> Signed-off-by: SelvaKumar S <selvakuma.s1@samsung.com>
> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
> ---
>  drivers/nvme/host/core.c |  2 +-
>  drivers/nvme/host/nvme.h |  6 ++++--
>  drivers/nvme/host/zns.c  | 39 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 1f5c7fc3d2c9..8b9c69172931 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1994,7 +1994,7 @@ static int __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
>  	case NVME_CSI_NVM:
>  		break;
>  	case NVME_CSI_ZNS:
> -		ret = nvme_update_zone_info(disk, ns, lbaf);
> +		ret = nvme_update_zone_info(disk, ns, lbaf, le64_to_cpu(id->nsze));
>  		if (ret)
>  			return ret;
>  		break;
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 662f95fbd909..ef80e0b4df56 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -407,6 +407,7 @@ struct nvme_ns {
>  	u32 sws;
>  	u8 pi_type;
>  #ifdef CONFIG_BLK_DEV_ZONED
> +	u64 nr_zones;
>  	u64 zsze;
>  #endif
>  	unsigned long features;
> @@ -700,7 +701,7 @@ static inline void nvme_mpath_start_freeze(struct nvme_subsystem *subsys)
>  
>  #ifdef CONFIG_BLK_DEV_ZONED
>  int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
> -			  unsigned lbaf);
> +			  unsigned int lbaf, sector_t nsze);
>  
>  int nvme_report_zones(struct gendisk *disk, sector_t sector,
>  		      unsigned int nr_zones, report_zones_cb cb, void *data);
> @@ -720,7 +721,8 @@ static inline blk_status_t nvme_setup_zone_mgmt_send(struct nvme_ns *ns,
>  
>  static inline int nvme_update_zone_info(struct gendisk *disk,
>  					struct nvme_ns *ns,
> -					unsigned lbaf)
> +					unsigned lbaf

missing a comma here. Does this even compiles ?

> +					sector_t nsze)
>  {
>  	dev_warn(ns->ctrl->device,
>  		 "Please enable CONFIG_BLK_DEV_ZONED to support ZNS devices\n");
> diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
> index b34d2ed13825..d785d179a343 100644
> --- a/drivers/nvme/host/zns.c
> +++ b/drivers/nvme/host/zns.c
> @@ -32,13 +32,36 @@ static int nvme_set_max_append(struct nvme_ctrl *ctrl)
>  	return 0;
>  }
>  
> +static u64 nvme_zns_nr_zones(struct nvme_ns *ns)
> +{
> +	struct nvme_command c = { };
> +	struct nvme_zone_report report;
> +	int buflen = sizeof(struct nvme_zone_report);
> +	int ret;
> +
> +	c.zmr.opcode = nvme_cmd_zone_mgmt_recv;
> +	c.zmr.nsid = cpu_to_le32(ns->head->ns_id);
> +	c.zmr.slba = cpu_to_le64(0);
> +	c.zmr.numd = cpu_to_le32(nvme_bytes_to_numd(buflen));
> +	c.zmr.zra = NVME_ZRA_ZONE_REPORT;
> +	c.zmr.zrasf = NVME_ZRASF_ZONE_REPORT_ALL;
> +	c.zmr.pr = 0;
> +
> +	ret = nvme_submit_sync_cmd(ns->queue, &c, &report, buflen);
> +	if (ret)
> +		return ret;
> +
> +	return le64_to_cpu(report.nr_zones);
> +}
> +
>  int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
> -			  unsigned lbaf)
> +			  unsigned int lbaf, sector_t nsze)
>  {
>  //	struct nvme_effects_log *log = ns->head->effects;
>  	struct request_queue *q = disk->queue;
>  	struct nvme_command c = { };
>  	struct nvme_id_ns_zns *id;
> +	sector_t cap_nr_zones;
>  	int status;
>  
>  	/* Driver requires zone append support */
> @@ -80,6 +103,20 @@ int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
>  		goto free_data;
>  	}
>  
> +	ns->nr_zones = nvme_zns_nr_zones(ns);
> +	if (!ns->nr_zones) {
> +		status = -EINVAL;
> +		goto free_data;
> +	}
> +
> +	cap_nr_zones = nvme_lba_to_sect(ns, le64_to_cpu(nsze)) >> ilog2(ns->zsze);
> +	if (ns->nr_zones != cap_nr_zones) {
> +		dev_err(ns->ctrl->device, "inconsistent zone count: %llu/%llu\n",
> +			ns->nr_zones, cap_nr_zones);
> +		status = -EINVAL;
> +		goto free_data;
> +	}
> +

I am not opposed to this, but I am not sure if this adds anything to the checks
in blk_revalidate_disk_zones() as that does a full zone report that is checked
against capacity. This seems more like a test of the drive conformance, checking
that the report header nr_zones is correct...

>  	q->limits.zoned = BLK_ZONED_HM;
>  	q->zone_flags = BLK_ZONE_REP_CAPACITY | BLK_ZONE_REP_OFFLINE;
>  	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
>
Johannes Thumshirn July 2, 2020, 8:19 a.m. UTC | #2
On 02/07/2020 08:55, Javier González wrote:
>  int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
> -			  unsigned lbaf)
> +			  unsigned int lbaf, sector_t nsze)
>  {
>  //	struct nvme_effects_log *log = ns->head->effects;
>  	struct request_queue *q = disk->queue;


Please no C++ style comments and no commenting out of code in an official submission.
Javier González July 2, 2020, 8:27 a.m. UTC | #3
On 02.07.2020 08:19, Johannes Thumshirn wrote:
>On 02/07/2020 08:55, Javier González wrote:
>>  int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
>> -			  unsigned lbaf)
>> +			  unsigned int lbaf, sector_t nsze)
>>  {
>>  //	struct nvme_effects_log *log = ns->head->effects;
>>  	struct request_queue *q = disk->queue;
>
>
>Please no C++ style comments and no commenting out of code in an official submission.

Sorry. I see some debug code leaked here.

Javier

Patch
diff mbox series

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1f5c7fc3d2c9..8b9c69172931 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1994,7 +1994,7 @@  static int __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
 	case NVME_CSI_NVM:
 		break;
 	case NVME_CSI_ZNS:
-		ret = nvme_update_zone_info(disk, ns, lbaf);
+		ret = nvme_update_zone_info(disk, ns, lbaf, le64_to_cpu(id->nsze));
 		if (ret)
 			return ret;
 		break;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 662f95fbd909..ef80e0b4df56 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -407,6 +407,7 @@  struct nvme_ns {
 	u32 sws;
 	u8 pi_type;
 #ifdef CONFIG_BLK_DEV_ZONED
+	u64 nr_zones;
 	u64 zsze;
 #endif
 	unsigned long features;
@@ -700,7 +701,7 @@  static inline void nvme_mpath_start_freeze(struct nvme_subsystem *subsys)
 
 #ifdef CONFIG_BLK_DEV_ZONED
 int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
-			  unsigned lbaf);
+			  unsigned int lbaf, sector_t nsze);
 
 int nvme_report_zones(struct gendisk *disk, sector_t sector,
 		      unsigned int nr_zones, report_zones_cb cb, void *data);
@@ -720,7 +721,8 @@  static inline blk_status_t nvme_setup_zone_mgmt_send(struct nvme_ns *ns,
 
 static inline int nvme_update_zone_info(struct gendisk *disk,
 					struct nvme_ns *ns,
-					unsigned lbaf)
+					unsigned lbaf
+					sector_t nsze)
 {
 	dev_warn(ns->ctrl->device,
 		 "Please enable CONFIG_BLK_DEV_ZONED to support ZNS devices\n");
diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
index b34d2ed13825..d785d179a343 100644
--- a/drivers/nvme/host/zns.c
+++ b/drivers/nvme/host/zns.c
@@ -32,13 +32,36 @@  static int nvme_set_max_append(struct nvme_ctrl *ctrl)
 	return 0;
 }
 
+static u64 nvme_zns_nr_zones(struct nvme_ns *ns)
+{
+	struct nvme_command c = { };
+	struct nvme_zone_report report;
+	int buflen = sizeof(struct nvme_zone_report);
+	int ret;
+
+	c.zmr.opcode = nvme_cmd_zone_mgmt_recv;
+	c.zmr.nsid = cpu_to_le32(ns->head->ns_id);
+	c.zmr.slba = cpu_to_le64(0);
+	c.zmr.numd = cpu_to_le32(nvme_bytes_to_numd(buflen));
+	c.zmr.zra = NVME_ZRA_ZONE_REPORT;
+	c.zmr.zrasf = NVME_ZRASF_ZONE_REPORT_ALL;
+	c.zmr.pr = 0;
+
+	ret = nvme_submit_sync_cmd(ns->queue, &c, &report, buflen);
+	if (ret)
+		return ret;
+
+	return le64_to_cpu(report.nr_zones);
+}
+
 int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
-			  unsigned lbaf)
+			  unsigned int lbaf, sector_t nsze)
 {
 //	struct nvme_effects_log *log = ns->head->effects;
 	struct request_queue *q = disk->queue;
 	struct nvme_command c = { };
 	struct nvme_id_ns_zns *id;
+	sector_t cap_nr_zones;
 	int status;
 
 	/* Driver requires zone append support */
@@ -80,6 +103,20 @@  int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
 		goto free_data;
 	}
 
+	ns->nr_zones = nvme_zns_nr_zones(ns);
+	if (!ns->nr_zones) {
+		status = -EINVAL;
+		goto free_data;
+	}
+
+	cap_nr_zones = nvme_lba_to_sect(ns, le64_to_cpu(nsze)) >> ilog2(ns->zsze);
+	if (ns->nr_zones != cap_nr_zones) {
+		dev_err(ns->ctrl->device, "inconsistent zone count: %llu/%llu\n",
+			ns->nr_zones, cap_nr_zones);
+		status = -EINVAL;
+		goto free_data;
+	}
+
 	q->limits.zoned = BLK_ZONED_HM;
 	q->zone_flags = BLK_ZONE_REP_CAPACITY | BLK_ZONE_REP_OFFLINE;
 	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);