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

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

[Max Gurtovoy]

This field is only valid when the VIRTIO_BLK_F_BLK_SIZE feature bit is
offered by the device.

The blk_size field actually represents the logical block size of the
device. It is always a power of two and typically ranges from 512 bytes
to larger values such as 4 KB.

Add description for this field to provide clarity on its constraints.

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---

changes from V1:
 - Addressed Stefan's and Daniel's comments:
       1. use SHOULD instead of MUST
       2. Add a note that devices may return IOERR upon misaligned IO
 - Add a note that devices may return IOERR if IO size is not following
   the block size granularity.
---
 device-types/blk/description.tex | 34 ++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/device-types/blk/description.tex b/device-types/blk/description.tex
index 2712ada..77eed80 100644
--- a/device-types/blk/description.tex
+++ b/device-types/blk/description.tex
@@ -135,6 +135,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 offered by the device.
+This field reports the block size of the device, expressed in bytes.
+
 The field \field{num_queues} only exists if VIRTIO_BLK_F_MQ is set. This field specifies
 the number of queues.
 
@@ -282,6 +285,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Block Device / Devic
 
 \drivernormative{\subsubsection}{Device Initialization}{Device Types / Block Device / Device Initialization}
 
+Drivers SHOULD negotiate VIRTIO_BLK_F_BLK_SIZE if the feature is offered by the
+device. When negotiated, drivers SHOULD interpret the \field{blk_size} as the
+logical block size.
+
+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.
 
