From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51267) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bY1Ot-00016T-Vv for qemu-devel@nongnu.org; Thu, 11 Aug 2016 21:36:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bY1Or-0006Ok-Bk for qemu-devel@nongnu.org; Thu, 11 Aug 2016 21:36:42 -0400 Date: Fri, 12 Aug 2016 09:29:30 +0800 From: Fam Zheng Message-ID: <20160812012930.GA2759@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> <20160811032347.GD9358@al.usersys.redhat.com> <0b184498-5dda-6710-d4b4-4359726f42cf@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <0b184498-5dda-6710-d4b4-4359726f42cf@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: Colin Lord Cc: Max Reitz , qemu-devel@nongnu.org, kwolf@redhat.com, Marc Mari , qemu-block@nongnu.org On Thu, 08/11 12:03, Colin Lord wrote: > On 08/10/2016 11:23 PM, Fam Zheng wrote: > > 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= be > >>>>> loaded dynamically on request. The only block drivers that can be > >>>>> converted into modules are the drivers that don't perform any ini= t > >>>>> 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 modulariz= ing > >>>>> them is of no benefit from a performance perspective. > >>>>> > >>>>> All the necessary module information is located in a new structur= e 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 modu= le */ > >>>>> + for (i =3D 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].librar= y_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 serie= s are > >>>> supposed to interact; the fact that the script added in patch 2 br= eaks > >>>> down with Fam's patch isn't exactly helping... > >>>> > >>>> Hm, so is this series now supposed to be applied without Fam's pat= ch > >>>> 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 wit= h > >>> this series actually. I'm kind of hoping it can be done on top of m= y > >>> 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 actu= ally > >>> 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 t= heir > >>> call to qemu, eg driver=3Dgluster, there was a bug in v5 where if t= he > >>> driver was modularized, it would not be found/loaded. So since glus= ter > >>> was modularized, if you said driver=3Dgluster on the command line, = the > >>> gluster module would not be found. The modules could be found by pr= obing > >>> perfectly fine, this only happened when the driver was specified > >>> manually. The reason is because the drivers get searched based on t= he > >>> format field if they're specified manually, which means bdrv_find_f= ormat > >>> 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. T= hat'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 > >> > >=20 > > If we apply this series first, there will be an intermediate state th= at the > > main QEMU binary is linked to libbz2. It doesn't hurt us, but it is b= etter to > > make it clear, in case downstream backportings want to keep the libra= ry > > dependency intact. > >=20 > > So, let's add a word to this commit message, at least. > >=20 > > Fam > >=20 > >=20 > >=20 > I guess I'm a little confused about the issue. It seems to me the only > difference between now and before is that if libbz2 is enabled, it will > be linked to the main binary rather than a module. But before, the > modules were always loaded unconditionally at startup anyway, so I'm no= t > seeing how there is a difference. Either way libbz2 would be loaded. I'= m > just not really sure what sort of message I should be adding to the > commit message to indicate this. What I propose to be added in the commit message is this: --- 8< --- This spoils the purpose of 5505e8b76f (block/dmg: make it modular). Before this patch, if module build is enabled, block-dmg.so is linked to libbz2, whereas the main binary is not. In downstream, theoretically, it = means only the qemu-block-extra package depends on libbz2, while the main QEMU package needn't to. With this patch, we (temporarily) change the case so= that the main QEMU depends on libbz2 again. --- >8 --- >=20 > Also, would you like me to try and port your patch for dmg to work on > top of this series? I'd still prefer if this series was applied first > because 1) if the dmg patch is done first, this series will have to > change parts of that patch anyway since the block module objects aren't > being loaded unconditionally anymore. That means the bz2 parts would > have to be converted to being dynamically linked at runtime, so it make= s > sense to me to just do it that way the first time rather than going bac= k > to change it. And 2) I am leaving soon and may or may not have time to > make this series work on top of the dmg patch. But I'm happy to try and > make your patch to work on top of this series in the little time I have > before I leave. I think it is fine for me to rebase on top of yours because: 1) practical= ly it probably has no impact - Debian and ArchLinux both put block-dmg.so in= the main QEMU package; 2) it's not a bug to introduce an extra dependency (th= e only special thing is, though, in this case it is not absolutely necessary, bu= t it makes the development easier); 3) we will fix it within the same release. Fam