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

[Colin Lord <clord@redhat.com>]

On 08/10/2016 03:24 PM, Colin Lord wrote:
> On 08/10/2016 03:04 PM, 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 <markmb@redhat.com>
>>>>
>>>> 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í <markmb@redhat.com>
>>>> Signed-off-by: Colin Lord <clord@redhat.com>
>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>> ---
>>>>  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.
>>
>> Colin
>>
> Oops, this was meant to be a reply to your response of patch 4/4, in
> case anyone gets confused.
> 
> Colin
> 
Nevermind, looks like I was confused. Seems like my eyes are playing
tricks on me, sorry for the spam.