From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44519) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXZCE-0002Xa-Tl for qemu-devel@nongnu.org; Wed, 10 Aug 2016 15:29:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXZCC-0007KZ-MO for qemu-devel@nongnu.org; Wed, 10 Aug 2016 15:29:45 -0400 References: <1470679640-18366-1-git-send-email-clord@redhat.com> <1470679640-18366-4-git-send-email-clord@redhat.com> <62914d01-ac55-efba-2b2d-866cb833c823@redhat.com> <56832672-88dd-0b93-3380-76a00580f829@redhat.com> <457c20b2-767f-858c-7431-b971b8c01426@redhat.com> From: Colin Lord Message-ID: Date: Wed, 10 Aug 2016 15:29:36 -0400 MIME-Version: 1.0 In-Reply-To: <457c20b2-767f-858c-7431-b971b8c01426@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v7 3/4] blockdev: Add dynamic module loading for block drivers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: kwolf@redhat.com, Marc Mari , qemu-block@nongnu.org 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 >>>> >>>> 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 the= y >>>> are the only ones which see significant performance benefits. The fo= rmat >>>> 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 f= ound >>>> in module_block.h >>>> >>>> Signed-off-by: Marc Mar=C3=AD >>>> Signed-off-by: Colin Lord >>>> Reviewed-by: Stefan Hajnoczi >>>> --- >>>> 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; >>>> } >>>> =20 >>>> -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; >>>> } >>>> =20 >>>> +BlockDriver *bdrv_find_format(const char *format_name) >>>> +{ >>>> + BlockDriver *drv1; >>>> + size_t i; >>>> + >>>> + drv1 =3D bdrv_do_find_format(format_name); >>>> + if (drv1) { >>>> + return drv1; >>>> + } >>>> + >>>> + /* The driver isn't registered, maybe we need to load a module = */ >>>> + for (i =3D 0; i < ARRAY_SIZE(block_driver_modules); ++i) { >>>> + if (!strcmp(block_driver_modules[i].format_name, format_nam= e)) { >>>> + block_module_load_one(block_driver_modules[i].library_n= ame); >>>> + break; >>>> + } >>>> + } >>>> + >>>> + return bdrv_do_find_format(format_name); >>>> +} >>>> + >>> >>> Did you reintroduce this function for dmg? I thought Fam is taking ca= re >>> of that? I'm confused as to how Fam's patch for dmg and this series a= re >>> supposed to interact; the fact that the script added in patch 2 break= s >>> 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[] =3D { >>> >> 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 actuall= y >> due to a bug I discovered in my patch series (I fixed it in v6, but yo= u >> may have missed that). >> >> Essentially, if a user specifies the driver explicitly as part of thei= r >> call to qemu, eg driver=3Dgluster, 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=3Dgluster on the command line, the >> gluster module would not be found. The modules could be found by probi= ng >> 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_form= at >> gets called when the driver is specified on the command line. This mak= es >> 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 ag= ain. >> >> Colin >> > Oops, this was meant to be a reply to your response of patch 4/4, in > case anyone gets confused. >=20 > Colin >=20 Nevermind, looks like I was confused. Seems like my eyes are playing tricks on me, sorry for the spam.