[Qemu-devel] [PATCH v2 0/2] Alignment checks cleanup

[Qemu-devel] [PATCH v2 0/2] Alignment checks cleanup

[Nir Soffer]

While working on 4k support, I noticed that there is lot of code using
BDRV_SECTOR_SIZE (512) for checking alignment. I wonder how this can work with
4k storage.

Lets start by cleaning up to make the code easier to understand:
- Use QEMU_IS_ALIGNED macro to check alignment
- Remove unneeded masks based on BDRV_SECTOR_SIZE

Nir Soffer (2):
  block: Use QEMU_IS_ALIGNED
  block: Remove unused masks

 block/bochs.c         | 4 ++--
 block/cloop.c         | 4 ++--
 block/dmg.c           | 4 ++--
 block/io.c            | 8 ++++----
 block/qcow2-cluster.c | 4 ++--
 block/qcow2.c         | 4 ++--
 block/vvfat.c         | 8 ++++----
 include/block/block.h | 2 --
 migration/block.c     | 2 +-
 qemu-img.c            | 2 +-
 10 files changed, 20 insertions(+), 22 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v2 1/2] block: Use QEMU_IS_ALIGNED

[Nir Soffer]

Replace instances of:

    (n & (BDRV_SECTOR_SIZE - 1)) == 0

And:

   (n & ~BDRV_SECTOR_MASK) == 0

With:

    QEMU_IS_ALIGNED(n, BDRV_SECTOR_SIZE)

Which reveals the intent of the code better, and makes it easier to
locate the code checking alignment.

Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 block/bochs.c         | 4 ++--
 block/cloop.c         | 4 ++--
 block/dmg.c           | 4 ++--
 block/io.c            | 8 ++++----
 block/qcow2-cluster.c | 4 ++--
 block/qcow2.c         | 4 ++--
 block/vvfat.c         | 8 ++++----
 qemu-img.c            | 2 +-
 8 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/block/bochs.c b/block/bochs.c
index 962f18592d..32bb83b268 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -248,8 +248,8 @@ bochs_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     QEMUIOVector local_qiov;
     int ret;
 
-    assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-    assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+    assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
+    assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
 
     qemu_iovec_init(&local_qiov, qiov->niov);
     qemu_co_mutex_lock(&s->lock);
diff --git a/block/cloop.c b/block/cloop.c
index 384c9735bb..4de94876d4 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -253,8 +253,8 @@ cloop_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     int nb_sectors = bytes >> BDRV_SECTOR_BITS;
     int ret, i;
 
-    assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-    assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+    assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
+    assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
 
     qemu_co_mutex_lock(&s->lock);
 
diff --git a/block/dmg.c b/block/dmg.c
index 45f6b28f17..4a045f2b3e 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -697,8 +697,8 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     int nb_sectors = bytes >> BDRV_SECTOR_BITS;
     int ret, i;
 
-    assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-    assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+    assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
+    assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
 
     qemu_co_mutex_lock(&s->lock);
 
