blockdev: modify blockdev-change-medium to change non-removable device
diff mbox series

Message ID 20191018120950.26849-1-dplotnikov@virtuozzo.com
State New
Headers show
Series
  • blockdev: modify blockdev-change-medium to change non-removable device
Related show

Commit Message

Denis Plotnikov Oct. 18, 2019, 12:09 p.m. UTC
The modification is useful to workaround exclusive file access restrictions,
e.g. to implement VM migration with shared disk stored on a 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>
---
 blockdev.c           | 69 +++++++++++++++++++++++++++++++-------------
 hmp.c                |  2 ++
 qapi/block-core.json |  7 +++--
 qmp.c                |  3 +-
 4 files changed, 57 insertions(+), 24 deletions(-)

Comments

Eric Blake Oct. 18, 2019, 2:35 p.m. UTC | #1
On 10/18/19 7:09 AM, Denis Plotnikov wrote:
> The modification is useful to workaround exclusive file access restrictions,
> e.g. to implement VM migration with shared disk stored on a 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>
> ---

> +++ b/qapi/block-core.json
> @@ -4769,6 +4769,8 @@
>   # @read-only-mode:  change the read-only mode of the device; defaults
>   #                   to 'retain'
>   #
> +# @medium-name:     drive-name when changing the media in non-removable devices
> +#                   ignored when changing media in removable devices

s/devices ignored/devices. Ignored/

Missing a '(since 4.2)' tag.
Max Reitz Oct. 18, 2019, 3:02 p.m. UTC | #2
On 18.10.19 14:09, Denis Plotnikov wrote:
> The modification is useful to workaround exclusive file access restrictions,
> e.g. to implement VM migration with shared disk stored on a 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>

Isn’t this what we would want to use reopen for?

Max
Denis Plotnikov Oct. 21, 2019, 6:50 a.m. UTC | #3
On 18.10.2019 18:02, Max Reitz wrote:
> On 18.10.19 14:09, Denis Plotnikov wrote:
>> The modification is useful to workaround exclusive file access restrictions,
>> e.g. to implement VM migration with shared disk stored on a 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>
> Isn’t this what we would want to use reopen for?
>
> Max

Could you please explain what is "use reopen"?

Denis

>
Max Reitz Oct. 22, 2019, 11:05 a.m. UTC | #4
On 21.10.19 08:50, Denis Plotnikov wrote:
> 
> On 18.10.2019 18:02, Max Reitz wrote:
>> On 18.10.19 14:09, Denis Plotnikov wrote:
>>> The modification is useful to workaround exclusive file access restrictions,
>>> e.g. to implement VM migration with shared disk stored on a 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>
>> Isn’t this what we would want to use reopen for?
>>
>> Max
> 
> Could you please explain what is "use reopen"?

I was thinking of using (x-)blockdev-reopen to change the file that is
used by the format node (e.g. from a null-co node to a real file); or to
change the filename of the protocol node.

Kevin has pointed out (on IRC) that this will not allow you to change
the node that is directly attached to the device.  While I don’t know
whether that’s really necessary in this case, if it were indeed
necessary, I’d prefer a method to change a guest device’s @drive option
because that seems more natural to me.

In contrast, the approach taken in this patch seems not quite right to
me, because it overloads the whole blockdev-change-medium command with a
completely new and different implementation based on whether there’s a
removable medium or not.  If the implementation is so different (and the
interface is, too, because in one path you must give @medium whereas the
other doesn’t evaluate it at all), it should be a new command.

I don’t know whether we need a new command at all, though.  On the node
level, we have (x-)blockdev-reopen.  So assuming we need something to
change the link between the guest device and the block layer, I wonder
whether there isn’t something similar; specifically, I’d prefer
something to simply change the device’s @drive option.

Kevin has pointed out (on IRC again) that there is indeed one such
command, and that’s qom-set.  Unfortunately, this is what happens if you
try to use it for @drive:

