diff mbox

[18/18] nbd: BLOCK_STATUS for standard get_block_status function: client part

Message ID 20170203154757.36140-19-vsementsov@virtuozzo.com
State New
Headers show

Commit Message

Vladimir Sementsov-Ogievskiy Feb. 3, 2017, 3:47 p.m. UTC 
Minimal realization: only first extent from the answer is used.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.c  | 41 ++++++++++++++++++++++++++++++++++++++++-
 block/nbd-client.h  |  5 +++++
 block/nbd.c         |  3 +++
 include/block/nbd.h |  2 +-
 nbd/client.c        | 23 ++++++++++++++++-------
 qemu-nbd.c          |  2 +-
 6 files changed, 66 insertions(+), 10 deletions(-)

Comments

Eric Blake Feb. 9, 2017, 4 p.m. UTC | #1 
On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> Minimal realization: only first extent from the answer is used.

If you're only going to use one extent, should you implement
NBD_CMD_FLAG_REQ_ONE support to save the server some effort?

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/nbd-client.c  | 41 ++++++++++++++++++++++++++++++++++++++++-
>  block/nbd-client.h  |  5 +++++
>  block/nbd.c         |  3 +++
>  include/block/nbd.h |  2 +-
>  nbd/client.c        | 23 ++++++++++++++++-------
>  qemu-nbd.c          |  2 +-
>  6 files changed, 66 insertions(+), 10 deletions(-)

> +int64_t coroutine_fn nbd_client_co_get_block_status(BlockDriverState *bs,
> +                                                    int64_t sector_num,
> +                                                    int nb_sectors, int *pnum,
> +                                                    BlockDriverState **file)
> +{
> +    int64_t ret;
> +    uint32_t nb_extents;
> +    NBDExtent *extents;
> +    NBDClientSession *client = nbd_get_client_session(bs);
> +
> +    if (!client->block_status_ok) {
> +        *pnum = nb_sectors;
> +        ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
> +        if (bs->drv->protocol_name) {
> +            ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
> +        }
> +        return ret;
> +    }

Looks like a sane fallback when we don't have anything more accurate.

> +
> +    ret = nbd_client_co_cmd_block_status(bs, sector_num << BDRV_SECTOR_BITS,
> +                                         nb_sectors << BDRV_SECTOR_BITS,
> +                                         &extents, &nb_extents);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    *pnum = extents[0].length >> BDRV_SECTOR_BITS;
> +    ret = (extents[0].flags & NBD_STATE_HOLE ? 0 : BDRV_BLOCK_ALLOCATED) |
> +          (extents[0].flags & NBD_STATE_ZERO ? BDRV_BLOCK_ZERO : 0);
> +
> +    if ((ret & BDRV_BLOCK_ALLOCATED) && !(ret & BDRV_BLOCK_ZERO)) {
> +        ret |= BDRV_BLOCK_DATA;
> +    }

And this conversion looks correct.

> @@ -579,7 +617,8 @@ int nbd_client_init(BlockDriverState *bs,
>                                  &client->size,
>                                  &client->structured_reply,
>                                  bitmap_name,
> -                                &client->bitmap_ok, errp);
> +                                &client->bitmap_ok,
> +                                &client->block_status_ok, errp);

That's a lot of parameters we're adding; I'm wondering if its smarter to
start passing things through via a struct.  In fact, I have posted
patches previously (and need to rebase and repost them) that introduce
such a struct, in order to make the INFO extension viable.