diff --git a/block/io.c b/block/io.c
index 56bbf195bb..7508703ecd 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1080,8 +1080,8 @@ static int coroutine_fn bdrv_driver_preadv(BlockDriverState *bs,
     sector_num = offset >> BDRV_SECTOR_BITS;
     nb_sectors = bytes >> BDRV_SECTOR_BITS;
 
-    assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-    assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+    assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
+    assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
     assert(bytes <= BDRV_REQUEST_MAX_BYTES);
     assert(drv->bdrv_co_readv);
 
@@ -1133,8 +1133,8 @@ static int coroutine_fn bdrv_driver_pwritev(BlockDriverState *bs,
     sector_num = offset >> BDRV_SECTOR_BITS;
     nb_sectors = bytes >> BDRV_SECTOR_BITS;
 
-    assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-    assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+    assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
+    assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
     assert(bytes <= BDRV_REQUEST_MAX_BYTES);
 
     assert(drv->bdrv_co_writev);
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index cc5609e27a..f2de74690d 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -470,8 +470,8 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
 {
     if (bytes && bs->encrypted) {
         BDRVQcow2State *s = bs->opaque;
-        assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
-        assert((bytes & ~BDRV_SECTOR_MASK) == 0);
+        assert(QEMU_IS_ALIGNED(offset_in_cluster, BDRV_SECTOR_SIZE));
+        assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
         assert(s->crypto);
         if (qcow2_co_encrypt(bs, cluster_offset,
                              src_cluster_offset + offset_in_cluster,
diff --git a/block/qcow2.c b/block/qcow2.c
index 7c5a4859f7..e82961c9cf 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2072,8 +2072,8 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
             }
             if (bs->encrypted) {
                 assert(s->crypto);
-                assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-                assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+                assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
+                assert(QEMU_IS_ALIGNED(cur_bytes, BDRV_SECTOR_SIZE));
                 if (qcow2_co_decrypt(bs, cluster_offset, offset,
                                      cluster_data, cur_bytes) < 0) {
                     ret = -EIO;
diff --git a/block/vvfat.c b/block/vvfat.c
index f6c28805dd..019b8f1341 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1547,8 +1547,8 @@ vvfat_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     int nb_sectors = bytes >> BDRV_SECTOR_BITS;
     void *buf;
 
-    assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-    assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+    assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
+    assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
 
     buf = g_try_malloc(bytes);
     if (bytes && buf == NULL) {
@@ -3082,8 +3082,8 @@ vvfat_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     int nb_sectors = bytes >> BDRV_SECTOR_BITS;
     void *buf;
 
-    assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
-    assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+    assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));
+    assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
 
     buf = g_try_malloc(bytes);
     if (bytes && buf == NULL) {
diff --git a/qemu-img.c b/qemu-img.c
index 7daa05e51a..cff21facf0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2141,7 +2141,7 @@ static int img_convert(int argc, char **argv)
             int64_t sval;
 
             sval = cvtnum(optarg);
-            if (sval < 0 || sval & (BDRV_SECTOR_SIZE - 1) ||
+            if (sval < 0 || !QEMU_IS_ALIGNED(sval, BDRV_SECTOR_SIZE) ||
                 sval / BDRV_SECTOR_SIZE > MAX_BUF_SECTORS) {
                 error_report("Invalid buffer size for sparse output specified. "
                     "Valid sizes are multiples of %llu up to %llu. Select "
-- 
2.20.1

[Qemu-devel] [PATCH v2 2/2] block: Remove unused masks

[Nir Soffer]

Replace confusing usage:

    ~BDRV_SECTOR_MASK

With more clear:

    (BDRV_SECTOR_SIZE - 1)

Remove BDRV_SECTOR_MASK and the unused BDRV_BLOCK_OFFSET_MASK which was
it's last user.

Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 include/block/block.h | 2 --
 migration/block.c     | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 124ad40809..37c9de7446 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -143,7 +143,6 @@ typedef struct HDGeometry {
 
 #define BDRV_SECTOR_BITS   9
 #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
-#define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
 
 #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
                                      INT_MAX >> BDRV_SECTOR_BITS)
@@ -195,7 +194,6 @@ typedef struct HDGeometry {
 #define BDRV_BLOCK_ALLOCATED    0x10
 #define BDRV_BLOCK_EOF          0x20
 #define BDRV_BLOCK_RECURSE      0x40
-#define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK
 
 typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
 
diff --git a/migration/block.c b/migration/block.c
index aa747b55fa..92c36b68ec 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -906,7 +906,7 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
     do {
         addr = qemu_get_be64(f);
 
-        flags = addr & ~BDRV_SECTOR_MASK;
+        flags = addr & (BDRV_SECTOR_SIZE - 1);
         addr >>= BDRV_SECTOR_BITS;
 
         if (flags & BLK_MIG_FLAG_DEVICE_BLOCK) {
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 2/2] block: Remove unused masks

[Juan Quintela]

Nir Soffer <nirsof@gmail.com> wrote:
> Replace confusing usage:
>
>     ~BDRV_SECTOR_MASK
>
> With more clear:
>
>     (BDRV_SECTOR_SIZE - 1)
>
> Remove BDRV_SECTOR_MASK and the unused BDRV_BLOCK_OFFSET_MASK which was
> it's last user.
>
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/2] block: Remove unused masks

[John Snow]

On 8/27/19 2:59 PM, Nir Soffer wrote:
> Replace confusing usage:
> 
>     ~BDRV_SECTOR_MASK
> 
> With more clear:
> 
>     (BDRV_SECTOR_SIZE - 1)
> 
> Remove BDRV_SECTOR_MASK and the unused BDRV_BLOCK_OFFSET_MASK which was
> it's last user.
> 

Kind of lateral in my opinion, but if there was only ONE user across TWO
definitions, then for sure it can go, especially because using the
ALIGNED macros is indeed nicer and should be encouraged.

Reviewed-by: John Snow <jsnow@redhat.com>

> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
>  include/block/block.h | 2 --
>  migration/block.c     | 2 +-
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 124ad40809..37c9de7446 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -143,7 +143,6 @@ typedef struct HDGeometry {
>  
>  #define BDRV_SECTOR_BITS   9
>  #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
> -#define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
>  
>  #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
>                                       INT_MAX >> BDRV_SECTOR_BITS)
> @@ -195,7 +194,6 @@ typedef struct HDGeometry {
>  #define BDRV_BLOCK_ALLOCATED    0x10
>  #define BDRV_BLOCK_EOF          0x20
>  #define BDRV_BLOCK_RECURSE      0x40
> -#define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK
>  
>  typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
>  
> diff --git a/migration/block.c b/migration/block.c
> index aa747b55fa..92c36b68ec 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -906,7 +906,7 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
>      do {
>          addr = qemu_get_be64(f);
>  
> -        flags = addr & ~BDRV_SECTOR_MASK;
> +        flags = addr & (BDRV_SECTOR_SIZE - 1);
>          addr >>= BDRV_SECTOR_BITS;
>  
>          if (flags & BLK_MIG_FLAG_DEVICE_BLOCK) {
> 

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/2] Alignment checks cleanup

[John Snow]

On 8/27/19 2:59 PM, Nir Soffer wrote:
> While working on 4k support, I noticed that there is lot of code using
> BDRV_SECTOR_SIZE (512) for checking alignment. I wonder how this can work with
> 4k storage.
> 
> Lets start by cleaning up to make the code easier to understand:
> - Use QEMU_IS_ALIGNED macro to check alignment
> - Remove unneeded masks based on BDRV_SECTOR_SIZE
> 
> Nir Soffer (2):
>   block: Use QEMU_IS_ALIGNED
>   block: Remove unused masks
> 
>  block/bochs.c         | 4 ++--
>  block/cloop.c         | 4 ++--
>  block/dmg.c           | 4 ++--
>  block/io.c            | 8 ++++----
>  block/qcow2-cluster.c | 4 ++--
>  block/qcow2.c         | 4 ++--
>  block/vvfat.c         | 8 ++++----
>  include/block/block.h | 2 --
>  migration/block.c     | 2 +-
>  qemu-img.c            | 2 +-
>  10 files changed, 20 insertions(+), 22 deletions(-)
> 

V2 changelog?

(Looks like adding patch 2 as a result of changing away users from the
BDRV_SECTOR_MASK.)

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/2] Alignment checks cleanup

[Nir Soffer]

On Wed, Aug 28, 2019 at 11:14 PM John Snow <jsnow@redhat.com> wrote:

>
>
> On 8/27/19 2:59 PM, Nir Soffer wrote:
> > While working on 4k support, I noticed that there is lot of code using
> > BDRV_SECTOR_SIZE (512) for checking alignment. I wonder how this can
> work with
> > 4k storage.
> >
> > Lets start by cleaning up to make the code easier to understand:
> > - Use QEMU_IS_ALIGNED macro to check alignment
> > - Remove unneeded masks based on BDRV_SECTOR_SIZE
> >
> > Nir Soffer (2):
> >   block: Use QEMU_IS_ALIGNED
> >   block: Remove unused masks
> >
> >  block/bochs.c         | 4 ++--
> >  block/cloop.c         | 4 ++--
> >  block/dmg.c           | 4 ++--
> >  block/io.c            | 8 ++++----
> >  block/qcow2-cluster.c | 4 ++--
> >  block/qcow2.c         | 4 ++--
> >  block/vvfat.c         | 8 ++++----
> >  include/block/block.h | 2 --
> >  migration/block.c     | 2 +-
> >  qemu-img.c            | 2 +-
> >  10 files changed, 20 insertions(+), 22 deletions(-)
> >
>
> V2 changelog?


> (Looks like adding patch 2 as a result of changing away users from the
> BDRV_SECTOR_MASK.)
>

Right.

Changes since v1:
- Replace usage of BDRV_SECTOR_MASK in qcow2-cluster.c (Max)
- Remove unused masks

v1 was here:
https://lists.nongnu.org/archive/html/qemu-block/2019-08/msg00875.html


> Reviewed-by: John Snow <jsnow@redhat.com>


Thanks!

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/2] Alignment checks cleanup

[Nir Soffer]

Max, can you review again?

On Fri, Aug 30, 2019 at 11:25 PM Nir Soffer <nsoffer@redhat.com> wrote:

> On Wed, Aug 28, 2019 at 11:14 PM John Snow <jsnow@redhat.com> wrote:
>
>>
>>
>> On 8/27/19 2:59 PM, Nir Soffer wrote:
>> > While working on 4k support, I noticed that there is lot of code using
>> > BDRV_SECTOR_SIZE (512) for checking alignment. I wonder how this can
>> work with
>> > 4k storage.
>> >
>> > Lets start by cleaning up to make the code easier to understand:
>> > - Use QEMU_IS_ALIGNED macro to check alignment
>> > - Remove unneeded masks based on BDRV_SECTOR_SIZE
>> >
>> > Nir Soffer (2):
>> >   block: Use QEMU_IS_ALIGNED
>> >   block: Remove unused masks
>> >
>> >  block/bochs.c         | 4 ++--
>> >  block/cloop.c         | 4 ++--
>> >  block/dmg.c           | 4 ++--
>> >  block/io.c            | 8 ++++----
>> >  block/qcow2-cluster.c | 4 ++--
>> >  block/qcow2.c         | 4 ++--
>> >  block/vvfat.c         | 8 ++++----
>> >  include/block/block.h | 2 --
>> >  migration/block.c     | 2 +-
>> >  qemu-img.c            | 2 +-
>> >  10 files changed, 20 insertions(+), 22 deletions(-)
>> >
>>
>> V2 changelog?
>
>
>> (Looks like adding patch 2 as a result of changing away users from the
>> BDRV_SECTOR_MASK.)
>>
>
> Right.
>
> Changes since v1:
> - Replace usage of BDRV_SECTOR_MASK in qcow2-cluster.c (Max)
> - Remove unused masks
>
> v1 was here:
> https://lists.nongnu.org/archive/html/qemu-block/2019-08/msg00875.html
>
>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>
>
> Thanks!
>

Re: [Qemu-devel] [PATCH v2 0/2] Alignment checks cleanup

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1360 bytes --]

On 27.08.19 20:59, Nir Soffer wrote:
> While working on 4k support, I noticed that there is lot of code using
> BDRV_SECTOR_SIZE (512) for checking alignment. I wonder how this can work with
> 4k storage.
> 
> Lets start by cleaning up to make the code easier to understand:
> - Use QEMU_IS_ALIGNED macro to check alignment
> - Remove unneeded masks based on BDRV_SECTOR_SIZE
> 
> Nir Soffer (2):
>   block: Use QEMU_IS_ALIGNED
>   block: Remove unused masks
> 
>  block/bochs.c         | 4 ++--
>  block/cloop.c         | 4 ++--
>  block/dmg.c           | 4 ++--
>  block/io.c            | 8 ++++----
>  block/qcow2-cluster.c | 4 ++--
>  block/qcow2.c         | 4 ++--
>  block/vvfat.c         | 8 ++++----
>  include/block/block.h | 2 --
>  migration/block.c     | 2 +-
>  qemu-img.c            | 2 +-
>  10 files changed, 20 insertions(+), 22 deletions(-)

Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block


(I think you should check the setting of git config’s user.email in your
qemu directory.  Even if you send the patches through a gmail address
(which I don’t quite know why you’re doing that, but it isn’t like that
really matters), the patches should then still contain a “From: ” that
shows the actual author as set by user.name and user.email.)

Max


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