diff mbox series

[1/1] nbd: implement bdrv_get_info callback

Message ID 20180118115158.17219-1-edgar.kaziakhmedov@virtuozzo.com
State New
Headers show
Series [1/1] nbd: implement bdrv_get_info callback | expand

Commit Message

Edgar Kaziakhmedov Jan. 18, 2018, 11:51 a.m. UTC 
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(+)

--
2.11.0

Comments

Paolo Bonzini Jan. 18, 2018, 12:09 p.m. UTC | #1 
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
Edgar Kaziakhmedov Jan. 26, 2018, 12:39 p.m. UTC | #2 
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
> .
>
Eric Blake Jan. 26, 2018, 2:28 p.m. UTC | #3 
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.
Edgar Kaziakhmedov Feb. 2, 2018, 2:06 p.m. UTC | #4 
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.
Eric Blake Feb. 2, 2018, 2:15 p.m. UTC | #5 
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
Edgar Kaziakhmedov Feb. 2, 2018, 2:23 p.m. UTC | #6 
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.
diff mbox series

Patch

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)