From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51120) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXghv-00074w-FK for qemu-devel@nongnu.org; Wed, 10 Aug 2016 23:31:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXght-0001S5-Vq for qemu-devel@nongnu.org; Wed, 10 Aug 2016 23:30:59 -0400 Date: Thu, 11 Aug 2016 11:23:47 +0800 From: Fam Zheng Message-ID: <20160811032347.GD9358@al.usersys.redhat.com> 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> <0ce88d12-a583-5a5c-14ce-7667c20a38ad@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <0ce88d12-a583-5a5c-14ce-7667c20a38ad@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [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 Cc: Colin Lord , qemu-devel@nongnu.org, kwolf@redhat.com, Marc Mari , qemu-block@nongnu.org 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 > >>> > >>> Extend the current module interface to allow for block drivers to b= e > >>> 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 th= ey > >>> are the only ones which see significant performance benefits. The f= ormat > >>> drivers do not generally link to external libraries, so modularizin= g > >>> 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=ED > >>> 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_na= me)) { > >>> + 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 c= are > >> 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 brea= ks > >> 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 no= t > > done with the dmg patch in mind. The change in find_format was actual= ly > > due to a bug I discovered in my patch series (I fixed it in v6, but y= ou > > may have missed that). > >=20 > > Essentially, if a user specifies the driver explicitly as part of the= ir > > 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 gluste= r > > was modularized, if you said driver=3Dgluster on the command line, th= e > > gluster module would not be found. The modules could be found by prob= ing > > 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_for= mat > > gets called when the driver is specified on the command line. This ma= kes > > it necessary for bdrv_find_format to take into account modularized > > drivers even though the format drivers are not being modularized. Tha= t's > > also why the format field was added to the module_block header file a= gain. >=20 > Ah, that makes sense, thanks for explaining. >=20 > Patches 1-3: >=20 > Reviewed-by: Max Reitz >=20 If we apply this series first, there will be an intermediate state that t= he main QEMU binary is linked to libbz2. It doesn't hurt us, but it is bette= r 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