From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=52794 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PBOmZ-0004ro-IM for qemu-devel@nongnu.org; Thu, 28 Oct 2010 05:28:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PBOmX-0004aH-J1 for qemu-devel@nongnu.org; Thu, 28 Oct 2010 05:28:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:62042) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PBOmX-0004a9-89 for qemu-devel@nongnu.org; Thu, 28 Oct 2010 05:28:25 -0400 Message-ID: <4CC94261.5090107@redhat.com> Date: Thu, 28 Oct 2010 11:29:05 +0200 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 1/4] scsi-disk: Implement rerror option References: <1288019856-16649-1-git-send-email-kwolf@redhat.com> <1288019856-16649-2-git-send-email-kwolf@redhat.com> <20101028091234.GA2811@stefan-thinkpad.transitives.com> In-Reply-To: <20101028091234.GA2811@stefan-thinkpad.transitives.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel@nongnu.org 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 >> --- >> 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