> @@ -681,11 +681,19 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
>                                                false, NULL) == 0;
>              }
>  
> -            if (!!structured_reply && *structured_reply && !!bitmap_name) {
> +            if (!!structured_reply && *structured_reply) {
>                  int ret;
> -                assert(!!bitmap_ok);
> -                ret = nbd_receive_query_bitmap(ioc, name, bitmap_name,
> -                                               bitmap_ok, errp) == 0;
> +
> +                if (!!bitmap_name) {
> +                    assert(!!bitmap_ok);
> +                    ret = nbd_receive_query_bitmap(ioc, name, bitmap_name,
> +                                                   bitmap_ok, errp) == 0;
> +                } else {
> +                    ret = nbd_receive_query_meta_context(ioc, name,
> +                                                         "base:allocation",
> +                                                         block_status_ok,
> +                                                         errp);
> +                }

This looks weird trying to grab 'base:allocation' only if bitmap_name is
not provided.  The logic will probably have to be different if we want
to allow a client to track both pieces of information at once.
Paolo Bonzini Feb. 15, 2017, 5:04 p.m. UTC | #2 
On 09/02/2017 17:00, Eric Blake wrote:
>> +    if (!client->block_status_ok) {
>> +        *pnum = nb_sectors;
>> +        ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
>> +        if (bs->drv->protocol_name) {

This condition is always true, I think?

>> +            ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
>> +        }
>> +        return ret;
>> +    }
> Looks like a sane fallback when we don't have anything more accurate.

>> +
>> +    ret = nbd_client_co_cmd_block_status(bs, sector_num << BDRV_SECTOR_BITS,
>> +                                         nb_sectors << BDRV_SECTOR_BITS,
>> +                                         &extents, &nb_extents);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    *pnum = extents[0].length >> BDRV_SECTOR_BITS;
>> +    ret = (extents[0].flags & NBD_STATE_HOLE ? 0 : BDRV_BLOCK_ALLOCATED) |
>> +          (extents[0].flags & NBD_STATE_ZERO ? BDRV_BLOCK_ZERO : 0);
>> +
>> +    if ((ret & BDRV_BLOCK_ALLOCATED) && !(ret & BDRV_BLOCK_ZERO)) {
>> +        ret |= BDRV_BLOCK_DATA;
>> +    }

You can always return BDRV_BLOCK_OFFSET_VALID here, too.

Paolo
diff mbox

Patch

diff --git a/block/nbd-client.c b/block/nbd-client.c
index c7eb21fb02..e419c1497c 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -528,6 +528,44 @@  int64_t nbd_client_co_load_bitmap_part(BlockDriverState *bs, uint64_t offset,
                     (uint64_t)bdrv_dirty_bitmap_granularity(bitmap));
 }
 
+int64_t coroutine_fn nbd_client_co_get_block_status(BlockDriverState *bs,
+                                                    int64_t sector_num,
+                                                    int nb_sectors, int *pnum,
+                                                    BlockDriverState **file)
+{
+    int64_t ret;
+    uint32_t nb_extents;
+    NBDExtent *extents;
+    NBDClientSession *client = nbd_get_client_session(bs);
+
+    if (!client->block_status_ok) {
+        *pnum = nb_sectors;
+        ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED;
+        if (bs->drv->protocol_name) {
+            ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE);
+        }
+        return ret;
+    }
+
+    ret = nbd_client_co_cmd_block_status(bs, sector_num << BDRV_SECTOR_BITS,
+                                         nb_sectors << BDRV_SECTOR_BITS,
+                                         &extents, &nb_extents);
+    if (ret < 0) {
+        return ret;
+    }
+
+    *pnum = extents[0].length >> BDRV_SECTOR_BITS;
+    ret = (extents[0].flags & NBD_STATE_HOLE ? 0 : BDRV_BLOCK_ALLOCATED) |
+          (extents[0].flags & NBD_STATE_ZERO ? BDRV_BLOCK_ZERO : 0);
+
+    if ((ret & BDRV_BLOCK_ALLOCATED) && !(ret & BDRV_BLOCK_ZERO)) {
+        ret |= BDRV_BLOCK_DATA;
+    }
+
+    g_free(extents);
+
+    return ret;
+}
 
 void nbd_client_detach_aio_context(BlockDriverState *bs)
 {
@@ -579,7 +617,8 @@  int nbd_client_init(BlockDriverState *bs,
                                 &client->size,
                                 &client->structured_reply,
                                 bitmap_name,
-                                &client->bitmap_ok, errp);
+                                &client->bitmap_ok,
+                                &client->block_status_ok, errp);
     if (ret < 0) {
         logout("Failed to negotiate with the NBD server\n");
         return ret;
diff --git a/block/nbd-client.h b/block/nbd-client.h
index e5ec89b9f6..9848732628 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -34,6 +34,7 @@  typedef struct NBDClientSession {
     bool is_unix;
 
     bool structured_reply;
+    bool block_status_ok;
     bool bitmap_ok;
     uint32_t meta_data_context_id;
 } NBDClientSession;
@@ -59,6 +60,10 @@  int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
                          uint64_t bytes, QEMUIOVector *qiov, int flags);
 int64_t nbd_client_co_load_bitmap_part(BlockDriverState *bs, uint64_t offset,
                                        uint64_t bytes, BdrvDirtyBitmap *bitmap);
