Re: [PATCH 2/3] scsi: make io_timeout configurable
On 22/09/21 17:47, Philippe Mathieu-Daudé wrote: @@ -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? This command is only invoked at startup and involves no I/O, so 6 seconds should be plenty. Paolo
Re: [PATCH 2/3] scsi: make io_timeout configurable
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
---
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 ReineckeKernel Storage Architect
h...@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
Re: [PATCH 2/3] scsi: make io_timeout configurable
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
---
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?
Re: [PATCH 2/3] scsi: make io_timeout configurable
On 21/09/21 07:39, Hannes Reinecke wrote: It would, but then anyone attempting to use tapes via qemu emulation deserves to suffer. Tapes are bitchy even when used normally, so attempting to use them under qemu emulation will land you with lots of unhappy experiences, where the timeout is the least of your problems. I sincerely doubt anyone will be using tapes here. Not in real-world scenarios. Hmm, I have customers that disagree. Probably the timeout should be kept infinite for tapes. Paolo
Re: [PATCH 2/3] scsi: make io_timeout configurable
On 9/20/21 8:56 PM, Paolo Bonzini wrote: On Mon, Nov 16, 2020 at 7:31 PM 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. Hannes, could 30 seconds be a bit too short for tape drives? It would, but then anyone attempting to use tapes via qemu emulation deserves to suffer. Tapes are bitchy even when used normally, so attempting to use them under qemu emulation will land you with lots of unhappy experiences, where the timeout is the least of your problems. I sincerely doubt anyone will be using tapes here. Not in real-world scenarios. Cheers, Hannes -- Dr. Hannes ReineckeKernel Storage Architect h...@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
Re: [PATCH 2/3] scsi: make io_timeout configurable
On Mon, Nov 16, 2020 at 7:31 PM 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. Hannes, could 30 seconds be a bit too short for tape drives? Paolo
