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

[Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>]

On 3/10/21 21:08, Damien Le Moal wrote:
> On 2021/03/11 13:39, Chaitanya Kulkarni wrote:
>> 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 <chaitanya.kulkarni@wdc.com>
>> ---
>>  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.

>>  
>> +	/*
>> +	 * 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.

>
>>  	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.
>>  	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.

>>  	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