+int64_t coroutine_fn nbd_client_co_get_block_status(BlockDriverState *bs,
+                                                    int64_t sector_num,
+                                                    int nb_sectors, int *pnum,
+                                                    BlockDriverState **file);
 
 void nbd_client_detach_aio_context(BlockDriverState *bs);
 void nbd_client_attach_aio_context(BlockDriverState *bs,
diff --git a/block/nbd.c b/block/nbd.c
index b2b6fd1cf9..b3a28f0746 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -604,6 +604,7 @@  static BlockDriver bdrv_nbd = {
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
     .bdrv_refresh_filename      = nbd_refresh_filename,
     .bdrv_dirty_bitmap_load     = nbd_dirty_bitmap_load,
+    .bdrv_co_get_block_status   = nbd_client_co_get_block_status,
 };
 
 static BlockDriver bdrv_nbd_tcp = {
@@ -624,6 +625,7 @@  static BlockDriver bdrv_nbd_tcp = {
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
     .bdrv_refresh_filename      = nbd_refresh_filename,
     .bdrv_dirty_bitmap_load     = nbd_dirty_bitmap_load,
+    .bdrv_co_get_block_status   = nbd_client_co_get_block_status,
 };
 
 static BlockDriver bdrv_nbd_unix = {
@@ -644,6 +646,7 @@  static BlockDriver bdrv_nbd_unix = {
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
     .bdrv_refresh_filename      = nbd_refresh_filename,
     .bdrv_dirty_bitmap_load     = nbd_dirty_bitmap_load,
+    .bdrv_co_get_block_status   = nbd_client_co_get_block_status,
 };
 
 static void bdrv_nbd_init(void)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 69aee1eda1..58c1a3866b 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -196,7 +196,7 @@  int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
                           QIOChannel **outioc,
                           off_t *size, bool *structured_reply,
                           const char *bitmap_name, bool *bitmap_ok,
-                          Error **errp);
+                          bool *block_status_ok, Error **errp);
 int nbd_init(int fd, QIOChannelSocket *sioc, uint16_t flags, off_t size);
 ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request);
 int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply);
diff --git a/nbd/client.c b/nbd/client.c
index c3817b84fa..1b478d112c 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -563,10 +563,10 @@  static int nbd_receive_query_bitmap(QIOChannel *ioc, const char *export,
 
 int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
                           QCryptoTLSCreds *tlscreds, const char *hostname,
-                          QIOChannel **outioc,
-                          off_t *size, bool *structured_reply,
+                          QIOChannel **outioc, off_t *size,
+                          bool *structured_reply,
                           const char *bitmap_name, bool *bitmap_ok,
-                          Error **errp)
+                          bool *block_status_ok, Error **errp)
 {
     char buf[256];
     uint64_t magic, s;
@@ -681,11 +681,19 @@  int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint16_t *flags,
                                               false, NULL) == 0;
             }
 
-            if (!!structured_reply && *structured_reply && !!bitmap_name) {
+            if (!!structured_reply && *structured_reply) {
                 int ret;
-                assert(!!bitmap_ok);
-                ret = nbd_receive_query_bitmap(ioc, name, bitmap_name,
-                                               bitmap_ok, errp) == 0;
+
+                if (!!bitmap_name) {
+                    assert(!!bitmap_ok);
+                    ret = nbd_receive_query_bitmap(ioc, name, bitmap_name,
+                                                   bitmap_ok, errp) == 0;
+                } else {
+                    ret = nbd_receive_query_meta_context(ioc, name,
+                                                         "base:allocation",
+                                                         block_status_ok,
+                                                         errp);
+                }
                 if (ret < 0) {
                     goto fail;
                 }
@@ -969,6 +977,7 @@  static int nbd_receive_structured_reply_chunk(QIOChannel *ioc, NBDReply *reply)
 
     switch (reply->type) {
     case NBD_REPLY_TYPE_NONE:
+    case NBD_REPLY_TYPE_BLOCK_STATUS:
         break;
     case NBD_REPLY_TYPE_OFFSET_DATA:
     case NBD_REPLY_TYPE_OFFSET_HOLE:
diff --git a/qemu-nbd.c b/qemu-nbd.c
index cf45444faf..e3a4733e60 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -272,7 +272,7 @@  static void *nbd_client_thread(void *arg)
 
     ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), NULL, &nbdflags,
                                 NULL, NULL, NULL,
-                                &size, NULL, NULL, NULL, &local_error);
+                                &size, NULL, NULL, NULL, NULL, &local_error);
     if (ret < 0) {
         if (local_error) {
             error_report_err(local_error);