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

[John Garry]

> > +
> >   
> >   What:		/sys/block/<disk>/diskseq
> >   Date:		February 2021
> > diff --git a/block/blk-settings.c b/block/blk-settings.c
> > index 0046b447268f..d151be394c98 100644
> > --- a/block/blk-settings.c
> > +++ b/block/blk-settings.c
> > @@ -59,6 +59,10 @@ void blk_set_default_limits(struct queue_limits *lim)
> >   	lim->zoned = BLK_ZONED_NONE;
> >   	lim->zone_write_granularity = 0;
> >   	lim->dma_alignment = 511;
> > +	lim->atomic_write_unit_min_sectors = 0;
> > +	lim->atomic_write_unit_max_sectors = 0;
> > +	lim->atomic_write_max_sectors = 0;
> > +	lim->atomic_write_boundary_sectors = 0;
>
> Can we move the four into single structure

There is no precedent for a similar structure in struct queue_limits. So 
would only passing a structure to the blk-settings.c API be ok?

> and setup them in single
> API? Then cross-validation can be done in this API.

I suppose so, if you think that it is better.

We rely on the driver to provide sound values. I suppose that we can 
sanitize them also (in a single API).

> >   }
> >   
> >   /**
> > @@ -183,6 +187,62 @@ void blk_queue_max_discard_sectors(struct request_queue *q,
> >   }
> >   EXPORT_SYMBOL(blk_queue_max_discard_sectors);
> >   
> > +/**
> > + * blk_queue_atomic_write_max_bytes - set max bytes supported by
> > + * the device for atomic write operations.
> > + * @q:  the request queue for the device
> > + * @size: maximum bytes supported
> > + */
> > +void blk_queue_atomic_write_max_bytes(struct request_queue *q,
> > +				      unsigned int bytes)
> > +{
> > +	q->limits.atomic_write_max_sectors = bytes >> SECTOR_SHIFT;
> > +}
> > +EXPORT_SYMBOL(blk_queue_atomic_write_max_bytes);
>
> What if driver doesn't call it but driver supports atomic write?

We rely on the driver to do this. Any basic level of testing will show 
an issue if they don't.

> I guess the default max sectors should be atomic_write_unit_max_sectors
> if the feature is enabled.

Sure. If we have a single API to set all values, then we don't need to 
worry about this (assuming the values are filled in properly).

> > +
> > +/**
> > + * blk_queue_atomic_write_boundary_bytes - Device's logical block address space
> > + * which an atomic write should not cross.
> > + * @q:  the request queue for the device
> > + * @bytes: must be a power-of-two.
> > + */
> > +void blk_queue_atomic_write_boundary_bytes(struct request_queue *q,
> > +					   unsigned int bytes)
> > +{
> > +	q->limits.atomic_write_boundary_sectors = bytes >> SECTOR_SHIFT;
> > +}
> > +EXPORT_SYMBOL(blk_queue_atomic_write_boundary_bytes);
>
> Default atomic_write_boundary_sectors should be
> atomic_write_unit_max_sectors in case of atomic write?

Having atomic_write_boundary_sectors default to 
atomic_write_unit_max_sectors is effectively same as a default of 0.

> > +
> > +/**
> > + * blk_queue_atomic_write_unit_min_sectors - smallest 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_min_sectors(struct request_queue *q,
> > +					     unsigned int sectors)
> > +{
> > +	struct queue_limits *limits = &q->limits;
> > +
> > +	limits->atomic_write_unit_min_sectors = sectors;
> > +}
> > +EXPORT_SYMBOL(blk_queue_atomic_write_unit_min_sectors);
>
> atomic_write_unit_min_sectors should be >= (physical block size >> 9)
> given the minimized atomic write unit is physical sector for all disk.

For SCSI, we have a granularity VPD value, and when set we pay attention 
to that. If not, we use the phys block size.

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.

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.

Thanks,
John