From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54982) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bYDVd-0005xd-L7 for qemu-devel@nongnu.org; Fri, 12 Aug 2016 10:32:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bYDVZ-0003u8-Qu for qemu-devel@nongnu.org; Fri, 12 Aug 2016 10:32:28 -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> <0ce88d12-a583-5a5c-14ce-7667c20a38ad@redhat.com> <20160811032347.GD9358@al.usersys.redhat.com> <0b184498-5dda-6710-d4b4-4359726f42cf@redhat.com> <20160812012930.GA2759@al.usersys.redhat.com> From: Colin Lord Message-ID: <76a4539f-e631-d6ce-2658-dc3b98f2d365@redhat.com> Date: Fri, 12 Aug 2016 09:13:39 -0400 MIME-Version: 1.0 In-Reply-To: <20160812012930.GA2759@al.usersys.redhat.com> Content-Type: text/plain; charset=windows-1252 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: Fam Zheng Cc: Max Reitz , qemu-devel@nongnu.org, kwolf@redhat.com, Marc Mari , qemu-block@nongnu.org On 08/11/2016 09:29 PM, Fam Zheng wrote: > 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 >>>> >>> >>> 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. >>> >>> So, let's add a word to this commit message, at least. >>> >>> Fam >>> >>> >>> >> 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 wil= l >> be linked to the main binary rather than a module. But before, the >> modules were always loaded unconditionally at startup anyway, so I'm n= ot >> 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. >=20 > What I propose to be added in the commit message is this: >=20 > --- 8< --- >=20 > This spoils the purpose of 5505e8b76f (block/dmg: make it modular). >=20 > Before this patch, if module build is enabled, block-dmg.so is linked t= o > libbz2, whereas the main binary is not. In downstream, theoretically, i= t means > only the qemu-block-extra package depends on libbz2, while the main QEM= U > package needn't to. With this patch, we (temporarily) change the case = so that > the main QEMU depends on libbz2 again. >=20 > --- >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 mak= es >> sense to me to just do it that way the first time rather than going ba= ck >> 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 an= d >> make your patch to work on top of this series in the little time I hav= e >> before I leave. >=20 > I think it is fine for me to rebase on top of yours because: 1) practic= ally > 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 (= the only > special thing is, though, in this case it is not absolutely necessary, = but it > makes the development easier); 3) we will fix it within the same releas= e. >=20 > Fam >=20 Okay sounds good, I'll add the comment and resend to the mailing list. thanks, Colin