[Qemu-devel] [PATCH 0/2] sheepdog: remove huge BSS object

[Qemu-devel] [PATCH 0/2] sheepdog: remove huge BSS object

[Paolo Bonzini]

block/sheepdog.o has a 4M static variable that is 90% of QEMU's whole .bss
section.  Since it doesn't really have to be static, we can just use a
heap allocated block.  We can actually make it smaller too. :)

Patch 1 is a related cleanup since we're touching that area of the code.

Paolo

Paolo Bonzini (2):
  sheepdog: cleanup repeated expression
  sheepdog: remove huge BSS object

 block/sheepdog.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH 1/2] sheepdog: cleanup repeated expression

[Paolo Bonzini]

The expression "SD_INODE_SIZE - sizeof(inode.data_vdi_id)" already has a macro
defined for the same value (though with a nicer definition using offsetof).
Replace it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/sheepdog.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 4237132419..23cf5a8430 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2337,7 +2337,7 @@ static int sd_truncate(BlockDriverState *bs, int64_t offset,
     }
 
     /* we don't need to update entire object */
-    datalen = SD_INODE_SIZE - sizeof(s->inode.data_vdi_id);
+    datalen = SD_INODE_HEADER_SIZE;
     s->inode.vdi_size = offset;
     ret = write_object(fd, s->bs, (char *)&s->inode,
                        vid_to_vdi_oid(s->inode.vdi_id), s->inode.nr_copies,
@@ -2705,7 +2705,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
      */
     strncpy(s->inode.tag, sn_info->name, sizeof(s->inode.tag));
     /* we don't need to update entire object */
-    datalen = SD_INODE_SIZE - sizeof(s->inode.data_vdi_id);
+    datalen = SD_INODE_HEADER_SIZE;
     inode = g_malloc(datalen);
 
     /* refresh inode. */
@@ -2991,7 +2991,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
         /* we don't need to read entire object */
         ret = read_object(fd, s->bs, (char *)&inode,
                           vid_to_vdi_oid(vid),
-                          0, SD_INODE_SIZE - sizeof(inode.data_vdi_id), 0,
+                          0, SD_INODE_HEADER_SIZE, 0,
                           s->cache_flags);
 
         if (ret) {
-- 
2.17.0

[Qemu-devel] [PATCH 2/2] sheepdog: remove huge BSS object

[Paolo Bonzini]

block/sheepdog.o has a 4M static variable that is 90% of QEMU's whole .bss
section.  Replace it with a heap-allocated block, and make it smaller too
since only the inode header is actually being used.

bss size goes down from 4464280 to 269976.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/sheepdog.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 23cf5a8430..2068166e3e 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2940,13 +2940,14 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
     QEMUSnapshotInfo *sn_tab = NULL;
     unsigned wlen, rlen;
     int found = 0;
-    static SheepdogInode inode;
+    SheepdogInode *inode;
     unsigned long *vdi_inuse;
     unsigned int start_nr;
     uint64_t hval;
     uint32_t vid;
 
     vdi_inuse = g_malloc(max);
+    inode = g_malloc(SD_INODE_HEADER_SIZE);
 
     fd = connect_to_sdog(s, &local_err);
     if (fd < 0) {
@@ -2989,7 +2990,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
         }
 
         /* we don't need to read entire object */
-        ret = read_object(fd, s->bs, (char *)&inode,
+        ret = read_object(fd, s->bs, (char *)inode,
                           vid_to_vdi_oid(vid),
                           0, SD_INODE_HEADER_SIZE, 0,
                           s->cache_flags);
@@ -2998,17 +2999,17 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
             continue;
         }
 
-        if (!strcmp(inode.name, s->name) && is_snapshot(&inode)) {
-            sn_tab[found].date_sec = inode.snap_ctime >> 32;
-            sn_tab[found].date_nsec = inode.snap_ctime & 0xffffffff;
-            sn_tab[found].vm_state_size = inode.vm_state_size;
-            sn_tab[found].vm_clock_nsec = inode.vm_clock_nsec;
+        if (!strcmp(inode->name, s->name) && is_snapshot(inode)) {
+            sn_tab[found].date_sec = inode->snap_ctime >> 32;
+            sn_tab[found].date_nsec = inode->snap_ctime & 0xffffffff;
+            sn_tab[found].vm_state_size = inode->vm_state_size;
+            sn_tab[found].vm_clock_nsec = inode->vm_clock_nsec;
 
             snprintf(sn_tab[found].id_str, sizeof(sn_tab[found].id_str),
-                     "%" PRIu32, inode.snap_id);
+                     "%" PRIu32, inode->snap_id);
             pstrcpy(sn_tab[found].name,
-                    MIN(sizeof(sn_tab[found].name), sizeof(inode.tag)),
-                    inode.tag);
+                    MIN(sizeof(sn_tab[found].name), sizeof(inode->tag)),
+                    inode->tag);
             found++;
         }
     }
