qemu-devel
[Top][All Lists]
Advanced

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

Re: [PATCH] block/rbd: implement bdrv_co_block_status


From: Peter Lieven
Subject: Re: [PATCH] block/rbd: implement bdrv_co_block_status
Date: Tue, 10 Aug 2021 15:29:29 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0

Am 10.08.21 um 10:51 schrieb Stefano Garzarella:
On Mon, Aug 09, 2021 at 03:41:36PM +0200, Peter Lieven wrote:

Please, can you add a description?
For example also describing what happens if RBD image does not support 
RBD_FEATURE_FAST_DIFF.


Sure.



Signed-off-by: Peter Lieven <pl@kamp.de>
---
block/rbd.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 119 insertions(+)

diff --git a/block/rbd.c b/block/rbd.c
index dcf82b15b8..ef1eaa6af3 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -88,6 +88,7 @@ typedef struct BDRVRBDState {
    char *namespace;
    uint64_t image_size;
    uint64_t object_size;
+    uint64_t features;
} BDRVRBDState;

typedef struct RBDTask {
@@ -983,6 +984,14 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
*options, int flags,
    s->image_size = info.size;
    s->object_size = info.obj_size;

+    r = rbd_get_features(s->image, &s->features);
+    if (r < 0) {
+        error_setg_errno(errp, -r, "error getting image features from %s",
+                         s->image_name);
+        rbd_close(s->image);
+        goto failed_open;
          ^
You can use `failed_post_open` label here, so you can avoid to call rbd_close().


Bad me, I developed this patch in a Qemu version where failed_post_open wasn't 
present...



+    }
+
    /* If we are using an rbd snapshot, we must be r/o, otherwise
     * leave as-is */
    if (s->snap != NULL) {
@@ -1259,6 +1268,115 @@ static ImageInfoSpecific 
*qemu_rbd_get_specific_info(BlockDriverState *bs,
    return spec_info;
}

+typedef struct rbd_diff_req {
+    uint64_t offs;
+    uint64_t bytes;
+    int exists;
+} rbd_diff_req;
+
+static int qemu_rbd_co_block_status_cb(uint64_t offs, size_t len,
+                                       int exists, void *opaque)
+{
+    struct rbd_diff_req *req = opaque;
+
+    assert(req->offs + req->bytes <= offs);
+    assert(offs >= req->offs + req->bytes);

I think just one of the two asserts is enough, isn't that the same condition?


Right.



+
+    if (req->exists && offs > req->offs + req->bytes) {
+        /*
+         * we started in an allocated area and jumped over an unallocated area,
+         * req->bytes contains the length of the allocated area before the
+         * unallocated area. stop further processing.
+         */
+        return -9000;
                 ^
What is this magical value?

Please add a macro (with a comment) and also use it below in other places.


Will add in V2.



+    }
+    if (req->exists && !exists) {
+        /*
+         * we started in an allocated area and reached a hole. req->bytes
+         * contains the length of the allocated area before the hole.
+         * stop further processing.
+         */
+        return -9000;
+    }
+    if (!req->exists && exists && offs > req->offs) {
+        /*
+         * we started in an unallocated area and hit the first allocated
+         * block. req->bytes must be set to the length of the unallocated area
+         * before the allocated area. stop further processing.
+         */
+        req->bytes = offs - req->offs;
+        return -9000;
+    }
+
+    /*
+     * assert that we catched all cases above and allocation state has not
+     * changed during callbacks.
+     */
+    assert(exists == req->exists || !req->bytes);
+    req->exists = exists;
+
+    /*
+     * assert that we either return an unallocated block or have got callbacks
+     * for all allocated blocks present.
+     */
+    assert(!req->exists || offs == req->offs + req->bytes);
+    req->bytes = offs + len - req->offs;
+
+    return 0;
+}
+
+static int coroutine_fn qemu_rbd_co_block_status(BlockDriverState *bs,
+                                                 bool want_zero, int64_t 
offset,
+                                                 int64_t bytes, int64_t *pnum,
+                                                 int64_t *map,
+ BlockDriverState **file)
+{
+    BDRVRBDState *s = bs->opaque;
+    int ret, r;
+    struct rbd_diff_req req = { .offs = offset };
+
+    assert(offset + bytes <= s->image_size);
+
+    /* default to all sectors allocated */
+    ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+    if (map) {
+        *map = offset;
+    }
+    *pnum = bytes;
+
+    /* RBD image does not support fast-diff */
+    if (!(s->features & RBD_FEATURE_FAST_DIFF)) {
+        goto out;
+    }
+
+    r = rbd_diff_iterate2(s->image, NULL, offset, bytes, true, true,
+                          qemu_rbd_co_block_status_cb, &req);
+    if (r < 0 && r != -9000) {
+        goto out;
+    }
+    assert(req.bytes <= bytes);
+    if (!req.exists) {
+        if (r == 0 && !req.bytes) {
+            /*
+             * rbd_diff_iterate2 does not invoke callbacks for unallocated 
areas
+             * except for the case where an overlay has a hole where the parent
+             * has not. This here catches the case where no callback was
+             * invoked at all.
+             */
+            req.bytes = bytes;
+        }
+        ret &= ~BDRV_BLOCK_DATA;
+        ret |= BDRV_BLOCK_ZERO;
+    }
+    *pnum = req.bytes;
+
+out:
+    if (ret > 0 && ret & BDRV_BLOCK_OFFSET_VALID && file) {

Can ret be zero at this point?
Doesn't BDRV_BLOCK_OFFSET_VALID always stay set?


Right, I decided to declare any area as allocated if rbd_diff_iterate2 would 
fail so this

can't happen.


I will send a V2 shortly.


Peter






reply via email to

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