Re: [PATCH] nvme: zns: limit max_zone_append by max_segments

[Damien Le Moal <dlemoal@kernel.org>]

On 7/31/23 22:46, Christoph Hellwig wrote:
> On Mon, Jul 31, 2023 at 08:46:32PM +0900, Shin'ichiro Kawasaki wrote:
>> To avoid the unexpected BIO split, reflect the max_segments limit to the
>> max_zone_append limit. In the worst case, the safe max_zone_append size
>> is max_segments multiplied by PAGE_SIZE. Compare it with the
>> max_zone_append size obtained from ZNS devices, and set the smaller
>> value as the max_zone_append limit.
> 
> This is true only for NVMe and a hand full of drivers that
> set the virt_boundary.  For others the maximum size is completely
> unrelated to the maximum number of segments.
> 
> Can you reword the commit log a bit to make that more clear?
> 
>> diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
>> index ec8557810c21..9ee77626c235 100644
>> --- a/drivers/nvme/host/zns.c
>> +++ b/drivers/nvme/host/zns.c
>> @@ -10,9 +10,11 @@
>>  int nvme_revalidate_zones(struct nvme_ns *ns)
>>  {
>>  	struct request_queue *q = ns->queue;
>> +	unsigned int max_sectors = queue_max_segments(q) << PAGE_SECTORS_SHIFT;
>>  
>>  	blk_queue_chunk_sectors(q, ns->zsze);
>> -	blk_queue_max_zone_append_sectors(q, ns->ctrl->max_zone_append);
>> +	max_sectors = min(max_sectors, ns->ctrl->max_zone_append);
>> +	blk_queue_max_zone_append_sectors(q, max_sectors);
> 
> And while this looks correct, it also feels pretty ugly.  Shouldn't a
> 
> 	blk_queue_max_zone_append_sectors(q,
> 		queue_max_sectors(q), ns->ctrl->max_zone_append);

blk_queue_max_zone_append_sectors() already does cap max zone append to
max_hw_sectors. We could also add the cap to queue_max_segments(q) <<
PAGE_SECTORS_SHIFT in that function as well given that all zoned devices should
have that anyway...

> 
> do the same thing in a somewhat more obvious way given that max_segments
> is already taken into account for the queue_max_sectors calculation?

-- 
Damien Le Moal
Western Digital Research