{"error": {"class": "GenericError", "desc": "Attempt to set property
'drive' on anonymous device (type 'virtio-blk-device') after it was
realized"}}

However, Kevin has claimed it would be technically possible to make an
exception for @drive.  Maybe this is worth investigating?


(As for blockdev-change-medium, as I’ve said, I don’t really think this
fits there.  Furthermore, blockdev-change-medium is kind of a legacy
command because I think every command but blockdev-add that does a
bdrv_open() kind of is a legacy command.  So if anything, it should be a
new command that then takes a node-name.
But OTOH, it would be a bit strange to add a separate command for
something that in theory should be covered by qom-set @drive.)

Max
Denis Plotnikov Oct. 22, 2019, 12:53 p.m. UTC | #5
On 22.10.2019 14:05, Max Reitz wrote:
> On 21.10.19 08:50, Denis Plotnikov wrote:
>> On 18.10.2019 18:02, Max Reitz wrote:
>>> On 18.10.19 14:09, Denis Plotnikov wrote:
>>>> The modification is useful to workaround exclusive file access restrictions,
>>>> e.g. to implement VM migration with shared disk stored on a 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>
>>> Isn’t this what we would want to use reopen for?
>>>
>>> Max
>> Could you please explain what is "use reopen"?
> I was thinking of using (x-)blockdev-reopen to change the file that is
> used by the format node (e.g. from a null-co node to a real file); or to
> change the filename of the protocol node.
>
> Kevin has pointed out (on IRC) that this will not allow you to change
> the node that is directly attached to the device.  While I don’t know
> whether that’s really necessary in this case, if it were indeed
> necessary, I’d prefer a method to change a guest device’s @drive option
> because that seems more natural to me.
>
> In contrast, the approach taken in this patch seems not quite right to
> me, because it overloads the whole blockdev-change-medium command with a
> completely new and different implementation based on whether there’s a
> removable medium or not.  If the implementation is so different (and the
> interface is, too, because in one path you must give @medium whereas the
> other doesn’t evaluate it at all), it should be a new command.
>
> I don’t know whether we need a new command at all, though.  On the node
> level, we have (x-)blockdev-reopen.  So assuming we need something to
> change the link between the guest device and the block layer, I wonder
> whether there isn’t something similar; specifically, I’d prefer
> something to simply change the device’s @drive option.
>
> Kevin has pointed out (on IRC again) that there is indeed one such
> command, and that’s qom-set.  Unfortunately, this is what happens if you
> try to use it for @drive:
>
> {"error": {"class": "GenericError", "desc": "Attempt to set property
> 'drive' on anonymous device (type 'virtio-blk-device') after it was
> realized"}}
>
> However, Kevin has claimed it would be technically possible to make an
> exception for @drive.  Maybe this is worth investigating?

Is there any guess how complex it might be? In the case if it's quite 
complex may be it's worth to make the separate command?

>
>
> (As for blockdev-change-medium, as I’ve said, I don’t really think this
> fits there.  Furthermore, blockdev-change-medium is kind of a legacy
> command because I think every command but blockdev-add that does a
> bdrv_open() kind of is a legacy command.
Out of curiosity, could you please explain why it's decided to be so?
> So if anything, it should be a
> new command that then takes a node-name.
> But OTOH, it would be a bit strange to add a separate command for
> something that in theory should be covered by qom-set @drive.)
>
> Max
>
Max Reitz Oct. 22, 2019, 1:18 p.m. UTC | #6
On 22.10.19 14:53, Denis Plotnikov wrote:
> 
> On 22.10.2019 14:05, Max Reitz wrote:
>> On 21.10.19 08:50, Denis Plotnikov wrote:
>>> On 18.10.2019 18:02, Max Reitz wrote:
>>>> On 18.10.19 14:09, Denis Plotnikov wrote:
>>>>> The modification is useful to workaround exclusive file access restrictions,
>>>>> e.g. to implement VM migration with shared disk stored on a 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>
>>>> Isn’t this what we would want to use reopen for?
>>>>
>>>> Max
>>> Could you please explain what is "use reopen"?
>> I was thinking of using (x-)blockdev-reopen to change the file that is
>> used by the format node (e.g. from a null-co node to a real file); or to
>> change the filename of the protocol node.
>>
>> Kevin has pointed out (on IRC) that this will not allow you to change
>> the node that is directly attached to the device.  While I don’t know
>> whether that’s really necessary in this case, if it were indeed
>> necessary, I’d prefer a method to change a guest device’s @drive option
>> because that seems more natural to me.
>>
>> In contrast, the approach taken in this patch seems not quite right to
>> me, because it overloads the whole blockdev-change-medium command with a
>> completely new and different implementation based on whether there’s a
>> removable medium or not.  If the implementation is so different (and the
>> interface is, too, because in one path you must give @medium whereas the
>> other doesn’t evaluate it at all), it should be a new command.
>>
>> I don’t know whether we need a new command at all, though.  On the node
>> level, we have (x-)blockdev-reopen.  So assuming we need something to
>> change the link between the guest device and the block layer, I wonder
>> whether there isn’t something similar; specifically, I’d prefer
>> something to simply change the device’s @drive option.
>>
>> Kevin has pointed out (on IRC again) that there is indeed one such
>> command, and that’s qom-set.  Unfortunately, this is what happens if you
>> try to use it for @drive:
>>
>> {"error": {"class": "GenericError", "desc": "Attempt to set property
>> 'drive' on anonymous device (type 'virtio-blk-device') after it was
>> realized"}}
>>
>> However, Kevin has claimed it would be technically possible to make an
>> exception for @drive.  Maybe this is worth investigating?
> 
> Is there any guess how complex it might be? In the case if it's quite 
> complex may be it's worth to make the separate command?

I can translate the chat log for you:

<kevin> In theory that’s called qom-set
<kevin> However, I believe it doesn’t support qdev properties
<kevin> Hm, but that could be changed specifically for the drive property
<kevin> qdev keeps confusing me.  Drive isn’t supposed to call
qdev_prop_set_after_realize(), but the error message’s still there.
Where is that hidden call...?
<kevin> Ah, set_pointer() does
<kevin> Yes, then it should be possible to make that work rather locally

And that took him about 10 minutes.

So I suppose it would be to check in set_drive() and
set_drive_iothread() whether the device is already realized, and if so,
divert it to some other function that does the runtime change?

(No idea how the qdev maintainers think about doing that in set_drive()
and set_drive_iothread(), though)

>> (As for blockdev-change-medium, as I’ve said, I don’t really think this
>> fits there.  Furthermore, blockdev-change-medium is kind of a legacy
>> command because I think every command but blockdev-add that does a
>> bdrv_open() kind of is a legacy command.
> Out of curiosity, could you please explain why it's decided to be so?

Because we have blockdev-add, which supports all block device options
there are and so on.  blockdev-change-medium (which is basically just a
more rigid “change”) only gets filename, which isn’t as expressive.

We generally want users to add new nodes with blockdev-add and let all
other commands only take node-names.

(There’s also the fact that historically we’ve used filenames to
identify BlockDriverStates, but that doesn’t work so well.  Thus I think
we should get away from using filenames as much as we can so people
don’t use them for identification again.)

Max
Denis Plotnikov Oct. 22, 2019, 1:24 p.m. UTC | #7
On 22.10.2019 16:18, Max Reitz wrote:
> On 22.10.19 14:53, Denis Plotnikov wrote:
>> On 22.10.2019 14:05, Max Reitz wrote:
>>> On 21.10.19 08:50, Denis Plotnikov wrote:
>>>> On 18.10.2019 18:02, Max Reitz wrote:
>>>>> On 18.10.19 14:09, Denis Plotnikov wrote:
>>>>>> The modification is useful to workaround exclusive file access restrictions,
>>>>>> e.g. to implement VM migration with shared disk stored on a 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>
>>>>> Isn’t this what we would want to use reopen for?
>>>>>
>>>>> Max
>>>> Could you please explain what is "use reopen"?
>>> I was thinking of using (x-)blockdev-reopen to change the file that is
>>> used by the format node (e.g. from a null-co node to a real file); or to
>>> change the filename of the protocol node.
>>>
>>> Kevin has pointed out (on IRC) that this will not allow you to change
>>> the node that is directly attached to the device.  While I don’t know
>>> whether that’s really necessary in this case, if it were indeed
>>> necessary, I’d prefer a method to change a guest device’s @drive option
>>> because that seems more natural to me.
>>>
>>> In contrast, the approach taken in this patch seems not quite right to
>>> me, because it overloads the whole blockdev-change-medium command with a
>>> completely new and different implementation based on whether there’s a
>>> removable medium or not.  If the implementation is so different (and the
>>> interface is, too, because in one path you must give @medium whereas the
>>> other doesn’t evaluate it at all), it should be a new command.
>>>
>>> I don’t know whether we need a new command at all, though.  On the node
>>> level, we have (x-)blockdev-reopen.  So assuming we need something to
>>> change the link between the guest device and the block layer, I wonder
>>> whether there isn’t something similar; specifically, I’d prefer
>>> something to simply change the device’s @drive option.
>>>
>>> Kevin has pointed out (on IRC again) that there is indeed one such
>>> command, and that’s qom-set.  Unfortunately, this is what happens if you
>>> try to use it for @drive:
>>>
>>> {"error": {"class": "GenericError", "desc": "Attempt to set property
>>> 'drive' on anonymous device (type 'virtio-blk-device') after it was
>>> realized"}}
>>>
>>> However, Kevin has claimed it would be technically possible to make an
>>> exception for @drive.  Maybe this is worth investigating?
>> Is there any guess how complex it might be? In the case if it's quite
>> complex may be it's worth to make the separate command?
> I can translate the chat log for you:
>
> <kevin> In theory that’s called qom-set
> <kevin> However, I believe it doesn’t support qdev properties
> <kevin> Hm, but that could be changed specifically for the drive property
> <kevin> qdev keeps confusing me.  Drive isn’t supposed to call
> qdev_prop_set_after_realize(), but the error message’s still there.
> Where is that hidden call...?
> <kevin> Ah, set_pointer() does
> <kevin> Yes, then it should be possible to make that work rather locally
>
> And that took him about 10 minutes.
>
> So I suppose it would be to check in set_drive() and
> set_drive_iothread() whether the device is already realized, and if so,
> divert it to some other function that does the runtime change?
ok, that might be a good starting point for me. Thanks.
>
> (No idea how the qdev maintainers think about doing that in set_drive()
> and set_drive_iothread(), though)
>
>>> (As for blockdev-change-medium, as I’ve said, I don’t really think this
>>> fits there.  Furthermore, blockdev-change-medium is kind of a legacy
>>> command because I think every command but blockdev-add that does a
>>> bdrv_open() kind of is a legacy command.
>> Out of curiosity, could you please explain why it's decided to be so?
> Because we have blockdev-add, which supports all block device options
> there are and so on.  blockdev-change-medium (which is basically just a
> more rigid “change”) only gets filename, which isn’t as expressive.
>
> We generally want users to add new nodes with blockdev-add and let all
> other commands only take node-names.
>
> (There’s also the fact that historically we’ve used filenames to
> identify BlockDriverStates, but that doesn’t work so well.  Thus I think
> we should get away from using filenames as much as we can so people
> don’t use them for identification again.)
>
> Max

Thanks for the explanation, Max!

Denis

>
Vladimir Sementsov-Ogievskiy Oct. 23, 2019, 1:56 p.m. UTC | #8
22.10.2019 14:05, Max Reitz wrote:
> On 21.10.19 08:50, Denis Plotnikov wrote:
>>
>> On 18.10.2019 18:02, Max Reitz wrote:
>>> On 18.10.19 14:09, Denis Plotnikov wrote:
>>>> The modification is useful to workaround exclusive file access restrictions,
>>>> e.g. to implement VM migration with shared disk stored on a 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>
>>> Isn’t this what we would want to use reopen for?
>>>
>>> Max
>>
>> Could you please explain what is "use reopen"?
> 
> I was thinking of using (x-)blockdev-reopen to change the file that is
> used by the format node (e.g. from a null-co node to a real file); or to
> change the filename of the protocol node.
> 
> Kevin has pointed out (on IRC) that this will not allow you to change
> the node that is directly attached to the device.  While I don’t know
> whether that’s really necessary in this case, if it were indeed
> necessary, I’d prefer a method to change a guest device’s @drive option
> because that seems more natural to me.
> 
> In contrast, the approach taken in this patch seems not quite right to
> me, because it overloads the whole blockdev-change-medium command with a
> completely new and different implementation based on whether there’s a
> removable medium or not.  If the implementation is so different (and the
> interface is, too, because in one path you must give @medium whereas the
> other doesn’t evaluate it at all), it should be a new command.
> 
> I don’t know whether we need a new command at all, though.  On the node
> level, we have (x-)blockdev-reopen.  So assuming we need something to
> change the link between the guest device and the block layer, I wonder
> whether there isn’t something similar; specifically, I’d prefer
> something to simply change the device’s @drive option.

Ok, assume we can set @drive option with help of improved qom-set.
But how to create this new blk? blockdev-add don't have 'id' parameter anymore
and don't create blk...

> 
> Kevin has pointed out (on IRC again) that there is indeed one such
> command, and that’s qom-set.  Unfortunately, this is what happens if you
> try to use it for @drive:
> 
> {"error": {"class": "GenericError", "desc": "Attempt to set property
> 'drive' on anonymous device (type 'virtio-blk-device') after it was
> realized"}}
> 
> However, Kevin has claimed it would be technically possible to make an
> exception for @drive.  Maybe this is worth investigating?
> 
> 
> (As for blockdev-change-medium, as I’ve said, I don’t really think this
> fits there.  Furthermore, blockdev-change-medium is kind of a legacy
> command because I think every command but blockdev-add that does a
> bdrv_open() kind of is a legacy command.  So if anything, it should be a
> new command that then takes a node-name.
> But OTOH, it would be a bit strange to add a separate command for
> something that in theory should be covered by qom-set @drive.)
> 
> Max
>
Vladimir Sementsov-Ogievskiy Oct. 23, 2019, 2:10 p.m. UTC | #9
23.10.2019 16:56, Vladimir Sementsov-Ogievskiy wrote:
> 22.10.2019 14:05, Max Reitz wrote:
>> On 21.10.19 08:50, Denis Plotnikov wrote:
>>>
>>> On 18.10.2019 18:02, Max Reitz wrote:
>>>> On 18.10.19 14:09, Denis Plotnikov wrote:
>>>>> The modification is useful to workaround exclusive file access restrictions,
>>>>> e.g. to implement VM migration with shared disk stored on a 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>
>>>> Isn’t this what we would want to use reopen for?
>>>>
>>>> Max
>>>
>>> Could you please explain what is "use reopen"?
>>
>> I was thinking of using (x-)blockdev-reopen to change the file that is
>> used by the format node (e.g. from a null-co node to a real file); or to
>> change the filename of the protocol node.
>>
>> Kevin has pointed out (on IRC) that this will not allow you to change
>> the node that is directly attached to the device.  While I don’t know
>> whether that’s really necessary in this case, if it were indeed
>> necessary, I’d prefer a method to change a guest device’s @drive option
>> because that seems more natural to me.
>>
>> In contrast, the approach taken in this patch seems not quite right to
>> me, because it overloads the whole blockdev-change-medium command with a
>> completely new and different implementation based on whether there’s a
>> removable medium or not.  If the implementation is so different (and the
>> interface is, too, because in one path you must give @medium whereas the
>> other doesn’t evaluate it at all), it should be a new command.
>>
>> I don’t know whether we need a new command at all, though.  On the node
>> level, we have (x-)blockdev-reopen.  So assuming we need something to
>> change the link between the guest device and the block layer, I wonder
>> whether there isn’t something similar; specifically, I’d prefer
>> something to simply change the device’s @drive option.
> 
> Ok, assume we can set @drive option with help of improved qom-set.
> But how to create this new blk? blockdev-add don't have 'id' parameter anymore
> and don't create blk...


Hmm, I see, we have command blockdev-insert-medium(id, node-name). Isn't it what
we want?

> 
>>
>> Kevin has pointed out (on IRC again) that there is indeed one such
>> command, and that’s qom-set.  Unfortunately, this is what happens if you
>> try to use it for @drive:
>>
>> {"error": {"class": "GenericError", "desc": "Attempt to set property
>> 'drive' on anonymous device (type 'virtio-blk-device') after it was
>> realized"}}
>>
>> However, Kevin has claimed it would be technically possible to make an
>> exception for @drive.  Maybe this is worth investigating?
>>
>>
>> (As for blockdev-change-medium, as I’ve said, I don’t really think this
>> fits there.  Furthermore, blockdev-change-medium is kind of a legacy
>> command because I think every command but blockdev-add that does a
>> bdrv_open() kind of is a legacy command.  So if anything, it should be a
>> new command that then takes a node-name.
>> But OTOH, it would be a bit strange to add a separate command for
>> something that in theory should be covered by qom-set @drive.)
>>
>> Max
>>
> 
>
Kevin Wolf Oct. 23, 2019, 4:02 p.m. UTC | #10
Am 23.10.2019 um 15:56 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 22.10.2019 14:05, Max Reitz wrote:
> > On 21.10.19 08:50, Denis Plotnikov wrote:
> >>
> >> On 18.10.2019 18:02, Max Reitz wrote:
> >>> On 18.10.19 14:09, Denis Plotnikov wrote:
> >>>> The modification is useful to workaround exclusive file access restrictions,
> >>>> e.g. to implement VM migration with shared disk stored on a 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>
> >>> Isn’t this what we would want to use reopen for?
> >>>
> >>> Max
> >>
> >> Could you please explain what is "use reopen"?
> > 
> > I was thinking of using (x-)blockdev-reopen to change the file that is
> > used by the format node (e.g. from a null-co node to a real file); or to
> > change the filename of the protocol node.
> > 
> > Kevin has pointed out (on IRC) that this will not allow you to change
> > the node that is directly attached to the device.  While I don’t know
> > whether that’s really necessary in this case, if it were indeed
> > necessary, I’d prefer a method to change a guest device’s @drive option
> > because that seems more natural to me.
> > 
> > In contrast, the approach taken in this patch seems not quite right to
> > me, because it overloads the whole blockdev-change-medium command with a
> > completely new and different implementation based on whether there’s a
> > removable medium or not.  If the implementation is so different (and the
> > interface is, too, because in one path you must give @medium whereas the
> > other doesn’t evaluate it at all), it should be a new command.
> > 
> > I don’t know whether we need a new command at all, though.  On the node
> > level, we have (x-)blockdev-reopen.  So assuming we need something to
> > change the link between the guest device and the block layer, I wonder
> > whether there isn’t something similar; specifically, I’d prefer
> > something to simply change the device’s @drive option.
> 
> Ok, assume we can set @drive option with help of improved qom-set.
> But how to create this new blk? blockdev-add don't have 'id' parameter anymore
> and don't create blk...

We don't need to create a new BlockBackend. You would set the drive qdev
property to a new node name and that would just internally call
blk_remove_bs() and blk_insert_bs() for the existing BlockBackend that
is owned by the device.

Kevin
Max Reitz Oct. 24, 2019, 9:31 a.m. UTC | #11
On 23.10.19 16:10, Vladimir Sementsov-Ogievskiy wrote:
> 23.10.2019 16:56, Vladimir Sementsov-Ogievskiy wrote:
>> 22.10.2019 14:05, Max Reitz wrote:
>>> On 21.10.19 08:50, Denis Plotnikov wrote:
>>>>
>>>> On 18.10.2019 18:02, Max Reitz wrote:
>>>>> On 18.10.19 14:09, Denis Plotnikov wrote:
>>>>>> The modification is useful to workaround exclusive file access restrictions,
>>>>>> e.g. to implement VM migration with shared disk stored on a 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>
>>>>> Isn’t this what we would want to use reopen for?
>>>>>
>>>>> Max
>>>>
>>>> Could you please explain what is "use reopen"?
>>>
>>> I was thinking of using (x-)blockdev-reopen to change the file that is
>>> used by the format node (e.g. from a null-co node to a real file); or to
>>> change the filename of the protocol node.
>>>
>>> Kevin has pointed out (on IRC) that this will not allow you to change
>>> the node that is directly attached to the device.  While I don’t know
>>> whether that’s really necessary in this case, if it were indeed
>>> necessary, I’d prefer a method to change a guest device’s @drive option
>>> because that seems more natural to me.
>>>
>>> In contrast, the approach taken in this patch seems not quite right to
>>> me, because it overloads the whole blockdev-change-medium command with a
>>> completely new and different implementation based on whether there’s a
>>> removable medium or not.  If the implementation is so different (and the
>>> interface is, too, because in one path you must give @medium whereas the
>>> other doesn’t evaluate it at all), it should be a new command.
>>>
>>> I don’t know whether we need a new command at all, though.  On the node
>>> level, we have (x-)blockdev-reopen.  So assuming we need something to
>>> change the link between the guest device and the block layer, I wonder
>>> whether there isn’t something similar; specifically, I’d prefer
>>> something to simply change the device’s @drive option.
>>
>> Ok, assume we can set @drive option with help of improved qom-set.
>> But how to create this new blk? blockdev-add don't have 'id' parameter anymore
>> and don't create blk...
> 
> 
> Hmm, I see, we have command blockdev-insert-medium(id, node-name). Isn't it what
> we want?

blockdev-insert-medium requires that the device be empty, so it would
need to be preceded by a blockdev-remove-medium.  And then it isn’t a
transaction...

Also, it currently is limited (intentionally) to devices with removable
media.

I suppose all of this could be changed, but basically I don’t see why we
wouldn’t use qom-set.  (Well, because it’s ugly to call this new
functionality from set_drive(), but other than that...)

(And I don’t quite know whether there isn’t more to be done for devices
that don’t inherently support removable media than just to swap the node
attached to the BB.  Don’t they also need to be drained or something?)

Max

Patch
diff mbox series

diff --git a/blockdev.c b/blockdev.c
index d358169995..23f3465cfc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2609,6 +2609,8 @@  void qmp_blockdev_change_medium(bool has_device, const char *device,
                                 bool has_format, const char *format,
                                 bool has_read_only,
                                 BlockdevChangeReadOnlyMode read_only,
+                                bool has_medium_name,
+                                const char *medium_name,
                                 Error **errp)
 {
     BlockBackend *blk;
@@ -2667,29 +2669,56 @@  void qmp_blockdev_change_medium(bool has_device, const char *device,
         goto fail;
     }
 
-    rc = do_open_tray(has_device ? device : NULL,
-                      has_id ? id : NULL,
-                      false, &err);
-    if (rc && rc != -ENOSYS) {
-        error_propagate(errp, err);
-        goto fail;
-    }
-    error_free(err);
-    err = NULL;
+    if (blk_dev_has_removable_media(blk)) {
+        rc = do_open_tray(has_device ? device : NULL,
+                          has_id ? id : NULL,
+                          false, &err);
+        if (rc && rc != -ENOSYS) {
+            error_propagate(errp, err);
+            goto fail;
+        }
+        error_free(err);
+        err = NULL;
 
-    blockdev_remove_medium(has_device, device, has_id, id, &err);
-    if (err) {
-        error_propagate(errp, err);
-        goto fail;
-    }
+        blockdev_remove_medium(has_device, device, has_id, id, &err);
+        if (err) {
+            error_propagate(errp, err);
+            goto fail;
+        }
 
-    qmp_blockdev_insert_anon_medium(blk, medium_bs, &err);
-    if (err) {
-        error_propagate(errp, err);
-        goto fail;
-    }
+        qmp_blockdev_insert_anon_medium(blk, medium_bs, &err);
+        if (err) {
+            error_propagate(errp, err);
+            goto fail;
+        }
+
+        qmp_blockdev_close_tray(has_device, device, has_id, id, errp);
+    } else {
+        if (!medium_name) {
+            error_setg(errp, "A medium name should be given");
+            goto fail;
+        }
 
-    qmp_blockdev_close_tray(has_device, device, has_id, id, errp);
+        if (runstate_is_running()) {
+            error_setg(errp, "Can't set a medium for non-removable device "
+                    "in a running VM");
+            goto fail;
+        }
+
+        if (strlen(blk_name(blk))) {
+            error_setg(errp, "The device already has a medium");
+            goto fail;
+        }
+
+        if (blk_insert_bs(blk, medium_bs, &err) < 0) {
+            error_propagate(errp, err);
+            goto fail;
+        }
+
+        if (!monitor_add_blk(blk, medium_name, &err)) {
+            error_propagate(errp, err);
+        }
+    }
 
 fail:
     /* If the medium has been inserted, the device has its own reference, so
diff --git a/hmp.c b/hmp.c
index 8eec768088..fc7bac5b4b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1948,6 +1948,7 @@  void hmp_change(Monitor *mon, const QDict *qdict)
     const char *target = qdict_get_str(qdict, "target");
     const char *arg = qdict_get_try_str(qdict, "arg");
     const char *read_only = qdict_get_try_str(qdict, "read-only-mode");
+    const char *target_name = qdict_get_try_str(qdict, "target-name");
     BlockdevChangeReadOnlyMode read_only_mode = 0;
     Error *err = NULL;
 
@@ -1982,6 +1983,7 @@  void hmp_change(Monitor *mon, const QDict *qdict)
 
         qmp_blockdev_change_medium(true, device, false, NULL, target,
                                    !!arg, arg, !!read_only, read_only_mode,
+                                   !!target_name, target_name,
                                    &err);
     }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ccbfff9d0..f493a7c737 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4769,6 +4769,8 @@ 
 # @read-only-mode:  change the read-only mode of the device; defaults
 #                   to 'retain'
 #
+# @medium-name:     drive-name when changing the media in non-removable devices
+#                   ignored when changing media in removable devices
 # Since: 2.5
 #
 # Examples:
@@ -4807,9 +4809,8 @@ 
             '*id': 'str',
             'filename': 'str',
             '*format': 'str',
-            '*read-only-mode': 'BlockdevChangeReadOnlyMode' } }
-
-
+            '*read-only-mode': 'BlockdevChangeReadOnlyMode',
+            '*medium-name': 'str' } }
 ##
 # @BlockErrorAction:
 #
diff --git a/qmp.c b/qmp.c
index b92d62cd5f..c95831a49f 100644
--- a/qmp.c
+++ b/qmp.c
@@ -399,7 +399,8 @@  void qmp_change(const char *device, const char *target,
 #endif
     } else {
         qmp_blockdev_change_medium(true, device, false, NULL, target,
-                                   has_arg, arg, false, 0, errp);
+                                   has_arg, arg, false, 0, false, NULL,
+                                   errp);
     }
 }