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

[Kevin Wolf]

Am 28.10.2010 11:12, schrieb Stefan Hajnoczi:
> 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 <address@hidden>
>> ---
>>  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.

Honestly, I don't know for sure. It's just the same as what we're doing
in scsi_write_disk.

But after a look at the callers, I think an assert should work.

>> +
>> +    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?

Whoops, good catch.

Kevin