From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44562) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YC1pR-0003Ix-D8 for qemu-devel@nongnu.org; Fri, 16 Jan 2015 03:00:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YC1pM-0003KG-DM for qemu-devel@nongnu.org; Fri, 16 Jan 2015 03:00:25 -0500 Received: from tama50.ecl.ntt.co.jp ([129.60.39.147]:58074) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YC1pL-0003Jy-Vd for qemu-devel@nongnu.org; Fri, 16 Jan 2015 03:00:20 -0500 Message-ID: <54B8C506.6070503@lab.ntt.co.jp> Date: Fri, 16 Jan 2015 17:00:06 +0900 From: Teruaki Ishizaki MIME-Version: 1.0 References: <1421138472-7679-1-git-send-email-ishizaki.teruaki@lab.ntt.co.jp> <8761c837en.wl%mitake.hitoshi@lab.ntt.co.jp> In-Reply-To: <8761c837en.wl%mitake.hitoshi@lab.ntt.co.jp> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [sheepdog] [PATCH] sheepdog: selectable object size support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hitoshi Mitake Cc: kwolf@redhat.com, namei.unix@gmail.com, sheepdog@lists.wpkg.org, qemu-devel@nongnu.org, stefanha@redhat.com Hi Hitoshi, Thanks for your check. (2015/01/15 15:05), Hitoshi Mitake wrote: > At Tue, 13 Jan 2015 17:41:12 +0900, > Teruaki Ishizaki wrote: >> >> Previously, qemu block driver of sheepdog used hard-coded VDI object s= ize. >> This patch enables users to handle "block_size_shift" value for >> calculating VDI object size. >> >> When you start qemu, you don't need to specify additional command opti= on. >> >> But when you create the VDI which doesn't have default object size >> with qemu-img command, you specify block_size_shift option. >> >> If you want to create a VDI of 8MB(1 << 23) object size, >> you need to specify following command option. >> >> # qemu-img create -o block_size_shift=3D23 sheepdog:test1 100M >> >> In addition, when you don't specify qemu-img command option, >> a default value of sheepdog cluster is used for creating VDI. >> >> # qemu-img create sheepdog:test2 100M >> >> Signed-off-by: Teruaki Ishizaki >> --- >> block/sheepdog.c | 138 +++++++++++++++++++++++++++++++++++= ++------- >> include/block/block_int.h | 1 + >> 2 files changed, 117 insertions(+), 22 deletions(-) > > >> >> @@ -1610,7 +1633,8 @@ out_with_err_set: >> if (bs) { >> bdrv_unref(bs); >> } >> - g_free(buf); >> + if (buf) > > The above line has a style problem (white space). I'll change it. > >> @@ -1669,6 +1693,17 @@ static int parse_redundancy(BDRVSheepdogState *= s, const char *opt) >> return 0; >> } >> >> +static int parse_block_size_shift(BDRVSheepdogState *s, const char *o= pt) >> +{ >> + struct SheepdogInode *inode =3D &s->inode; >> + inode->block_size_shift =3D (uint8_t)atoi(opt); > > This patch cannot create VDI with specified block size shift. Below is > the reason: > > Initializing block_size_shift of the inode object here (in > parse_block_size_shift()) doesn't make sense. Because the > block_size_shift of newly created VDI is specified by block_size_shift > of SheepdogVdiReq (in sheepdog source code, sd_req.vdi). You need to > synchronize the struct and initialize the parameter. Below is a slack > solution: > > diff --git a/block/sheepdog.c b/block/sheepdog.c > index 525254e..3dc3359 100644 > --- a/block/sheepdog.c > +++ b/block/sheepdog.c > @@ -168,7 +168,8 @@ typedef struct SheepdogVdiReq { > uint32_t base_vdi_id; > uint8_t copies; > uint8_t copy_policy; > - uint8_t reserved[2]; > + uint8_t store_policy; > + uint8_t block_size_shift; > uint32_t snapid; > uint32_t type; > uint32_t pad[2]; > @@ -1560,6 +1561,7 @@ static int do_sd_create(BDRVSheepdogState *s, > uint32_t *vdi_id, int snapshot, > hdr.vdi_size =3D s->inode.vdi_size; > hdr.copy_policy =3D s->inode.copy_policy; > hdr.copies =3D s->inode.nr_copies; > + hdr.block_size_shift =3D s->inode.block_size_shift; > > ret =3D do_req(fd, s->aio_context, (SheepdogReq *)&hdr, buf, &wle= n, > &rlen); > Oh, sorry for my miss-edit for the patch.... I had tested the source code as you sugested fixing. I'll fix the patch like that! > # This is a very slack solution. You need to avoid using inode as a > # temporal space of the parameter. Originally, "do_sd_create()" had argument "struct BDRVSheepdogState" which included inode, and "parse_redundancy()" substitute copy_policy and=E3=80=80copies substituted for inode. So, I created "parse_block_size_shift()" like "parse_redundancy()" to handle block_size_shift like copy_policy and copies options. Which point is temporal? Thanks, Teruaki Ishizaki