[Qemu-devel] [PATCH 1/1] nbd: implement bdrv_get_info callback

[Qemu-devel] [PATCH 1/1] nbd: implement bdrv_get_info callback

[Edgar Kaziakhmedov]

Since mirror job supports efficient zero out target mechanism (see
in mirror_dirty_init()), implement bdrv_get_info to make it work
over NBD. Such improvement will allow using the largest chunk possible
and will decrease the number of NBD_CMD_WRITE_ZEROES requests on the wire.

Signed-off-by: Edgar Kaziakhmedov <edgar.kaziakhmedov@virtuozzo.com>
---
 block/nbd.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/block/nbd.c b/block/nbd.c
index 8b8ba56cdd..94220f6d14 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -566,6 +566,14 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
     bs->full_open_options = opts;
 }

+static int nbd_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
+{
+    if (bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP) {
+        bdi->can_write_zeroes_with_unmap = true;
+    }
+    return 0;
+}
+
 static BlockDriver bdrv_nbd = {
     .format_name                = "nbd",
     .protocol_name              = "nbd",
@@ -583,6 +591,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 = {
@@ -602,6 +611,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 = {
@@ -621,6 +631,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)
--
2.11.0

Re: [Qemu-devel] [PATCH 1/1] nbd: implement bdrv_get_info callback

[Paolo Bonzini]

On 18/01/2018 12:51, Edgar Kaziakhmedov wrote:
> 
> +static int nbd_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> +{
> +    if (bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP) {
> +        bdi->can_write_zeroes_with_unmap = true;
> +    }
> +    return 0;
> +}
> +

Other drivers set the flag always, while NBD only sets it if the server
knows the flag.

I think NBD is more correct, so:

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

However, it would be nice to remove can_write_zeroes_with_unmap from
BlockDriverInfo, and make bdrv_can_write_zeroes_with_unmap just return
!!(bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP).  Kevin, what do you
think?

Paolo

Re: [Qemu-devel] [PATCH 1/1] nbd: implement bdrv_get_info callback

[Edgar Kaziakhmedov]

PIng

So, let me know if I need to make any changes in patch

On 1/18/18 1:09 PM, Paolo Bonzini wrote:
> On 18/01/2018 12:51, Edgar Kaziakhmedov wrote:
>> +static int nbd_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>> +{
>> +    if (bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP) {
>> +        bdi->can_write_zeroes_with_unmap = true;
>> +    }
>> +    return 0;
>> +}
>> +
> Other drivers set the flag always, while NBD only sets it if the server
> knows the flag.
>
> I think NBD is more correct, so:
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> However, it would be nice to remove can_write_zeroes_with_unmap from
> BlockDriverInfo, and make bdrv_can_write_zeroes_with_unmap just return
> !!(bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP).  Kevin, what do you
> think?
>
> Paolo
> .
>

Re: [Qemu-devel] [PATCH 1/1] nbd: implement bdrv_get_info callback

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1593 bytes --]

On 01/26/2018 06:39 AM, Edgar Kaziakhmedov wrote:
> PIng
> 
> So, let me know if I need to make any changes in patch
> 
> On 1/18/18 1:09 PM, Paolo Bonzini wrote:
>> On 18/01/2018 12:51, Edgar Kaziakhmedov wrote:
>>> +static int nbd_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>>> +{
>>> +    if (bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP) {
>>> +        bdi->can_write_zeroes_with_unmap = true;
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>> Other drivers set the flag always, while NBD only sets it if the server
>> knows the flag.

Well, other drivers may be able to always implement it (NBD can only
implement it if the server supports WRITE_ZEROES - and I'm even in the
middle of working up an nbdkit patch [1] that makes it easier to write
an NBD server that specifically does not support WRITE_ZEROES to make
code paths like this easier to test)

[1]

>>
>> I think NBD is more correct, so:
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Agreed; I'm fine queueing this on my NBD queue, except I'd first like to
hear Kevin's opinion:

>>
>> However, it would be nice to remove can_write_zeroes_with_unmap from
>> BlockDriverInfo, and make bdrv_can_write_zeroes_with_unmap just return
>> !!(bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP).  Kevin, what do you
>> think?

Actually, I may even just give a shot at writing this alternative patch,
to make Kevin's decision easier.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/1] nbd: implement bdrv_get_info callback

[Edgar Kaziakhmedov]

On 01/26/2018 05:28 PM, Eric Blake wrote:
> On 01/26/2018 06:39 AM, Edgar Kaziakhmedov wrote:
>> PIng
>>
>> So, let me know if I need to make any changes in patch
>>
>> On 1/18/18 1:09 PM, Paolo Bonzini wrote:
>>> On 18/01/2018 12:51, Edgar Kaziakhmedov wrote:
>>>> +static int nbd_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>>>> +{
>>>> +    if (bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP) {
>>>> +        bdi->can_write_zeroes_with_unmap = true;
>>>> +    }
>>>> +    return 0;
>>>> +}
>>>> +
>>> Other drivers set the flag always, while NBD only sets it if the server
>>> knows the flag.
> Well, other drivers may be able to always implement it (NBD can only
> implement it if the server supports WRITE_ZEROES - and I'm even in the
> middle of working up an nbdkit patch [1] that makes it easier to write
> an NBD server that specifically does not support WRITE_ZEROES to make
> code paths like this easier to test)
>
> [1]
>
>>> I think NBD is more correct, so:
>>>
>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Agreed; I'm fine queueing this on my NBD queue, except I'd first like to
> hear Kevin's opinion:
>
>>> However, it would be nice to remove can_write_zeroes_with_unmap from
>>> BlockDriverInfo, and make bdrv_can_write_zeroes_with_unmap just return
>>> !!(bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP).  Kevin, what do you
>>> think?
> Actually, I may even just give a shot at writing this alternative patch,
> to make Kevin's decision easier.
But actually qcow2 performs some checks for version inside get_info 
callback before setting can_write_zeroes_with_unmap flag,
so we can't take into account such checks in 
bdrv_can_write_zeroes_with_unmap subroutine. Therefore, I don't think it 
is possible to do it like that.

Re: [Qemu-devel] [PATCH 1/1] nbd: implement bdrv_get_info callback

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 971 bytes --]

On 02/02/2018 08:06 AM, Edgar Kaziakhmedov wrote:

>>>> However, it would be nice to remove can_write_zeroes_with_unmap from
>>>> BlockDriverInfo, and make bdrv_can_write_zeroes_with_unmap just return
>>>> !!(bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP).  Kevin, what do you
>>>> think?
>> Actually, I may even just give a shot at writing this alternative patch,
>> to make Kevin's decision easier.
> But actually qcow2 performs some checks for version inside get_info
> callback before setting can_write_zeroes_with_unmap flag,
> so we can't take into account such checks in
> bdrv_can_write_zeroes_with_unmap subroutine. Therefore, I don't think it
> is possible to do it like that.

Here's the patch I proposed (it looks like I forgot to CC you):

https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06471.html

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/1] nbd: implement bdrv_get_info callback

[Edgar Kaziakhmedov]

On 02/02/2018 05:15 PM, Eric Blake wrote:
> On 02/02/2018 08:06 AM, Edgar Kaziakhmedov wrote:
>
>>>>> However, it would be nice to remove can_write_zeroes_with_unmap from
>>>>> BlockDriverInfo, and make bdrv_can_write_zeroes_with_unmap just return
>>>>> !!(bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP).  Kevin, what do you
>>>>> think?
>>> Actually, I may even just give a shot at writing this alternative patch,
>>> to make Kevin's decision easier.
>> But actually qcow2 performs some checks for version inside get_info
>> callback before setting can_write_zeroes_with_unmap flag,
>> so we can't take into account such checks in
>> bdrv_can_write_zeroes_with_unmap subroutine. Therefore, I don't think it
>> is possible to do it like that.
> Here's the patch I proposed (it looks like I forgot to CC you):
>
> https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06471.html
>
Yes, it was possible to move check to open, ok, get it.