[v6,1/7] block: Introduce the flag QUEUE_FLAG_NO_ZONE_WRITE_LOCK

Writes in sequential write required zones must happen at the write
pointer. Even if the submitter of the write commands (e.g. a filesystem)
submits writes for sequential write required zones in order, the block
layer or the storage controller may reorder these write commands.

The zone locking mechanism in the mq-deadline I/O scheduler serializes
write commands for sequential zones. Some but not all storage controllers
require this serialization. Introduce a new request queue flag to allow
block drivers to indicate that they preserve the order of write commands
and thus do not require serialization of writes per zone.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/linux/blkdev.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

On 8/4/23 9:47?AM, Bart Van Assche wrote:
> Writes in sequential write required zones must happen at the write
> pointer. Even if the submitter of the write commands (e.g. a filesystem)
> submits writes for sequential write required zones in order, the block
> layer or the storage controller may reorder these write commands.
> 
> The zone locking mechanism in the mq-deadline I/O scheduler serializes
> write commands for sequential zones. Some but not all storage controllers
> require this serialization. Introduce a new request queue flag to allow
> block drivers to indicate that they preserve the order of write commands
> and thus do not require serialization of writes per zone.

Looking at how this is used, why not call it QUEUE_FLAG_ZONE_WRITE_LOCK
instead? That'd make the code easier to immediately grok, rather than
deal with double negations.

On 8/8/23 14:19, Jens Axboe wrote:
> On 8/4/23 9:47?AM, Bart Van Assche wrote:
>> Writes in sequential write required zones must happen at the write
>> pointer. Even if the submitter of the write commands (e.g. a filesystem)
>> submits writes for sequential write required zones in order, the block
>> layer or the storage controller may reorder these write commands.
>>
>> The zone locking mechanism in the mq-deadline I/O scheduler serializes
>> write commands for sequential zones. Some but not all storage controllers
>> require this serialization. Introduce a new request queue flag to allow
>> block drivers to indicate that they preserve the order of write commands
>> and thus do not require serialization of writes per zone.
> 
> Looking at how this is used, why not call it QUEUE_FLAG_ZONE_WRITE_LOCK
> instead? That'd make the code easier to immediately grok, rather than
> deal with double negations.

Hi Jens,

Do I understand correctly that you want me to set the
QUEUE_FLAG_ZONE_WRITE_LOCK flag for all request queues by adding it to
QUEUE_FLAG_MQ_DEFAULT and also that the UFS driver should clear the
QUEUE_FLAG_ZONE_WRITE_LOCK flag?

Thanks,

Bart.

On 8/8/23 3:46?PM, Bart Van Assche wrote:
> On 8/8/23 14:19, Jens Axboe wrote:
>> On 8/4/23 9:47?AM, Bart Van Assche wrote:
>>> Writes in sequential write required zones must happen at the write
>>> pointer. Even if the submitter of the write commands (e.g. a filesystem)
>>> submits writes for sequential write required zones in order, the block
>>> layer or the storage controller may reorder these write commands.
>>>
>>> The zone locking mechanism in the mq-deadline I/O scheduler serializes
>>> write commands for sequential zones. Some but not all storage controllers
>>> require this serialization. Introduce a new request queue flag to allow
>>> block drivers to indicate that they preserve the order of write commands
>>> and thus do not require serialization of writes per zone.
>>
>> Looking at how this is used, why not call it QUEUE_FLAG_ZONE_WRITE_LOCK
>> instead? That'd make the code easier to immediately grok, rather than
>> deal with double negations.
> 
> Hi Jens,
> 
> Do I understand correctly that you want me to set the
> QUEUE_FLAG_ZONE_WRITE_LOCK flag for all request queues by adding it to
> QUEUE_FLAG_MQ_DEFAULT and also that the UFS driver should clear the
> QUEUE_FLAG_ZONE_WRITE_LOCK flag?

I don't think setting that flag by default makes a lot of sense, if the
device in question isn't zoned. Maybe have variants of BLK_ZONED_* which
has a locked and unlocked variant for each where it applies? Perhaps
have the lock flag be common between them so you can check them in the
same way? That'd keep the fact that it's zoned and if it needs locking
in the same spot, rather than scatter them in two spots.

On 8/8/23 15:27, Jens Axboe wrote:
> I don't think setting that flag by default makes a lot of sense, if the
> device in question isn't zoned. Maybe have variants of BLK_ZONED_* which
> has a locked and unlocked variant for each where it applies? Perhaps
> have the lock flag be common between them so you can check them in the
> same way? That'd keep the fact that it's zoned and if it needs locking
> in the same spot, rather than scatter them in two spots.

Hi Jens,

That's an interesting suggestion but there is a complication: the zone type
is set by different code than the code that decides whether or not locking
is required. For SCSI devices the zone type is set from inside
drivers/scsi/sd.c or drivers/scsi/sd_zbc.c while the locking requirements
come from the SCSI LLD (drivers/ufs/core/ufshcd.c). Additionally, the code
for converting between model type and model string in scsi_debug.c would
look weird if BLK_ZONED_* is split into locked and unlocked variants. See
also the zbc_model_strs_*[] arrays and the sdeb_zbc_model_str() function.

It should be possible to move the flag that indicates whether or not zone
locking is required into struct queue_limits next to "enum blk_zoned_model
zoned;". If nobody objects I will select this alternative.

Thanks,

Bart.