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

[Colin Lord]

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.

Colin