@@ -319,6 +329,10 @@ \subsection{Device Initialization}\label{sec:Device Types / Block Device / Devic
 
 \devicenormative{\subsubsection}{Device Initialization}{Device Types / Block Device / Device Initialization}
 
+Devices SHOULD always offer VIRTIO_BLK_F_BLK_SIZE feature. When this feature is
+offered, devices MUST initialize \field{blk_size} to a power of two greater
+than or equal to 512.
+
 Devices SHOULD always offer VIRTIO_BLK_F_FLUSH, and MUST offer it
 if they offer VIRTIO_BLK_F_CONFIG_WCE.
 
@@ -879,6 +893,14 @@ \subsection{Device Operation}\label{sec:Device Types / Block Device / Device Ope
 The length of \field{data} MUST be a multiple of 512 bytes for VIRTIO_BLK_T_IN
 and VIRTIO_BLK_T_OUT requests.
 
+The length of \field{data} SHOULD be a multiple of \field{blk_size} for VIRTIO_BLK_T_IN
+and VIRTIO_BLK_T_OUT requests, when the VIRTIO_BLK_F_BLK_SIZE feature has been
+offered by the device.
+
+The value of \field{sector} (multiplied by 512) SHOULD be aligned with
+\field{blk_size} for VIRTIO_BLK_T_IN and VIRTIO_BLK_T_OUT requests, when the
+VIRTIO_BLK_F_BLK_SIZE feature has been offered by the device.
+
 The length of \field{data} MUST be a multiple of the size of struct
 virtio_blk_discard_write_zeroes for VIRTIO_BLK_T_DISCARD,
 VIRTIO_BLK_T_SECURE_ERASE and VIRTIO_BLK_T_WRITE_ZEROES requests.
@@ -966,6 +988,18 @@ \subsection{Device Operation}\label{sec:Device Types / Block Device / Device Ope
 for a write request if the VIRTIO_BLK_F_RO feature if offered, and MUST NOT
 write any data.
 
+A device MAY set the \field{status} to VIRTIO_BLK_S_IOERR for
+VIRTIO_BLK_T_IN or VIRTIO_BLK_T_OUT requests if the requested
+\field{sector} (multiplied by 512) is not aligned with the device's
+\field{blk_size}, when the VIRTIO_BLK_F_BLK_SIZE feature has been offered by
+the device.
+
+A device MAY set the \field{status} to VIRTIO_BLK_S_IOERR for
+VIRTIO_BLK_T_IN or VIRTIO_BLK_T_OUT requests if the length of the
+requested \field{data} is not an integer multiple of the device's
+\field{blk_size}, when the VIRTIO_BLK_F_BLK_SIZE feature has been offered by
+the device.
+
 The device MUST set the \field{status} byte to VIRTIO_BLK_S_UNSUPP for
 discard, secure erase and write zeroes commands if any unknown flag is set.
 Furthermore, the device MUST set the \field{status} byte to
-- 
2.18.1

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

[Daniel Verkamp]

On Sun, Oct 6, 2024 at 9:56 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
>
> This field is only valid when the VIRTIO_BLK_F_BLK_SIZE feature bit is
> offered by the device.
>
> The blk_size field actually represents the logical block size of the
> device. It is always a power of two and typically ranges from 512 bytes
> to larger values such as 4 KB.
>
> Add description for this field to provide clarity on its constraints.
>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>
> changes from V1:
>  - Addressed Stefan's and Daniel's comments:
>        1. use SHOULD instead of MUST
>        2. Add a note that devices may return IOERR upon misaligned IO
>  - Add a note that devices may return IOERR if IO size is not following
>    the block size granularity.
> ---
>  device-types/blk/description.tex | 34 ++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/device-types/blk/description.tex b/device-types/blk/description.tex
> index 2712ada..77eed80 100644
> --- a/device-types/blk/description.tex
> +++ b/device-types/blk/description.tex
> @@ -135,6 +135,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 offered by the device.
> +This field reports the block size of the device, expressed in bytes.
> +
>  The field \field{num_queues} only exists if VIRTIO_BLK_F_MQ is set. This field specifies
>  the number of queues.
>
> @@ -282,6 +285,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Block Device / Devic
>
>  \drivernormative{\subsubsection}{Device Initialization}{Device Types / Block Device / Device Initialization}
>
> +Drivers SHOULD negotiate VIRTIO_BLK_F_BLK_SIZE if the feature is offered by the
> +device. When negotiated, drivers SHOULD interpret the \field{blk_size} as the
> +logical block size.
> +
> +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.
>
> @@ -319,6 +329,10 @@ \subsection{Device Initialization}\label{sec:Device Types / Block Device / Devic
>
>  \devicenormative{\subsubsection}{Device Initialization}{Device Types / Block Device / Device Initialization}
>
> +Devices SHOULD always offer VIRTIO_BLK_F_BLK_SIZE feature. When this feature is
> +offered, devices MUST initialize \field{blk_size} to a power of two greater
> +than or equal to 512.
> +
>  Devices SHOULD always offer VIRTIO_BLK_F_FLUSH, and MUST offer it
>  if they offer VIRTIO_BLK_F_CONFIG_WCE.
>
> @@ -879,6 +893,14 @@ \subsection{Device Operation}\label{sec:Device Types / Block Device / Device Ope
>  The length of \field{data} MUST be a multiple of 512 bytes for VIRTIO_BLK_T_IN
>  and VIRTIO_BLK_T_OUT requests.
>
> +The length of \field{data} SHOULD be a multiple of \field{blk_size} for VIRTIO_BLK_T_IN
> +and VIRTIO_BLK_T_OUT requests, when the VIRTIO_BLK_F_BLK_SIZE feature has been
> +offered by the device.
> +
> +The value of \field{sector} (multiplied by 512) SHOULD be aligned with
> +\field{blk_size} for VIRTIO_BLK_T_IN and VIRTIO_BLK_T_OUT requests, when the
> +VIRTIO_BLK_F_BLK_SIZE feature has been offered by the device.

Instead of "aligned", perhaps "SHOULD be a multiple of
\field{blk_size}" to be consistent with existing spec wording like
"MUST be a multiple of 512 bytes".

>  The length of \field{data} MUST be a multiple of the size of struct
>  virtio_blk_discard_write_zeroes for VIRTIO_BLK_T_DISCARD,
>  VIRTIO_BLK_T_SECURE_ERASE and VIRTIO_BLK_T_WRITE_ZEROES requests.
> @@ -966,6 +988,18 @@ \subsection{Device Operation}\label{sec:Device Types / Block Device / Device Ope
>  for a write request if the VIRTIO_BLK_F_RO feature if offered, and MUST NOT
>  write any data.
>
> +A device MAY set the \field{status} to VIRTIO_BLK_S_IOERR for
> +VIRTIO_BLK_T_IN or VIRTIO_BLK_T_OUT requests if the requested
> +\field{sector} (multiplied by 512) is not aligned with the device's
> +\field{blk_size}, when the VIRTIO_BLK_F_BLK_SIZE feature has been offered by
> +the device.
> +
> +A device MAY set the \field{status} to VIRTIO_BLK_S_IOERR for
> +VIRTIO_BLK_T_IN or VIRTIO_BLK_T_OUT requests if the length of the
> +requested \field{data} is not an integer multiple of the device's
> +\field{blk_size}, when the VIRTIO_BLK_F_BLK_SIZE feature has been offered by
> +the device.
> +
>  The device MUST set the \field{status} byte to VIRTIO_BLK_S_UNSUPP for
>  discard, secure erase and write zeroes commands if any unknown flag is set.
>  Furthermore, the device MUST set the \field{status} byte to
> --
> 2.18.1
>

This all looks reasonable to me, so Reviewed-by: Daniel Verkamp
<dverkamp@chromium.org> if you'd like.

One other thought: if a device truly can't handle misaligned I/O
requests, it could also choose to fail at feature negotiation time if
it offers VIRTIO_BLK_F_BLK_SIZE and the driver does not accept it. I
am not sure this is in the spirit of the existing general section
"Device Requirements: Feature Bits" (it says "any valid subset of
features", but maybe your device would consider not negotiating
VIRTIO_BLK_F_BLK_SIZE to be an invalid subset of features?)

Thanks,
-- Daniel

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

[Stefan Hajnoczi]

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

On Sun, Oct 06, 2024 at 07:56:09PM +0300, Max Gurtovoy wrote:
> This field is only valid when the VIRTIO_BLK_F_BLK_SIZE feature bit is
> offered by the device.
> 
> The blk_size field actually represents the logical block size of the
> device. It is always a power of two and typically ranges from 512 bytes
> to larger values such as 4 KB.
> 
> Add description for this field to provide clarity on its constraints.
> 
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
> 
> changes from V1:
>  - Addressed Stefan's and Daniel's comments:
>        1. use SHOULD instead of MUST
>        2. Add a note that devices may return IOERR upon misaligned IO
>  - Add a note that devices may return IOERR if IO size is not following
>    the block size granularity.
> ---
>  device-types/blk/description.tex | 34 ++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Stefan Hajnoczi]

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

On Wed, Oct 09, 2024 at 01:46:17AM +0300, Max Gurtovoy wrote:
> 
> On 08/10/2024 23:15, Stefan Hajnoczi wrote:
> > On Sun, Oct 06, 2024 at 07:56:09PM +0300, Max Gurtovoy wrote:
> > > This field is only valid when the VIRTIO_BLK_F_BLK_SIZE feature bit is
> > > offered by the device.
> > > 
> > > The blk_size field actually represents the logical block size of the
> > > device. It is always a power of two and typically ranges from 512 bytes
> > > to larger values such as 4 KB.
> > > 
> > > Add description for this field to provide clarity on its constraints.
> > > 
> > > Signed-off-by: Max Gurtovoy<mgurtovoy@nvidia.com>
> > > ---
> > > 
> > > changes from V1:
> > >   - Addressed Stefan's and Daniel's comments:
> > >         1. use SHOULD instead of MUST
> > >         2. Add a note that devices may return IOERR upon misaligned IO
> > >   - Add a note that devices may return IOERR if IO size is not following
> > >     the block size granularity.
> > > ---
> > >   device-types/blk/description.tex | 34 ++++++++++++++++++++++++++++++++
> > >   1 file changed, 34 insertions(+)
> > Reviewed-by: Stefan Hajnoczi<stefanha@redhat.com>
> 
> I'll add both yours and Daniel's Reviewed-by signatures and fix a small
> comment from Daniel.
> 
> Can you please remind me the next step I should do ?
> 
> Should I open an issue at the oasis github and mention it as "Fixes: " in
> the commit message ?

The GitHub repo is here:
https://github.com/oasis-tcs/virtio-spec

The instructions for requesting a vote on a spec patch are:

  To request a TC vote on resolving a specific comment:

  1. Create a github issue, or edit an existing issue, with a short summary of the comment. The issue MUST specify the link to the latest proposal in the TC mailing list archives. Note: the link MUST be in the issue description itself - not in the comments.
  2. Reply by email to the comment email, requesting that the TC vote on resolving the issue. The mail requesting the vote should include the following, on a line by itself:

    Fixes: https://github.com/oasis-tcs/virtio-spec/issues/NNN
    (NNN is the issue number)
  3. Please make sure to allow time for review between posting a comment and asking for a vote.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]