Message ID | 20180118115158.17219-1-edgar.kaziakhmedov@virtuozzo.com |
---|---|
State | New |
Headers | show |
Series | [1/1] nbd: implement bdrv_get_info callback | expand |
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
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 > . >
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.
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.
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
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 --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)
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