[01/11] block: switch blk_write_compressed() to byte-based interface
diff mbox

Message ID 1464686130-12265-2-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev May 31, 2016, 9:15 a.m. UTC
From: Pavel Butsykin <pbutsykin@virtuozzo.com>

This is a preparatory patch, which continues the general trend of the transition
to the byte-based interfaces.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: John Snow <jsnow@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c          | 8 ++++----
 block/io.c                     | 9 +++++----
 include/block/block.h          | 4 ++--
 include/sysemu/block-backend.h | 4 ++--
 qemu-img.c                     | 6 ++++--
 qemu-io-cmds.c                 | 2 +-
 6 files changed, 18 insertions(+), 15 deletions(-)

Comments

Stefan Hajnoczi June 13, 2016, 1:01 p.m. UTC | #1
On Tue, May 31, 2016 at 12:15:20PM +0300, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> This is a preparatory patch, which continues the general trend of the transition
> to the byte-based interfaces.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c          | 8 ++++----
>  block/io.c                     | 9 +++++----
>  include/block/block.h          | 4 ++--
>  include/sysemu/block-backend.h | 4 ++--
>  qemu-img.c                     | 6 ++++--
>  qemu-io-cmds.c                 | 2 +-
>  6 files changed, 18 insertions(+), 15 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Eric Blake June 13, 2016, 2:23 p.m. UTC | #2
On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> This is a preparatory patch, which continues the general trend of the transition
> to the byte-based interfaces.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c          | 8 ++++----
>  block/io.c                     | 9 +++++----
>  include/block/block.h          | 4 ++--
>  include/sysemu/block-backend.h | 4 ++--
>  qemu-img.c                     | 6 ++++--
>  qemu-io-cmds.c                 | 2 +-
>  6 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 34500e6..3c1fc50 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1477,15 +1477,15 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
>                            flags | BDRV_REQ_ZERO_WRITE);
>  }
>  
> -int blk_write_compressed(BlockBackend *blk, int64_t sector_num,
> -                         const uint8_t *buf, int nb_sectors)
> +int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
> +                          int count)

Why are you switching the type of buf?  It's not necessarily wrong, but
the commit message should call it out as intentional.


> -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
> -                          const uint8_t *buf, int nb_sectors)
> +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
> +                           const void *buf, int count)
>  {
>      BlockDriver *drv = bs->drv;
>      int ret;
> @@ -1791,14 +1791,15 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>      if (!drv->bdrv_write_compressed) {
>          return -ENOTSUP;
>      }
> -    ret = bdrv_check_request(bs, sector_num, nb_sectors);
> +    ret = bdrv_check_byte_request(bs, offset, count);
>      if (ret < 0) {
>          return ret;
>      }
>  
>      assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>  
> -    return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
> +    return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
> +                                      count >> BDRV_SECTOR_BITS);

If you are going to shift right, you need to first assert that offset
and count are aligned (and thus that our call to a sector interface
isn't going to operate on the wrong data).  See for example commit 166fe960.
Pavel Butsykin June 22, 2016, 12:25 p.m. UTC | #3
On 13.06.2016 17:23, Eric Blake wrote:
> On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> This is a preparatory patch, which continues the general trend of the transition
>> to the byte-based interfaces.
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: John Snow <jsnow@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   block/block-backend.c          | 8 ++++----
>>   block/io.c                     | 9 +++++----
>>   include/block/block.h          | 4 ++--
>>   include/sysemu/block-backend.h | 4 ++--
>>   qemu-img.c                     | 6 ++++--
>>   qemu-io-cmds.c                 | 2 +-
>>   6 files changed, 18 insertions(+), 15 deletions(-)
>>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 34500e6..3c1fc50 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -1477,15 +1477,15 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
>>                             flags | BDRV_REQ_ZERO_WRITE);
>>   }
>>
>> -int blk_write_compressed(BlockBackend *blk, int64_t sector_num,
>> -                         const uint8_t *buf, int nb_sectors)
>> +int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
>> +                          int count)
>
> Why are you switching the type of buf?  It's not necessarily wrong, but
> the commit message should call it out as intentional.

Here I just tried to make the interface like blk_pwrite, it has no other
meaning more..

