Re: [PATCH V11 4/4] nvme: add comments to nvme_zns_alloc_report_buffer

On 3/10/21 21:08, Damien Le Moal wrote:

On 2021/03/11 13:39, Chaitanya Kulkarni wrote:

quoted

The report zone buffer calculation is dependent nvme report zones
header, nvme report zone descriptor and on the various block
layer request queue attributes such as queue_max_hw_sectors(),
queue_max_segments(). These queue_XXX attributes are calculated on
different ctrl values in the nvme-core.

Add clear comments about what values we are using and how they are
calculated based on the controller's attributes.

This is needed since when referencing the code after long time it is not
straight forward to understand how we calculate the buffer size given
that there are variables and ctrl attributes involved.

Signed-off-by: Chaitanya Kulkarni <redacted>
---
 drivers/nvme/host/zns.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
index bc2f344f0ae0..c2d08d9cc269 100644
--- a/drivers/nvme/host/zns.c
+++ b/drivers/nvme/host/zns.c

@@ -125,16 +125,38 @@ static void *nvme_zns_alloc_report_buffer(struct nvme_ns *ns,
 	size_t bufsize;
 	void *buf;
 
+	/*
+	 * Set the minimum buffer size for report zone header and one zone
+	 * descriptor.
+	 */
 	const size_t min_bufsize = sizeof(struct nvme_zone_report) +
 				   sizeof(struct nvme_zone_descriptor);

This seems unused. And not really useful anyway since if this function is being
called, it should be clear already that at least one zone can be reported, that
is, the report start sector is below capacity.

Okay will remove the comment V12.

quoted

 
+	/*
+	 * Recalculate the number of zones based on disk size of zone size.
+	 */
 	nr_zones = min_t(unsigned int, nr_zones,
 			 get_capacity(ns->disk) >> ilog2(ns->zsze));
 
+	/*
+	 * Calculate the buffer size based on the report zone header and number
+	 * of zone descriptors are required for each zone.
+	 */

This comment is not really useful.

Okay will remove it in V12.

quoted

 	bufsize = sizeof(struct nvme_zone_report) +
 		nr_zones * sizeof(struct nvme_zone_descriptor);
+
+	/*
+	 * Recalculate and Limit the buffer size to queue max hw sectors. For
+	 * NVMe queue max hw sectors are calcualted based on controller's
+	 * Maximum Data Transfer Size (MDTS).
+	 */

Will remove the above comment.

What combining this comment and the next into:

	/*
	 * Limit the buffer size to the maximum data transfer size and on
	 * the maximum number of segments allowed.
	 */

Will use the above comment.

Simpler in my opinion.

quoted

 	bufsize = min_t(size_t, bufsize,
 			queue_max_hw_sectors(q) << SECTOR_SHIFT);
+	/*
+	 * Recalculate and Limit the buffer size to queue max segments. For
+	 * NVMe queue max segments are calculated based on how many controller
+	 * pages are needed to fit the max hw sectors.
+	 */

Will remove above comment.

quoted

 	bufsize = min_t(size_t, bufsize, queue_max_segments(q) << PAGE_SHIFT);
 
 	while (bufsize >= min_bufsize) {

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