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. > +{ > + 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? > + } 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? Kevin |