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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4828 bytes --]

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 <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.

Ah, that makes sense, thanks for explaining.

Patches 1-3:

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]