Re: [PATCH 1/1] virtio-blk: Add description for blk_size field

[Max Gurtovoy <mgurtovoy@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 9080 bytes --]

Hi Daniel,

On 25/09/2024 23:57, Daniel Verkamp wrote:
> On Wed, Sep 25, 2024 at 8:05 AM Max Gurtovoy<mgurtovoy@nvidia.com> wrote:
>> This field is only valid when the VIRTIO_BLK_F_BLK_SIZE feature bit is
>> set.
>>
>> The blk_size field represents the smallest addressable unit of data that
>> can be read from or written to the device. It is always a power of two
>> and typically ranges from 512 bytes to larger values such as 4 KB.
> I agree that this is what the blk_size field probably *should* have
> meant (matching other storage protocols like ATA, SCSI, NVMe, ...),
> but I believe rewriting the spec like this changes the meaning of the
> blk_size field in an incompatible way.

I believe that this is more of a clarification than a re-write.

>
> Previously, blk_size was described as the "optimal" sector size, so it
> seems clear that the driver is still allowed to send requests that are
> not aligned to blk_size ("awareness of the correct value can affect
> performance" but no mention of correctness). A device may need to do
> extra work to support this, of course, such as turning an unaligned
> write into a read-modify-write sequence if the underlying storage
> device doesn't support such an access directly, but such requests are
> still valid. It's also perfectly fine for a driver to ignore
> device-offered features like VIRTIO_BLK_F_BLK_SIZE and still operate
> correctly.

A device should report its block size (logical) using the 
VIRTIO_BLK_F_BLK_SIZE feature. For legacy devices that don't report this 
feature, Linux assumes a 512-byte block size.

Drivers should not ignore this feature when it's offered by the device. 
It's crucial for correct operation IMO.

The statement "driver is still allowed to send requests that are not 
aligned to blk_size" needs explicit clarification in the spec.

 From my understanding of the spec, the driver is not allowed to do so 
(otherwise error will be returned or the behavior is undefined).

In Linux, the reported blk_size is used to set logical_block_size, while 
physical_block_exp is used to calculate the physical_block_size.

Regarding the specification statement: "This does not affect the units 
used in the protocol (always 512 bytes)":

This likely refers to how certain fields in the virtio-blk protocol are 
expressed, not the actual logical block size used for I/O operations. 
For example: capacity reporting, max_discard_sectors and Other fields 
that are expressed in 512 byte units.

>> Linux/Windows systems typically use 512-byte/4-KB block sizes.
> While probably true, this doesn't really have anything to do with the
> change (and the logical block size is a property of the storage medium
> and controller, not anything to do with the operating system).

Please ignore it. I'll remove this from v2.

>
>> This description provides clarity on the constraints of the blk_size
>> field.
>>
>> Signed-off-by: Max Gurtovoy<mgurtovoy@nvidia.com>
>> ---
>>   device-types/blk/description.tex | 20 ++++++++++++++++++--
>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/device-types/blk/description.tex b/device-types/blk/description.tex
>> index 2712ada..88a7591 100644
>> --- a/device-types/blk/description.tex
>> +++ b/device-types/blk/description.tex
>> @@ -3,7 +3,10 @@ \section{Block Device}\label{sec:Device Types / Block Device}
>>   The virtio block device is a simple virtual block device (ie.
>>   disk). Read and write requests (and other exotic requests) are
>>   placed in one of its queues, and serviced (probably out of order) by the
>> -device except where noted.
>> +device except where noted. Each block device consists of a sequence of logical
>> +blocks. A logical block represents the smallest addressable unit of data that
>> +can be read from or written to the device. The logical block size is always a
>> +power of two. Logical block sizes may be 512 bytes, 1KB, 2KB, 4KB, 8KB, etc.
> Does this imply that a device may return an error for a request where
> the sector or num_sectors fields are not aligned to the blk_size? If
> so, it should be called out as a more explicit normative requirement,
> but that would definitely be an incompatible change.

As mentioned, the logical block size, such as 512 bytes, defines the 
smallest addressable unit for I/O operations on the storage device.
This has important implications for I/O requests:
1. Minimum I/O size: You cannot send I/O requests smaller than the 
logical block size. For example, with a 512-byte logical block size, 
requests of 128 bytes are not valid.
2. I/O alignment: All I/O requests must be aligned to the logical block 
size. This means the target start address (on the media device) of the 
I/O must be a multiple of the logical block size (the expressed units in 
the spec are not relevant).
3. I/O size granularity: The size of I/O requests must be a multiple of 
the logical block size. For a 512-byte logical block size, valid I/O 
sizes would be 512 bytes, 1024 bytes, 1536 bytes, and so on.

>
> The "smallest addressable unit" wording is also more confusing than
> clarifying here, since in virtio-blk, the "smallest addressable unit"
> is always 512 bytes, regardless of blk_size (even if a change like
> this would enforce limits on the sector values that would actually be
> allowed, it would still be possible to create a request that refers to
> a smaller unit than blk_size, unlike other storage protocols where
> logical block size changes the multiplier of the address, making it
> impossible to address smaller units).

Can you please point me to the place in spec that explicitly say that 
"in virtio-blk, the smallest addressable unit is always 512 bytes, 
regardless of blk_size" ?

I didn't find it in the spec and I don't agree with this call if it is 
implicit.

The implementation of the Linux kernel driver isn't implemented in this 
manner as well.

>
>>   \subsection{Device ID}\label{sec:Device Types / Block Device / Device ID}
>>     2
>> @@ -135,6 +138,9 @@ \subsection{Device configuration layout}\label{sec:Device Types / Block Device /
>>   present. The availability of the others all depend on various feature
>>   bits as indicated above.
>>
>> +The field \field{blk_size} exists only if VIRTIO_BLK_F_BLK_SIZE is set. This field
>> +reports the logical block size of the device, expressed in bytes.
> Not a new problem with this change (see another example right below),
> but "set" is unclear - does that mean it is present if the device
> offered the feature, or only if the driver also accepted it?

I just followed the spec wording.

I meant to say "The field \field{blk_size} exists only if 
VIRTIO_BLK_F_BLK_SIZE is offered by the device.

I can change it in v2.

>
>>   The field \field{num_queues} only exists if VIRTIO_BLK_F_MQ is set. This field specifies
>>   the number of queues.
>>
>> @@ -228,7 +234,7 @@ \subsection{Device Initialization}\label{sec:Device Types / Block Device / Devic
>>   \item The device size can be read from \field{capacity}.
>>
>>   \item If the VIRTIO_BLK_F_BLK_SIZE feature is negotiated,
>> -  \field{blk_size} can be read to determine the optimal sector size
>> +  \field{blk_size} can be read to determine the logical block size
>>     for the driver to use. This does not affect the units used in
>>     the protocol (always 512 bytes), but awareness of the correct
>>     value can affect performance.
>> @@ -282,6 +288,12 @@ \subsection{Device Initialization}\label{sec:Device Types / Block Device / Devic
>>
>>   \drivernormative{\subsubsection}{Device Initialization}{Device Types / Block Device / Device Initialization}
>>
>> +Drivers MUST negotiate VIRTIO_BLK_F_BLK_SIZE if the feature is offered by the
>> +device.
> Why is this a MUST? This is also an incompatible change since there
> was no such requirement before, and drivers are free to ignore
> features they don't care about.

I can say SHOULD, but then the driver that is ignoring this feature may 
guess/assume the logical block size and it can be wrong.

This may lead to an undefined behavior.

We do it for legacy devices that don't offer this feature, because this 
is the best we can do.

>
>> +If the VIRTIO_BLK_F_BLK_SIZE feature is not offered by the device, then drivers
>> +MAY assume that the logical block size is 512 bytes.
>> +
>>   Drivers SHOULD NOT negotiate VIRTIO_BLK_F_FLUSH if they are incapable of
>>   sending VIRTIO_BLK_T_FLUSH commands.
>>
> [...]
>
> In my opinion, this change should probably be something more like a
> non-normative comment, along the lines of "blk_size is often the
> logical block size of the storage medium", without changing the
> existing meaning from previous spec versions. If there is a desire to
> introduce a hard requirement for logical-block-aligned I/O requests,
> that seems like it would have to be a new field and/or feature bit.

I believe we can use the existing VIRTIO_BLK_F_BLK_SIZE feature.

This patch makes the relationship between blk_size field and the 
smallest addressable unit (logical block) for READ/WRITE operations clear.

>
> Thanks,
> -- Daniel

[-- Attachment #2: Type: text/html, Size: 12581 bytes --]