Re: [PATCH 1/1] virito-blk: Support NOSPC error

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jun 19, 2024 at 10:09:19PM +0900, Keiichi Watanabe wrote:
> Thank you for the feedback. Having the new feature bit makes sense.
> So, I think we can update the spec as follows:
> 
> * Define VIRTIO_BLK_F_ERRNO feature bit
> * If VIRTIO_BLK_F_ERRNO is negotiated, the driver sends the following
> virito_blk_req_with_errno instead of virtio_blk_req.
> * If VIRTIO_BLK_F_ERRNO is negotiated, when the device sets status to
> VIRTIO_BLK_S_IOERR, the device may set errno to a positive value.
> 
> struct virtio_blk_req_with_errno {
>   le32 type;
>   le32 reserved;
>   le64 sector;
>   u8 data[];
>   u8 status;
>   u8 padding[3];
>   le32 errno;
> };
> 
> Does it make sense? If so, I'll make v2 patch.
> Note that I used le32 for errno above because POSIX spec [1] says
> errno is int and int is usually 4 bytes.
> 
> [1]: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html
> 
> Keiichi

Thought hard about it. I think expanding the request for errno is
unwelcome. What we can do, if you really want to do it, is when
VIRTIO_BLK_F_ERRNO is negotiated, then make error codes different.

./include/uapi/asm-generic/errno.h has numbers that only go up
to about 60. So ok for now.

As to whether just keep your approach, can we see some
analysis which errors are likely to happen at the block level?
If there are many more of them, then reusing errno begins to
make sense.

> 
> On Tue, Jun 18, 2024 at 9:18 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Jun 18, 2024 at 11:54:10AM +0000, Parav Pandit wrote:
> > > Hi Keiichi,
> > >
> > > > From: Keiichi Watanabe <keiichiw@chromium.org>
> > > > Sent: Tuesday, June 18, 2024 1:49 PM
> > > > To: virtio-comment@lists.linux.dev
> > > > Cc: keiichiw@chromium.org; uekawa@chromium.org;
> > > > takayas@chromium.org; dverkamp@chromium.org; tytso@google.com
> > > > Subject: [PATCH 1/1] virito-blk: Support NOSPC error
> > > >
> > > > Add a status code to indicate that the storage is full on the virtio-blk device
> > > > side.
> > > >
> > > > This is useful in scenarios where a virtio-blk disk image is provided on a sparse
> > > > disk and the host storage is full.
> > > > In this scenario, the driver cannot create a new file to the disk image because
> > > > the sparse disk cannot be expanded.
> > > > However, if the host have a service that clean up host stroage regularly by
> > > s/stroage/storage
> > >
> > > > wiping cache files, the guest may want to retry the request.
> > > > For this type of special handling, this error should have a separate error code
> > > > in virito-blk.
> > > >
> > > > Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
> > > > ---
> > > >  device-types/blk/description.tex | 13 ++++++++-----
> > > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/device-types/blk/description.tex b/device-types/blk/description.tex
> > > > index f04c932..5a34bd5 100644
> > > > --- a/device-types/blk/description.tex
> > > > +++ b/device-types/blk/description.tex
> > > > @@ -532,12 +532,14 @@ \subsection{Device Operation}\label{sec:Device
> > > > Types / Block Device / Device Ope
> > > >
> > > >  The final \field{status} byte is written by the device: either  VIRTIO_BLK_S_OK
> > > > for success, VIRTIO_BLK_S_IOERR for device or driver -error or
> > > > VIRTIO_BLK_S_UNSUPP for a request unsupported by device:
> > > > +error, VIRTIO_BLK_S_UNSUPP for a request unsupported by device or
> > > > +VIRTIO_BLK_S_NOSPC for an error that device doesn't have enough space.
> > > >
> > > >  \begin{lstlisting}
> > > >  #define VIRTIO_BLK_S_OK        0
> > > >  #define VIRTIO_BLK_S_IOERR     1
> > > >  #define VIRTIO_BLK_S_UNSUPP    2
> > > > +#define VIRTIO_BLK_S_NOSPC     7
> > > >  \end{lstlisting}
> > > >
> > > With recent work from Michael and others in admin command area,
> > >
> > > we try to use existing Linux error codes referenced by the spec [errno] in the "Normative References" section.
> > > So for this new error code also, it would be good to reuse value 28 (instead of 7) of [2].
> > >
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/asm-generic/errno-base.h
> > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/asm-generic/errno-base.h#n32
> >
> >
> > The problem with this idea, is that e.g. EIO is 5 which is already used
> > up by zone flags. And errno values go beyond 127 so with a single byte
> > status we can't reserve a bit for that, either.
> >
> >
> > > >  The status of individual segments is indeterminate when a discard or write
> > > > zero @@ -567,10 +569,11 @@ \subsection{Device
> > > > Operation}\label{sec:Device Types / Block Device / Device Ope  Requests of
> > > > type VIRTIO_BLK_T_OUT, VIRTIO_BLK_T_ZONE_OPEN,
> > > > VIRTIO_BLK_T_ZONE_CLOSE, VIRTIO_BLK_T_ZONE_FINISH,
> > > > VIRTIO_BLK_T_ZONE_APPEND,  VIRTIO_BLK_T_ZONE_RESET or
> > > > VIRTIO_BLK_T_ZONE_RESET_ALL may be completed by the -device with
> > > > VIRTIO_BLK_S_OK, VIRTIO_BLK_S_IOERR or VIRTIO_BLK_S_UNSUPP -
> > > > \field{status}, or, additionally, with  VIRTIO_BLK_S_ZONE_INVALID_CMD, -
> > > > VIRTIO_BLK_S_ZONE_UNALIGNED_WP,
> > > > VIRTIO_BLK_S_ZONE_OPEN_RESOURCE or -
> > > > VIRTIO_BLK_S_ZONE_ACTIVE_RESOURCE ZBD-specific status codes.
> > > > +device with VIRTIO_BLK_S_OK, VIRTIO_BLK_S_IOERR,
> > > > VIRTIO_BLK_S_UNSUPP or
> > > > +VIRTIO_BLK_S_NOSPC \field{status}, or, additionally, with
> > > > +VIRTIO_BLK_S_ZONE_INVALID_CMD,
> > > > VIRTIO_BLK_S_ZONE_UNALIGNED_WP,
> > > > +VIRTIO_BLK_S_ZONE_OPEN_RESOURCE or
> > > > VIRTIO_BLK_S_ZONE_ACTIVE_RESOURCE
> > > > +ZBD-specific status codes.
> > > >
> > > >  Besides the request status, VIRTIO_BLK_T_ZONE_APPEND requests return the
> > > > starting sector of the appended data back to the driver. For this reason,
> > > > --
> > > > 2.45.2.627.g7a2c4fd464-goog
> > > >
> > >
> >