Re: [Qemu-block] [Qemu-devel] [PATCH v7 3/4] blockdev: Add dynamic module loading for block drivers

[Fam Zheng]

On Thu, 08/11 12:03, Colin Lord wrote:
> On 08/10/2016 11:23 PM, Fam Zheng wrote:
> > On Wed, 08/10 21:06, Max Reitz wrote:
> >> On 10.08.2016 21:04, Colin Lord wrote:
> >>> On 08/10/2016 02:37 PM, Max Reitz wrote:
> >>>> On 08.08.2016 20:07, Colin Lord wrote:
> >>>>> From: Marc Mari <address@hidden>
> >>>>>
> >>>>> Extend the current module interface to allow for block drivers to be
> >>>>> loaded dynamically on request. The only block drivers that can be
> >>>>> converted into modules are the drivers that don't perform any init
> >>>>> operation except for registering themselves.
> >>>>>
> >>>>> In addition, only the protocol drivers are being modularized, as they
> >>>>> are the only ones which see significant performance benefits. The format
> >>>>> drivers do not generally link to external libraries, so modularizing
> >>>>> them is of no benefit from a performance perspective.
> >>>>>
> >>>>> All the necessary module information is located in a new structure found
> >>>>> in module_block.h
> >>>>>
> >>>>> Signed-off-by: Marc Marí <address@hidden>
> >>>>> Signed-off-by: Colin Lord <address@hidden>
> >>>>> Reviewed-by: Stefan Hajnoczi <address@hidden>
> >>>>> ---
> >>>>>  Makefile              |  3 ---
> >>>>>  block.c               | 62 
> >>>>> +++++++++++++++++++++++++++++++++++++++++++++------
> >>>>>  block/Makefile.objs   |  3 +--
> >>>>>  include/qemu/module.h |  3 +++
> >>>>>  util/module.c         | 38 +++++++++----------------------
> >>>>>  5 files changed, 70 insertions(+), 39 deletions(-)
> >>>>>
> >>>>
> >>>> [...]
> >>>>
> >>>>> diff --git a/block.c b/block.c
> >>>>> index 30d64e6..6c5e249 100644
> >>>>> --- a/block.c
> >>>>> +++ b/block.c
> >>>>> @@ -26,6 +26,7 @@
> >>>>>  #include "block/block_int.h"
> >>>>>  #include "block/blockjob.h"
> >>>>>  #include "qemu/error-report.h"
> >>>>> +#include "module_block.h"
> >>>>>  #include "qemu/module.h"
> >>>>>  #include "qapi/qmp/qerror.h"
> >>>>>  #include "qapi/qmp/qbool.h"
> >>>>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void)
> >>>>>      return bs;
> >>>>>  }
> >>>>>  
> >>>>> -BlockDriver *bdrv_find_format(const char *format_name)
> >>>>> +static BlockDriver *bdrv_do_find_format(const char *format_name)
> >>>>>  {
> >>>>>      BlockDriver *drv1;
> >>>>> +
> >>>>>      QLIST_FOREACH(drv1, &bdrv_drivers, list) {
> >>>>>          if (!strcmp(drv1->format_name, format_name)) {
> >>>>>              return drv1;
> >>>>>          }
> >>>>>      }
> >>>>> +
> >>>>>      return NULL;
> >>>>>  }
> >>>>>  
> >>>>> +BlockDriver *bdrv_find_format(const char *format_name)
> >>>>> +{
> >>>>> +    BlockDriver *drv1;
> >>>>> +    size_t i;
> >>>>> +
> >>>>> +    drv1 = bdrv_do_find_format(format_name);
> >>>>> +    if (drv1) {
> >>>>> +        return drv1;
> >>>>> +    }
> >>>>> +
> >>>>> +    /* The driver isn't registered, maybe we need to load a module */
> >>>>> +    for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) {
> >>>>> +        if (!strcmp(block_driver_modules[i].format_name, format_name)) 
> >>>>> {
> >>>>> +            
> >>>>> block_module_load_one(block_driver_modules[i].library_name);
> >>>>> +            break;
> >>>>> +        }
> >>>>> +    }
> >>>>> +
> >>>>> +    return bdrv_do_find_format(format_name);
> >>>>> +}
> >>>>> +
> >>>>
> >>>> Did you reintroduce this function for dmg? I thought Fam is taking care
> >>>> of that? I'm confused as to how Fam's patch for dmg and this series are
> >>>> supposed to interact; the fact that the script added in patch 2 breaks
> >>>> down with Fam's patch isn't exactly helping...
> >>>>
> >>>> Hm, so is this series now supposed to be applied without Fam's patch
> >>>> with the idea of sorting dmg out later on?
> >>>>
> >>>> Max
> >>>>
> >>>>>  static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
> >>>>>  {
> >>>>>      static const char *whitelist_rw[] = {
> >>>>
> >>> I'm not completely sure how Fam's patch is supposed to interact with
> >>> this series actually. I'm kind of hoping it can be done on top of my
> >>> patches at some future point, but in either case this revision was not
> >>> done with the dmg patch in mind. The change in find_format was actually
> >>> due to a bug I discovered in my patch series (I fixed it in v6, but you
> >>> may have missed that).
> >>>
> >>> Essentially, if a user specifies the driver explicitly as part of their
> >>> call to qemu, eg driver=gluster, there was a bug in v5 where if the
> >>> driver was modularized, it would not be found/loaded. So since gluster
> >>> was modularized, if you said driver=gluster on the command line, the
> >>> gluster module would not be found. The modules could be found by probing
> >>> perfectly fine, this only happened when the driver was specified
> >>> manually. The reason is because the drivers get searched based on the
> >>> format field if they're specified manually, which means bdrv_find_format
> >>> gets called when the driver is specified on the command line. This makes
> >>> it necessary for bdrv_find_format to take into account modularized
> >>> drivers even though the format drivers are not being modularized. That's
> >>> also why the format field was added to the module_block header file again.
> >>
> >> Ah, that makes sense, thanks for explaining.
> >>
> >> Patches 1-3:
> >>
> >> Reviewed-by: Max Reitz <address@hidden>
> >>
> > 
> > If we apply this series first, there will be an intermediate state that the
> > main QEMU binary is linked to libbz2. It doesn't hurt us, but it is better 
> > to
> > make it clear, in case downstream backportings want to keep the library
> > dependency intact.
> > 
> > So, let's add a word to this commit message, at least.
> > 
> > Fam
> > 
> > 
> > 
> I guess I'm a little confused about the issue. It seems to me the only
> difference between now and before is that if libbz2 is enabled, it will
> be linked to the main binary rather than a module. But before, the
> modules were always loaded unconditionally at startup anyway, so I'm not
> seeing how there is a difference. Either way libbz2 would be loaded. I'm
> just not really sure what sort of message I should be adding to the
> commit message to indicate this.

What I propose to be added in the commit message is this:

--- 8< ---

This spoils the purpose of 5505e8b76f (block/dmg: make it modular).

Before this patch, if module build is enabled, block-dmg.so is linked to
libbz2, whereas the main binary is not. In downstream, theoretically, it means
only the qemu-block-extra package depends on libbz2, while the main QEMU
package needn't to.  With this patch, we (temporarily) change the case so that
the main QEMU depends on libbz2 again.

--- >8 ---

> 
> Also, would you like me to try and port your patch for dmg to work on
> top of this series? I'd still prefer if this series was applied first
> because 1) if the dmg patch is done first, this series will have to
> change parts of that patch anyway since the block module objects aren't
> being loaded unconditionally anymore. That means the bz2 parts would
> have to be converted to being dynamically linked at runtime, so it makes
> sense to me to just do it that way the first time rather than going back
> to change it. And 2) I am leaving soon and may or may not have time to
> make this series work on top of the dmg patch. But I'm happy to try and
> make your patch to work on top of this series in the little time I have
> before I leave.

I think it is fine for me to rebase on top of yours because: 1) practically
it probably has no impact - Debian and ArchLinux both put block-dmg.so in the
main QEMU package; 2) it's not a bug to introduce an extra dependency (the only
special thing is, though, in this case it is not absolutely necessary, but it
makes the development easier); 3) we will fix it within the same release.

Fam