-- 
2.17.0

Re: [Qemu-devel] [PATCH 1/2] sheepdog: cleanup repeated expression

[Philippe Mathieu-Daudé]

On 05/22/2018 05:10 PM, Paolo Bonzini wrote:
> The expression "SD_INODE_SIZE - sizeof(inode.data_vdi_id)" already has a macro
> defined for the same value (though with a nicer definition using offsetof).
> Replace it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  block/sheepdog.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 4237132419..23cf5a8430 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -2337,7 +2337,7 @@ static int sd_truncate(BlockDriverState *bs, int64_t offset,
>      }
>  
>      /* we don't need to update entire object */
> -    datalen = SD_INODE_SIZE - sizeof(s->inode.data_vdi_id);
> +    datalen = SD_INODE_HEADER_SIZE;
>      s->inode.vdi_size = offset;
>      ret = write_object(fd, s->bs, (char *)&s->inode,
>                         vid_to_vdi_oid(s->inode.vdi_id), s->inode.nr_copies,
> @@ -2705,7 +2705,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>       */
>      strncpy(s->inode.tag, sn_info->name, sizeof(s->inode.tag));
>      /* we don't need to update entire object */
> -    datalen = SD_INODE_SIZE - sizeof(s->inode.data_vdi_id);
> +    datalen = SD_INODE_HEADER_SIZE;
>      inode = g_malloc(datalen);
>  
>      /* refresh inode. */
> @@ -2991,7 +2991,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>          /* we don't need to read entire object */
>          ret = read_object(fd, s->bs, (char *)&inode,
>                            vid_to_vdi_oid(vid),
> -                          0, SD_INODE_SIZE - sizeof(inode.data_vdi_id), 0,
> +                          0, SD_INODE_HEADER_SIZE, 0,
>                            s->cache_flags);
>  
>          if (ret) {
> 

Re: [Qemu-devel] [PATCH 2/2] sheepdog: remove huge BSS object

[Philippe Mathieu-Daudé]

On 05/22/2018 05:10 PM, Paolo Bonzini wrote:
> block/sheepdog.o has a 4M static variable that is 90% of QEMU's whole .bss
> section.  Replace it with a heap-allocated block, and make it smaller too
> since only the inode header is actually being used.
> 
> bss size goes down from 4464280 to 269976.

\o/

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

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  block/sheepdog.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 23cf5a8430..2068166e3e 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -2940,13 +2940,14 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>      QEMUSnapshotInfo *sn_tab = NULL;
>      unsigned wlen, rlen;
>      int found = 0;
> -    static SheepdogInode inode;
> +    SheepdogInode *inode;
>      unsigned long *vdi_inuse;
>      unsigned int start_nr;
>      uint64_t hval;
>      uint32_t vid;
>  
>      vdi_inuse = g_malloc(max);
> +    inode = g_malloc(SD_INODE_HEADER_SIZE);
>  
>      fd = connect_to_sdog(s, &local_err);
>      if (fd < 0) {
> @@ -2989,7 +2990,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>          }
>  
>          /* we don't need to read entire object */
> -        ret = read_object(fd, s->bs, (char *)&inode,
> +        ret = read_object(fd, s->bs, (char *)inode,
>                            vid_to_vdi_oid(vid),
>                            0, SD_INODE_HEADER_SIZE, 0,
>                            s->cache_flags);
> @@ -2998,17 +2999,17 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>              continue;
>          }
>  
> -        if (!strcmp(inode.name, s->name) && is_snapshot(&inode)) {
> -            sn_tab[found].date_sec = inode.snap_ctime >> 32;
> -            sn_tab[found].date_nsec = inode.snap_ctime & 0xffffffff;
> -            sn_tab[found].vm_state_size = inode.vm_state_size;
> -            sn_tab[found].vm_clock_nsec = inode.vm_clock_nsec;
> +        if (!strcmp(inode->name, s->name) && is_snapshot(inode)) {
> +            sn_tab[found].date_sec = inode->snap_ctime >> 32;
> +            sn_tab[found].date_nsec = inode->snap_ctime & 0xffffffff;
> +            sn_tab[found].vm_state_size = inode->vm_state_size;
> +            sn_tab[found].vm_clock_nsec = inode->vm_clock_nsec;
>  
>              snprintf(sn_tab[found].id_str, sizeof(sn_tab[found].id_str),
> -                     "%" PRIu32, inode.snap_id);
> +                     "%" PRIu32, inode->snap_id);
>              pstrcpy(sn_tab[found].name,
> -                    MIN(sizeof(sn_tab[found].name), sizeof(inode.tag)),
> -                    inode.tag);
> +                    MIN(sizeof(sn_tab[found].name), sizeof(inode->tag)),
> +                    inode->tag);
>              found++;
>          }
>      }
> 

