[PULL 00/10] Bitmaps patches

John Snow posted 10 patches 4 years ago
Failed in applying to current master (apply log)
There is a newer version of this series
include/block/dirty-bitmap.h |   9 +-
include/qemu/hbitmap.h       |  95 +++--------
block/dirty-bitmap.c         |  16 +-
block/qcow2-bitmap.c         |  15 +-
nbd/server.c                 | 251 ++++++++++++++--------------
tests/test-hbitmap.c         | 316 +++++++++++++----------------------
util/hbitmap.c               | 134 +++++++++------
7 files changed, 375 insertions(+), 461 deletions(-)
[PULL 00/10] Bitmaps patches
Posted by John Snow 4 years ago
The following changes since commit 6e8a73e911f066527e775e04b98f31ebd19db600:

  Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2020-03-11 14:41:27 +0000)

are available in the Git repository at:

  https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request

for you to fetch changes up to 34b456d485a4df3a88116fb5ef0c418f2f12990d:

  block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty (2020-03-12 16:36:46 -0400)

----------------------------------------------------------------
Pull request

----------------------------------------------------------------

Vladimir Sementsov-Ogievskiy (10):
  hbitmap: assert that we don't create bitmap larger than INT64_MAX
  hbitmap: move hbitmap_iter_next_word to hbitmap.c
  hbitmap: unpublish hbitmap_iter_skip_words
  hbitmap: drop meta bitmaps as they are unused
  block/dirty-bitmap: switch _next_dirty_area and _next_zero to int64_t
  block/dirty-bitmap: add _next_dirty API
  block/dirty-bitmap: improve _next_dirty_area API
  nbd/server: introduce NBDExtentArray
  nbd/server: use bdrv_dirty_bitmap_next_dirty_area
  block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty

 include/block/dirty-bitmap.h |   9 +-
 include/qemu/hbitmap.h       |  95 +++--------
 block/dirty-bitmap.c         |  16 +-
 block/qcow2-bitmap.c         |  15 +-
 nbd/server.c                 | 251 ++++++++++++++--------------
 tests/test-hbitmap.c         | 316 +++++++++++++----------------------
 util/hbitmap.c               | 134 +++++++++------
 7 files changed, 375 insertions(+), 461 deletions(-)

-- 
2.21.1

Re: [PULL 00/10] Bitmaps patches
Posted by Peter Maydell 4 years ago
On Tue, 17 Mar 2020 at 04:38, John Snow <jsnow@redhat.com> wrote:
>
> The following changes since commit 6e8a73e911f066527e775e04b98f31ebd19db600:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2020-03-11 14:41:27 +0000)
>
> are available in the Git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request
>
> for you to fetch changes up to 34b456d485a4df3a88116fb5ef0c418f2f12990d:
>
>   block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty (2020-03-12 16:36:46 -0400)
>
> ----------------------------------------------------------------
> Pull request
>
> ---------------------------------------------------------------

Hi; this fails to compile with clang:

/home/petmay01/linaro/qemu-for-merges/nbd/server.c:1937:1: error:
unused function 'glib_listautoptr_cleanup_NBDExtentArray'
[-Werror,-Wunused-function]
G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
^
/usr/include/glib-2.0/glib/gmacros.h:462:22: note: expanded from macro
'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
  static inline void _GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName) (GList
**_l) { g_list_free_full (*_l, (GDestroyNotify) func); } \
                     ^
/usr/include/glib-2.0/glib/gmacros.h:443:48: note: expanded from macro
'_GLIB_AUTOPTR_LIST_FUNC_NAME'
#define _GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName)
glib_listautoptr_cleanup_##TypeName
                                               ^
<scratch space>:49:1: note: expanded from here
glib_listautoptr_cleanup_NBDExtentArray
^

thanks
-- PMM


Re: [PULL 00/10] Bitmaps patches
Posted by Eric Blake 4 years ago
On 3/17/20 9:00 AM, Peter Maydell wrote:
> On Tue, 17 Mar 2020 at 04:38, John Snow <jsnow@redhat.com> wrote:

