| Message ID | 1456151945-11225-3-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Paolo Bonzini <pbonzini@redhat.com> writes: > While the next patch will anticipate the death of the DriveInfo > data structure, the BlockBackend must survive after unrealize, > for example in case there are outstanding operations on it. > The good thing is that we can just use reference counting to > do it. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/core/qdev-properties-system.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c > index 469ba8a..5e84b55 100644 > --- a/hw/core/qdev-properties-system.c > +++ b/hw/core/qdev-properties-system.c > @@ -93,6 +93,7 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr, if (blk_attach_dev(blk, dev) < 0) { DriveInfo *dinfo = blk_legacy_dinfo(blk); if (dinfo->type != IF_NONE) { error_setg(errp, "Drive '%s' is already in use because " "it has been automatically connected to another " "device (did you need 'if=none' in the drive options?)", str); } else { error_setg(errp, "Drive '%s' is already in use by another device", str); } > return; > } > *ptr = blk; > + blk_ref(blk); blk_attach_dev() already takes a reference. I'm not sure I understand why you need to take a second one. You say "in case there are outstanding operations on it." What operations could that be? And shouldn't they take their own reference? I hasten to add that I'm not going to demand you fix them to take their own references. It's okay to take a hacky second reference here, then fix "them" at our leisure. But I need to understand what exactly this second reference protects. It probably needs to be explained in the source, too. > } > > static void release_drive(Object *obj, const char *name, void *opaque) > @@ -101,13 +102,17 @@ static void release_drive(Object *obj, const char *name, void *opaque) > Property *prop = opaque; > BlockBackend **ptr = qdev_get_prop_ptr(dev, prop); > > - if (*ptr && blk_get_attached_dev(*ptr) != NULL) { > - /* Unrealize has already called blk_detach_dev and blockdev_del_drive > - * if the device has been realized; in that case, blk_get_attached_dev > - * returns NULL. Thus, we get here if the device failed to realize, > - * and the -drive must not be released. > - */ > - blk_detach_dev(*ptr, dev); > + if (*ptr) { > + if (blk_get_attached_dev(*ptr) != NULL) { > + /* Unrealize has already called blk_detach_dev and > + * blockdev_del_drive if the device has been realized; > + * in that case, blk_get_attached_dev returns NULL. Thus, > + * we get here if the device failed to realize, and the > + * -drive must not be released. > + */ > + blk_detach_dev(*ptr, dev); > + } > + blk_unref(*ptr); > } > }
On 21/03/2016 16:22, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> While the next patch will anticipate the death of the DriveInfo >> data structure, the BlockBackend must survive after unrealize, >> for example in case there are outstanding operations on it. >> The good thing is that we can just use reference counting to >> do it. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> hw/core/qdev-properties-system.c | 19 ++++++++++++------- >> 1 file changed, 12 insertions(+), 7 deletions(-) >> >> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c >> index 469ba8a..5e84b55 100644 >> --- a/hw/core/qdev-properties-system.c >> +++ b/hw/core/qdev-properties-system.c >> @@ -93,6 +93,7 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr, > if (blk_attach_dev(blk, dev) < 0) { > DriveInfo *dinfo = blk_legacy_dinfo(blk); > > if (dinfo->type != IF_NONE) { > error_setg(errp, "Drive '%s' is already in use because " > "it has been automatically connected to another " > "device (did you need 'if=none' in the drive options?)", > str); > } else { > error_setg(errp, "Drive '%s' is already in use by another device", > str); > } >> return; >> } >> *ptr = blk; >> + blk_ref(blk); > > blk_attach_dev() already takes a reference. I'm not sure I understand > why you need to take a second one. You say "in case there are > outstanding operations on it." What operations could that be? There could be asynchronous I/O operations which are still active after unrealize. The device would not be finalized until they are completed. > And shouldn't they take their own reference? Generally the block layer doesn't try to ref/unref on every use. It assumes that someone else does it for you. A better justification for this patch is that blk_attach_dev/blk_detach_dev actually does not need to take a reference, so we can add it to parse_drive/release_drive and remove it from blk_attach_dev/blk_detach_dev instead. Paolo > I hasten to add that I'm not going to demand you fix them to take their > own references. It's okay to take a hacky second reference here, then > fix "them" at our leisure. But I need to understand what exactly this > second reference protects. It probably needs to be explained in the > source, too. > >> } >> >> static void release_drive(Object *obj, const char *name, void *opaque) >> @@ -101,13 +102,17 @@ static void release_drive(Object *obj, const char *name, void *opaque) >> Property *prop = opaque; >> BlockBackend **ptr = qdev_get_prop_ptr(dev, prop); >> >> - if (*ptr && blk_get_attached_dev(*ptr) != NULL) { >> - /* Unrealize has already called blk_detach_dev and blockdev_del_drive >> - * if the device has been realized; in that case, blk_get_attached_dev >> - * returns NULL. Thus, we get here if the device failed to realize, >> - * and the -drive must not be released. >> - */ >> - blk_detach_dev(*ptr, dev); >> + if (*ptr) { >> + if (blk_get_attached_dev(*ptr) != NULL) { >> + /* Unrealize has already called blk_detach_dev and >> + * blockdev_del_drive if the device has been realized; >> + * in that case, blk_get_attached_dev returns NULL. Thus, >> + * we get here if the device failed to realize, and the >> + * -drive must not be released. >> + */ >> + blk_detach_dev(*ptr, dev); >> + } >> + blk_unref(*ptr); >> } >> }
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index 469ba8a..5e84b55 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -93,6 +93,7 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr, return; } *ptr = blk; + blk_ref(blk); } static void release_drive(Object *obj, const char *name, void *opaque) @@ -101,13 +102,17 @@ static void release_drive(Object *obj, const char *name, void *opaque) Property *prop = opaque; BlockBackend **ptr = qdev_get_prop_ptr(dev, prop); - if (*ptr && blk_get_attached_dev(*ptr) != NULL) { - /* Unrealize has already called blk_detach_dev and blockdev_del_drive - * if the device has been realized; in that case, blk_get_attached_dev - * returns NULL. Thus, we get here if the device failed to realize, - * and the -drive must not be released. - */ - blk_detach_dev(*ptr, dev); + if (*ptr) { + if (blk_get_attached_dev(*ptr) != NULL) { + /* Unrealize has already called blk_detach_dev and + * blockdev_del_drive if the device has been realized; + * in that case, blk_get_attached_dev returns NULL. Thus, + * we get here if the device failed to realize, and the + * -drive must not be released. + */ + blk_detach_dev(*ptr, dev); + } + blk_unref(*ptr); } }
While the next patch will anticipate the death of the DriveInfo data structure, the BlockBackend must survive after unrealize, for example in case there are outstanding operations on it. The good thing is that we can just use reference counting to do it. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/core/qdev-properties-system.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)