Re: [Qemu-devel] [PATCH 1/4] scsi-disk: Implement rerror option

[Stefan Hajnoczi <stefanha@gmail.com>]

On Mon, Oct 25, 2010 at 05:17:33PM +0200, Kevin Wolf wrote:
> This implements the rerror option for SCSI disks.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c     |    2 +-
>  hw/scsi-disk.c |   91 ++++++++++++++++++++++++++++++++++++++------------------
>  2 files changed, 63 insertions(+), 30 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index ff7602b..160fa84 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -326,7 +326,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
>  
>      on_read_error = BLOCK_ERR_REPORT;
>      if ((buf = qemu_opt_get(opts, "rerror")) != NULL) {
> -        if (type != IF_IDE && type != IF_VIRTIO && type != IF_NONE) {
> +        if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type != IF_NONE) {
>              fprintf(stderr, "rerror is no supported by this format\n");

This is a good opportunity to fix the "is *no* supported" typo in the error message.

>              return NULL;
>          }
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index 9628b39..55ba558 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -41,7 +41,10 @@ do { fprintf(stderr, "scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
>  #define SCSI_DMA_BUF_SIZE    131072
>  #define SCSI_MAX_INQUIRY_LEN 256
>  
> -#define SCSI_REQ_STATUS_RETRY 0x01
> +#define SCSI_REQ_STATUS_RETRY           0x01
> +#define SCSI_REQ_STATUS_RETRY_TYPE_MASK 0x06
> +#define SCSI_REQ_STATUS_RETRY_READ      0x00
> +#define SCSI_REQ_STATUS_RETRY_WRITE     0x02
>  
>  typedef struct SCSIDiskState SCSIDiskState;
>  
> @@ -70,6 +73,8 @@ struct SCSIDiskState
>      char *serial;
>  };
>  
> +static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
> +
>  static SCSIDiskReq *scsi_new_request(SCSIDiskState *s, uint32_t tag,
>          uint32_t lun)
>  {
> @@ -127,34 +132,30 @@ static void scsi_cancel_io(SCSIDevice *d, uint32_t tag)
>  static void scsi_read_complete(void * opaque, int ret)
>  {
>      SCSIDiskReq *r = (SCSIDiskReq *)opaque;
> +    int n;
>  
>      r->req.aiocb = NULL;
>  
>      if (ret) {
> -        DPRINTF("IO error\n");
> -        r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, 0);
> -        scsi_command_complete(r, CHECK_CONDITION, NO_SENSE);
> -        return;
> +        if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_READ)) {
> +            return;
> +        }
>      }
> +
>      DPRINTF("Data ready tag=0x%x len=%zd\n", r->req.tag, r->iov.iov_len);
>  
> +    n = r->iov.iov_len / 512;
> +    r->sector += n;
> +    r->sector_count -= n;
>      r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, r->iov.iov_len);
>  }
>  
> -/* Read more data from scsi device into buffer.  */
> -static void scsi_read_data(SCSIDevice *d, uint32_t tag)
> +
> +static void scsi_read_request(SCSIDiskReq *r)
>  {
> -    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
> -    SCSIDiskReq *r;
> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>      uint32_t n;
>  
> -    r = scsi_find_request(s, tag);
> -    if (!r) {
> -        BADF("Bad read tag 0x%x\n", tag);
> -        /* ??? This is the wrong error.  */
> -        scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
> -        return;
> -    }
>      if (r->sector_count == (uint32_t)-1) {
>          DPRINTF("Read buf_len=%zd\n", r->iov.iov_len);
>          r->sector_count = 0;
> @@ -177,14 +178,34 @@ static void scsi_read_data(SCSIDevice *d, uint32_t tag)
>                                scsi_read_complete, r);
>      if (r->req.aiocb == NULL)
>          scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
> -    r->sector += n;
> -    r->sector_count -= n;
>  }
>  
> -static int scsi_handle_write_error(SCSIDiskReq *r, int error)
> +/* Read more data from scsi device into buffer.  */
> +static void scsi_read_data(SCSIDevice *d, uint32_t tag)
>  {
> +    SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, d);
> +    SCSIDiskReq *r;
> +
> +    r = scsi_find_request(s, tag);
> +    if (!r) {
> +        BADF("Bad read tag 0x%x\n", tag);
> +        /* ??? This is the wrong error.  */
> +        scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
> +        return;
> +    }
> +
> +    if (r->req.aiocb) {
> +        BADF("Data transfer already in progress\n");
> +    }

Can this be triggered from the guest?  If yes, then we need to (optionally) cancel the request with an error and then return.  If no, then this should be an assert.

> +
> +    scsi_read_request(r);
> +}
> +
> +static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
> +{
> +    int is_read = (type == SCSI_REQ_STATUS_RETRY_READ);
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> -    BlockErrorAction action = bdrv_get_on_error(s->bs, 0);
> +    BlockErrorAction action = bdrv_get_on_error(s->bs, is_read);
>  
>      if (action == BLOCK_ERR_IGNORE) {
>          bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, 0);
> @@ -193,10 +214,16 @@ static int scsi_handle_write_error(SCSIDiskReq *r, int error)
>  
>      if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
>              || action == BLOCK_ERR_STOP_ANY) {
> -        r->status |= SCSI_REQ_STATUS_RETRY;
> +
> +        type &= SCSI_REQ_STATUS_RETRY_TYPE_MASK;
> +        r->status |= SCSI_REQ_STATUS_RETRY | type;
> +
>          bdrv_mon_event(s->bs, BDRV_ACTION_STOP, 0);
>          vm_stop(0);
>      } else {
> +        if (type == SCSI_REQ_STATUS_RETRY_READ) {
> +            r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, 0);
> +        }
>          scsi_command_complete(r, CHECK_CONDITION,
>                  HARDWARE_ERROR);
>          bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, 0);

We don't report whether this was a read or a write here?

Stefan