Re: [PATCH v2 01/16] block: Add atomic write operations to request_queue limits

[John Garry]

On 13/12/2023 12:28, Ming Lei wrote:
> > For NVMe, we use the logical block size. For physical block size, that can
> > be greater than the logical block size for npwg set, and I don't think it's
> > suitable use that as minimum atomic write unit.
> I highly suspect it is wrong to use logical block size as minimum
> support atomic write unit, given physical block size is supposed to
> be the minimum atomic write unit.

I would tend to agree, but I am still a bit curious on how npwg is used 
to calculate atomic_bs/phys_bs as it seems to be a recommended 
performance-related value. It would hint to me that it is the phys_bs, 
but is that same as atomic min granularity?

> > Anyway, I am not too keen on sanitizing this value in this way.
> >
> > > > +
> > > > +/*
> > > > + * blk_queue_atomic_write_unit_max_sectors - largest unit that can be written
> > > > + * atomically to the device.
> > > > + * @q: the request queue for the device
> > > > + * @sectors: must be a power-of-two.
> > > > + */
> > > > +void blk_queue_atomic_write_unit_max_sectors(struct request_queue *q,
> > > > +					     unsigned int sectors)
> > > > +{
> > > > +	struct queue_limits *limits = &q->limits;
> > > > +
> > > > +	limits->atomic_write_unit_max_sectors = sectors;
> > > > +}
> > > > +EXPORT_SYMBOL(blk_queue_atomic_write_unit_max_sectors);
> > > atomic_write_unit_max_sectors should be >= atomic_write_unit_min_sectors.
> > Again, we rely on the driver to provide sound values. However, as mentioned,
> > we can sanitize.
> Relying on driver to provide sound value is absolutely bad design from API
> viewpoint.

OK, fine, I'll look to revise the API to make it more robust.

Thanks,
John