Re: [PATCH 2/3] scsi: make io_timeout configurable

[Hannes Reinecke]

On 9/22/21 5:47 PM, Philippe Mathieu-Daudé wrote:
> Hi Hannes,
>
> On 11/16/20 19:31, Hannes Reinecke wrote:
> > The current code sets an infinite timeout on SG_IO requests,
> > causing the guest to stall if the host experiences a frame
> > loss.
> > This patch adds an 'io_timeout' parameter for SCSIDevice to
> > make the SG_IO timeout configurable, and also shortens the
> > default timeout to 30 seconds to avoid infinite stalls.
> >
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > ---
> >   hw/scsi/scsi-disk.c    |  6 ++++--
> >   hw/scsi/scsi-generic.c | 17 +++++++++++------
> >   include/hw/scsi/scsi.h |  4 +++-
> >   3 files changed, 18 insertions(+), 9 deletions(-)
>
> >   int scsi_SG_IO_FROM_DEV(BlockBackend *blk, uint8_t *cmd, uint8_t 
> > cmd_size,
> > -                        uint8_t *buf, uint8_t buf_size)
> > +                        uint8_t *buf, uint8_t buf_size, uint32_t 
> > timeout)
> >   {
> >       sg_io_hdr_t io_header;
> >       uint8_t sensebuf[8];
> > @@ -520,7 +522,7 @@ int scsi_SG_IO_FROM_DEV(BlockBackend *blk, uint8_t 
> > *cmd, uint8_t cmd_size,
> >       io_header.cmd_len = cmd_size;
> >       io_header.mx_sb_len = sizeof(sensebuf);
> >       io_header.sbp = sensebuf;
> > -    io_header.timeout = 6000; /* XXX */
> > +    io_header.timeout = timeout * 1000;
>
> > @@ -637,7 +639,7 @@ static int get_stream_blocksize(BlockBackend *blk)
> >       cmd[0] = MODE_SENSE;
> >       cmd[4] = sizeof(buf);
> > -    ret = scsi_SG_IO_FROM_DEV(blk, cmd, sizeof(cmd), buf, sizeof(buf));
> > +    ret = scsi_SG_IO_FROM_DEV(blk, cmd, sizeof(cmd), buf, 
> > sizeof(buf), 6);
>
> Why is this timeout hardcoded? Due to the /* XXX */ comment?

60 seconds is the default command timeout on linux.
And the problem is that the guest might set a command timeout on the 
comands being send from the guests user space, but the guest is unable
to communicate this timeout to the host.

Really, one should fix up the virtio spec here to allow for a 'timeout' 
field. But in the absence of that being able to configure it is the next 
best attempt.

Cheers,

Hannes
--
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer