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

[Colin Lord]

On 08/11/2016 09:29 PM, Fam Zheng wrote:
> 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
> 
Okay sounds good, I'll add the comment and resend to the mailing list.

thanks,
Colin