>>
>>    block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty (2020-03-12 16:36:46 -0400)
>>
>> ----------------------------------------------------------------
>> Pull request
>>
>> ---------------------------------------------------------------
> 
> Hi; this fails to compile with clang:
> 
> /home/petmay01/linaro/qemu-for-merges/nbd/server.c:1937:1: error:
> unused function 'glib_listautoptr_cleanup_NBDExtentArray'
> [-Werror,-Wunused-function]
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
> ^
> /usr/include/glib-2.0/glib/gmacros.h:462:22: note: expanded from macro
> 'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
>    static inline void _GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName) (GList
> **_l) { g_list_free_full (*_l, (GDestroyNotify) func); } \
>                       ^
> /usr/include/glib-2.0/glib/gmacros.h:443:48: note: expanded from macro
> '_GLIB_AUTOPTR_LIST_FUNC_NAME'
> #define _GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName)
> glib_listautoptr_cleanup_##TypeName
>                                                 ^
> <scratch space>:49:1: note: expanded from here
> glib_listautoptr_cleanup_NBDExtentArray
> ^

Should we add -Wno-unused-function to CFLAGS when dealing with a version 
of clang that complains about that version of glib's headers?  Is it 
fixed in a newer version of glib, where we could just backport a newer 
definition of G_DEFINE_AUTOPTR_CLEANUP_FUNC() that adds whatever 
annotations are needed to shut the compiler up?

On IRC, danpb pointed me to libvirt's solution:
https://libvirt.org/git/?p=libvirt.git;a=commit;h=44e7f029

Maybe we just write our own macro wrapper around 
G_DEFINE_AUTOPTR_CLEANUP_FUNC which takes care of adding necessary 
annotations and use that instead (and our macro name might be shorter...)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PULL 00/10] Bitmaps patches
Posted by Daniel P. Berrangé 4 years ago
On Tue, Mar 17, 2020 at 09:40:00AM -0500, Eric Blake wrote:
> On 3/17/20 9:00 AM, Peter Maydell wrote:
> > On Tue, 17 Mar 2020 at 04:38, John Snow <jsnow@redhat.com> wrote:
> 
> > > 
> > >    block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty (2020-03-12 16:36:46 -0400)
> > > 
> > > ----------------------------------------------------------------
> > > Pull request
> > > 
> > > ---------------------------------------------------------------
> > 
> > Hi; this fails to compile with clang:
> > 
> > /home/petmay01/linaro/qemu-for-merges/nbd/server.c:1937:1: error:
> > unused function 'glib_listautoptr_cleanup_NBDExtentArray'
> > [-Werror,-Wunused-function]
> > G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
> > ^
> > /usr/include/glib-2.0/glib/gmacros.h:462:22: note: expanded from macro
> > 'G_DEFINE_AUTOPTR_CLEANUP_FUNC'
> >    static inline void _GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName) (GList
> > **_l) { g_list_free_full (*_l, (GDestroyNotify) func); } \
> >                       ^
> > /usr/include/glib-2.0/glib/gmacros.h:443:48: note: expanded from macro
> > '_GLIB_AUTOPTR_LIST_FUNC_NAME'
> > #define _GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName)
> > glib_listautoptr_cleanup_##TypeName
> >                                                 ^
> > <scratch space>:49:1: note: expanded from here
> > glib_listautoptr_cleanup_NBDExtentArray
> > ^
> 
> Should we add -Wno-unused-function to CFLAGS when dealing with a version of
> clang that complains about that version of glib's headers?  Is it fixed in a
> newer version of glib, where we could just backport a newer definition of
> G_DEFINE_AUTOPTR_CLEANUP_FUNC() that adds whatever annotations are needed to
> shut the compiler up?
> 
> On IRC, danpb pointed me to libvirt's solution:
> https://libvirt.org/git/?p=libvirt.git;a=commit;h=44e7f029
> 
> Maybe we just write our own macro wrapper around
> G_DEFINE_AUTOPTR_CLEANUP_FUNC which takes care of adding necessary
> annotations and use that instead (and our macro name might be shorter...)

My preference is to stick with regular glib functions/macros whereever
practical, rather than inventing QEMU replacements which add a knowledge
burden for contributors. That's why we moved to -Wno-unused-function in
libvirt.

I don't feel like -Wno-unused-function looses anything significant, as
the GCC builds will still be reporting unused functions which will
catch majority of cases.

Possibly we could figure out a patch for glib upstream that uses pragma
push/pop to squelch the warning ?  They are quite receptive to patches
IME. 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PULL 00/10] Bitmaps patches
Posted by Peter Maydell 4 years ago
On Tue, 17 Mar 2020 at 14:57, Daniel P. Berrangé <berrange@redhat.com> wrote:
> I don't feel like -Wno-unused-function looses anything significant, as
> the GCC builds will still be reporting unused functions which will
> catch majority of cases.

The most interesting difference is that clang will catch unused
static inline functions which gcc does not.

thanks
-- PMM


