| Message ID | 1456776532-1008-3-git-send-email-jcody@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29/02/2016 21:08, Jeff Cody wrote: > From: Vasiliy Tolstov <v.tolstov@selfip.ru> > > This patch implements a blockdriver function bdrv_snapshot_delete() in > the sheepdog driver. With the new function, snapshots of sheepdog can > be deleted from libvirt. > > Cc: Jeff Cody <jcody@redhat.com> > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp> > Signed-off-by: Vasiliy Tolstov <v.tolstov@selfip.ru> > Message-id: 1450873346-22334-1-git-send-email-mitake.hitoshi@lab.ntt.co.jp > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/sheepdog.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 123 insertions(+), 2 deletions(-) > > diff --git a/block/sheepdog.c b/block/sheepdog.c > index a0098c1..8739acc 100644 > --- a/block/sheepdog.c > +++ b/block/sheepdog.c > @@ -284,6 +284,12 @@ static inline bool is_snapshot(struct SheepdogInode *inode) > return !!inode->snap_ctime; > } > > +static inline size_t count_data_objs(const struct SheepdogInode *inode) > +{ > + return DIV_ROUND_UP(inode->vdi_size, > + (1UL << inode->block_size_shift)); > +} > + > #undef DPRINTF > #ifdef DEBUG_SDOG > #define DPRINTF(fmt, args...) \ > @@ -2478,13 +2484,128 @@ out: > return ret; > } > > +#define NR_BATCHED_DISCARD 128 > + > +static bool remove_objects(BDRVSheepdogState *s) > +{ > + int fd, i = 0, nr_objs = 0; > + Error *local_err = NULL; > + int ret = 0; > + bool result = true; > + SheepdogInode *inode = &s->inode; > + > + fd = connect_to_sdog(s, &local_err); > + if (fd < 0) { > + error_report_err(local_err); > + return false; > + } > + > + nr_objs = count_data_objs(inode); > + while (i < nr_objs) { > + int start_idx, nr_filled_idx; > + > + while (i < nr_objs && !inode->data_vdi_id[i]) { > + i++; > + } > + start_idx = i; > + > + nr_filled_idx = 0; > + while (i < nr_objs && nr_filled_idx < NR_BATCHED_DISCARD) { > + if (inode->data_vdi_id[i]) { > + inode->data_vdi_id[i] = 0; > + nr_filled_idx++; > + } > + > + i++; > + } > + > + ret = write_object(fd, s->aio_context, > + (char *)&inode->data_vdi_id[start_idx], > + vid_to_vdi_oid(s->inode.vdi_id), inode->nr_copies, > + (i - start_idx) * sizeof(uint32_t), > + offsetof(struct SheepdogInode, > + data_vdi_id[start_idx]), > + false, s->cache_flags); > + if (ret < 0) { > + error_report("failed to discard snapshot inode."); > + result = false; > + goto out; > + } > + } > + > +out: > + closesocket(fd); > + return result; > +} > + > static int sd_snapshot_delete(BlockDriverState *bs, > const char *snapshot_id, > const char *name, > Error **errp) > { > - /* FIXME: Delete specified snapshot id. */ > - return 0; > + uint32_t snap_id = 0; > + char snap_tag[SD_MAX_VDI_TAG_LEN]; > + Error *local_err = NULL; > + int fd, ret; > + char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN]; > + BDRVSheepdogState *s = bs->opaque; > + unsigned int wlen = SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN, rlen = 0; > + uint32_t vid; > + SheepdogVdiReq hdr = { > + .opcode = SD_OP_DEL_VDI, > + .data_length = wlen, > + .flags = SD_FLAG_CMD_WRITE, > + }; > + SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr; > + > + if (!remove_objects(s)) { > + return -1; > + } > + > + memset(buf, 0, sizeof(buf)); > + memset(snap_tag, 0, sizeof(snap_tag)); > + pstrcpy(buf, SD_MAX_VDI_LEN, s->name); > + if (qemu_strtoul(snapshot_id, NULL, 10, (unsigned long *)&snap_id)) { This cast seems like a very bad idea, and in fact it is. qemu_strtoul will access 8 bytes on 64-bit machines. Please change snap_id to be an unsigned long instead. Paolo
On Wed, Mar 02, 2016 at 03:11:27PM +0100, Paolo Bonzini wrote: > > > On 29/02/2016 21:08, Jeff Cody wrote: > > From: Vasiliy Tolstov <v.tolstov@selfip.ru> > > > > This patch implements a blockdriver function bdrv_snapshot_delete() in > > the sheepdog driver. With the new function, snapshots of sheepdog can > > be deleted from libvirt. > > > > Cc: Jeff Cody <jcody@redhat.com> > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp> > > Signed-off-by: Vasiliy Tolstov <v.tolstov@selfip.ru> > > Message-id: 1450873346-22334-1-git-send-email-mitake.hitoshi@lab.ntt.co.jp > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > --- > > block/sheepdog.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 123 insertions(+), 2 deletions(-) [...] > + memset(buf, 0, sizeof(buf)); > > + memset(snap_tag, 0, sizeof(snap_tag)); > > + pstrcpy(buf, SD_MAX_VDI_LEN, s->name); > > + if (qemu_strtoul(snapshot_id, NULL, 10, (unsigned long *)&snap_id)) { > > This cast seems like a very bad idea, and in fact it is. qemu_strtoul > will access 8 bytes on 64-bit machines. Please change snap_id to be an > unsigned long instead. > Thanks Paolo, I missed that. I just sent a patch to fix this. -Jeff
diff --git a/block/sheepdog.c b/block/sheepdog.c index a0098c1..8739acc 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -284,6 +284,12 @@ static inline bool is_snapshot(struct SheepdogInode *inode) return !!inode->snap_ctime; } +static inline size_t count_data_objs(const struct SheepdogInode *inode) +{ + return DIV_ROUND_UP(inode->vdi_size, + (1UL << inode->block_size_shift)); +} + #undef DPRINTF #ifdef DEBUG_SDOG #define DPRINTF(fmt, args...) \ @@ -2478,13 +2484,128 @@ out: return ret; } +#define NR_BATCHED_DISCARD 128 + +static bool remove_objects(BDRVSheepdogState *s) +{ + int fd, i = 0, nr_objs = 0; + Error *local_err = NULL; + int ret = 0; + bool result = true; + SheepdogInode *inode = &s->inode; + + fd = connect_to_sdog(s, &local_err); + if (fd < 0) { + error_report_err(local_err); + return false; + } + + nr_objs = count_data_objs(inode); + while (i < nr_objs) { + int start_idx, nr_filled_idx; + + while (i < nr_objs && !inode->data_vdi_id[i]) { + i++; + } + start_idx = i; + + nr_filled_idx = 0; + while (i < nr_objs && nr_filled_idx < NR_BATCHED_DISCARD) { + if (inode->data_vdi_id[i]) { + inode->data_vdi_id[i] = 0; + nr_filled_idx++; + } + + i++; + } + + ret = write_object(fd, s->aio_context, + (char *)&inode->data_vdi_id[start_idx], + vid_to_vdi_oid(s->inode.vdi_id), inode->nr_copies, + (i - start_idx) * sizeof(uint32_t), + offsetof(struct SheepdogInode, + data_vdi_id[start_idx]), + false, s->cache_flags); + if (ret < 0) { + error_report("failed to discard snapshot inode."); + result = false; + goto out; + } + } + +out: + closesocket(fd); + return result; +} + static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id, const char *name, Error **errp) { - /* FIXME: Delete specified snapshot id. */ - return 0; + uint32_t snap_id = 0; + char snap_tag[SD_MAX_VDI_TAG_LEN]; + Error *local_err = NULL; + int fd, ret; + char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN]; + BDRVSheepdogState *s = bs->opaque; + unsigned int wlen = SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN, rlen = 0; + uint32_t vid; + SheepdogVdiReq hdr = { + .opcode = SD_OP_DEL_VDI, + .data_length = wlen, + .flags = SD_FLAG_CMD_WRITE, + }; + SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)&hdr; + + if (!remove_objects(s)) { + return -1; + } + + memset(buf, 0, sizeof(buf)); + memset(snap_tag, 0, sizeof(snap_tag)); + pstrcpy(buf, SD_MAX_VDI_LEN, s->name); + if (qemu_strtoul(snapshot_id, NULL, 10, (unsigned long *)&snap_id)) { + return -1; + } + + if (snap_id) { + hdr.snapid = snap_id; + } else { + pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id); + pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag); + } + + ret = find_vdi_name(s, s->name, snap_id, snap_tag, &vid, true, + &local_err); + if (ret) { + return ret; + } + + fd = connect_to_sdog(s, &local_err); + if (fd < 0) { + error_report_err(local_err); + return -1; + } + + ret = do_req(fd, s->aio_context, (SheepdogReq *)&hdr, + buf, &wlen, &rlen); + closesocket(fd); + if (ret) { + return ret; + } + + switch (rsp->result) { + case SD_RES_NO_VDI: + error_report("%s was already deleted", s->name); + case SD_RES_SUCCESS: + break; + default: + error_report("%s, %s", sd_strerror(rsp->result), s->name); + return -1; + } + + return ret; } static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)