Re: [Qemu-devel] [PATCH 2/2] sheepdog: remove huge BSS object

[Fam Zheng]

On Tue, 05/22 22:10, Paolo Bonzini wrote:
> block/sheepdog.o has a 4M static variable that is 90% of QEMU's whole .bss
> section.  Replace it with a heap-allocated block, and make it smaller too
> since only the inode header is actually being used.
> 
> bss size goes down from 4464280 to 269976.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/sheepdog.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 23cf5a8430..2068166e3e 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -2940,13 +2940,14 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>      QEMUSnapshotInfo *sn_tab = NULL;
>      unsigned wlen, rlen;
>      int found = 0;
> -    static SheepdogInode inode;
> +    SheepdogInode *inode;
>      unsigned long *vdi_inuse;
>      unsigned int start_nr;
>      uint64_t hval;
>      uint32_t vid;
>  
>      vdi_inuse = g_malloc(max);
> +    inode = g_malloc(SD_INODE_HEADER_SIZE);

Should you g_free() it before returning?


>  
>      fd = connect_to_sdog(s, &local_err);
>      if (fd < 0) {
> @@ -2989,7 +2990,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>          }
>  
>          /* we don't need to read entire object */
> -        ret = read_object(fd, s->bs, (char *)&inode,
> +        ret = read_object(fd, s->bs, (char *)inode,
>                            vid_to_vdi_oid(vid),
>                            0, SD_INODE_HEADER_SIZE, 0,
>                            s->cache_flags);
> @@ -2998,17 +2999,17 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>              continue;
>          }
>  
> -        if (!strcmp(inode.name, s->name) && is_snapshot(&inode)) {
> -            sn_tab[found].date_sec = inode.snap_ctime >> 32;
> -            sn_tab[found].date_nsec = inode.snap_ctime & 0xffffffff;
> -            sn_tab[found].vm_state_size = inode.vm_state_size;
> -            sn_tab[found].vm_clock_nsec = inode.vm_clock_nsec;
> +        if (!strcmp(inode->name, s->name) && is_snapshot(inode)) {
> +            sn_tab[found].date_sec = inode->snap_ctime >> 32;
> +            sn_tab[found].date_nsec = inode->snap_ctime & 0xffffffff;
> +            sn_tab[found].vm_state_size = inode->vm_state_size;
> +            sn_tab[found].vm_clock_nsec = inode->vm_clock_nsec;
>  
>              snprintf(sn_tab[found].id_str, sizeof(sn_tab[found].id_str),
> -                     "%" PRIu32, inode.snap_id);
> +                     "%" PRIu32, inode->snap_id);
>              pstrcpy(sn_tab[found].name,
> -                    MIN(sizeof(sn_tab[found].name), sizeof(inode.tag)),
> -                    inode.tag);
> +                    MIN(sizeof(sn_tab[found].name), sizeof(inode->tag)),
> +                    inode->tag);
>              found++;
>          }
>      }
> -- 
> 2.17.0
> 
> 