Re: [PULL 00/10] Bitmaps patches
Posted by Daniel P. Berrangé 4 years ago
On Tue, Mar 17, 2020 at 03:00:48PM +0000, Peter Maydell wrote:
> On Tue, 17 Mar 2020 at 14:57, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > I don't feel like -Wno-unused-function looses anything significant, as
> > the GCC builds will still be reporting unused functions which will
> > catch majority of cases.
> 
> The most interesting difference is that clang will catch unused
> static inline functions which gcc does not.

That's mostly just about dead code cruft detection IIUC. That code won't
make it into the binary if it isn't used.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


Re: [PULL 00/10] Bitmaps patches
Posted by Peter Maydell 4 years ago
On Tue, 17 Mar 2020 at 15:05, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Mar 17, 2020 at 03:00:48PM +0000, Peter Maydell wrote:
> > On Tue, 17 Mar 2020 at 14:57, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > I don't feel like -Wno-unused-function looses anything significant, as
> > > the GCC builds will still be reporting unused functions which will
> > > catch majority of cases.
> >
> > The most interesting difference is that clang will catch unused
> > static inline functions which gcc does not.
>
> That's mostly just about dead code cruft detection IIUC. That code won't
> make it into the binary if it isn't used.

Indeed, but it's nice to have the dead code cruft detection. You
can always mark the function as __attribute__((unused)) if you really
mean that it might be present but not used.

thanks
-- PMM


Re: [PULL 00/10] Bitmaps patches
Posted by Daniel P. Berrangé 4 years ago
On Tue, Mar 17, 2020 at 03:07:34PM +0000, Peter Maydell wrote:
> On Tue, 17 Mar 2020 at 15:05, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Tue, Mar 17, 2020 at 03:00:48PM +0000, Peter Maydell wrote:
> > > On Tue, 17 Mar 2020 at 14:57, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > > I don't feel like -Wno-unused-function looses anything significant, as
> > > > the GCC builds will still be reporting unused functions which will
> > > > catch majority of cases.
> > >
> > > The most interesting difference is that clang will catch unused
> > > static inline functions which gcc does not.
> >
> > That's mostly just about dead code cruft detection IIUC. That code won't
> > make it into the binary if it isn't used.
> 
> Indeed, but it's nice to have the dead code cruft detection. You
> can always mark the function as __attribute__((unused)) if you really
> mean that it might be present but not used.

The *BSDs  seem to track latest glib pretty quickly. So if we got the
unused attribute into upstream glib, we would probably have about
6-9 months before we get a build platform with the fixed glib included
where we can conditionally re-enable the unused-function warning.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PULL 00/10] Bitmaps patches
Posted by Eric Blake 4 years ago
On 3/17/20 10:11 AM, Daniel P. Berrangé wrote:
> On Tue, Mar 17, 2020 at 03:07:34PM +0000, Peter Maydell wrote:
>> On Tue, 17 Mar 2020 at 15:05, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>
>>> On Tue, Mar 17, 2020 at 03:00:48PM +0000, Peter Maydell wrote:
>>>> On Tue, 17 Mar 2020 at 14:57, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>>> I don't feel like -Wno-unused-function looses anything significant, as
>>>>> the GCC builds will still be reporting unused functions which will
>>>>> catch majority of cases.
>>>>
>>>> The most interesting difference is that clang will catch unused
>>>> static inline functions which gcc does not.
>>>
>>> That's mostly just about dead code cruft detection IIUC. That code won't
>>> make it into the binary if it isn't used.
>>
>> Indeed, but it's nice to have the dead code cruft detection. You
>> can always mark the function as __attribute__((unused)) if you really
>> mean that it might be present but not used.
> 
> The *BSDs  seem to track latest glib pretty quickly. So if we got the
> unused attribute into upstream glib, we would probably have about
> 6-9 months before we get a build platform with the fixed glib included
> where we can conditionally re-enable the unused-function warning.

Looks like glib commit https://github.com/GNOME/glib/commit/b41bff1f 
(2.57.2) added G_GNUC_UNUSED to all of the functions declared during 
G_DEFINE_AUTOPTR_CLEANUP_FUNC.  Which version of glib is on the NetBSD 
machine?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PULL 00/10] Bitmaps patches
Posted by Eric Blake 4 years ago
On 3/17/20 9:00 AM, Peter Maydell wrote:

>>    block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty (2020-03-12 16:36:46 -0400)
>>
>> ----------------------------------------------------------------
>> Pull request
>>
>> ---------------------------------------------------------------
> 
> Hi; this fails to compile with clang:
> 

As pointed out here, my recommendation is for John to send a v2 pull 
request with one more patch added:
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg05969.html

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org