Re: [PATCH 3/4] nvme: fix max_discard_sectors calculation

[Keith Busch <kbusch@kernel.org>]

On Tue, Dec 26, 2023 at 08:58:43AM +0000, Christoph Hellwig wrote:
> ctrl->max_discard_sectors stores a value that is potentially based of
> the DMRSL field in Identify Controller, which is in units of LBAs and
> thus dependent on the Format of a namespace.

Is it just me, or is this one of the more annoying conventions in NVMe
standard: controller scoped identification fields are in units of
namespace specific formats?! Why not define these in namespace
identifications, or provide a fixed unit size? As it is now, these types
of fields would be in units of the largest LBA size of any attached
namespace, and that's kind of awkward in a multi-namespace environment.
 
> @@ -1750,7 +1751,7 @@ static void nvme_config_discard(struct nvme_ctrl *ctrl, struct gendisk *disk,
>  	if (queue->limits.max_discard_sectors)
>  		return;

Since DMRSL can change when you format a namespace to a new LBA size,
might the old limit be using the wrong max_discard_size if we skip
updating it here? Probably not a big deal.

And let's say the user explicitly disabled max_discard_sectors through
sysfs. The next driver namespace rescan will turn it back on, probably
against the user's expectations. Should this limit check use
'max_hw_discard_sectors' instead?

Anyway, the driver was like that already, and the series looks good to
me. I just noticed this weirdness while reviewing.

> -	blk_queue_max_discard_sectors(queue, ctrl->max_discard_sectors);
> +	blk_queue_max_discard_sectors(queue, max_discard_sectors);
>  	blk_queue_max_discard_segments(queue, ctrl->max_discard_segments);
>  	queue->limits.discard_granularity = queue_logical_block_size(queue);