>
>> -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>> -                          const uint8_t *buf, int nb_sectors)
>> +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
>> +                           const void *buf, int count)
>>   {
>>       BlockDriver *drv = bs->drv;
>>       int ret;
>> @@ -1791,14 +1791,15 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>       if (!drv->bdrv_write_compressed) {
>>           return -ENOTSUP;
>>       }
>> -    ret = bdrv_check_request(bs, sector_num, nb_sectors);
>> +    ret = bdrv_check_byte_request(bs, offset, count);
>>       if (ret < 0) {
>>           return ret;
>>       }
>>
>>       assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>>
>> -    return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
>> +    return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
>> +                                      count >> BDRV_SECTOR_BITS);
>
> If you are going to shift right, you need to first assert that offset
> and count are aligned (and thus that our call to a sector interface
> isn't going to operate on the wrong data).  See for example commit 166fe960.
>

ok, thanks
Kevin Wolf June 28, 2016, 10:53 a.m. UTC | #4
Am 13.06.2016 um 16:23 hat Eric Blake geschrieben:
> On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
> > From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> > 
> > This is a preparatory patch, which continues the general trend of the transition
> > to the byte-based interfaces.
> > 
> > Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > CC: Jeff Cody <jcody@redhat.com>
> > CC: Markus Armbruster <armbru@redhat.com>
> > CC: Eric Blake <eblake@redhat.com>
> > CC: John Snow <jsnow@redhat.com>
> > CC: Stefan Hajnoczi <stefanha@redhat.com>
> > CC: Kevin Wolf <kwolf@redhat.com>

> > -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
> > -                          const uint8_t *buf, int nb_sectors)
> > +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
> > +                           const void *buf, int count)
> >  {
> >      BlockDriver *drv = bs->drv;
> >      int ret;
> > @@ -1791,14 +1791,15 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
> >      if (!drv->bdrv_write_compressed) {
> >          return -ENOTSUP;
> >      }
> > -    ret = bdrv_check_request(bs, sector_num, nb_sectors);
> > +    ret = bdrv_check_byte_request(bs, offset, count);
> >      if (ret < 0) {
> >          return ret;
> >      }
> >  
> >      assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> >  
> > -    return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
> > +    return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
> > +                                      count >> BDRV_SECTOR_BITS);
> 
> If you are going to shift right, you need to first assert that offset
> and count are aligned (and thus that our call to a sector interface
> isn't going to operate on the wrong data).  See for example commit 166fe960.

Yes, I would like to have these assertions at least.

But I'm wondering what the point of converting the interface is when we
don't intend to actually support sub-sector requests and the sector
alignment is still required at the end of the series.

Kevin
Pavel Butsykin June 28, 2016, 11:32 a.m. UTC | #5
On 28.06.2016 13:53, Kevin Wolf wrote:
> Am 13.06.2016 um 16:23 hat Eric Blake geschrieben:
>> On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
>>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>>
>>> This is a preparatory patch, which continues the general trend of the transition
>>> to the byte-based interfaces.
>>>
>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Jeff Cody <jcody@redhat.com>
>>> CC: Markus Armbruster <armbru@redhat.com>
>>> CC: Eric Blake <eblake@redhat.com>
>>> CC: John Snow <jsnow@redhat.com>
>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>> CC: Kevin Wolf <kwolf@redhat.com>
>
>>> -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>> -                          const uint8_t *buf, int nb_sectors)
>>> +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
>>> +                           const void *buf, int count)
>>>   {
>>>       BlockDriver *drv = bs->drv;
>>>       int ret;
>>> @@ -1791,14 +1791,15 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>>       if (!drv->bdrv_write_compressed) {
>>>           return -ENOTSUP;
>>>       }
>>> -    ret = bdrv_check_request(bs, sector_num, nb_sectors);
>>> +    ret = bdrv_check_byte_request(bs, offset, count);
>>>       if (ret < 0) {
>>>           return ret;
>>>       }
>>>
>>>       assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>>>
>>> -    return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
>>> +    return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
>>> +                                      count >> BDRV_SECTOR_BITS);
>>
>> If you are going to shift right, you need to first assert that offset
>> and count are aligned (and thus that our call to a sector interface
>> isn't going to operate on the wrong data).  See for example commit 166fe960.
>
> Yes, I would like to have these assertions at least.
>
> But I'm wondering what the point of converting the interface is when we
> don't intend to actually support sub-sector requests and the sector
> alignment is still required at the end of the series.

Because at the time of sending patches, the format drivers still had
the sector-based interfaces. In the end, the assertions are not
necessary, because the interface format driver now also byte-based.

> Kevin
>

Patch
diff mbox

diff --git a/block/block-backend.c b/block/block-backend.c
index 34500e6..3c1fc50 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1477,15 +1477,15 @@  int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
                           flags | BDRV_REQ_ZERO_WRITE);
 }
 
