diff mbox

[RFC,7/7] nbd/replication: implement .bdrv_get_info() for nbd and replication driver

Message ID 1476971860-20860-8-git-send-email-zhang.zhanghailiang@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zhanghailiang Oct. 20, 2016, 1:57 p.m. UTC
Without this callback, there will be an error reports in the primary side:
"qemu-system-x86_64: Couldn't determine the cluster size of the target image,
which has no backing file: Operation not supported
Aborting, since this may create an unusable destination image"

For nbd driver, it doesn't have cluster size, so here we return
a fake value for it.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 block/nbd.c         | 12 ++++++++++++
 block/replication.c |  6 ++++++
 2 files changed, 18 insertions(+)

Comments

Eric Blake Oct. 20, 2016, 3:34 p.m. UTC | #1
On 10/20/2016 08:57 AM, zhanghailiang wrote:
> Without this callback, there will be an error reports in the primary side:
> "qemu-system-x86_64: Couldn't determine the cluster size of the target image,
> which has no backing file: Operation not supported
> Aborting, since this may create an unusable destination image"
> 
> For nbd driver, it doesn't have cluster size, so here we return
> a fake value for it.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  block/nbd.c         | 12 ++++++++++++
>  block/replication.c |  6 ++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 6bc06d6..96d7023 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -40,6 +40,8 @@
>  
>  #define EN_OPTSTR ":exportname="
>  
> +#define NBD_FAKE_CLUSTER_SIZE 512

Why 512?  NBD allows byte-addressable operations (even if it is more
efficient on aligned I/O); and I've been working hard to convert things
to the point that NBD does not enforce alignment on other layers.
Wouldn't 1 be better?

> +static int nbd_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> +{
> +    bdi->cluster_size  = NBD_FAKE_CLUSTER_SIZE;

I also have patches written (but waiting for NBD write zeroes support to
be reviewed first) that add support for the experimental NBD block info,
that lets a server advertise actual sizes to the client rather than
having to guess.  Here's the last time I posted a preview of it:

https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg03567.html

It would be nice to use that instead of just faking things.
Zhanghailiang Oct. 24, 2016, 2:44 a.m. UTC | #2
Hi,

On 2016/10/20 23:34, Eric Blake wrote:
> On 10/20/2016 08:57 AM, zhanghailiang wrote:
>> Without this callback, there will be an error reports in the primary side:
>> "qemu-system-x86_64: Couldn't determine the cluster size of the target image,
>> which has no backing file: Operation not supported
>> Aborting, since this may create an unusable destination image"
>>
>> For nbd driver, it doesn't have cluster size, so here we return
>> a fake value for it.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>>   block/nbd.c         | 12 ++++++++++++
>>   block/replication.c |  6 ++++++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 6bc06d6..96d7023 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -40,6 +40,8 @@
>>
>>   #define EN_OPTSTR ":exportname="
>>
>> +#define NBD_FAKE_CLUSTER_SIZE 512
>
> Why 512?  NBD allows byte-addressable operations (even if it is more
> efficient on aligned I/O); and I've been working hard to convert things
> to the point that NBD does not enforce alignment on other layers.
> Wouldn't 1 be better?
>

Yes, that makes no difference for block replication driver. :)

>> +static int nbd_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>> +{
>> +    bdi->cluster_size  = NBD_FAKE_CLUSTER_SIZE;
>
> I also have patches written (but waiting for NBD write zeroes support to
> be reviewed first) that add support for the experimental NBD block info,
> that lets a server advertise actual sizes to the client rather than
> having to guess.  Here's the last time I posted a preview of it:
>
> https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg03567.html
>
> It would be nice to use that instead of just faking things.
>

That's great, that's what we really want, here it is just a
temporary solution, I'll drop this patch after you nbd patch been merged.

Thanks,
Hailiang
diff mbox

Patch

diff --git a/block/nbd.c b/block/nbd.c
index 6bc06d6..96d7023 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -40,6 +40,8 @@ 
 
 #define EN_OPTSTR ":exportname="
 
+#define NBD_FAKE_CLUSTER_SIZE 512
+
 typedef struct BDRVNBDState {
     NbdClientSession client;
 
@@ -483,6 +485,13 @@  static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
     bs->full_open_options = opts;
 }
 
+static int nbd_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
+{
+    bdi->cluster_size  = NBD_FAKE_CLUSTER_SIZE;
+
+    return 0;
+}
+
 static BlockDriver bdrv_nbd = {
     .format_name                = "nbd",
     .protocol_name              = "nbd",
@@ -499,6 +508,7 @@  static BlockDriver bdrv_nbd = {
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
     .bdrv_refresh_filename      = nbd_refresh_filename,
+    .bdrv_get_info              = nbd_get_info,
 };
 
 static BlockDriver bdrv_nbd_tcp = {
@@ -517,6 +527,7 @@  static BlockDriver bdrv_nbd_tcp = {
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
     .bdrv_refresh_filename      = nbd_refresh_filename,
+    .bdrv_get_info              = nbd_get_info,
 };
 
 static BlockDriver bdrv_nbd_unix = {
@@ -535,6 +546,7 @@  static BlockDriver bdrv_nbd_unix = {
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
     .bdrv_refresh_filename      = nbd_refresh_filename,
+    .bdrv_get_info              = nbd_get_info,
 };
 
 static void bdrv_nbd_init(void)
diff --git a/block/replication.c b/block/replication.c
index e66b1ca..14c718e 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -707,6 +707,11 @@  static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
     aio_context_release(aio_context);
 }
 
+static int replication_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
+{
+    return bdrv_get_info(bs->file->bs, bdi);
+}
+
 BlockDriver bdrv_replication = {
     .format_name                = "replication",
     .protocol_name              = "replication",
@@ -719,6 +724,7 @@  BlockDriver bdrv_replication = {
     .bdrv_co_readv              = replication_co_readv,
     .bdrv_co_writev             = replication_co_writev,
 
+    .bdrv_get_info              = replication_get_info,
     .is_filter                  = true,
     .bdrv_recurse_is_first_non_filter = replication_recurse_is_first_non_filter,