Re: [Qemu-devel] [sheepdog] [PATCH] sheepdog: selectable object size support

[Teruaki Ishizaki]

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 size.
> > 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 option.
> >
> > 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=23 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 <address@hidden>
> > ---
> >   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 *opt)
> > +{
> > +    struct SheepdogInode *inode = &s->inode;
> > +    inode->block_size_shift = (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 = s->inode.vdi_size;
>       hdr.copy_policy = s->inode.copy_policy;
>       hdr.copies = s->inode.nr_copies;
> +    hdr.block_size_shift = s->inode.block_size_shift;
>
>       ret = do_req(fd, s->aio_context, (SheepdogReq *)&hdr, buf, &wlen,
>       &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　copies 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