-int blk_write_compressed(BlockBackend *blk, int64_t sector_num,
-                         const uint8_t *buf, int nb_sectors)
+int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
+                          int count)
 {
-    int ret = blk_check_request(blk, sector_num, nb_sectors);
+    int ret = blk_check_byte_request(blk, offset, count);
     if (ret < 0) {
         return ret;
     }
 
-    return bdrv_write_compressed(blk_bs(blk), sector_num, buf, nb_sectors);
+    return bdrv_pwrite_compressed(blk_bs(blk), offset, buf, count);
 }
 
 int blk_truncate(BlockBackend *blk, int64_t offset)
diff --git a/block/io.c b/block/io.c
index 2d832aa..c5bb6ae 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1779,8 +1779,8 @@  int bdrv_is_allocated_above(BlockDriverState *top,
     return 0;
 }
 
-int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
-                          const uint8_t *buf, int nb_sectors)
+int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
+                           const void *buf, int count)
 {
     BlockDriver *drv = bs->drv;
     int ret;
@@ -1791,14 +1791,15 @@  int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
     if (!drv->bdrv_write_compressed) {
         return -ENOTSUP;
     }
-    ret = bdrv_check_request(bs, sector_num, nb_sectors);
+    ret = bdrv_check_byte_request(bs, offset, count);
     if (ret < 0) {
         return ret;
     }
 
     assert(QLIST_EMPTY(&bs->dirty_bitmaps));
 
-    return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
+    return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
+                                      count >> BDRV_SECTOR_BITS);
 }
 
 int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
diff --git a/include/block/block.h b/include/block/block.h
index 70ea299..b687c0d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -421,8 +421,8 @@  const char *bdrv_get_node_name(const BlockDriverState *bs);
 const char *bdrv_get_device_name(const BlockDriverState *bs);
 const char *bdrv_get_device_or_node_name(const BlockDriverState *bs);
 int bdrv_get_flags(BlockDriverState *bs);
-int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
-                          const uint8_t *buf, int nb_sectors);
+int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
+                           const void *buf, int count);
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
 ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
 void bdrv_round_to_clusters(BlockDriverState *bs,
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index c04af8e..57df069 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -203,8 +203,8 @@  void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
                   BlockCompletionFunc *cb, void *opaque);
 int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
                                       int count, BdrvRequestFlags flags);
-int blk_write_compressed(BlockBackend *blk, int64_t sector_num,
-                         const uint8_t *buf, int nb_sectors);
+int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
+                          int count);
 int blk_truncate(BlockBackend *blk, int64_t offset);
 int blk_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors);
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
diff --git a/qemu-img.c b/qemu-img.c
index 4b56ad3..eb744d4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1580,7 +1580,9 @@  static int convert_write(ImgConvertState *s, int64_t sector_num, int nb_sectors,
                     break;
                 }
 
-                ret = blk_write_compressed(s->target, sector_num, buf, n);
+                ret = blk_pwrite_compressed(s->target,
+                                            sector_num << BDRV_SECTOR_BITS,
+                                            buf, n << BDRV_SECTOR_BITS);
                 if (ret < 0) {
                     return ret;
                 }
@@ -1717,7 +1719,7 @@  static int convert_do_copy(ImgConvertState *s)
 
     if (s->compressed) {
         /* signal EOF to align */
-        ret = blk_write_compressed(s->target, 0, NULL, 0);
+        ret = blk_pwrite_compressed(s->target, 0, NULL, 0);
         if (ret < 0) {
             goto fail;
         }
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 09e879f..2ae3b14 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -504,7 +504,7 @@  static int do_write_compressed(BlockBackend *blk, char *buf, int64_t offset,
         return -ERANGE;
     }
 
-    ret = blk_write_compressed(blk, offset >> 9, (uint8_t *)buf, count >> 9);
+    ret = blk_pwrite_compressed(blk, offset, buf, count);
     if (ret < 0) {
         return ret;
     }