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

[Teruaki Ishizaki <ishizaki.teruaki@lab.ntt.co.jp>]

Hi, Hitoshi

Thanks for your review!

(2015/01/22 14:22), Hitoshi Mitake wrote:
> At Tue, 20 Jan 2015 16:14:28 +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 <ishizaki.teruaki@lab.ntt.co.jp>
>> ---
>> V2:
>>   - Fix coding style problem (white space).
>>   - Add members, store_policy and block_size_shift to struct SheepdogVdiReq
>>   - Initialize request header to use block_size_shift specified by user.
>> ---
>>   block/sheepdog.c          |  142 ++++++++++++++++++++++++++++++++++++++-------
>>   include/block/block_int.h |    1 +
>>   2 files changed, 121 insertions(+), 22 deletions(-)
>>
>
>> @@ -1757,6 +1800,48 @@ static int sd_create(const char *filename, QemuOpts *opts,
>>       }
>>
>>       s->aio_context = qemu_get_aio_context();
>> +
>> +    /* if block_size_shift is not specified, get cluster default value */
>> +    if (s->inode.block_size_shift == 0) {
>> +        SheepdogVdiReq hdr;
>> +        SheepdogClusterRsp *rsp = (SheepdogClusterRsp *)&hdr;
>> +        Error *local_err = NULL;
>> +        int fd;
>> +        unsigned int wlen = 0, rlen = 0;
>> +
>> +        fd = connect_to_sdog(s, &local_err);
>> +        if (fd < 0) {
>> +            error_report("%s", error_get_pretty(local_err));
>> +            error_free(local_err);
>> +            ret = -EIO;
>> +            goto out;
>> +        }
>> +
>> +        memset(&hdr, 0, sizeof(hdr));
>
> SD_OP_GET_CLUSTER_DEFAULT doesn't require succeeding data, so the below memset() for buf and
>
>> +        memset(&buf, 0, sizeof(buf));
>> +        hdr.opcode = SD_OP_GET_CLUSTER_DEFAULT;
>> +        hdr.proto_ver = SD_PROTO_VER;
>
> the below two statements aren't required.
>
>> +        hdr.data_length = wlen;
>> +        hdr.flags = SD_FLAG_CMD_WRITE;

I comprehend above things you pointed out.
I'll fix the patch!

Thanks,
Teruaki Ishizaki