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

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

On Thu, Jul 04, 2024 at 09:52:52AM +0000, Parav Pandit wrote:
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, July 4, 2024 1:34 PM
> > 
> > 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.ht
> > ml
> > >
> > > Keiichi
> > 
> > Thought hard about it. I think expanding the request for errno is unwelcome.
> True, seems overkill.
> 
> > What we can do, if you really want to do it, is when VIRTIO_BLK_F_ERRNO is
> > negotiated, then make error codes different.
> > 
> Do we also need a feature bit?
> If the device reports req.status == ENOSPC convert into the NOSPC error code.
> If it returns VIRTIO_BLK_S_IOERR, than generic EIO?
> 
> This is because there is no gain in telling the device to return special error code on hitting a special error.
> It returns the error if it has support for it. that’s it.
> 
> I am unable to see the benefit of extra negotiation.
> Would this simplicity work?

No - old drivers won't know how to handle the new ENOSPC.


> > ./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
> > > > > >
> > > > >
> > > >
>