| Message ID | 1456151945-11225-4-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
Paolo Bonzini <pbonzini@redhat.com> writes: > Currently, blockdev_del_drive (and before it blk_auto_del) does a blk_unref > that will cause blk_delete to be called and the DriveInfo to be freed. > But really, we want to free the drive info as soon as the device is > detached, even if there are other references for whatever reason, so > that the QemuOpts are freed as well and the id can be reused. Let me write up how I understand this, so you can correct my misunderstandings. Management applications expect that on receipt of event DEVICE_DELETED * the frontend is fully gone, and * any of its warty block backends are fully gone. A frontend's block backend is warty if it gets automatically deleted along with the frontend. "Fully gone" implies the ID can safely be reused. The whole series is about warty automatic block backend deletion. Non-warty block backends need to be deleted explicitly with x-blockdev-del, which fails when there are other references. We can't do the same for warty deletion, because the *frontend* got deleted just fine (thus DEVICE_DELETED must be sent), even when something else is keeping the backend alive. Back when the wart was born, this wasn't possible. So, what to do? The commit message sounds like the patch hides the backend then, so it's "fully gone" from the management application's view. But isn't that an impossible mission if the "something else" is something the management application can see? For instance, what if there's a block job tied to the backend? The management application can see that tie. If we succeed in hiding the backend, we have a dangling tie. If we keep the tie, we failed at hiding. We avoid the case of a block job the hamfisted way: we cancel it. Okay, but that begs the question what else could hold a reference, whether it's similarly exposed to the management appliction, and whether it needs to be "cancelled" as well. I'm sure Shrek would love this swamp. > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/block-backend.c | 13 +++++++++---- > blockdev.c | 9 +++++---- > include/sysemu/blockdev.h | 1 + > 3 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/block/block-backend.c b/block/block-backend.c > index ebdf78a..0a85c6a 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -65,8 +65,6 @@ static const AIOCBInfo block_backend_aiocb_info = { > .aiocb_size = sizeof(BlockBackendAIOCB), > }; > > -static void drive_info_del(DriveInfo *dinfo); > - > /* All the BlockBackends (except for hidden ones) */ > static QTAILQ_HEAD(, BlockBackend) blk_backends = > QTAILQ_HEAD_INITIALIZER(blk_backends); > @@ -165,6 +163,7 @@ static void blk_delete(BlockBackend *blk) > { > assert(!blk->refcnt); > assert(!blk->dev); > + assert(!blk->legacy_dinfo); > if (blk->bs) { > blk_remove_bs(blk); > } > @@ -179,19 +178,25 @@ static void blk_delete(BlockBackend *blk) > QTAILQ_REMOVE(&blk_backends, blk, link); > } > g_free(blk->name); > - drive_info_del(blk->legacy_dinfo); > block_acct_cleanup(&blk->stats); > g_free(blk); > } This and the assertion above effectively say "you must delete a BlockBackend's legacy_dinfo before you drop its last reference." We'll see below why that works. > > -static void drive_info_del(DriveInfo *dinfo) > +void blk_release_legacy_dinfo(BlockBackend *blk) > { > + DriveInfo *dinfo = blk->legacy_dinfo; > + > if (!dinfo) { > return; > } > qemu_opts_del(dinfo->opts); > g_free(dinfo->serial); > g_free(dinfo); > + blk->legacy_dinfo = NULL; > + /* We are not interested anymore in retrieving the BlockBackend > + * via blk_by_legacy_dinfo, so let it die. > + */ > + blk_unref(blk); > } > This looks like DriveInfo now owns a reference to BlockBackend, even though the pointer still goes in the other direction. > int blk_get_refcnt(BlockBackend *blk) > diff --git a/blockdev.c b/blockdev.c > index 2dfb2d8..85f0cb5 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -120,10 +120,10 @@ void override_max_devs(BlockInterfaceType type, int max_devs) > */ > void blockdev_del_drive(BlockBackend *blk) > { > - DriveInfo *dinfo = blk_legacy_dinfo(blk); > BlockDriverState *bs = blk_bs(blk); > AioContext *aio_context; > > + blk_ref(blk); > if (bs) { > aio_context = bdrv_get_aio_context(bs); > aio_context_acquire(aio_context); > @@ -135,9 +135,10 @@ void blockdev_del_drive(BlockBackend *blk) > aio_context_release(aio_context); > } > > - if (dinfo) { > - blk_unref(blk); > + if (blk_legacy_dinfo(blk)) { > + blk_release_legacy_dinfo(blk); > } > + blk_unref(blk); > } Before: we drop a reference here if we have a DriveInfo. After: we drop a reference in blk_release_legacy_dinfo() if we have a DriveInfo. We also take a temporary reference here. I guess the only reason is to avoid tripping the assertion we just discussed. > > /** > @@ -2811,7 +2812,7 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) /* if we have a device attached to this BlockDriverState * then we need to make the drive anonymous until the device * can be removed. If this is a drive with no device backing * then we can just get rid of the block driver state right here. */ if (blk_get_attached_dev(blk)) { blk_hide_on_behalf_of_hmp_drive_del(blk); /* Further I/O must not pause the guest */ > blk_set_on_error(blk, BLOCKDEV_ON_ERROR_REPORT, > BLOCKDEV_ON_ERROR_REPORT); > } else { > - blk_unref(blk); > + blockdev_del_drive(blk); Now we come to the spot I've dreaded all along... If blk_get_attached_dev(), we can't delete the backend, so we must hide it. Not changed by your patch. Else, we delete the backend. Before your patch, we delete the obvious way: drop the reference. If something else holds another reference, we fail to delete. If this can happen, it's a bug. I'm not sure it can't happen. After your patch, we can't just drop the reference, because that would trip the assertion we just discussed. So we call blockdev_del_drive() instead. In addition to dropping the reference, this * Cancels block jobs. Could well be a fix for the bug I just described, but it needs to be featured in the commit message then. * blk_release_legacy_dinfo(). Same addition as in blockdev_del_drive() above. Now back to why "you must delete a BlockBackend's legacy_dinfo before you drop its last reference" works. A BlockBackend has a legacy_dinfo exactly when it was created with -drive or drive_add. The block layer holds a reference for lookup by name then. This reference goes away only when the backend is deleted by the user, in one of the following ways: * Explicitly with drive_del Deletes right away if no frontend is attached. Your patch has the necessary replacement of blk_unref(blk) by blockdev_del_drive(blk), in hmp_drive_del(). Else, deletion is delayed until the frontend detaches. Where is the legacy_dinfo released then? * Explicitly with x-blockdev-del Fails unless no other reference exists. Where is the legacy_dinfo released? * Implicitly via warty automatic deletion Your PATCH 01 has the necessary replacement of blk_unref(blk) by blkdev_del_drive(blk) for some devices (virtio-blk.c, scsi-bus.c, xen_disk,c, piix.c), butas far as I can see not for others such as nvme.c. > } > > aio_context_release(aio_context); > diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h > index ae7ad67..5722b9f 100644 > --- a/include/sysemu/blockdev.h > +++ b/include/sysemu/blockdev.h > @@ -44,6 +44,7 @@ struct DriveInfo { > DriveInfo *blk_legacy_dinfo(BlockBackend *blk); > DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo); > BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo); > +void blk_release_legacy_dinfo(BlockBackend *blk); > > void override_max_devs(BlockInterfaceType type, int max_devs);
On 21/03/2016 17:15, Markus Armbruster wrote: > * Explicitly with x-blockdev-del > > Fails unless no other reference exists. Where is the legacy_dinfo > released? Can a -drive block device be deleted with x-blockdev-del even?!? In other words, you said "This looks like DriveInfo now owns a reference to BlockBackend, even though the pointer still goes in the other direction". I say, "I thought this was the idea all along"... Shall I add a check to x-blockdev-del that gives an error if the BlockBackend has a DriveInfo attached? Paolo > * Implicitly via warty automatic deletion > > Your PATCH 01 has the necessary replacement of blk_unref(blk) by > blkdev_del_drive(blk) for some devices (virtio-blk.c, scsi-bus.c, > xen_disk,c, piix.c), butas far as I can see not for others such as > nvme.c.
Paolo Bonzini <pbonzini@redhat.com> writes: > On 21/03/2016 17:15, Markus Armbruster wrote: >> * Explicitly with x-blockdev-del >> >> Fails unless no other reference exists. Where is the legacy_dinfo >> released? > > Can a -drive block device be deleted with x-blockdev-del even?!? When I wrote my review, I forgot that I expect x-blockdev-del to accept only backends created with blockdev-add. With that, my question is indeed moot. However, I've now tested my expectation, and it turned out to be wrong. I'm inclined to call that a bug. > In other words, you said "This looks like DriveInfo now owns a reference > to BlockBackend, even though the pointer still goes in the other > direction". I say, "I thought this was the idea all along"... For me, the DriveInfo doesn't own anything, but a BlockBackend may have a DriveInfo. Evidence: * The pointer goes from the BlockBackend to the DriveInfo * To go back, you search the blk_backends for the one that has the DriveInfo. See blk_by_legacy_dinfo(). * There is no list of DriveInfo. If you want to find one, you search blk_backends. See drive_get() & friends. > Shall I add a check to x-blockdev-del that gives an error if the > BlockBackend has a DriveInfo attached? Yes, please. But do double-check with Kevin & Max, who might have different ideas on blockdev-add/del than I do. > > Paolo > >> * Implicitly via warty automatic deletion >> >> Your PATCH 01 has the necessary replacement of blk_unref(blk) by >> blkdev_del_drive(blk) for some devices (virtio-blk.c, scsi-bus.c, >> xen_disk,c, piix.c), butas far as I can see not for others such as >> nvme.c.
On 21/03/2016 18:30, Markus Armbruster wrote: > However, I've now tested my expectation, and it turned out to be wrong. > I'm inclined to call that a bug. --verbose, what is wrong and what was your expectation? > > In other words, you said "This looks like DriveInfo now owns a reference > > to BlockBackend, even though the pointer still goes in the other > > direction". I say, "I thought this was the idea all along"... > > For me, the DriveInfo doesn't own anything, but a BlockBackend may have > a DriveInfo. Evidence: > > * The pointer goes from the BlockBackend to the DriveInfo > > * To go back, you search the blk_backends for the one that has the > DriveInfo. See blk_by_legacy_dinfo(). > > * There is no list of DriveInfo. If you want to find one, you search > blk_backends. See drive_get() & friends. That's from the point of view of the code. But from the point of view of the user, he specifies a drive=... and the device converts that under the hood to a BlockBackend; and when he calls drive_del on an unassigned drive, the BlockBackend is destroyed. There is no action on a BlockBackend that destroys the DriveInfo---except auto-deletion on unplug, but even then the user in the first place had provided a DriveInfo. So from the point of view of the user it's always been the DriveInfo that owned a BlockBackend. The lack of a list of DriveInfo is just an implementation detail. Paolo
Am 21.03.2016 um 18:30 hat Markus Armbruster geschrieben: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > On 21/03/2016 17:15, Markus Armbruster wrote: > >> * Explicitly with x-blockdev-del > >> > >> Fails unless no other reference exists. Where is the legacy_dinfo > >> released? > > > > Can a -drive block device be deleted with x-blockdev-del even?!? > > When I wrote my review, I forgot that I expect x-blockdev-del to accept > only backends created with blockdev-add. With that, my question is > indeed moot. > > However, I've now tested my expectation, and it turned out to be wrong. > I'm inclined to call that a bug. Yes. > > Shall I add a check to x-blockdev-del that gives an error if the > > BlockBackend has a DriveInfo attached? > > Yes, please. But do double-check with Kevin & Max, who might have > different ideas on blockdev-add/del than I do. I'm pretty sure that I said that failing on -drive/drive_add created BlockBackends was a requirement for x-blockdev-del. Apparently I failed to catch the bug in the review then. So go ahead and let's fix it now. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 21.03.2016 um 18:30 hat Markus Armbruster geschrieben: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >> > On 21/03/2016 17:15, Markus Armbruster wrote: >> >> * Explicitly with x-blockdev-del >> >> >> >> Fails unless no other reference exists. Where is the legacy_dinfo >> >> released? >> > >> > Can a -drive block device be deleted with x-blockdev-del even?!? >> >> When I wrote my review, I forgot that I expect x-blockdev-del to accept >> only backends created with blockdev-add. With that, my question is >> indeed moot. >> >> However, I've now tested my expectation, and it turned out to be wrong. >> I'm inclined to call that a bug. > > Yes. > >> > Shall I add a check to x-blockdev-del that gives an error if the >> > BlockBackend has a DriveInfo attached? >> >> Yes, please. But do double-check with Kevin & Max, who might have >> different ideas on blockdev-add/del than I do. > > I'm pretty sure that I said that failing on -drive/drive_add created > BlockBackends was a requirement for x-blockdev-del. Apparently I failed > to catch the bug in the review then. > > So go ahead and let's fix it now. Yes, please.
Paolo Bonzini <pbonzini@redhat.com> writes: > On 21/03/2016 18:30, Markus Armbruster wrote: >> However, I've now tested my expectation, and it turned out to be wrong. >> I'm inclined to call that a bug. > > --verbose, what is wrong and what was your expectation? x-blockdev-del should refuse to touch anything not created with blockdev-add. >> > In other words, you said "This looks like DriveInfo now owns a reference >> > to BlockBackend, even though the pointer still goes in the other >> > direction". I say, "I thought this was the idea all along"... >> >> For me, the DriveInfo doesn't own anything, but a BlockBackend may have >> a DriveInfo. Evidence: >> >> * The pointer goes from the BlockBackend to the DriveInfo >> >> * To go back, you search the blk_backends for the one that has the >> DriveInfo. See blk_by_legacy_dinfo(). >> >> * There is no list of DriveInfo. If you want to find one, you search >> blk_backends. See drive_get() & friends. > > That's from the point of view of the code. But from the point of view > of the user, he specifies a drive=... and the device converts that under > the hood to a BlockBackend; and when he calls drive_del on an unassigned > drive, the BlockBackend is destroyed. > > There is no action on a BlockBackend that destroys the > DriveInfo---except auto-deletion on unplug, but even then the user in > the first place had provided a DriveInfo. So from the point of view of > the user it's always been the DriveInfo that owned a BlockBackend. The > lack of a list of DriveInfo is just an implementation detail. From the user's point of view, neither BlockBackend nor DriveInfo are visible :) A BlockBackend may have a DriveInfo. If it has one, then destroying the BlockBackend also destroys its DriveInfo. DriveInfo exists only to capture a -drive in a more convenient form than its QemuOpts. We use it for creating a BlockBackend. It lives on after that only because -drive mixes up front- and backend matter. Keeping DriveInfo around hanging off BlockBackend lets us keep frontend matter out of BlockBackend: if need to access mixed up frontend matter for back-compat, we find it in the BlockBackend's DriveInfo. Imagine a future where we drop -drive / drive_add, or at least its mixed up aspects (doesn't have to be practical for imagining it). In that future, we'd also drop DriveInfo.
Am 21.03.2016 um 19:14 hat Markus Armbruster geschrieben: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > On 21/03/2016 18:30, Markus Armbruster wrote: > >> However, I've now tested my expectation, and it turned out to be wrong. > >> I'm inclined to call that a bug. > > > > --verbose, what is wrong and what was your expectation? > > x-blockdev-del should refuse to touch anything not created with > blockdev-add. > > >> > In other words, you said "This looks like DriveInfo now owns a reference > >> > to BlockBackend, even though the pointer still goes in the other > >> > direction". I say, "I thought this was the idea all along"... > >> > >> For me, the DriveInfo doesn't own anything, but a BlockBackend may have > >> a DriveInfo. Evidence: > >> > >> * The pointer goes from the BlockBackend to the DriveInfo > >> > >> * To go back, you search the blk_backends for the one that has the > >> DriveInfo. See blk_by_legacy_dinfo(). > >> > >> * There is no list of DriveInfo. If you want to find one, you search > >> blk_backends. See drive_get() & friends. > > > > That's from the point of view of the code. But from the point of view > > of the user, he specifies a drive=... and the device converts that under > > the hood to a BlockBackend; and when he calls drive_del on an unassigned > > drive, the BlockBackend is destroyed. > > > > There is no action on a BlockBackend that destroys the > > DriveInfo---except auto-deletion on unplug, but even then the user in > > the first place had provided a DriveInfo. So from the point of view of > > the user it's always been the DriveInfo that owned a BlockBackend. The > > lack of a list of DriveInfo is just an implementation detail. > > From the user's point of view, neither BlockBackend nor DriveInfo are > visible :) > > A BlockBackend may have a DriveInfo. If it has one, then destroying the > BlockBackend also destroys its DriveInfo. > > DriveInfo exists only to capture a -drive in a more convenient form than > its QemuOpts. We use it for creating a BlockBackend. It lives on after > that only because -drive mixes up front- and backend matter. Keeping > DriveInfo around hanging off BlockBackend lets us keep frontend matter > out of BlockBackend: if need to access mixed up frontend matter for > back-compat, we find it in the BlockBackend's DriveInfo. > > Imagine a future where we drop -drive / drive_add, or at least its mixed > up aspects (doesn't have to be practical for imagining it). In that > future, we'd also drop DriveInfo. While we're dreaming up things... Imagine a future where users don't have to know about BlockBackend, but devices automatically create their BlockBackend. Which happens to be something that I'd really like to have and at least I haven't seen the show stopper for it yet. Which might just be because we never really looked much into the details, but anyway... In this case, the relationship between DriveInfo and BlockBackend couldn't be "BB owns DriveInfo" any more, because the BB would be created later than the DriveInfo. (DriveInfo controls the creation of the guest device which would create the BB during its initialisation.) By the way, what's the reason again for keeping DriveInfo around even after having created the guest device? Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 21.03.2016 um 19:14 hat Markus Armbruster geschrieben: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >> > On 21/03/2016 18:30, Markus Armbruster wrote: >> >> However, I've now tested my expectation, and it turned out to be wrong. >> >> I'm inclined to call that a bug. >> > >> > --verbose, what is wrong and what was your expectation? >> >> x-blockdev-del should refuse to touch anything not created with >> blockdev-add. >> >> >> > In other words, you said "This looks like DriveInfo now owns a reference >> >> > to BlockBackend, even though the pointer still goes in the other >> >> > direction". I say, "I thought this was the idea all along"... >> >> >> >> For me, the DriveInfo doesn't own anything, but a BlockBackend may have >> >> a DriveInfo. Evidence: >> >> >> >> * The pointer goes from the BlockBackend to the DriveInfo >> >> >> >> * To go back, you search the blk_backends for the one that has the >> >> DriveInfo. See blk_by_legacy_dinfo(). >> >> >> >> * There is no list of DriveInfo. If you want to find one, you search >> >> blk_backends. See drive_get() & friends. >> > >> > That's from the point of view of the code. But from the point of view >> > of the user, he specifies a drive=... and the device converts that under >> > the hood to a BlockBackend; and when he calls drive_del on an unassigned >> > drive, the BlockBackend is destroyed. >> > >> > There is no action on a BlockBackend that destroys the >> > DriveInfo---except auto-deletion on unplug, but even then the user in >> > the first place had provided a DriveInfo. So from the point of view of >> > the user it's always been the DriveInfo that owned a BlockBackend. The >> > lack of a list of DriveInfo is just an implementation detail. >> >> From the user's point of view, neither BlockBackend nor DriveInfo are >> visible :) >> >> A BlockBackend may have a DriveInfo. If it has one, then destroying the >> BlockBackend also destroys its DriveInfo. >> >> DriveInfo exists only to capture a -drive in a more convenient form than >> its QemuOpts. We use it for creating a BlockBackend. It lives on after >> that only because -drive mixes up front- and backend matter. Keeping >> DriveInfo around hanging off BlockBackend lets us keep frontend matter >> out of BlockBackend: if need to access mixed up frontend matter for >> back-compat, we find it in the BlockBackend's DriveInfo. >> >> Imagine a future where we drop -drive / drive_add, or at least its mixed >> up aspects (doesn't have to be practical for imagining it). In that >> future, we'd also drop DriveInfo. > > While we're dreaming up things... Imagine a future where users don't > have to know about BlockBackend, but devices automatically create their > BlockBackend. Which happens to be something that I'd really like to have > and at least I haven't seen the show stopper for it yet. Which might > just be because we never really looked much into the details, but > anyway... > > In this case, the relationship between DriveInfo and BlockBackend > couldn't be "BB owns DriveInfo" any more, because the BB would be > created later than the DriveInfo. (DriveInfo controls the creation of > the guest device which would create the BB during its initialisation.) I don't think that would necessarily affect ownership. Regardless of how and when we create BlockBackend, we'll want to keep the clean separation between frontend and backend internally and at the user interface. DriveInfo has no role in cleanly separate creation of frontend and backend now, and it shouldn't get one in the future. Its purpose is to support the legacy user interface that has frontend and backend matters mixed up. Two things, actually: * Letting board code find legacy drive configuration information, so it can create their frontends. This code lingers because we still haven't reduced legacy drives to sugar for modern configuration. I believe we haven't tried because we got a number of boards nobody wants to touch. That's our old "stuff is too precious to retire, yet too worthless for anybody spending the time it takes to update it to modern interfaces" problem. * Letting devices fall back to legacy configuration. Things like "if qdev property "serial" isn't given, try getting the serial number from DriveInfo. The way we do this now is to package up the relevant parts of QemuOpts as a DriveInfo and stick them to the backend when we create the backend from the QemuOpts. "The backend" here is a BlockBackend. If we change that to be something else, I guess we'll tack the DriveInfo to whatever that something else may be. Reverting the "owns" relationship to make DriveInfo own BlockBackend (or whatever) makes no sense to me, because DriveInfo exists only for *some* backends. The others need an owner two. I think that owner should own all of them, not just the ones without a DriveInfo. > By the way, what's the reason again for keeping DriveInfo around even > after having created the guest device? Inertia? I know it's accessed from some realize() methods. We'd have to review whether it's accessed after realize().
On 22/03/2016 11:25, Markus Armbruster wrote: > Regardless of how and when we create BlockBackend, we'll want to keep > the clean separation between frontend and backend internally and at the > user interface. This means that the BlockBackend should not own the DriveInfo. The backend and frontend need not know of the object that mixes concepts from both of them. Instead, the DriveInfo can instantiate itself into a BlockBackend and the board can (if required) use the frontend parts of DriveInfo to instantiate a device and connect it to the BlocKBackend. In Kevin's idea there would be no ownership either way. Until then, I think my patch actually gets us closer to the ideal. Paolo > DriveInfo has no role in cleanly separate creation of frontend and > backend now, and it shouldn't get one in the future. Its purpose is to > support the legacy user interface that has frontend and backend matters > mixed up.
Paolo Bonzini <pbonzini@redhat.com> writes: > On 22/03/2016 11:25, Markus Armbruster wrote: >> Regardless of how and when we create BlockBackend, we'll want to keep >> the clean separation between frontend and backend internally and at the >> user interface. > > This means that the BlockBackend should not own the DriveInfo. The > backend and frontend need not know of the object that mixes concepts > from both of them. Instead, the DriveInfo can instantiate itself into a > BlockBackend and the board can (if required) use the frontend parts of > DriveInfo to instantiate a device and connect it to the BlocKBackend. You missed or are glossing over the "letting devices fall back to legacy configuration" part. Let me explain it in more detail, using frontend property "serial" as example. You can use -drive parameter serial to control it for many devices. Example: -drive if=ide,index=3,file=tmp.qcow2,serial=Stockhausen The board examines the if=ide drives and creates frontends for them. It could certainly recognize -drive parameter serial and configure the frontend accordingly. However, it doesn't. To show you why, I need another example. Example: -drive if=none,id=ide3,file=tmp.qcow2,serial=Stockhausen \ -device ide-hd,bus=ide.1,unit=1,drive=ide3 The board is not involved here. Instead, the *frontend* implements the legacy fallback: if its property "serial" isn't set, it checks whether its backend's DriveInfo has a serial, and if yes, it uses that. Only some frontends do that, namely the ones where the legacy configuration actually needs to be preserved. Newer ones don't. Look for blkconf_serial(). > In Kevin's idea there would be no ownership either way. Until then, I > think my patch actually gets us closer to the ideal. I'm afraid it gets us closer to where we used to be six years ago :) Qdev drive properties used to point to a DriveInfo, and the DriveInfo pointed to BlockDriverState. Commit f8b6cc0 cut out the DriveInfo middleman. This was a tiny step towards DriveInfo-less blockdev-add. DriveInfo is legacy configuration. Tacking it to BlockBackend is simple and convenient. If it ceases to be simple and convenient, we can try to find another home. But it really has no life of its own! It's ancillary information for whatever -drive creates (currently: BlockBackend), and therefore should simply die with whatever it is ancillary to. It's owned by that, not the other way round. Now, you can certainly take a reference without being the owner. But my review comment wasn't so much about ownership, it was more about the oddness of taking a reference (in the sense of incrementing the reference count) without actually *having* a reference (in the sense of a pointer to the reference-counted object). I find that confusing. Confusion can be countered with a comment. However, I still don't understand why we need to take this new reference. Can you explain? >> DriveInfo has no role in cleanly separate creation of frontend and >> backend now, and it shouldn't get one in the future. Its purpose is to >> support the legacy user interface that has frontend and backend matters >> mixed up.
On 23/03/2016 10:18, Markus Armbruster wrote: >> In Kevin's idea there would be no ownership either way. Until then, I >> think my patch actually gets us closer to the ideal. > > I'm afraid it gets us closer to where we used to be six years ago :) > > Qdev drive properties used to point to a DriveInfo, and the DriveInfo > pointed to BlockDriverState. Commit f8b6cc0 cut out the DriveInfo > middleman. This was a tiny step towards DriveInfo-less blockdev-add. > > DriveInfo is legacy configuration. Tacking it to BlockBackend is simple > and convenient. If it ceases to be simple and convenient, we can try to > find another home. But it really has no life of its own! I disagree; the life of DriveInfo is exactly the same as the -drive QemuOpts. But anyway, with your idea of adding an unrealize callback to the drive properties, I can move the extra reference within the device. It should become cleaner. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 23/03/2016 10:18, Markus Armbruster wrote: >>> In Kevin's idea there would be no ownership either way. Until then, I >>> think my patch actually gets us closer to the ideal. >> >> I'm afraid it gets us closer to where we used to be six years ago :) >> >> Qdev drive properties used to point to a DriveInfo, and the DriveInfo >> pointed to BlockDriverState. Commit f8b6cc0 cut out the DriveInfo >> middleman. This was a tiny step towards DriveInfo-less blockdev-add. >> >> DriveInfo is legacy configuration. Tacking it to BlockBackend is simple >> and convenient. If it ceases to be simple and convenient, we can try to >> find another home. But it really has no life of its own! > > I disagree; the life of DriveInfo is exactly the same as the -drive > QemuOpts. But anyway, with your idea of adding an unrealize callback to > the drive properties, I can move the extra reference within the device. > It should become cleaner. I guess discussing the finer semantic points some more wouldn't be productive now. Instead, you do a v2, and then we'll see. Working code can make philosophical differences evaporate :) Okay?
diff --git a/block/block-backend.c b/block/block-backend.c index ebdf78a..0a85c6a 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -65,8 +65,6 @@ static const AIOCBInfo block_backend_aiocb_info = { .aiocb_size = sizeof(BlockBackendAIOCB), }; -static void drive_info_del(DriveInfo *dinfo); - /* All the BlockBackends (except for hidden ones) */ static QTAILQ_HEAD(, BlockBackend) blk_backends = QTAILQ_HEAD_INITIALIZER(blk_backends); @@ -165,6 +163,7 @@ static void blk_delete(BlockBackend *blk) { assert(!blk->refcnt); assert(!blk->dev); + assert(!blk->legacy_dinfo); if (blk->bs) { blk_remove_bs(blk); } @@ -179,19 +178,25 @@ static void blk_delete(BlockBackend *blk) QTAILQ_REMOVE(&blk_backends, blk, link); } g_free(blk->name); - drive_info_del(blk->legacy_dinfo); block_acct_cleanup(&blk->stats); g_free(blk); } -static void drive_info_del(DriveInfo *dinfo) +void blk_release_legacy_dinfo(BlockBackend *blk) { + DriveInfo *dinfo = blk->legacy_dinfo; + if (!dinfo) { return; } qemu_opts_del(dinfo->opts); g_free(dinfo->serial); g_free(dinfo); + blk->legacy_dinfo = NULL; + /* We are not interested anymore in retrieving the BlockBackend + * via blk_by_legacy_dinfo, so let it die. + */ + blk_unref(blk); } int blk_get_refcnt(BlockBackend *blk) diff --git a/blockdev.c b/blockdev.c index 2dfb2d8..85f0cb5 100644 --- a/blockdev.c +++ b/blockdev.c @@ -120,10 +120,10 @@ void override_max_devs(BlockInterfaceType type, int max_devs) */ void blockdev_del_drive(BlockBackend *blk) { - DriveInfo *dinfo = blk_legacy_dinfo(blk); BlockDriverState *bs = blk_bs(blk); AioContext *aio_context; + blk_ref(blk); if (bs) { aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); @@ -135,9 +135,10 @@ void blockdev_del_drive(BlockBackend *blk) aio_context_release(aio_context); } - if (dinfo) { - blk_unref(blk); + if (blk_legacy_dinfo(blk)) { + blk_release_legacy_dinfo(blk); } + blk_unref(blk); } /** @@ -2811,7 +2812,7 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict) blk_set_on_error(blk, BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT); } else { - blk_unref(blk); + blockdev_del_drive(blk); } aio_context_release(aio_context); diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h index ae7ad67..5722b9f 100644 --- a/include/sysemu/blockdev.h +++ b/include/sysemu/blockdev.h @@ -44,6 +44,7 @@ struct DriveInfo { DriveInfo *blk_legacy_dinfo(BlockBackend *blk); DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo); BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo); +void blk_release_legacy_dinfo(BlockBackend *blk); void override_max_devs(BlockInterfaceType type, int max_devs);
Currently, blockdev_del_drive (and before it blk_auto_del) does a blk_unref that will cause blk_delete to be called and the DriveInfo to be freed. But really, we want to free the drive info as soon as the device is detached, even if there are other references for whatever reason, so that the QemuOpts are freed as well and the id can be reused. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- block/block-backend.c | 13 +++++++++---- blockdev.c | 9 +++++---- include/sysemu/blockdev.h | 1 + 3 files changed, 15 insertions(+), 8 deletions(-)