Re: [Qemu-devel] [PATCH 1/2] sheepdog: cleanup repeated expression

[Fam Zheng]

On Tue, 05/22 22:10, Paolo Bonzini wrote:
> The expression "SD_INODE_SIZE - sizeof(inode.data_vdi_id)" already has a macro
> defined for the same value (though with a nicer definition using offsetof).
> Replace it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 1/2] sheepdog: cleanup repeated expression

[Jeff Cody]

On Tue, May 22, 2018 at 10:10:55PM +0200, Paolo Bonzini wrote:
> The expression "SD_INODE_SIZE - sizeof(inode.data_vdi_id)" already has a macro
> defined for the same value (though with a nicer definition using offsetof).
> Replace it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/sheepdog.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 4237132419..23cf5a8430 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -2337,7 +2337,7 @@ static int sd_truncate(BlockDriverState *bs, int64_t offset,
>      }
>  
>      /* we don't need to update entire object */
> -    datalen = SD_INODE_SIZE - sizeof(s->inode.data_vdi_id);
> +    datalen = SD_INODE_HEADER_SIZE;
>      s->inode.vdi_size = offset;
>      ret = write_object(fd, s->bs, (char *)&s->inode,
>                         vid_to_vdi_oid(s->inode.vdi_id), s->inode.nr_copies,
> @@ -2705,7 +2705,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>       */
>      strncpy(s->inode.tag, sn_info->name, sizeof(s->inode.tag));
>      /* we don't need to update entire object */
> -    datalen = SD_INODE_SIZE - sizeof(s->inode.data_vdi_id);
> +    datalen = SD_INODE_HEADER_SIZE;
>      inode = g_malloc(datalen);
>  
>      /* refresh inode. */
> @@ -2991,7 +2991,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>          /* we don't need to read entire object */
>          ret = read_object(fd, s->bs, (char *)&inode,
>                            vid_to_vdi_oid(vid),
> -                          0, SD_INODE_SIZE - sizeof(inode.data_vdi_id), 0,
> +                          0, SD_INODE_HEADER_SIZE, 0,
>                            s->cache_flags);
>  
>          if (ret) {
> -- 
> 2.17.0
> 
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [PATCH 2/2] sheepdog: remove huge BSS object

[Philippe Mathieu-Daudé]

On 05/22/2018 10:43 PM, Fam Zheng wrote:
> On Tue, 05/22 22:10, Paolo Bonzini wrote:
>> block/sheepdog.o has a 4M static variable that is 90% of QEMU's whole .bss
>> section.  Replace it with a heap-allocated block, and make it smaller too
>> since only the inode header is actually being used.
>>
>> bss size goes down from 4464280 to 269976.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  block/sheepdog.c | 21 +++++++++++----------
>>  1 file changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index 23cf5a8430..2068166e3e 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -2940,13 +2940,14 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>>      QEMUSnapshotInfo *sn_tab = NULL;
>>      unsigned wlen, rlen;
>>      int found = 0;
>> -    static SheepdogInode inode;
>> +    SheepdogInode *inode;
>>      unsigned long *vdi_inuse;
>>      unsigned int start_nr;
>>      uint64_t hval;
>>      uint32_t vid;
>>  
>>      vdi_inuse = g_malloc(max);
>> +    inode = g_malloc(SD_INODE_HEADER_SIZE);
> 
> Should you g_free() it before returning?

Oops good catch

> 
> 
>>  
>>      fd = connect_to_sdog(s, &local_err);
>>      if (fd < 0) {
>> @@ -2989,7 +2990,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>>          }
>>  
>>          /* we don't need to read entire object */
>> -        ret = read_object(fd, s->bs, (char *)&inode,
>> +        ret = read_object(fd, s->bs, (char *)inode,
>>                            vid_to_vdi_oid(vid),
>>                            0, SD_INODE_HEADER_SIZE, 0,
>>                            s->cache_flags);
>> @@ -2998,17 +2999,17 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>>              continue;
>>          }
>>  
>> -        if (!strcmp(inode.name, s->name) && is_snapshot(&inode)) {
>> -            sn_tab[found].date_sec = inode.snap_ctime >> 32;
>> -            sn_tab[found].date_nsec = inode.snap_ctime & 0xffffffff;
>> -            sn_tab[found].vm_state_size = inode.vm_state_size;
>> -            sn_tab[found].vm_clock_nsec = inode.vm_clock_nsec;
>> +        if (!strcmp(inode->name, s->name) && is_snapshot(inode)) {
>> +            sn_tab[found].date_sec = inode->snap_ctime >> 32;
>> +            sn_tab[found].date_nsec = inode->snap_ctime & 0xffffffff;
>> +            sn_tab[found].vm_state_size = inode->vm_state_size;
>> +            sn_tab[found].vm_clock_nsec = inode->vm_clock_nsec;
>>  
>>              snprintf(sn_tab[found].id_str, sizeof(sn_tab[found].id_str),
>> -                     "%" PRIu32, inode.snap_id);
>> +                     "%" PRIu32, inode->snap_id);
>>              pstrcpy(sn_tab[found].name,
>> -                    MIN(sizeof(sn_tab[found].name), sizeof(inode.tag)),
>> -                    inode.tag);
>> +                    MIN(sizeof(sn_tab[found].name), sizeof(inode->tag)),
>> +                    inode->tag);
>>              found++;
>>          }
>>      }
>> -- 
>> 2.17.0
>>
>>
> 

