Message ID | 1366879354-5120-1-git-send-email-namei.unix@gmail.com |
---|---|
State | New |
Headers | show |
At Thu, 25 Apr 2013 16:42:34 +0800, Liu Yuan wrote: > > +/* Delete current working VDI on the snapshot chain */ > +static bool sd_delete(BDRVSheepdogState *s) > +{ > + unsigned int wlen = SD_MAX_VDI_LEN, rlen = 0; > + SheepdogVdiReq hdr = { > + .opcode = SD_OP_DEL_VDI, > + .vdi_id = s->inode.vdi_id, > + .data_length = wlen, > + .flags = SD_FLAG_CMD_WRITE, > + }; > + SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr; > + int fd, ret; > + > + fd = connect_to_sdog(s); > + if (fd < 0) { > + return false; > + } > + > + ret = do_req(fd, (SheepdogReq *)&hdr, s->name, &wlen, &rlen); > + closesocket(fd); > + if (ret || (rsp->result != SD_RES_SUCCESS && > + rsp->result != SD_RES_NO_VDI)) { > + error_report("%s, %s", sd_strerror(rsp->result), s->name); > + return false; > + } > + > + return true; > +} Isn't it better to show an error message when the result code is SD_RES_NO_VDI? Thanks, Kazutaka
On 04/25/2013 05:40 PM, MORITA Kazutaka wrote: > Isn't it better to show an error message when the result code is > SD_RES_NO_VDI? This information isn't useful even for debugging, what it for? Thanks, Yuan
At Thu, 25 Apr 2013 17:44:41 +0800, Liu Yuan wrote: > > On 04/25/2013 05:40 PM, MORITA Kazutaka wrote: > > Isn't it better to show an error message when the result code is > > SD_RES_NO_VDI? > > This information isn't useful even for debugging, what it for? The block driver tries to delete the vdi, but the sheepdog servers return "No such vdi" - I thought that something goes wrong in this case. What's the scenario where the sheepdog servers return SD_RES_NO_VDI? Can we ignore it safely? Thanks, Kazutaka
On 04/25/2013 06:03 PM, MORITA Kazutaka wrote: > The block driver tries to delete the vdi, but the sheepdog servers > return "No such vdi" - I thought that something goes wrong in this > case. > > What's the scenario where the sheepdog servers return SD_RES_NO_VDI? > Can we ignore it safely? V2 has this problem, if we loadvm twice in a short of time that no sd_create_branch is called in the time window, the second loadvm will get this NO_VDI error. But with V3 we don't have this problem. Anyway it is okay to printf message for this case for v3. I'll update v4. Thanks, Yuan
diff --git a/block/sheepdog.c b/block/sheepdog.c index 9f30a87..019ccbe 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -36,6 +36,7 @@ #define SD_OP_GET_VDI_INFO 0x14 #define SD_OP_READ_VDIS 0x15 #define SD_OP_FLUSH_VDI 0x16 +#define SD_OP_DEL_VDI 0x17 #define SD_FLAG_CMD_WRITE 0x01 #define SD_FLAG_CMD_COW 0x02 @@ -1569,6 +1570,35 @@ out: sd_finish_aiocb(acb); } +/* Delete current working VDI on the snapshot chain */ +static bool sd_delete(BDRVSheepdogState *s) +{ + unsigned int wlen = SD_MAX_VDI_LEN, rlen = 0; + SheepdogVdiReq hdr = { + .opcode = SD_OP_DEL_VDI, + .vdi_id = s->inode.vdi_id, + .data_length = wlen, + .flags = SD_FLAG_CMD_WRITE, + }; + SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr; + int fd, ret; + + fd = connect_to_sdog(s); + if (fd < 0) { + return false; + } + + ret = do_req(fd, (SheepdogReq *)&hdr, s->name, &wlen, &rlen); + closesocket(fd); + if (ret || (rsp->result != SD_RES_SUCCESS && + rsp->result != SD_RES_NO_VDI)) { + error_report("%s, %s", sd_strerror(rsp->result), s->name); + return false; + } + + return true; +} + /* * Create a writable VDI from a snapshot */ @@ -1577,12 +1607,20 @@ static int sd_create_branch(BDRVSheepdogState *s) int ret, fd; uint32_t vid; char *buf; + bool deleted; dprintf("%" PRIx32 " is snapshot.\n", s->inode.vdi_id); buf = g_malloc(SD_INODE_SIZE); - ret = do_sd_create(s, s->name, s->inode.vdi_size, s->inode.vdi_id, &vid, 1); + /* + * Even If deletion fails, we will just create extra snapshot based on + * the workding VDI which was supposed to be deleted. So no need to + * false bail out. + */ + deleted = sd_delete(s); + ret = do_sd_create(s, s->name, s->inode.vdi_size, s->inode.vdi_id, &vid, + !deleted); if (ret) { goto out; } @@ -1898,6 +1936,12 @@ cleanup: return ret; } +/* + * We implement rollback(loadvm) operation to the specified snapshot by + * 1) switch to the snapshot + * 2) rely on sd_create_branch to delete working VDI and + * 3) create a new working VDI based on the speicified snapshot + */ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) { BDRVSheepdogState *s = bs->opaque;