qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Teruaki Ishizaki
Subject: Re: [Qemu-devel] [PATCH v3] sheepdog: selectable object size support
Date: Tue, 27 Jan 2015 12:57:34 +0900
User-agent: Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Thunderbird/24.6.0

(2015/01/26 19:33), Kevin Wolf wrote:
Am 26.01.2015 um 10:52 hat Teruaki Ishizaki geschrieben:
Hi, Kevin

Thanks for your review!

(2015/01/23 22:53), Kevin Wolf wrote:
Am 23.01.2015 um 09:24 hat Teruaki Ishizaki geschrieben:
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>
---
V3:
  - Delete the needless operation of buffer.
  - Delete the needless operations of request header
    for SD_OP_GET_CLUSTER_DEFAULT.
  - Fix coding style problems.

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          |  140 ++++++++++++++++++++++++++++++++++++++-------
  include/block/block_int.h |    1 +
  2 files changed, 119 insertions(+), 22 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index be3176f..c9f06db 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -37,6 +37,7 @@
  #define SD_OP_READ_VDIS      0x15
  #define SD_OP_FLUSH_VDI      0x16
  #define SD_OP_DEL_VDI        0x17
+#define SD_OP_GET_CLUSTER_DEFAULT   0x18

  #define SD_FLAG_CMD_WRITE    0x01
  #define SD_FLAG_CMD_COW      0x02
@@ -167,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];
@@ -186,6 +188,21 @@ typedef struct SheepdogVdiRsp {
      uint32_t pad[5];
  } SheepdogVdiRsp;

+typedef struct SheepdogClusterRsp {
+    uint8_t proto_ver;
+    uint8_t opcode;
+    uint16_t flags;
+    uint32_t epoch;
+    uint32_t id;
+    uint32_t data_length;
+    uint32_t result;
+    uint8_t nr_copies;
+    uint8_t copy_policy;
+    uint8_t block_size_shift;
+    uint8_t __pad1;
+    uint32_t __pad2[6];
+} SheepdogClusterRsp;
+
  typedef struct SheepdogInode {
      char name[SD_MAX_VDI_LEN];
      char tag[SD_MAX_VDI_TAG_LEN];
@@ -1544,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);

@@ -1569,9 +1587,11 @@ static int do_sd_create(BDRVSheepdogState *s, uint32_t 
*vdi_id, int snapshot,
  static int sd_prealloc(const char *filename, Error **errp)
  {
      BlockDriverState *bs = NULL;
+    BDRVSheepdogState *base = NULL;
      uint32_t idx, max_idx;
+    uint32_t object_size;
      int64_t vdi_size;
-    void *buf = g_malloc0(SD_DATA_OBJ_SIZE);
+    void *buf = NULL;
      int ret;

      ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
@@ -1585,18 +1605,23 @@ static int sd_prealloc(const char *filename, Error 
**errp)
          ret = vdi_size;
          goto out;
      }
-    max_idx = DIV_ROUND_UP(vdi_size, SD_DATA_OBJ_SIZE);
+
+    base = bs->opaque;
+    object_size = (UINT32_C(1) << base->inode.block_size_shift);
+    buf = g_malloc0(object_size);

If I understand correctly, block_size_shift can be up to 31, i.e. this
is a 2 GB allocation. Do you really think this is a good idea?
I think that it is not best idea.
Sheepdog internal limit of block_size_shift is 31, so I set logical
limit of block_size_shift for 31.

I think there is no real requirement to use the object size here, this
is only the buffer size for some bdrv_read/write calls, which works fine
in smaller chunks.

So you could still allow block_size_shift up to 31, but use a different
number here. Perhaps always leaving it at 4 MB is good enough, otherwise
you could use something like MIN(24, base->inode.block_size_shift).

At least use g_try_malloc0() here, so that a memory allocation failure
doesn't crash qemu. (Same goes for all potentially huge allocations that
you make in the whole codebase.)
As you pointed out, memory allocation was needed for that value.
I suppose that block_size_shift should be up to 26(64MB) or 27(128M)
actually.

Should it be checked actual limits in that source code?

I suggest limiting the maximum buffer size as above so that a failure
becomes unlikely, and using g_try_malloc() to handle any allocation
failure that occurs anyway gracefully.
OK, I'll change bdrv_read/write buffer size up to 4MB.

Thanks,
Teruaki Ishizaki





reply via email to

[Prev in Thread] Current Thread [Next in Thread]