At Fri, 01 Jul 2011 10:29:09 +0200, Kevin Wolf wrote: > > Am 21.05.2011 14:35, schrieb MORITA Kazutaka: > > This introduces a qemu-img create option for sheepdog which allows the > > data to be preallocated (note that sheepdog always preallocates > > metadata). This is necessary to use Sheepdog volumes as a backend > > storage for iSCSI target. More information is available at > > https://sourceforge.net/apps/trac/sheepdog/wiki/General%20Protocol%20Support > > > > The option is disabled by default and you need to enable it like the > > following: > > > > qemu-img create sheepdog:test -o preallocation=data 1G > > > > Signed-off-by: MORITA Kazutaka <morita.kazutaka at lab.ntt.co.jp> > > Signed-off-by: FUJITA Tomonori <fujita.tomonori at lab.ntt.co.jp> > > Hm, looks like I forgot about this patch and nobody pinged me... > > > block/sheepdog.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++- > > 1 files changed, 72 insertions(+), 1 deletions(-) > > > > diff --git a/block/sheepdog.c b/block/sheepdog.c > > index 0392ca8..38ca9aa 100644 > > --- a/block/sheepdog.c > > +++ b/block/sheepdog.c > > @@ -1292,6 +1292,57 @@ static int do_sd_create(char *filename, int64_t vdi_size, > > return 0; > > } > > > > +static int sd_prealloc(uint32_t vid, int64_t vdi_size) > > General comment to this function: Wouldn't it be easier to call the > existing bdrv_write function in a loop instead of reimplementing the > write manually? Though there may be a reason for it that I'm just missing. Yes, it's much easier! I'll rewrite this function with those, thanks. > > > +{ > > + int fd, ret; > > + SheepdogInode *inode; > > + char *buf; > > + unsigned long idx, max_idx; > > + > > + fd = connect_to_sdog(NULL, NULL); > > + if (fd < 0) { > > + return -EIO; > > + } > > + > > + inode = qemu_malloc(sizeof(*inode)); > > + buf = qemu_malloc(SD_DATA_OBJ_SIZE); > > + > > + ret = read_object(fd, (char *)inode, vid_to_vdi_oid(vid), > > + 0, sizeof(*inode), 0); > > No error handling? > > > + > > + max_idx = (vdi_size + SD_DATA_OBJ_SIZE - 1) / SD_DATA_OBJ_SIZE; > > + > > + for (idx = 0; idx < max_idx; idx++) { > > + uint64_t oid; > > + oid = vid_to_data_oid(vid, idx); > > + > > + if (inode->data_vdi_id[idx]) { > > + ret = read_object(fd, buf, vid_to_vdi_oid(inode->data_vdi_id[idx]), > > + 1, SD_DATA_OBJ_SIZE, 0); > > + if (ret) > > + goto out; > > Missing braces. > > Also, what is this if branch doing? Is it to ensure that we don't > overwrite existing data? But then, isn't an image always empty when we > preallocate it? This branch is for handling a cloned image, which is created with -b option. This branch reads data from the backing file (read_object returns zero when it succeeds) instead of filling buffer with zero. > > > + } else { > > + memset(buf, 0, SD_DATA_OBJ_SIZE); > > + } > > + > > + ret = write_object(fd, buf, oid, 1, SD_DATA_OBJ_SIZE, 0, 1); > > + if (ret) > > + goto out; > > Braces > > > + > > + inode->data_vdi_id[idx] = vid; > > + ret = write_object(fd, (char *)inode, vid_to_vdi_oid(vid), > > + 1, sizeof(*inode), 0, 0); > > + if (ret) > > + goto out; > > Same here > > > + } > > +out: > > + free(inode); > > + free(buf); > > + closesocket(fd); > > + > > + return ret; > > +} > > + > > static int sd_create(const char *filename, QEMUOptionParameter *options) > > { > > int ret; > > @@ -1301,6 +1352,7 @@ static int sd_create(const char *filename, QEMUOptionParameter *options) > > BDRVSheepdogState s; > > char vdi[SD_MAX_VDI_LEN], tag[SD_MAX_VDI_TAG_LEN]; > > uint32_t snapid; > > + int prealloc = 0; > > > > strstart(filename, "sheepdog:", (const char **)&filename); > > > > @@ -1317,6 +1369,16 @@ static int sd_create(const char *filename, QEMUOptionParameter *options) > > vdi_size = options->value.n; > > } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) { > > backing_file = options->value.s; > > + } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) { > > + if (!options->value.s || !strcmp(options->value.s, "off")) { > > + prealloc = 0; > > + } else if (!strcmp(options->value.s, "data")) { > > + prealloc = 1; > > + } else { > > + error_report("Invalid preallocation mode: '%s'\n", > > + options->value.s); > > + return -EINVAL; > > + } > > } > > options++; > > } > > @@ -1354,7 +1416,11 @@ static int sd_create(const char *filename, QEMUOptionParameter *options) > > bdrv_delete(bs); > > } > > > > - return do_sd_create((char *)vdi, vdi_size, base_vid, &vid, 0, s.addr, s.port); > > + ret = do_sd_create((char *)vdi, vdi_size, base_vid, &vid, 0, s.addr, s.port); > > + if (!prealloc || ret) > > + return ret; > > And here. > > > + > > + return sd_prealloc(vid, vdi_size); > > } > > > > static void sd_close(BlockDriverState *bs) > > @@ -1990,6 +2056,11 @@ static QEMUOptionParameter sd_create_options[] = { > > .type = OPT_STRING, > > .help = "File name of a base image" > > }, > > + { > > + .name = BLOCK_OPT_PREALLOC, > > + .type = OPT_STRING, > > + .help = "Preallocation mode (allowed values: off, data)" > > + }, > > { NULL } > > }; > > In my RFC for qcow2, I called this preallocation mode "full" instead of > "data". I don't mind much which we pick, but we should keep it > consistent. Any preferences? I don't mind too. I'll use "full" in the v2 patch. Thanks, Kazutaka |