* [PATCH v0 0/2] allow to set 'drive' property on a realized block device @ 2019-11-10 19:03 Denis Plotnikov 2019-11-10 19:03 ` [PATCH v0 1/2] qdev-properties-system: extend set_pionter for unrealized devices Denis Plotnikov ` (3 more replies) 0 siblings, 4 replies; 20+ messages in thread From: Denis Plotnikov @ 2019-11-10 19:03 UTC (permalink / raw) To: qemu-block Cc: kwolf, vsementsov, berrange, ehabkost, qemu-devel, mreitz, pbonzini, den This allows to replace the file on a block device and is useful to workaround the cases (migration) when the VM image is placed on some shared storage with exclusive file opening model but the image should be open form more than one app. The previous version of approaching the workaround was based on the "blockdev-change-medium" command modification but had some flaws: * semantics: blockdev-change-medium is aimed to be used with removable devices only * interface: it can't accept all possible combination of parameters for the "drive" replacement (creation). More details here: http://patchwork.ozlabs.org/patch/1179329/ The current series suggests another approach: 1. blockdev-add 2. qom-set disk.drive = the blockdev added (this is what the series adds) Denis Plotnikov (2): qdev-properties-system: extend set_pionter for unrealized devices block: allow to set 'drive' property on a realized block device hw/core/qdev-properties-system.c | 117 +++++++++++++++++++++++++------ 1 file changed, 96 insertions(+), 21 deletions(-) -- 2.17.0 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v0 1/2] qdev-properties-system: extend set_pionter for unrealized devices 2019-11-10 19:03 [PATCH v0 0/2] allow to set 'drive' property on a realized block device Denis Plotnikov @ 2019-11-10 19:03 ` Denis Plotnikov 2019-11-18 18:54 ` Eduardo Habkost 2019-11-10 19:03 ` [PATCH v0 2/2] block: allow to set 'drive' property on a realized block device Denis Plotnikov ` (2 subsequent siblings) 3 siblings, 1 reply; 20+ messages in thread From: Denis Plotnikov @ 2019-11-10 19:03 UTC (permalink / raw) To: qemu-block Cc: kwolf, vsementsov, berrange, ehabkost, qemu-devel, mreitz, pbonzini, den Some device's property can be changed if the device has been already realized. For example, it could be "drive" property of a scsi disk device. So far, set_pointer could operate only on a relized device. The patch extends its interface for operation on an unrealized device. Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> --- hw/core/qdev-properties-system.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index ba412dd2ca..c534590dcd 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -38,9 +38,14 @@ static void get_pointer(Object *obj, Visitor *v, Property *prop, } static void set_pointer(Object *obj, Visitor *v, Property *prop, - void (*parse)(DeviceState *dev, const char *str, - void **ptr, const char *propname, - Error **errp), + void (*parse_realized)(DeviceState *dev, + const char *str, void **ptr, + const char *propname, + Error **errp), + void (*parse_unrealized)(DeviceState *dev, + const char *str, void **ptr, + const char *propname, + Error **errp), const char *name, Error **errp) { DeviceState *dev = DEVICE(obj); @@ -48,11 +53,6 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop, void **ptr = qdev_get_prop_ptr(dev, prop); char *str; - if (dev->realized) { - qdev_prop_set_after_realize(dev, name, errp); - return; - } - visit_type_str(v, name, &str, &local_err); if (local_err) { error_propagate(errp, local_err); @@ -63,7 +63,17 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop, *ptr = NULL; return; } - parse(dev, str, ptr, prop->name, errp); + + if (dev->realized) { + if (parse_realized) { + parse_realized(dev, str, ptr, prop->name, errp); + } else { + qdev_prop_set_after_realize(dev, name, errp); + } + } else { + parse_unrealized(dev, str, ptr, prop->name, errp); + } + g_free(str); } @@ -178,13 +188,13 @@ static void get_drive(Object *obj, Visitor *v, const char *name, void *opaque, static void set_drive(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { - set_pointer(obj, v, opaque, parse_drive, name, errp); + set_pointer(obj, v, opaque, NULL, parse_drive, name, errp); } static void set_drive_iothread(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { - set_pointer(obj, v, opaque, parse_drive_iothread, name, errp); + set_pointer(obj, v, opaque, NULL, parse_drive_iothread, name, errp); } const PropertyInfo qdev_prop_drive = { -- 2.17.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v0 1/2] qdev-properties-system: extend set_pionter for unrealized devices 2019-11-10 19:03 ` [PATCH v0 1/2] qdev-properties-system: extend set_pionter for unrealized devices Denis Plotnikov @ 2019-11-18 18:54 ` Eduardo Habkost 2019-11-22 11:36 ` Denis Plotnikov 0 siblings, 1 reply; 20+ messages in thread From: Eduardo Habkost @ 2019-11-18 18:54 UTC (permalink / raw) To: Denis Plotnikov Cc: kwolf, vsementsov, berrange, den, qemu-block, qemu-devel, mreitz, pbonzini On Sun, Nov 10, 2019 at 10:03:09PM +0300, Denis Plotnikov wrote: > Some device's property can be changed if the device has been already > realized. For example, it could be "drive" property of a scsi disk device. > > So far, set_pointer could operate only on a relized device. The patch > extends its interface for operation on an unrealized device. > > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > --- > hw/core/qdev-properties-system.c | 32 +++++++++++++++++++++----------- > 1 file changed, 21 insertions(+), 11 deletions(-) > > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c > index ba412dd2ca..c534590dcd 100644 > --- a/hw/core/qdev-properties-system.c > +++ b/hw/core/qdev-properties-system.c > @@ -38,9 +38,14 @@ static void get_pointer(Object *obj, Visitor *v, Property *prop, > } > > static void set_pointer(Object *obj, Visitor *v, Property *prop, > - void (*parse)(DeviceState *dev, const char *str, > - void **ptr, const char *propname, > - Error **errp), > + void (*parse_realized)(DeviceState *dev, > + const char *str, void **ptr, > + const char *propname, > + Error **errp), > + void (*parse_unrealized)(DeviceState *dev, > + const char *str, void **ptr, > + const char *propname, > + Error **errp), > const char *name, Error **errp) Wouldn't it be simpler to just add a PropertyInfo::allow_set_after_realize bool field, and call the same setter function? Then you can simply change do_parse_drive() to check if realized is true. > { > DeviceState *dev = DEVICE(obj); > @@ -48,11 +53,6 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop, > void **ptr = qdev_get_prop_ptr(dev, prop); > char *str; > > - if (dev->realized) { > - qdev_prop_set_after_realize(dev, name, errp); > - return; > - } > - > visit_type_str(v, name, &str, &local_err); > if (local_err) { > error_propagate(errp, local_err); > @@ -63,7 +63,17 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop, > *ptr = NULL; > return; > } > - parse(dev, str, ptr, prop->name, errp); > + > + if (dev->realized) { > + if (parse_realized) { > + parse_realized(dev, str, ptr, prop->name, errp); > + } else { > + qdev_prop_set_after_realize(dev, name, errp); > + } > + } else { > + parse_unrealized(dev, str, ptr, prop->name, errp); > + } > + > g_free(str); > } > > @@ -178,13 +188,13 @@ static void get_drive(Object *obj, Visitor *v, const char *name, void *opaque, > static void set_drive(Object *obj, Visitor *v, const char *name, void *opaque, > Error **errp) > { > - set_pointer(obj, v, opaque, parse_drive, name, errp); > + set_pointer(obj, v, opaque, NULL, parse_drive, name, errp); > } > > static void set_drive_iothread(Object *obj, Visitor *v, const char *name, > void *opaque, Error **errp) > { > - set_pointer(obj, v, opaque, parse_drive_iothread, name, errp); > + set_pointer(obj, v, opaque, NULL, parse_drive_iothread, name, errp); > } > > const PropertyInfo qdev_prop_drive = { > -- > 2.17.0 > -- Eduardo ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v0 1/2] qdev-properties-system: extend set_pionter for unrealized devices 2019-11-18 18:54 ` Eduardo Habkost @ 2019-11-22 11:36 ` Denis Plotnikov 2019-11-25 15:30 ` Eduardo Habkost 0 siblings, 1 reply; 20+ messages in thread From: Denis Plotnikov @ 2019-11-22 11:36 UTC (permalink / raw) To: Eduardo Habkost Cc: kwolf, Vladimir Sementsov-Ogievskiy, berrange, Denis Lunev, qemu-block, qemu-devel, mreitz, pbonzini On 18.11.2019 21:54, Eduardo Habkost wrote: > On Sun, Nov 10, 2019 at 10:03:09PM +0300, Denis Plotnikov wrote: >> Some device's property can be changed if the device has been already >> realized. For example, it could be "drive" property of a scsi disk device. >> >> So far, set_pointer could operate only on a relized device. The patch >> extends its interface for operation on an unrealized device. >> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> >> --- >> hw/core/qdev-properties-system.c | 32 +++++++++++++++++++++----------- >> 1 file changed, 21 insertions(+), 11 deletions(-) >> >> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c >> index ba412dd2ca..c534590dcd 100644 >> --- a/hw/core/qdev-properties-system.c >> +++ b/hw/core/qdev-properties-system.c >> @@ -38,9 +38,14 @@ static void get_pointer(Object *obj, Visitor *v, Property *prop, >> } >> >> static void set_pointer(Object *obj, Visitor *v, Property *prop, >> - void (*parse)(DeviceState *dev, const char *str, >> - void **ptr, const char *propname, >> - Error **errp), >> + void (*parse_realized)(DeviceState *dev, >> + const char *str, void **ptr, >> + const char *propname, >> + Error **errp), >> + void (*parse_unrealized)(DeviceState *dev, >> + const char *str, void **ptr, >> + const char *propname, >> + Error **errp), >> const char *name, Error **errp) > Wouldn't it be simpler to just add a PropertyInfo::allow_set_after_realize > bool field, and call the same setter function? Then you can > simply change do_parse_drive() to check if realized is true. May be, but I thought It would be more clear to have a separate callback for all the devices supporting the property setting when realized. Also the "drive" property setting on realized and non-realized device a little bit different: in the realized case the setter function expects to get BlockDriverState only, when in the unrealized case the setter can accept both BlockBackend and BlockDriverState. Also, in the unrealized case the setter function doesn't expect to have a device with an empty BlockBackend. I decided that extending do_parse_drive would make it more complex for understanding. That's why I made two separate functions for both cases. I'd like to mention that I have a few concerns about do_parse_drive_realized (please see the next patch from the series) and I'd like them to be reviewed as well. After that, may be it would be better to go the way you suggested. Thanks for reviewing! Denis > >> { >> DeviceState *dev = DEVICE(obj); >> @@ -48,11 +53,6 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop, >> void **ptr = qdev_get_prop_ptr(dev, prop); >> char *str; >> >> - if (dev->realized) { >> - qdev_prop_set_after_realize(dev, name, errp); >> - return; >> - } >> - >> visit_type_str(v, name, &str, &local_err); >> if (local_err) { >> error_propagate(errp, local_err); >> @@ -63,7 +63,17 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop, >> *ptr = NULL; >> return; >> } >> - parse(dev, str, ptr, prop->name, errp); >> + >> + if (dev->realized) { >> + if (parse_realized) { >> + parse_realized(dev, str, ptr, prop->name, errp); >> + } else { >> + qdev_prop_set_after_realize(dev, name, errp); >> + } >> + } else { >> + parse_unrealized(dev, str, ptr, prop->name, errp); >> + } >> + >> g_free(str); >> } >> >> @@ -178,13 +188,13 @@ static void get_drive(Object *obj, Visitor *v, const char *name, void *opaque, >> static void set_drive(Object *obj, Visitor *v, const char *name, void *opaque, >> Error **errp) >> { >> - set_pointer(obj, v, opaque, parse_drive, name, errp); >> + set_pointer(obj, v, opaque, NULL, parse_drive, name, errp); >> } >> >> static void set_drive_iothread(Object *obj, Visitor *v, const char *name, >> void *opaque, Error **errp) >> { >> - set_pointer(obj, v, opaque, parse_drive_iothread, name, errp); >> + set_pointer(obj, v, opaque, NULL, parse_drive_iothread, name, errp); >> } >> >> const PropertyInfo qdev_prop_drive = { >> -- >> 2.17.0 >> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v0 1/2] qdev-properties-system: extend set_pionter for unrealized devices 2019-11-22 11:36 ` Denis Plotnikov @ 2019-11-25 15:30 ` Eduardo Habkost 2019-11-26 6:49 ` Denis Plotnikov 0 siblings, 1 reply; 20+ messages in thread From: Eduardo Habkost @ 2019-11-25 15:30 UTC (permalink / raw) To: Denis Plotnikov Cc: kwolf, Vladimir Sementsov-Ogievskiy, berrange, Denis Lunev, qemu-block, qemu-devel, mreitz, pbonzini On Fri, Nov 22, 2019 at 11:36:30AM +0000, Denis Plotnikov wrote: > > > On 18.11.2019 21:54, Eduardo Habkost wrote: > > On Sun, Nov 10, 2019 at 10:03:09PM +0300, Denis Plotnikov wrote: > >> Some device's property can be changed if the device has been already > >> realized. For example, it could be "drive" property of a scsi disk device. > >> > >> So far, set_pointer could operate only on a relized device. The patch > >> extends its interface for operation on an unrealized device. > >> > >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > >> --- > >> hw/core/qdev-properties-system.c | 32 +++++++++++++++++++++----------- > >> 1 file changed, 21 insertions(+), 11 deletions(-) > >> > >> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c > >> index ba412dd2ca..c534590dcd 100644 > >> --- a/hw/core/qdev-properties-system.c > >> +++ b/hw/core/qdev-properties-system.c > >> @@ -38,9 +38,14 @@ static void get_pointer(Object *obj, Visitor *v, Property *prop, > >> } > >> > >> static void set_pointer(Object *obj, Visitor *v, Property *prop, > >> - void (*parse)(DeviceState *dev, const char *str, > >> - void **ptr, const char *propname, > >> - Error **errp), > >> + void (*parse_realized)(DeviceState *dev, > >> + const char *str, void **ptr, > >> + const char *propname, > >> + Error **errp), > >> + void (*parse_unrealized)(DeviceState *dev, > >> + const char *str, void **ptr, > >> + const char *propname, > >> + Error **errp), > >> const char *name, Error **errp) > > Wouldn't it be simpler to just add a PropertyInfo::allow_set_after_realize > > bool field, and call the same setter function? Then you can > > simply change do_parse_drive() to check if realized is true. > May be, but I thought It would be more clear to have a separate callback > for all the devices supporting the property setting when realized. > Also the "drive" property setting on realized and non-realized device a > little bit different: in the realized case the setter function expects > to get > BlockDriverState only, when in the unrealized case the setter can accept > both BlockBackend and BlockDriverState. Also, in the unrealized case the > setter function doesn't expect to have a device with an empty BlockBackend. > I decided that extending do_parse_drive would make it more complex for > understanding. That's why I made two separate functions for both cases. I understand you might want two separate functions in the specific case of drive. You can still call different functions after checking dev->realized inside do_parse_drive(). My point was that you don't need to make set_pointer() require two separate function pointers just to propagate 1 bit of information that is already available in DeviceState. In patch 2/2 you had to create 4 different copies of parse_drive*() because of this. > > I'd like to mention that I have a few concerns about > do_parse_drive_realized (please see the next patch from the series) and > I'd like them to be reviewed as well. After that, may be it would be > better to go the way you suggested. In the case if your questions in patch 2/2, I'm afraid I don't know the answers and we need help from the block maintainers. -- Eduardo ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v0 1/2] qdev-properties-system: extend set_pionter for unrealized devices 2019-11-25 15:30 ` Eduardo Habkost @ 2019-11-26 6:49 ` Denis Plotnikov 2019-11-26 16:38 ` Kevin Wolf 0 siblings, 1 reply; 20+ messages in thread From: Denis Plotnikov @ 2019-11-26 6:49 UTC (permalink / raw) To: Eduardo Habkost Cc: kwolf, Vladimir Sementsov-Ogievskiy, berrange, Denis Lunev, qemu-block, qemu-devel, mreitz, pbonzini On 25.11.2019 18:30, Eduardo Habkost wrote: > On Fri, Nov 22, 2019 at 11:36:30AM +0000, Denis Plotnikov wrote: >> >> On 18.11.2019 21:54, Eduardo Habkost wrote: >>> On Sun, Nov 10, 2019 at 10:03:09PM +0300, Denis Plotnikov wrote: >>>> Some device's property can be changed if the device has been already >>>> realized. For example, it could be "drive" property of a scsi disk device. >>>> >>>> So far, set_pointer could operate only on a relized device. The patch >>>> extends its interface for operation on an unrealized device. >>>> >>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> >>>> --- >>>> hw/core/qdev-properties-system.c | 32 +++++++++++++++++++++----------- >>>> 1 file changed, 21 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c >>>> index ba412dd2ca..c534590dcd 100644 >>>> --- a/hw/core/qdev-properties-system.c >>>> +++ b/hw/core/qdev-properties-system.c >>>> @@ -38,9 +38,14 @@ static void get_pointer(Object *obj, Visitor *v, Property *prop, >>>> } >>>> >>>> static void set_pointer(Object *obj, Visitor *v, Property *prop, >>>> - void (*parse)(DeviceState *dev, const char *str, >>>> - void **ptr, const char *propname, >>>> - Error **errp), >>>> + void (*parse_realized)(DeviceState *dev, >>>> + const char *str, void **ptr, >>>> + const char *propname, >>>> + Error **errp), >>>> + void (*parse_unrealized)(DeviceState *dev, >>>> + const char *str, void **ptr, >>>> + const char *propname, >>>> + Error **errp), >>>> const char *name, Error **errp) >>> Wouldn't it be simpler to just add a PropertyInfo::allow_set_after_realize >>> bool field, and call the same setter function? Then you can >>> simply change do_parse_drive() to check if realized is true. >> May be, but I thought It would be more clear to have a separate callback >> for all the devices supporting the property setting when realized. >> Also the "drive" property setting on realized and non-realized device a >> little bit different: in the realized case the setter function expects >> to get >> BlockDriverState only, when in the unrealized case the setter can accept >> both BlockBackend and BlockDriverState. Also, in the unrealized case the >> setter function doesn't expect to have a device with an empty BlockBackend. >> I decided that extending do_parse_drive would make it more complex for >> understanding. That's why I made two separate functions for both cases. > I understand you might want two separate functions in the > specific case of drive. You can still call different > functions after checking dev->realized inside do_parse_drive(). > > My point was that you don't need to make set_pointer() require > two separate function pointers just to propagate 1 bit of > information that is already available in DeviceState. In patch > 2/2 you had to create 4 different copies of parse_drive*() > because of this. Yes, that's true. I wanted to suggest a more general way to deal with a device on realized and non-realized state. I may be too much and not necessary. May be we should wait for a feedback from the block maintainers? > > >> I'd like to mention that I have a few concerns about >> do_parse_drive_realized (please see the next patch from the series) and >> I'd like them to be reviewed as well. After that, may be it would be >> better to go the way you suggested. > In the case if your questions in patch 2/2, I'm afraid I don't > know the answers and we need help from the block maintainers. Anyway, thanks for taking a glance. > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v0 1/2] qdev-properties-system: extend set_pionter for unrealized devices 2019-11-26 6:49 ` Denis Plotnikov @ 2019-11-26 16:38 ` Kevin Wolf 0 siblings, 0 replies; 20+ messages in thread From: Kevin Wolf @ 2019-11-26 16:38 UTC (permalink / raw) To: Denis Plotnikov Cc: Vladimir Sementsov-Ogievskiy, berrange, Eduardo Habkost, qemu-block, qemu-devel, mreitz, pbonzini, Denis Lunev Am 26.11.2019 um 07:49 hat Denis Plotnikov geschrieben: > > > On 25.11.2019 18:30, Eduardo Habkost wrote: > > On Fri, Nov 22, 2019 at 11:36:30AM +0000, Denis Plotnikov wrote: > >> > >> On 18.11.2019 21:54, Eduardo Habkost wrote: > >>> On Sun, Nov 10, 2019 at 10:03:09PM +0300, Denis Plotnikov wrote: > >>>> Some device's property can be changed if the device has been already > >>>> realized. For example, it could be "drive" property of a scsi disk device. > >>>> > >>>> So far, set_pointer could operate only on a relized device. The patch > >>>> extends its interface for operation on an unrealized device. > >>>> > >>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > >>>> --- > >>>> hw/core/qdev-properties-system.c | 32 +++++++++++++++++++++----------- > >>>> 1 file changed, 21 insertions(+), 11 deletions(-) > >>>> > >>>> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c > >>>> index ba412dd2ca..c534590dcd 100644 > >>>> --- a/hw/core/qdev-properties-system.c > >>>> +++ b/hw/core/qdev-properties-system.c > >>>> @@ -38,9 +38,14 @@ static void get_pointer(Object *obj, Visitor *v, Property *prop, > >>>> } > >>>> > >>>> static void set_pointer(Object *obj, Visitor *v, Property *prop, > >>>> - void (*parse)(DeviceState *dev, const char *str, > >>>> - void **ptr, const char *propname, > >>>> - Error **errp), > >>>> + void (*parse_realized)(DeviceState *dev, > >>>> + const char *str, void **ptr, > >>>> + const char *propname, > >>>> + Error **errp), > >>>> + void (*parse_unrealized)(DeviceState *dev, > >>>> + const char *str, void **ptr, > >>>> + const char *propname, > >>>> + Error **errp), > >>>> const char *name, Error **errp) > >>> Wouldn't it be simpler to just add a PropertyInfo::allow_set_after_realize > >>> bool field, and call the same setter function? Then you can > >>> simply change do_parse_drive() to check if realized is true. > >> May be, but I thought It would be more clear to have a separate callback > >> for all the devices supporting the property setting when realized. > >> Also the "drive" property setting on realized and non-realized device a > >> little bit different: in the realized case the setter function expects > >> to get > >> BlockDriverState only, when in the unrealized case the setter can accept > >> both BlockBackend and BlockDriverState. Also, in the unrealized case the > >> setter function doesn't expect to have a device with an empty BlockBackend. > >> I decided that extending do_parse_drive would make it more complex for > >> understanding. That's why I made two separate functions for both cases. > > I understand you might want two separate functions in the > > specific case of drive. You can still call different > > functions after checking dev->realized inside do_parse_drive(). > > > > My point was that you don't need to make set_pointer() require > > two separate function pointers just to propagate 1 bit of > > information that is already available in DeviceState. In patch > > 2/2 you had to create 4 different copies of parse_drive*() > > because of this. > Yes, that's true. I wanted to suggest a more general way to deal with a > device on realized and non-realized state. > I may be too much and not necessary. May be we should wait for a > feedback from the block maintainers? I think I agree with Eduardo on this one. This is generic infrastructure and we already see that it may require lots of wrapper functions that call into the same function with different parameters. Checking dev->realized where necessary can do the same. I haven't had a closer look at the specific functions, but maybe they can actually share the same implementation with just some if (dev->realized) blocks. Kevin ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v0 2/2] block: allow to set 'drive' property on a realized block device 2019-11-10 19:03 [PATCH v0 0/2] allow to set 'drive' property on a realized block device Denis Plotnikov 2019-11-10 19:03 ` [PATCH v0 1/2] qdev-properties-system: extend set_pionter for unrealized devices Denis Plotnikov @ 2019-11-10 19:03 ` Denis Plotnikov 2019-11-10 19:08 ` Denis Plotnikov 2019-11-18 10:30 ` [PATCH v0 0/2] " Denis Plotnikov 2020-03-02 13:38 ` Kevin Wolf 3 siblings, 1 reply; 20+ messages in thread From: Denis Plotnikov @ 2019-11-10 19:03 UTC (permalink / raw) To: qemu-block Cc: kwolf, vsementsov, berrange, ehabkost, qemu-devel, mreitz, pbonzini, den This allows to change (replace) the file on a block device and is useful to workaround exclusive file access restrictions, e.g. to implement VM migration with a shared disk stored on some storage with the exclusive file opening model: a destination VM is started waiting for incomming migration with a fake image drive, and later, on the last migration phase, the fake image file is replaced with the real one. Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> --- hw/core/qdev-properties-system.c | 89 +++++++++++++++++++++++++++----- 1 file changed, 77 insertions(+), 12 deletions(-) diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index c534590dcd..aaab1370a4 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -79,8 +79,55 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop, /* --- drive --- */ -static void do_parse_drive(DeviceState *dev, const char *str, void **ptr, - const char *propname, bool iothread, Error **errp) +static void do_parse_drive_realized(DeviceState *dev, const char *str, + void **ptr, const char *propname, + bool iothread, Error **errp) +{ + BlockBackend *blk = *ptr; + BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL); + int ret; + bool blk_created = false; + + if (!bs) { + error_setg(errp, "Can't find blockdev '%s'", str); + return; + } + + if (!blk) { + AioContext *ctx = iothread ? bdrv_get_aio_context(bs) : + qemu_get_aio_context(); + blk = blk_new(ctx, BLK_PERM_ALL, BLK_PERM_ALL); + blk_created = true; + } else { + if (blk_bs(blk)) { + blk_remove_bs(blk); + } + } + + ret = blk_insert_bs(blk, bs, errp); + + if (!ret && blk_created) { + if (blk_attach_dev(blk, dev) < 0) { + /* + * Shouldn't be any errors here since we just created + * the new blk because the device doesn't have any. + * Leave the message here in case blk_attach_dev is changed + */ + error_setg(errp, "Can't attach drive '%s' to device '%s'", + str, object_get_typename(OBJECT(dev))); + } else { + *ptr = blk; + } + } + + if (blk_created) { + blk_unref(blk); + } +} + +static void do_parse_drive_unrealized(DeviceState *dev, const char *str, + void **ptr, const char *propname, + bool iothread, Error **errp) { BlockBackend *blk; bool blk_created = false; @@ -137,18 +184,34 @@ fail: } } -static void parse_drive(DeviceState *dev, const char *str, void **ptr, - const char *propname, Error **errp) -{ - do_parse_drive(dev, str, ptr, propname, false, errp); -} - -static void parse_drive_iothread(DeviceState *dev, const char *str, void **ptr, +static void parse_drive_realized(DeviceState *dev, const char *str, void **ptr, const char *propname, Error **errp) { - do_parse_drive(dev, str, ptr, propname, true, errp); + do_parse_drive_realized(dev, str, ptr, propname, false, errp); } +static void parse_drive_realized_iothread(DeviceState *dev, const char *str, + void **ptr, const char *propname, + Error **errp) +{ + do_parse_drive_realized(dev, str, ptr, propname, true, errp); +} + +static void parse_drive_unrealized(DeviceState *dev, const char *str, + void **ptr, const char *propname, + Error **errp) +{ + do_parse_drive_unrealized(dev, str, ptr, propname, false, errp); +} + +static void parse_drive_unrealized_iothread(DeviceState *dev, const char *str, + void **ptr, const char *propname, + Error **errp) +{ + do_parse_drive_unrealized(dev, str, ptr, propname, true, errp); +} + + static void release_drive(Object *obj, const char *name, void *opaque) { DeviceState *dev = DEVICE(obj); @@ -188,13 +251,15 @@ static void get_drive(Object *obj, Visitor *v, const char *name, void *opaque, static void set_drive(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { - set_pointer(obj, v, opaque, NULL, parse_drive, name, errp); + set_pointer(obj, v, opaque, parse_drive_realized, parse_drive_unrealized, + name, errp); } static void set_drive_iothread(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { - set_pointer(obj, v, opaque, NULL, parse_drive_iothread, name, errp); + set_pointer(obj, v, opaque, parse_drive_realized_iothread, + parse_drive_unrealized_iothread, name, errp); } const PropertyInfo qdev_prop_drive = { -- 2.17.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v0 2/2] block: allow to set 'drive' property on a realized block device 2019-11-10 19:03 ` [PATCH v0 2/2] block: allow to set 'drive' property on a realized block device Denis Plotnikov @ 2019-11-10 19:08 ` Denis Plotnikov 2019-11-18 10:50 ` Denis Plotnikov 0 siblings, 1 reply; 20+ messages in thread From: Denis Plotnikov @ 2019-11-10 19:08 UTC (permalink / raw) To: qemu-block Cc: kwolf, Vladimir Sementsov-Ogievskiy, berrange, ehabkost, qemu-devel, mreitz, pbonzini, Denis Lunev On 10.11.2019 22:03, Denis Plotnikov wrote: > This allows to change (replace) the file on a block device and is useful > to workaround exclusive file access restrictions, e.g. to implement VM > migration with a shared disk stored on some storage with the exclusive > file opening model: a destination VM is started waiting for incomming > migration with a fake image drive, and later, on the last migration > phase, the fake image file is replaced with the real one. > > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > --- > hw/core/qdev-properties-system.c | 89 +++++++++++++++++++++++++++----- > 1 file changed, 77 insertions(+), 12 deletions(-) > > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c > index c534590dcd..aaab1370a4 100644 > --- a/hw/core/qdev-properties-system.c > +++ b/hw/core/qdev-properties-system.c > @@ -79,8 +79,55 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop, > > /* --- drive --- */ > > -static void do_parse_drive(DeviceState *dev, const char *str, void **ptr, > - const char *propname, bool iothread, Error **errp) > +static void do_parse_drive_realized(DeviceState *dev, const char *str, > + void **ptr, const char *propname, > + bool iothread, Error **errp) > +{ > + BlockBackend *blk = *ptr; > + BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL); > + int ret; > + bool blk_created = false; > + > + if (!bs) { > + error_setg(errp, "Can't find blockdev '%s'", str); > + return; > + } > + > + if (!blk) { > + AioContext *ctx = iothread ? bdrv_get_aio_context(bs) : > + qemu_get_aio_context(); > + blk = blk_new(ctx, BLK_PERM_ALL, BLK_PERM_ALL); > + blk_created = true; Actually, I have concerns about situation where blk=null. Is there any case when scsi-hd (or others) doesn't have a blk assigned and it's legal? > + } else { > + if (blk_bs(blk)) { > + blk_remove_bs(blk); > + } > + } > + > + ret = blk_insert_bs(blk, bs, errp); > + > + if (!ret && blk_created) { > + if (blk_attach_dev(blk, dev) < 0) { > + /* > + * Shouldn't be any errors here since we just created > + * the new blk because the device doesn't have any. > + * Leave the message here in case blk_attach_dev is changed > + */ > + error_setg(errp, "Can't attach drive '%s' to device '%s'", > + str, object_get_typename(OBJECT(dev))); > + } else { > + *ptr = blk; > + } > + } > + > + if (blk_created) { > + blk_unref(blk); > + } > +} > + > +static void do_parse_drive_unrealized(DeviceState *dev, const char *str, > + void **ptr, const char *propname, > + bool iothread, Error **errp) > { > BlockBackend *blk; > bool blk_created = false; > @@ -137,18 +184,34 @@ fail: > } > } > > -static void parse_drive(DeviceState *dev, const char *str, void **ptr, > - const char *propname, Error **errp) > -{ > - do_parse_drive(dev, str, ptr, propname, false, errp); > -} > - > -static void parse_drive_iothread(DeviceState *dev, const char *str, void **ptr, > +static void parse_drive_realized(DeviceState *dev, const char *str, void **ptr, > const char *propname, Error **errp) > { > - do_parse_drive(dev, str, ptr, propname, true, errp); > + do_parse_drive_realized(dev, str, ptr, propname, false, errp); > } > > +static void parse_drive_realized_iothread(DeviceState *dev, const char *str, > + void **ptr, const char *propname, > + Error **errp) > +{ > + do_parse_drive_realized(dev, str, ptr, propname, true, errp); > +} > + > +static void parse_drive_unrealized(DeviceState *dev, const char *str, > + void **ptr, const char *propname, > + Error **errp) > +{ > + do_parse_drive_unrealized(dev, str, ptr, propname, false, errp); > +} > + > +static void parse_drive_unrealized_iothread(DeviceState *dev, const char *str, > + void **ptr, const char *propname, > + Error **errp) > +{ > + do_parse_drive_unrealized(dev, str, ptr, propname, true, errp); > +} > + > + > static void release_drive(Object *obj, const char *name, void *opaque) > { > DeviceState *dev = DEVICE(obj); > @@ -188,13 +251,15 @@ static void get_drive(Object *obj, Visitor *v, const char *name, void *opaque, > static void set_drive(Object *obj, Visitor *v, const char *name, void *opaque, > Error **errp) > { > - set_pointer(obj, v, opaque, NULL, parse_drive, name, errp); > + set_pointer(obj, v, opaque, parse_drive_realized, parse_drive_unrealized, > + name, errp); > } > > static void set_drive_iothread(Object *obj, Visitor *v, const char *name, > void *opaque, Error **errp) > { > - set_pointer(obj, v, opaque, NULL, parse_drive_iothread, name, errp); > + set_pointer(obj, v, opaque, parse_drive_realized_iothread, > + parse_drive_unrealized_iothread, name, errp); > } > > const PropertyInfo qdev_prop_drive = { ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v0 2/2] block: allow to set 'drive' property on a realized block device 2019-11-10 19:08 ` Denis Plotnikov @ 2019-11-18 10:50 ` Denis Plotnikov 2019-12-13 7:30 ` [PING]Re: " Denis Plotnikov 2019-12-13 10:32 ` Kevin Wolf 0 siblings, 2 replies; 20+ messages in thread From: Denis Plotnikov @ 2019-11-18 10:50 UTC (permalink / raw) To: qemu-block Cc: kwolf, Vladimir Sementsov-Ogievskiy, berrange, ehabkost, qemu-devel, mreitz, pbonzini, Denis Lunev On 10.11.2019 22:08, Denis Plotnikov wrote: > > On 10.11.2019 22:03, Denis Plotnikov wrote: >> This allows to change (replace) the file on a block device and is useful >> to workaround exclusive file access restrictions, e.g. to implement VM >> migration with a shared disk stored on some storage with the exclusive >> file opening model: a destination VM is started waiting for incomming >> migration with a fake image drive, and later, on the last migration >> phase, the fake image file is replaced with the real one. >> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> >> --- >> hw/core/qdev-properties-system.c | 89 +++++++++++++++++++++++++++----- >> 1 file changed, 77 insertions(+), 12 deletions(-) >> >> diff --git a/hw/core/qdev-properties-system.c >> b/hw/core/qdev-properties-system.c >> index c534590dcd..aaab1370a4 100644 >> --- a/hw/core/qdev-properties-system.c >> +++ b/hw/core/qdev-properties-system.c >> @@ -79,8 +79,55 @@ static void set_pointer(Object *obj, Visitor *v, >> Property *prop, >> /* --- drive --- */ >> -static void do_parse_drive(DeviceState *dev, const char *str, void >> **ptr, >> - const char *propname, bool iothread, >> Error **errp) >> +static void do_parse_drive_realized(DeviceState *dev, const char *str, >> + void **ptr, const char *propname, >> + bool iothread, Error **errp) >> +{ >> + BlockBackend *blk = *ptr; >> + BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL); >> + int ret; >> + bool blk_created = false; >> + >> + if (!bs) { >> + error_setg(errp, "Can't find blockdev '%s'", str); >> + return; >> + } >> + >> + if (!blk) { >> + AioContext *ctx = iothread ? bdrv_get_aio_context(bs) : >> + qemu_get_aio_context(); >> + blk = blk_new(ctx, BLK_PERM_ALL, BLK_PERM_ALL); >> + blk_created = true; > > Actually, I have concerns about situation where blk=null. > > Is there any case when scsi-hd (or others) doesn't have a blk assigned > and it's legal? > >> + } else { >> + if (blk_bs(blk)) { >> + blk_remove_bs(blk); >> + } >> + } >> + >> + ret = blk_insert_bs(blk, bs, errp); >> + >> + if (!ret && blk_created) { >> + if (blk_attach_dev(blk, dev) < 0) { >> + /* >> + * Shouldn't be any errors here since we just created >> + * the new blk because the device doesn't have any. >> + * Leave the message here in case blk_attach_dev is changed >> + */ >> + error_setg(errp, "Can't attach drive '%s' to device '%s'", >> + str, object_get_typename(OBJECT(dev))); >> + } else { >> + *ptr = blk; >> + } >> + } Another problem here, is that the "size" of the device dev may not match after setting a drive. So, we should update it after the drive setting. It was found, that it could be done by calling BlockDevOps.bdrv_parent_cb_resize. But I have some concerns about doing it so. In the case of virtio scsi disk we have the following callstack bdrv_parent_cb_resize calls() -> scsi_device_report_change(dev, SENSE_CODE(CAPACITY_CHANGED)) -> virtio_scsi_change -> virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_PARAM_CHANGE, sense.asc | (sense.ascq << 8)); virtio_scsi_change pushes the event to the guest to make the guest ask for size refreshing. If I'm not mistaken, here we can get a race condition when some another request is processed with an unchanged size and then the size changing request is processed. I didn't find a better way to update device size so any comments are welcome. Thanks! Denis >> + >> + if (blk_created) { >> + blk_unref(blk); >> + } >> +} >> + >> +static void do_parse_drive_unrealized(DeviceState *dev, const char >> *str, >> + void **ptr, const char *propname, >> + bool iothread, Error **errp) >> { >> BlockBackend *blk; >> bool blk_created = false; >> @@ -137,18 +184,34 @@ fail: >> } >> } >> -static void parse_drive(DeviceState *dev, const char *str, void >> **ptr, >> - const char *propname, Error **errp) >> -{ >> - do_parse_drive(dev, str, ptr, propname, false, errp); >> -} >> - >> -static void parse_drive_iothread(DeviceState *dev, const char *str, >> void **ptr, >> +static void parse_drive_realized(DeviceState *dev, const char *str, >> void **ptr, >> const char *propname, Error **errp) >> { >> - do_parse_drive(dev, str, ptr, propname, true, errp); >> + do_parse_drive_realized(dev, str, ptr, propname, false, errp); >> } >> +static void parse_drive_realized_iothread(DeviceState *dev, const >> char *str, >> + void **ptr, const char >> *propname, >> + Error **errp) >> +{ >> + do_parse_drive_realized(dev, str, ptr, propname, true, errp); >> +} >> + >> +static void parse_drive_unrealized(DeviceState *dev, const char *str, >> + void **ptr, const char *propname, >> + Error **errp) >> +{ >> + do_parse_drive_unrealized(dev, str, ptr, propname, false, errp); >> +} >> + >> +static void parse_drive_unrealized_iothread(DeviceState *dev, const >> char *str, >> + void **ptr, const char >> *propname, >> + Error **errp) >> +{ >> + do_parse_drive_unrealized(dev, str, ptr, propname, true, errp); >> +} >> + >> + >> static void release_drive(Object *obj, const char *name, void *opaque) >> { >> DeviceState *dev = DEVICE(obj); >> @@ -188,13 +251,15 @@ static void get_drive(Object *obj, Visitor *v, >> const char *name, void *opaque, >> static void set_drive(Object *obj, Visitor *v, const char *name, >> void *opaque, >> Error **errp) >> { >> - set_pointer(obj, v, opaque, NULL, parse_drive, name, errp); >> + set_pointer(obj, v, opaque, parse_drive_realized, >> parse_drive_unrealized, >> + name, errp); >> } >> static void set_drive_iothread(Object *obj, Visitor *v, const >> char *name, >> void *opaque, Error **errp) >> { >> - set_pointer(obj, v, opaque, NULL, parse_drive_iothread, name, >> errp); >> + set_pointer(obj, v, opaque, parse_drive_realized_iothread, >> + parse_drive_unrealized_iothread, name, errp); >> } >> const PropertyInfo qdev_prop_drive = { ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PING]Re: [PATCH v0 2/2] block: allow to set 'drive' property on a realized block device 2019-11-18 10:50 ` Denis Plotnikov @ 2019-12-13 7:30 ` Denis Plotnikov 2019-12-13 10:32 ` Kevin Wolf 1 sibling, 0 replies; 20+ messages in thread From: Denis Plotnikov @ 2019-12-13 7:30 UTC (permalink / raw) To: qemu-block Cc: kwolf, Vladimir Sementsov-Ogievskiy, berrange, ehabkost, qemu-devel, mreitz, pbonzini, Denis Lunev On 18.11.2019 13:50, Denis Plotnikov wrote: > > > On 10.11.2019 22:08, Denis Plotnikov wrote: >> >> On 10.11.2019 22:03, Denis Plotnikov wrote: >>> This allows to change (replace) the file on a block device and is >>> useful >>> to workaround exclusive file access restrictions, e.g. to implement VM >>> migration with a shared disk stored on some storage with the exclusive >>> file opening model: a destination VM is started waiting for incomming >>> migration with a fake image drive, and later, on the last migration >>> phase, the fake image file is replaced with the real one. >>> >>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> >>> --- >>> hw/core/qdev-properties-system.c | 89 >>> +++++++++++++++++++++++++++----- >>> 1 file changed, 77 insertions(+), 12 deletions(-) >>> >>> diff --git a/hw/core/qdev-properties-system.c >>> b/hw/core/qdev-properties-system.c >>> index c534590dcd..aaab1370a4 100644 >>> --- a/hw/core/qdev-properties-system.c >>> +++ b/hw/core/qdev-properties-system.c >>> @@ -79,8 +79,55 @@ static void set_pointer(Object *obj, Visitor *v, >>> Property *prop, >>> /* --- drive --- */ >>> -static void do_parse_drive(DeviceState *dev, const char *str, >>> void **ptr, >>> - const char *propname, bool iothread, >>> Error **errp) >>> +static void do_parse_drive_realized(DeviceState *dev, const char *str, >>> + void **ptr, const char *propname, >>> + bool iothread, Error **errp) >>> +{ >>> + BlockBackend *blk = *ptr; >>> + BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL); >>> + int ret; >>> + bool blk_created = false; >>> + >>> + if (!bs) { >>> + error_setg(errp, "Can't find blockdev '%s'", str); >>> + return; >>> + } >>> + >>> + if (!blk) { >>> + AioContext *ctx = iothread ? bdrv_get_aio_context(bs) : >>> + qemu_get_aio_context(); >>> + blk = blk_new(ctx, BLK_PERM_ALL, BLK_PERM_ALL); >>> + blk_created = true; >> >> Actually, I have concerns about situation where blk=null. >> >> Is there any case when scsi-hd (or others) doesn't have a blk >> assigned and it's legal? >> >>> + } else { >>> + if (blk_bs(blk)) { >>> + blk_remove_bs(blk); >>> + } >>> + } >>> + >>> + ret = blk_insert_bs(blk, bs, errp); >>> + >>> + if (!ret && blk_created) { >>> + if (blk_attach_dev(blk, dev) < 0) { >>> + /* >>> + * Shouldn't be any errors here since we just created >>> + * the new blk because the device doesn't have any. >>> + * Leave the message here in case blk_attach_dev is >>> changed >>> + */ >>> + error_setg(errp, "Can't attach drive '%s' to device >>> '%s'", >>> + str, object_get_typename(OBJECT(dev))); >>> + } else { >>> + *ptr = blk; >>> + } >>> + } > Another problem here, is that the "size" of the device dev may not > match after setting a drive. > So, we should update it after the drive setting. > It was found, that it could be done by calling > BlockDevOps.bdrv_parent_cb_resize. > > But I have some concerns about doing it so. In the case of virtio scsi > disk we have the following callstack > > bdrv_parent_cb_resize calls() -> > scsi_device_report_change(dev, SENSE_CODE(CAPACITY_CHANGED)) -> > virtio_scsi_change -> > virtio_scsi_push_event(s, dev, > VIRTIO_SCSI_T_PARAM_CHANGE, > sense.asc | > (sense.ascq << 8)); > > > virtio_scsi_change pushes the event to the guest to make the guest > ask for size refreshing. > If I'm not mistaken, here we can get a race condition when some > another request is processed with an unchanged > size and then the size changing request is processed. > > I didn't find a better way to update device size so any comments are > welcome. > > Thanks! > > Denis >>> + >>> + if (blk_created) { >>> + blk_unref(blk); >>> + } >>> +} >>> + >>> +static void do_parse_drive_unrealized(DeviceState *dev, const char >>> *str, >>> + void **ptr, const char >>> *propname, >>> + bool iothread, Error **errp) >>> { >>> BlockBackend *blk; >>> bool blk_created = false; >>> @@ -137,18 +184,34 @@ fail: >>> } >>> } >>> -static void parse_drive(DeviceState *dev, const char *str, void >>> **ptr, >>> - const char *propname, Error **errp) >>> -{ >>> - do_parse_drive(dev, str, ptr, propname, false, errp); >>> -} >>> - >>> -static void parse_drive_iothread(DeviceState *dev, const char *str, >>> void **ptr, >>> +static void parse_drive_realized(DeviceState *dev, const char *str, >>> void **ptr, >>> const char *propname, Error **errp) >>> { >>> - do_parse_drive(dev, str, ptr, propname, true, errp); >>> + do_parse_drive_realized(dev, str, ptr, propname, false, errp); >>> } >>> +static void parse_drive_realized_iothread(DeviceState *dev, const >>> char *str, >>> + void **ptr, const char >>> *propname, >>> + Error **errp) >>> +{ >>> + do_parse_drive_realized(dev, str, ptr, propname, true, errp); >>> +} >>> + >>> +static void parse_drive_unrealized(DeviceState *dev, const char *str, >>> + void **ptr, const char *propname, >>> + Error **errp) >>> +{ >>> + do_parse_drive_unrealized(dev, str, ptr, propname, false, errp); >>> +} >>> + >>> +static void parse_drive_unrealized_iothread(DeviceState *dev, const >>> char *str, >>> + void **ptr, const char >>> *propname, >>> + Error **errp) >>> +{ >>> + do_parse_drive_unrealized(dev, str, ptr, propname, true, errp); >>> +} >>> + >>> + >>> static void release_drive(Object *obj, const char *name, void >>> *opaque) >>> { >>> DeviceState *dev = DEVICE(obj); >>> @@ -188,13 +251,15 @@ static void get_drive(Object *obj, Visitor *v, >>> const char *name, void *opaque, >>> static void set_drive(Object *obj, Visitor *v, const char *name, >>> void *opaque, >>> Error **errp) >>> { >>> - set_pointer(obj, v, opaque, NULL, parse_drive, name, errp); >>> + set_pointer(obj, v, opaque, parse_drive_realized, >>> parse_drive_unrealized, >>> + name, errp); >>> } >>> static void set_drive_iothread(Object *obj, Visitor *v, const >>> char *name, >>> void *opaque, Error **errp) >>> { >>> - set_pointer(obj, v, opaque, NULL, parse_drive_iothread, name, >>> errp); >>> + set_pointer(obj, v, opaque, parse_drive_realized_iothread, >>> + parse_drive_unrealized_iothread, name, errp); >>> } >>> const PropertyInfo qdev_prop_drive = { > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v0 2/2] block: allow to set 'drive' property on a realized block device 2019-11-18 10:50 ` Denis Plotnikov 2019-12-13 7:30 ` [PING]Re: " Denis Plotnikov @ 2019-12-13 10:32 ` Kevin Wolf 2019-12-16 14:51 ` Denis Plotnikov 1 sibling, 1 reply; 20+ messages in thread From: Kevin Wolf @ 2019-12-13 10:32 UTC (permalink / raw) To: Denis Plotnikov Cc: Vladimir Sementsov-Ogievskiy, berrange, ehabkost, qemu-block, qemu-devel, mreitz, pbonzini, Denis Lunev Am 18.11.2019 um 11:50 hat Denis Plotnikov geschrieben: > > > On 10.11.2019 22:08, Denis Plotnikov wrote: > > > > On 10.11.2019 22:03, Denis Plotnikov wrote: > >> This allows to change (replace) the file on a block device and is useful > >> to workaround exclusive file access restrictions, e.g. to implement VM > >> migration with a shared disk stored on some storage with the exclusive > >> file opening model: a destination VM is started waiting for incomming > >> migration with a fake image drive, and later, on the last migration > >> phase, the fake image file is replaced with the real one. > >> > >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > >> --- > >> hw/core/qdev-properties-system.c | 89 +++++++++++++++++++++++++++----- > >> 1 file changed, 77 insertions(+), 12 deletions(-) > >> > >> diff --git a/hw/core/qdev-properties-system.c > >> b/hw/core/qdev-properties-system.c > >> index c534590dcd..aaab1370a4 100644 > >> --- a/hw/core/qdev-properties-system.c > >> +++ b/hw/core/qdev-properties-system.c > >> @@ -79,8 +79,55 @@ static void set_pointer(Object *obj, Visitor *v, > >> Property *prop, > >> /* --- drive --- */ > >> -static void do_parse_drive(DeviceState *dev, const char *str, void > >> **ptr, > >> - const char *propname, bool iothread, > >> Error **errp) > >> +static void do_parse_drive_realized(DeviceState *dev, const char *str, > >> + void **ptr, const char *propname, > >> + bool iothread, Error **errp) > >> +{ > >> + BlockBackend *blk = *ptr; > >> + BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL); > >> + int ret; > >> + bool blk_created = false; > >> + > >> + if (!bs) { > >> + error_setg(errp, "Can't find blockdev '%s'", str); > >> + return; > >> + } > >> + > >> + if (!blk) { > >> + AioContext *ctx = iothread ? bdrv_get_aio_context(bs) : > >> + qemu_get_aio_context(); > >> + blk = blk_new(ctx, BLK_PERM_ALL, BLK_PERM_ALL); > >> + blk_created = true; > > > > Actually, I have concerns about situation where blk=null. > > > > Is there any case when scsi-hd (or others) doesn't have a blk assigned > > and it's legal? No, block devices will always have a BlockBackend, even if it doesn't have a root node inserted. > >> + } else { > >> + if (blk_bs(blk)) { > >> + blk_remove_bs(blk); > >> + } > >> + } > >> + > >> + ret = blk_insert_bs(blk, bs, errp); > >> + > >> + if (!ret && blk_created) { > >> + if (blk_attach_dev(blk, dev) < 0) { > >> + /* > >> + * Shouldn't be any errors here since we just created > >> + * the new blk because the device doesn't have any. > >> + * Leave the message here in case blk_attach_dev is changed > >> + */ > >> + error_setg(errp, "Can't attach drive '%s' to device '%s'", > >> + str, object_get_typename(OBJECT(dev))); > >> + } else { > >> + *ptr = blk; > >> + } > >> + } > Another problem here, is that the "size" of the device dev may not match > after setting a drive. > So, we should update it after the drive setting. > It was found, that it could be done by calling > BlockDevOps.bdrv_parent_cb_resize. > > But I have some concerns about doing it so. In the case of virtio scsi > disk we have the following callstack > > bdrv_parent_cb_resize calls() -> > scsi_device_report_change(dev, SENSE_CODE(CAPACITY_CHANGED)) -> > virtio_scsi_change -> > virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_PARAM_CHANGE, > sense.asc | > (sense.ascq << 8)); I think the safest option for now (and which should solve the case you want to address) is checking whether old and new size match and returning an error otherwise. > virtio_scsi_change pushes the event to the guest to make the guest > ask for size refreshing. If I'm not mistaken, here we can get a race > condition when some another request is processed with an unchanged > size and then the size changing request is processed. I think this is actually a problem even without resizing: We need to quiesce the device between removing the old root and inserting the new one. They way to achieve this is probably by splitting blk_drain() into a blk_drain_begin()/end() and then draining the BlockBackend here while we're working on it. Kevin ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v0 2/2] block: allow to set 'drive' property on a realized block device 2019-12-13 10:32 ` Kevin Wolf @ 2019-12-16 14:51 ` Denis Plotnikov 2019-12-16 15:38 ` Kevin Wolf 0 siblings, 1 reply; 20+ messages in thread From: Denis Plotnikov @ 2019-12-16 14:51 UTC (permalink / raw) To: Kevin Wolf Cc: Vladimir Sementsov-Ogievskiy, berrange, ehabkost, qemu-block, qemu-devel, mreitz, pbonzini, Denis Lunev On 13.12.2019 13:32, Kevin Wolf wrote: > Am 18.11.2019 um 11:50 hat Denis Plotnikov geschrieben: >> >> On 10.11.2019 22:08, Denis Plotnikov wrote: >>> On 10.11.2019 22:03, Denis Plotnikov wrote: >>>> This allows to change (replace) the file on a block device and is useful >>>> to workaround exclusive file access restrictions, e.g. to implement VM >>>> migration with a shared disk stored on some storage with the exclusive >>>> file opening model: a destination VM is started waiting for incomming >>>> migration with a fake image drive, and later, on the last migration >>>> phase, the fake image file is replaced with the real one. >>>> >>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> >>>> --- >>>> hw/core/qdev-properties-system.c | 89 +++++++++++++++++++++++++++----- >>>> 1 file changed, 77 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/hw/core/qdev-properties-system.c >>>> b/hw/core/qdev-properties-system.c >>>> index c534590dcd..aaab1370a4 100644 >>>> --- a/hw/core/qdev-properties-system.c >>>> +++ b/hw/core/qdev-properties-system.c >>>> @@ -79,8 +79,55 @@ static void set_pointer(Object *obj, Visitor *v, >>>> Property *prop, >>>> /* --- drive --- */ >>>> -static void do_parse_drive(DeviceState *dev, const char *str, void >>>> **ptr, >>>> - const char *propname, bool iothread, >>>> Error **errp) >>>> +static void do_parse_drive_realized(DeviceState *dev, const char *str, >>>> + void **ptr, const char *propname, >>>> + bool iothread, Error **errp) >>>> +{ >>>> + BlockBackend *blk = *ptr; >>>> + BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL); >>>> + int ret; >>>> + bool blk_created = false; >>>> + >>>> + if (!bs) { >>>> + error_setg(errp, "Can't find blockdev '%s'", str); >>>> + return; >>>> + } >>>> + >>>> + if (!blk) { >>>> + AioContext *ctx = iothread ? bdrv_get_aio_context(bs) : >>>> + qemu_get_aio_context(); >>>> + blk = blk_new(ctx, BLK_PERM_ALL, BLK_PERM_ALL); >>>> + blk_created = true; >>> Actually, I have concerns about situation where blk=null. >>> >>> Is there any case when scsi-hd (or others) doesn't have a blk assigned >>> and it's legal? > No, block devices will always have a BlockBackend, even if it doesn't > have a root node inserted. > >>>> + } else { >>>> + if (blk_bs(blk)) { >>>> + blk_remove_bs(blk); >>>> + } >>>> + } >>>> + >>>> + ret = blk_insert_bs(blk, bs, errp); >>>> + >>>> + if (!ret && blk_created) { >>>> + if (blk_attach_dev(blk, dev) < 0) { >>>> + /* >>>> + * Shouldn't be any errors here since we just created >>>> + * the new blk because the device doesn't have any. >>>> + * Leave the message here in case blk_attach_dev is changed >>>> + */ >>>> + error_setg(errp, "Can't attach drive '%s' to device '%s'", >>>> + str, object_get_typename(OBJECT(dev))); >>>> + } else { >>>> + *ptr = blk; >>>> + } >>>> + } >> Another problem here, is that the "size" of the device dev may not match >> after setting a drive. >> So, we should update it after the drive setting. >> It was found, that it could be done by calling >> BlockDevOps.bdrv_parent_cb_resize. >> >> But I have some concerns about doing it so. In the case of virtio scsi >> disk we have the following callstack >> >> bdrv_parent_cb_resize calls() -> >> scsi_device_report_change(dev, SENSE_CODE(CAPACITY_CHANGED)) -> >> virtio_scsi_change -> >> virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_PARAM_CHANGE, >> sense.asc | >> (sense.ascq << 8)); > I think the safest option for now (and which should solve the case you > want to address) is checking whether old and new size match and > returning an error otherwise. > >> virtio_scsi_change pushes the event to the guest to make the guest >> ask for size refreshing. If I'm not mistaken, here we can get a race >> condition when some another request is processed with an unchanged >> size and then the size changing request is processed. > I think this is actually a problem even without resizing: We need to > quiesce the device between removing the old root and inserting the new > one. They way to achieve this is probably by splitting blk_drain() into > a blk_drain_begin()/end() and then draining the BlockBackend here while > we're working on it. > > Kevin Why don't we use bdrv_drained_begin/end directly? This is what blk_drain does. If we want to split blk_drain we must keep track if blk's brdv isn't change otherwise we can end up with drain_begin one and drain end another bdrv if we do remove/insert in between. Another thing is should we really care about this if we have VM stopped and the sizes matched? Denis > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v0 2/2] block: allow to set 'drive' property on a realized block device 2019-12-16 14:51 ` Denis Plotnikov @ 2019-12-16 15:38 ` Kevin Wolf 2019-12-16 15:58 ` Denis Plotnikov 0 siblings, 1 reply; 20+ messages in thread From: Kevin Wolf @ 2019-12-16 15:38 UTC (permalink / raw) To: Denis Plotnikov Cc: Vladimir Sementsov-Ogievskiy, berrange, ehabkost, qemu-block, qemu-devel, mreitz, pbonzini, Denis Lunev Am 16.12.2019 um 15:51 hat Denis Plotnikov geschrieben: > On 13.12.2019 13:32, Kevin Wolf wrote: > > Am 18.11.2019 um 11:50 hat Denis Plotnikov geschrieben: > >> Another problem here, is that the "size" of the device dev may not match > >> after setting a drive. > >> So, we should update it after the drive setting. > >> It was found, that it could be done by calling > >> BlockDevOps.bdrv_parent_cb_resize. > >> > >> But I have some concerns about doing it so. In the case of virtio scsi > >> disk we have the following callstack > >> > >> bdrv_parent_cb_resize calls() -> > >> scsi_device_report_change(dev, SENSE_CODE(CAPACITY_CHANGED)) -> > >> virtio_scsi_change -> > >> virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_PARAM_CHANGE, > >> sense.asc | > >> (sense.ascq << 8)); > > I think the safest option for now (and which should solve the case you > > want to address) is checking whether old and new size match and > > returning an error otherwise. > > > >> virtio_scsi_change pushes the event to the guest to make the guest > >> ask for size refreshing. If I'm not mistaken, here we can get a race > >> condition when some another request is processed with an unchanged > >> size and then the size changing request is processed. > > I think this is actually a problem even without resizing: We need to > > quiesce the device between removing the old root and inserting the new > > one. They way to achieve this is probably by splitting blk_drain() into > > a blk_drain_begin()/end() and then draining the BlockBackend here while > > we're working on it. > > > > Kevin > Why don't we use bdrv_drained_begin/end directly? This is what > blk_drain does. > If we want to split blk_drain we must keep track if blk's brdv isn't > change otherwise we can end up with drain_begin one and drain end > another bdrv if we do remove/insert in between. Hmm, true, we would have to keep track of draining at the BlockBackend level and consider it in blk_remove_bs() and blk_insert_bs(). Maybe that's not worth it. If we use bdrv_drained_begin/end directly, I think we need to drain both the old and the new root node during the process. > Another thing is should we really care about this if we have VM > stopped and the sizes matched? How do we know that the VM is stopped? And why would we require this? Your patch doesn't implement or at least check this, and it seems a bit impractical for example when all you want is inserting a filter node. Kevin ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v0 2/2] block: allow to set 'drive' property on a realized block device 2019-12-16 15:38 ` Kevin Wolf @ 2019-12-16 15:58 ` Denis Plotnikov 0 siblings, 0 replies; 20+ messages in thread From: Denis Plotnikov @ 2019-12-16 15:58 UTC (permalink / raw) To: Kevin Wolf Cc: Vladimir Sementsov-Ogievskiy, berrange, ehabkost, qemu-block, qemu-devel, mreitz, pbonzini, Denis Lunev On 16.12.2019 18:38, Kevin Wolf wrote: > Am 16.12.2019 um 15:51 hat Denis Plotnikov geschrieben: >> On 13.12.2019 13:32, Kevin Wolf wrote: >>> Am 18.11.2019 um 11:50 hat Denis Plotnikov geschrieben: >>>> Another problem here, is that the "size" of the device dev may not match >>>> after setting a drive. >>>> So, we should update it after the drive setting. >>>> It was found, that it could be done by calling >>>> BlockDevOps.bdrv_parent_cb_resize. >>>> >>>> But I have some concerns about doing it so. In the case of virtio scsi >>>> disk we have the following callstack >>>> >>>> bdrv_parent_cb_resize calls() -> >>>> scsi_device_report_change(dev, SENSE_CODE(CAPACITY_CHANGED)) -> >>>> virtio_scsi_change -> >>>> virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_PARAM_CHANGE, >>>> sense.asc | >>>> (sense.ascq << 8)); >>> I think the safest option for now (and which should solve the case you >>> want to address) is checking whether old and new size match and >>> returning an error otherwise. >>> >>>> virtio_scsi_change pushes the event to the guest to make the guest >>>> ask for size refreshing. If I'm not mistaken, here we can get a race >>>> condition when some another request is processed with an unchanged >>>> size and then the size changing request is processed. >>> I think this is actually a problem even without resizing: We need to >>> quiesce the device between removing the old root and inserting the new >>> one. They way to achieve this is probably by splitting blk_drain() into >>> a blk_drain_begin()/end() and then draining the BlockBackend here while >>> we're working on it. >>> >>> Kevin >> Why don't we use bdrv_drained_begin/end directly? This is what >> blk_drain does. >> If we want to split blk_drain we must keep track if blk's brdv isn't >> change otherwise we can end up with drain_begin one and drain end >> another bdrv if we do remove/insert in between. > Hmm, true, we would have to keep track of draining at the BlockBackend > level and consider it in blk_remove_bs() and blk_insert_bs(). Maybe > that's not worth it. > > If we use bdrv_drained_begin/end directly, I think we need to drain both > the old and the new root node during the process. > >> Another thing is should we really care about this if we have VM >> stopped and the sizes matched? > How do we know that the VM is stopped? And why would we require this? I implied the scenario of VM migration over a shared storage with an exclusive file access model. The VM is stopped on drive changing phase. If there is no use to require it, than ok. Denis > Your patch doesn't implement or at least check this, and it seems a bit > impractical for example when all you want is inserting a filter node. > > Kevin ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v0 0/2] allow to set 'drive' property on a realized block device 2019-11-10 19:03 [PATCH v0 0/2] allow to set 'drive' property on a realized block device Denis Plotnikov 2019-11-10 19:03 ` [PATCH v0 1/2] qdev-properties-system: extend set_pionter for unrealized devices Denis Plotnikov 2019-11-10 19:03 ` [PATCH v0 2/2] block: allow to set 'drive' property on a realized block device Denis Plotnikov @ 2019-11-18 10:30 ` Denis Plotnikov 2020-03-02 13:38 ` Kevin Wolf 3 siblings, 0 replies; 20+ messages in thread From: Denis Plotnikov @ 2019-11-18 10:30 UTC (permalink / raw) To: qemu-block Cc: kwolf, Vladimir Sementsov-Ogievskiy, berrange, ehabkost, qemu-devel, mreitz, pbonzini, Denis Lunev Ping! On 10.11.2019 22:03, Denis Plotnikov wrote: > This allows to replace the file on a block device and is useful > to workaround the cases (migration) when the VM image is placed on > some shared storage with exclusive file opening model but the image > should be open form more than one app. > > The previous version of approaching the workaround was based on the > "blockdev-change-medium" command modification but had some flaws: > * semantics: blockdev-change-medium is aimed to be used with removable devices > only > * interface: it can't accept all possible combination of parameters for > the "drive" replacement (creation). > > More details here: http://patchwork.ozlabs.org/patch/1179329/ > > The current series suggests another approach: > 1. blockdev-add > 2. qom-set disk.drive = the blockdev added (this is what the series adds) > > > Denis Plotnikov (2): > qdev-properties-system: extend set_pionter for unrealized devices > block: allow to set 'drive' property on a realized block device > > hw/core/qdev-properties-system.c | 117 +++++++++++++++++++++++++------ > 1 file changed, 96 insertions(+), 21 deletions(-) > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v0 0/2] allow to set 'drive' property on a realized block device 2019-11-10 19:03 [PATCH v0 0/2] allow to set 'drive' property on a realized block device Denis Plotnikov ` (2 preceding siblings ...) 2019-11-18 10:30 ` [PATCH v0 0/2] " Denis Plotnikov @ 2020-03-02 13:38 ` Kevin Wolf 2020-03-02 13:55 ` Denis Plotnikov 3 siblings, 1 reply; 20+ messages in thread From: Kevin Wolf @ 2020-03-02 13:38 UTC (permalink / raw) To: Denis Plotnikov Cc: vsementsov, berrange, ehabkost, qemu-block, qemu-devel, mreitz, pbonzini, den Am 10.11.2019 um 20:03 hat Denis Plotnikov geschrieben: > This allows to replace the file on a block device and is useful > to workaround the cases (migration) when the VM image is placed on > some shared storage with exclusive file opening model but the image > should be open form more than one app. > > The previous version of approaching the workaround was based on the > "blockdev-change-medium" command modification but had some flaws: > * semantics: blockdev-change-medium is aimed to be used with removable devices > only > * interface: it can't accept all possible combination of parameters for > the "drive" replacement (creation). > > More details here: http://patchwork.ozlabs.org/patch/1179329/ > > The current series suggests another approach: > 1. blockdev-add > 2. qom-set disk.drive = the blockdev added (this is what the series adds) Are you still planning to send another version? Kevin ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v0 0/2] allow to set 'drive' property on a realized block device 2020-03-02 13:38 ` Kevin Wolf @ 2020-03-02 13:55 ` Denis Plotnikov 2020-03-02 15:39 ` Kevin Wolf 0 siblings, 1 reply; 20+ messages in thread From: Denis Plotnikov @ 2020-03-02 13:55 UTC (permalink / raw) To: Kevin Wolf Cc: vsementsov, berrange, ehabkost, qemu-block, qemu-devel, mreitz, pbonzini, den On 02.03.2020 16:38, Kevin Wolf wrote: > Am 10.11.2019 um 20:03 hat Denis Plotnikov geschrieben: >> This allows to replace the file on a block device and is useful >> to workaround the cases (migration) when the VM image is placed on >> some shared storage with exclusive file opening model but the image >> should be open form more than one app. >> >> The previous version of approaching the workaround was based on the >> "blockdev-change-medium" command modification but had some flaws: >> * semantics: blockdev-change-medium is aimed to be used with removable devices >> only >> * interface: it can't accept all possible combination of parameters for >> the "drive" replacement (creation). >> >> More details here: http://patchwork.ozlabs.org/patch/1179329/ >> >> The current series suggests another approach: >> 1. blockdev-add >> 2. qom-set disk.drive = the blockdev added (this is what the series adds) > Are you still planning to send another version? > > Kevin Not in the near future :) There is an unresolved problem with bitmap-migration is case of block dev replacement. Still don't know how to do it in the proper way. Denis > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v0 0/2] allow to set 'drive' property on a realized block device 2020-03-02 13:55 ` Denis Plotnikov @ 2020-03-02 15:39 ` Kevin Wolf 2020-03-03 7:43 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 20+ messages in thread From: Kevin Wolf @ 2020-03-02 15:39 UTC (permalink / raw) To: Denis Plotnikov Cc: vsementsov, berrange, ehabkost, qemu-block, qemu-devel, mreitz, pbonzini, den Am 02.03.2020 um 14:55 hat Denis Plotnikov geschrieben: > > > On 02.03.2020 16:38, Kevin Wolf wrote: > > Am 10.11.2019 um 20:03 hat Denis Plotnikov geschrieben: > > > This allows to replace the file on a block device and is useful > > > to workaround the cases (migration) when the VM image is placed on > > > some shared storage with exclusive file opening model but the image > > > should be open form more than one app. > > > > > > The previous version of approaching the workaround was based on the > > > "blockdev-change-medium" command modification but had some flaws: > > > * semantics: blockdev-change-medium is aimed to be used with removable devices > > > only > > > * interface: it can't accept all possible combination of parameters for > > > the "drive" replacement (creation). > > > > > > More details here: http://patchwork.ozlabs.org/patch/1179329/ > > > > > > The current series suggests another approach: > > > 1. blockdev-add > > > 2. qom-set disk.drive = the blockdev added (this is what the series adds) > > Are you still planning to send another version? > > > > Kevin > Not in the near future :) There is an unresolved problem with > bitmap-migration is case of block dev replacement. > Still don't know how to do it in the proper way. I seem to remember that in some discussion a while ago we came to the conclusion that we need a way for the managemnt tool to provide a mapping from source node-names to destination node-names. Or is the problem you mean unrelated to identifying the node to which a bitmap should belong? Kevin ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v0 0/2] allow to set 'drive' property on a realized block device 2020-03-02 15:39 ` Kevin Wolf @ 2020-03-03 7:43 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 20+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2020-03-03 7:43 UTC (permalink / raw) To: Kevin Wolf, Denis Plotnikov Cc: berrange, ehabkost, qemu-block, qemu-devel, mreitz, pbonzini, den 02.03.2020 18:39, Kevin Wolf wrote: > Am 02.03.2020 um 14:55 hat Denis Plotnikov geschrieben: >> >> >> On 02.03.2020 16:38, Kevin Wolf wrote: >>> Am 10.11.2019 um 20:03 hat Denis Plotnikov geschrieben: >>>> This allows to replace the file on a block device and is useful >>>> to workaround the cases (migration) when the VM image is placed on >>>> some shared storage with exclusive file opening model but the image >>>> should be open form more than one app. >>>> >>>> The previous version of approaching the workaround was based on the >>>> "blockdev-change-medium" command modification but had some flaws: >>>> * semantics: blockdev-change-medium is aimed to be used with removable devices >>>> only >>>> * interface: it can't accept all possible combination of parameters for >>>> the "drive" replacement (creation). >>>> >>>> More details here: http://patchwork.ozlabs.org/patch/1179329/ >>>> >>>> The current series suggests another approach: >>>> 1. blockdev-add >>>> 2. qom-set disk.drive = the blockdev added (this is what the series adds) >>> Are you still planning to send another version? >>> >>> Kevin >> Not in the near future :) There is an unresolved problem with >> bitmap-migration is case of block dev replacement. >> Still don't know how to do it in the proper way. > > I seem to remember that in some discussion a while ago we came to the > conclusion that we need a way for the managemnt tool to provide a > mapping from source node-names to destination node-names. Yes, it's planned solution for bitmaps migration, but it doesn't help here. > > Or is the problem you mean unrelated to identifying the node to which a > bitmap should belong? > The problem here is following: We need to workaround migration on shared fs, which doesn't allow to open file on node A if it is opened RW on node B (and visa-versa). So, we tried to start target with null-co stub instead of shared qcow2 and than switch to correct file at some moment. The problem with bitmaps migration is that we migrate bitmaps to null-co and than they are lost... So we need to implement some moving bitmaps from null-co to qcow2 node, keeping in mind that they are in progress of migration. We didn't try to implement such moving, it seemed too tricky. I also had an idea of a flag for file-posix to close fd on inactivation.. But then we need to not open it when openining in inactive mode, and this needs support in qcow2, which doesn't seem better than null-co stub. So, I see two ways of solving this: 1. Use null-co stub and deal with moving bitmaps during postcopy 2. Move to UNOPENED mode for bdrv, which is similar to INACTIVE, but doesn't allow to open any files on fs. We'll have to provide size as an option anyway.. Sounds bad. Could you give an advice? -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-03-03 7:44 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-10 19:03 [PATCH v0 0/2] allow to set 'drive' property on a realized block device Denis Plotnikov 2019-11-10 19:03 ` [PATCH v0 1/2] qdev-properties-system: extend set_pionter for unrealized devices Denis Plotnikov 2019-11-18 18:54 ` Eduardo Habkost 2019-11-22 11:36 ` Denis Plotnikov 2019-11-25 15:30 ` Eduardo Habkost 2019-11-26 6:49 ` Denis Plotnikov 2019-11-26 16:38 ` Kevin Wolf 2019-11-10 19:03 ` [PATCH v0 2/2] block: allow to set 'drive' property on a realized block device Denis Plotnikov 2019-11-10 19:08 ` Denis Plotnikov 2019-11-18 10:50 ` Denis Plotnikov 2019-12-13 7:30 ` [PING]Re: " Denis Plotnikov 2019-12-13 10:32 ` Kevin Wolf 2019-12-16 14:51 ` Denis Plotnikov 2019-12-16 15:38 ` Kevin Wolf 2019-12-16 15:58 ` Denis Plotnikov 2019-11-18 10:30 ` [PATCH v0 0/2] " Denis Plotnikov 2020-03-02 13:38 ` Kevin Wolf 2020-03-02 13:55 ` Denis Plotnikov 2020-03-02 15:39 ` Kevin Wolf 2020-03-03 7:43 ` Vladimir Sementsov-Ogievskiy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).