[Qemu-devel] [PATCH 1/2] sheepdog: cleanup repeated expression

[Paolo Bonzini]

The expression "SD_INODE_SIZE - sizeof(inode.data_vdi_id)" already has a macro
defined for the same value (though with a nicer definition using offsetof).
Replace it.

Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/sheepdog.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 4237132419..23cf5a8430 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2337,7 +2337,7 @@ static int sd_truncate(BlockDriverState *bs, int64_t offset,
     }
 
     /* we don't need to update entire object */
-    datalen = SD_INODE_SIZE - sizeof(s->inode.data_vdi_id);
+    datalen = SD_INODE_HEADER_SIZE;
     s->inode.vdi_size = offset;
     ret = write_object(fd, s->bs, (char *)&s->inode,
                        vid_to_vdi_oid(s->inode.vdi_id), s->inode.nr_copies,
@@ -2705,7 +2705,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
      */
     strncpy(s->inode.tag, sn_info->name, sizeof(s->inode.tag));
     /* we don't need to update entire object */
-    datalen = SD_INODE_SIZE - sizeof(s->inode.data_vdi_id);
+    datalen = SD_INODE_HEADER_SIZE;
     inode = g_malloc(datalen);
 
     /* refresh inode. */
@@ -2991,7 +2991,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
         /* we don't need to read entire object */
         ret = read_object(fd, s->bs, (char *)&inode,
                           vid_to_vdi_oid(vid),
-                          0, SD_INODE_SIZE - sizeof(inode.data_vdi_id), 0,
+                          0, SD_INODE_HEADER_SIZE, 0,
                           s->cache_flags);
 
         if (ret) {
-- 
2.17.0