Re: [Qemu-block] [PATCH v7 28/39] blockdev: Add blockdev-open-tray

[Max Reitz]

On 23.10.2015 15:26, Kevin Wolf wrote:
> Am 19.10.2015 um 17:53 hat Max Reitz geschrieben:
>> Signed-off-by: Max Reitz 
>> ---
>>  blockdev.c   | 49 +
>>  qapi/block-core.json | 23 +++
>>  qmp-commands.hx  | 39 +++
>>  3 files changed, 111 insertions(+)
> 
>> +bs = blk_bs(blk);
>> +if (bs) {
>> +aio_context = bdrv_get_aio_context(bs);
>> +aio_context_acquire(aio_context);
>> +
>> +if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
>> +goto out;
>> +}
> 
> Is this blocker really for protecting against opening the tray? I think
> the BDS shouldn't care about whether the guest can access it. So it's
> probably more about preventing a bdrv_close() from happening, i.e. it
> would be relevant only for the blockdev-remove-medium command.

I don't think so. I intended blockdev-open-tray to be what it is for
real hardware: Opening the tray severs all the links between the medium
and software trying to access the drive. blockdev-remove-medium should
never fail if the tray is already open, since it would never fail in
real life.

By the way, that's the reason why I generally preferred
blk_is_available() over blk_is_inserted() in this series.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v7 28/39] blockdev: Add blockdev-open-tray

[Kevin Wolf]

Am 23.10.2015 um 17:25 hat Max Reitz geschrieben:
> On 23.10.2015 16:45, Kevin Wolf wrote:
> > Am 23.10.2015 um 16:26 hat Max Reitz geschrieben:
> >> On 23.10.2015 15:26, Kevin Wolf wrote:
> >>> Am 19.10.2015 um 17:53 hat Max Reitz geschrieben:
>  Signed-off-by: Max Reitz 
>  ---
>   blockdev.c   | 49 
>  +
>   qapi/block-core.json | 23 +++
>   qmp-commands.hx  | 39 +++
>   3 files changed, 111 insertions(+)
> >>>
>  +bs = blk_bs(blk);
>  +if (bs) {
>  +aio_context = bdrv_get_aio_context(bs);
>  +aio_context_acquire(aio_context);
>  +
>  +if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
>  +goto out;
>  +}
> >>>
> >>> Is this blocker really for protecting against opening the tray? I think
> >>> the BDS shouldn't care about whether the guest can access it. So it's
> >>> probably more about preventing a bdrv_close() from happening, i.e. it
> >>> would be relevant only for the blockdev-remove-medium command.
> >>
> >> I don't think so. I intended blockdev-open-tray to be what it is for
> >> real hardware: Opening the tray severs all the links between the medium
> >> and software trying to access the drive. blockdev-remove-medium should
> >> never fail if the tray is already open, since it would never fail in
> >> real life.
> > 
> > Comparison with real hardware works only so far. Real hardware doesn't
> > have block jobs and will therefore never set the eject blocker.
> 
> It does have software accessing the disk and will therefore lock the tray.

That's an entirely different matter and checked by the
blk_dev_is_medium_locked() call below.

> > As I said, though, it's mostly protecting against bdrv_close(). Now that
> > we don't call that any more, we don't strictly need the blocker any
> > more in order to keep block jobs happy.
> > 
> > However, we still need to prevent that the connection between BB and BDS
> > is severed in case the old BDS was created implicitly and therefore
> > would disappear from query-block while the image is still open and in
> > use, which we don't want. This touches blockdev-del land more than op
> > blockers, though... I think the eject op blocker can go.
> 
> OK, so the thing is that block jobs don't use the BB but generally
> access the BDS directly. Therefore, they don't care whether the BDS is
> still accessible from some guest device/BB.
> 
> I'm fine with removing the eject op blocker, but I think you'll
> understand if I'd rather not make it part of this series.

Fine with me.

> > With your check, you prevent the user from opening the tray using QMP
> > and then they can't get into a bad state with blockdev-remove-medium
> > because without an opened tray that would fail. However, they could
> > still eject the medium from within the guest and then use
> > blockdev-remove-medium, which will get them in the bad state that we
> > wanted to prevent.
> 
> Well, the logical conclusion from this and the above simply is "remove
> that op blocker". The block job shouldn't care about some guest device
> behind some BB opening its tray; so consequently it shouldn't care about
> the BDS being removed from that BB either.

Yes, the block job shouldn't care.

As I said above, though, there's an another principle at danger: That
the monitor reference is always the last one that is dropped, so that
all open image files have a corresponding query-block entry.

> Oh, but there is a case where the block job should care: If you've
> specified the name of that BB when creating the block job. To me, that
> implies that the job should run through that BB and the related guest
> device may not open its tray.

I'm not sure about that. I think in most cases the BB name is just used
as an alias for its root node.

And of course, it has nothing to do with the guest device tray. A block
job can go through the BB even though the guest tray is open. The tray
isn't a BB concept, but a device emulation one.

> Anyway, that's definitely outside of this series's realm. I guess I'll
> move the check to qmp_blockdev_remove_medium(), as you suggested.

That should solve the problems.

> >> By the way, that's the reason why I generally preferred
> >> blk_is_available() over blk_is_inserted() in this series.
> > 
> > I actually think this use is too restrictive in many cases (and in this
> > patch opening the tray is pointlessly forbidden), but I didn't comment
> > on it because we can fix it whenever someone needs more.
> 
> My opinion still is that if you're accessing a BDS tree through a BB
> which is attached to a guest device, you should assume the guest
> device's view on the BDS tree, that is, if the device's tray is open,
> you won't get any data.

Currently the check whether the tray is open is done in the device code,
not in the BB. In the few 

Re: [Qemu-block] [PATCH v7 28/39] blockdev: Add blockdev-open-tray

[Kevin Wolf]

Am 19.10.2015 um 17:53 hat Max Reitz geschrieben:
> Signed-off-by: Max Reitz 
> ---
>  blockdev.c   | 49 +
>  qapi/block-core.json | 23 +++
>  qmp-commands.hx  | 39 +++
>  3 files changed, 111 insertions(+)

> +
> +if (blk_dev_is_medium_locked(blk)) {
> +blk_dev_eject_request(blk, force);
> +} else {
> +blk_dev_change_media_cb(blk, false);
> +}

This seems to be inconsistent with the command documentation: In the
case of a forced eject request, the tray is just unlocked, but not
opened.

> +##
> +# @blockdev-open-tray:
> +#
> +# Opens a block device's tray. If there is a block driver state tree 
> inserted as
> +# a medium, it will become inaccessible to the guest (but it will remain
> +# associated to the block device, so closing the tray will make it accessible
> +# again).
> +#
> +# If the tray was already open before, this will be a no-op.
> +#
> +# @device: block device name
> +#
> +# @force:  #optional if false (the default), an eject request will be sent to
> +#  the guest if it has locked the tray (and the tray will not be 
> opened
> +#  immediately); if true, the tray will be opened regardless of 
> whether
> +#  it is locked
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'blockdev-open-tray',
> +  'data': { 'device': 'str',
> +'*force': 'bool' } }

This API makes it impossible for the management application to know
whether the tray has actually been opened or just an eject request has
been made.

Is the expected use that management sets a timeout and waits for a
tray opened event? If so, worth documenting?

Kevin

Re: [Qemu-block] [PATCH v7 28/39] blockdev: Add blockdev-open-tray

[Kevin Wolf]

Am 19.10.2015 um 17:53 hat Max Reitz geschrieben:
> Signed-off-by: Max Reitz 
> ---
>  blockdev.c   | 49 +
>  qapi/block-core.json | 23 +++
>  qmp-commands.hx  | 39 +++
>  3 files changed, 111 insertions(+)

> +bs = blk_bs(blk);
> +if (bs) {
> +aio_context = bdrv_get_aio_context(bs);
> +aio_context_acquire(aio_context);
> +
> +if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
> +goto out;
> +}

Is this blocker really for protecting against opening the tray? I think
the BDS shouldn't care about whether the guest can access it. So it's
probably more about preventing a bdrv_close() from happening, i.e. it
would be relevant only for the blockdev-remove-medium command.

Kevin

Re: [Qemu-block] [PATCH v7 28/39] blockdev: Add blockdev-open-tray

[Max Reitz]

On 23.10.2015 16:45, Kevin Wolf wrote:
> Am 23.10.2015 um 16:26 hat Max Reitz geschrieben:
>> On 23.10.2015 15:26, Kevin Wolf wrote:
>>> Am 19.10.2015 um 17:53 hat Max Reitz geschrieben:
 Signed-off-by: Max Reitz 
 ---
  blockdev.c   | 49 
 +
  qapi/block-core.json | 23 +++
  qmp-commands.hx  | 39 +++
  3 files changed, 111 insertions(+)
>>>
 +bs = blk_bs(blk);
 +if (bs) {
 +aio_context = bdrv_get_aio_context(bs);
 +aio_context_acquire(aio_context);
 +
 +if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
 +goto out;
 +}
>>>
>>> Is this blocker really for protecting against opening the tray? I think
>>> the BDS shouldn't care about whether the guest can access it. So it's
>>> probably more about preventing a bdrv_close() from happening, i.e. it
>>> would be relevant only for the blockdev-remove-medium command.
>>
>> I don't think so. I intended blockdev-open-tray to be what it is for
>> real hardware: Opening the tray severs all the links between the medium
>> and software trying to access the drive. blockdev-remove-medium should
>> never fail if the tray is already open, since it would never fail in
>> real life.
> 
> Comparison with real hardware works only so far. Real hardware doesn't
> have block jobs and will therefore never set the eject blocker.

It does have software accessing the disk and will therefore lock the tray.

> As I said, though, it's mostly protecting against bdrv_close(). Now that
> we don't call that any more, we don't strictly need the blocker any
> more in order to keep block jobs happy.
> 
> However, we still need to prevent that the connection between BB and BDS
> is severed in case the old BDS was created implicitly and therefore
> would disappear from query-block while the image is still open and in
> use, which we don't want. This touches blockdev-del land more than op
> blockers, though... I think the eject op blocker can go.

OK, so the thing is that block jobs don't use the BB but generally
access the BDS directly. Therefore, they don't care whether the BDS is
still accessible from some guest device/BB.

I'm fine with removing the eject op blocker, but I think you'll
understand if I'd rather not make it part of this series.

> With your check, you prevent the user from opening the tray using QMP
> and then they can't get into a bad state with blockdev-remove-medium
> because without an opened tray that would fail. However, they could
> still eject the medium from within the guest and then use
> blockdev-remove-medium, which will get them in the bad state that we
> wanted to prevent.

Well, the logical conclusion from this and the above simply is "remove
that op blocker". The block job shouldn't care about some guest device
behind some BB opening its tray; so consequently it shouldn't care about
the BDS being removed from that BB either.

Oh, but there is a case where the block job should care: If you've
specified the name of that BB when creating the block job. To me, that
implies that the job should run through that BB and the related guest
device may not open its tray.

Anyway, that's definitely outside of this series's realm. I guess I'll
move the check to qmp_blockdev_remove_medium(), as you suggested.

>> By the way, that's the reason why I generally preferred
>> blk_is_available() over blk_is_inserted() in this series.
> 
> I actually think this use is too restrictive in many cases (and in this
> patch opening the tray is pointlessly forbidden), but I didn't comment
> on it because we can fix it whenever someone needs more.

My opinion still is that if you're accessing a BDS tree through a BB
which is attached to a guest device, you should assume the guest
device's view on the BDS tree, that is, if the device's tray is open,
you won't get any data.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v7 28/39] blockdev: Add blockdev-open-tray

[Kevin Wolf]

Am 23.10.2015 um 16:26 hat Max Reitz geschrieben:
> On 23.10.2015 15:26, Kevin Wolf wrote:
> > Am 19.10.2015 um 17:53 hat Max Reitz geschrieben:
> >> Signed-off-by: Max Reitz 
> >> ---
> >>  blockdev.c   | 49 
> >> +
> >>  qapi/block-core.json | 23 +++
> >>  qmp-commands.hx  | 39 +++
> >>  3 files changed, 111 insertions(+)
> > 
> >> +bs = blk_bs(blk);
> >> +if (bs) {
> >> +aio_context = bdrv_get_aio_context(bs);
> >> +aio_context_acquire(aio_context);
> >> +
> >> +if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
> >> +goto out;
> >> +}
> > 
> > Is this blocker really for protecting against opening the tray? I think
> > the BDS shouldn't care about whether the guest can access it. So it's
> > probably more about preventing a bdrv_close() from happening, i.e. it
> > would be relevant only for the blockdev-remove-medium command.
> 
> I don't think so. I intended blockdev-open-tray to be what it is for
> real hardware: Opening the tray severs all the links between the medium
> and software trying to access the drive. blockdev-remove-medium should
> never fail if the tray is already open, since it would never fail in
> real life.

Comparison with real hardware works only so far. Real hardware doesn't
have block jobs and will therefore never set the eject blocker.

As I said, though, it's mostly protecting against bdrv_close(). Now that
we don't call that any more, we don't strictly need the blocker any
more in order to keep block jobs happy.

However, we still need to prevent that the connection between BB and BDS
is severed in case the old BDS was created implicitly and therefore
would disappear from query-block while the image is still open and in
use, which we don't want. This touches blockdev-del land more than op
blockers, though... I think the eject op blocker can go.

With your check, you prevent the user from opening the tray using QMP
and then they can't get into a bad state with blockdev-remove-medium
because without an opened tray that would fail. However, they could
still eject the medium from within the guest and then use
blockdev-remove-medium, which will get them in the bad state that we
wanted to prevent.

> By the way, that's the reason why I generally preferred
> blk_is_available() over blk_is_inserted() in this series.

I actually think this use is too restrictive in many cases (and in this
patch opening the tray is pointlessly forbidden), but I didn't comment
on it because we can fix it whenever someone needs more.

Kevin


pgpG4uHQHXdLn.pgp
Description: PGP signature

[Qemu-block] [PATCH v7 28/39] blockdev: Add blockdev-open-tray

[Max Reitz]

Signed-off-by: Max Reitz 
---
 blockdev.c   | 49 +
 qapi/block-core.json | 23 +++
 qmp-commands.hx  | 39 +++
 3 files changed, 111 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 35e5719..aa68c36 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2049,6 +2049,55 @@ out:
 aio_context_release(aio_context);
 }
 
+void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
+Error **errp)
+{
+BlockBackend *blk;
+BlockDriverState *bs;
+AioContext *aio_context = NULL;
+
+if (!has_force) {
+force = false;
+}
+
+blk = blk_by_name(device);
+if (!blk) {
+error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+  "Device '%s' not found", device);
+return;
+}
+
+if (!blk_dev_has_removable_media(blk)) {
+error_setg(errp, "Device '%s' is not removable", device);
+return;
+}
+
+if (blk_dev_is_tray_open(blk)) {
+return;
+}
+
+bs = blk_bs(blk);
+if (bs) {
+aio_context = bdrv_get_aio_context(bs);
+aio_context_acquire(aio_context);
+
+if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
+goto out;
+}
+}
+
+if (blk_dev_is_medium_locked(blk)) {
+blk_dev_eject_request(blk, force);
+} else {
+blk_dev_change_media_cb(blk, false);
+}
+
+out:
+if (aio_context) {
+aio_context_release(aio_context);
+}
+}
+
 /* throttling disk I/O limits */
 void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
int64_t bps_wr,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 425fdab..b9b4a24 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1876,6 +1876,29 @@
 ##
 { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
 
+##
+# @blockdev-open-tray:
+#
+# Opens a block device's tray. If there is a block driver state tree inserted 
as
+# a medium, it will become inaccessible to the guest (but it will remain
+# associated to the block device, so closing the tray will make it accessible
+# again).
+#
+# If the tray was already open before, this will be a no-op.
+#
+# @device: block device name
+#
+# @force:  #optional if false (the default), an eject request will be sent to
+#  the guest if it has locked the tray (and the tray will not be opened
+#  immediately); if true, the tray will be opened regardless of whether
+#  it is locked
+#
+# Since: 2.5
+##
+{ 'command': 'blockdev-open-tray',
+  'data': { 'device': 'str',
+'*force': 'bool' } }
+
 
 ##
 # @BlockErrorAction
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d7cf0ff..595aee2 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3936,6 +3936,45 @@ Example (2):
 EQMP
 
 {
+.name   = "blockdev-open-tray",
+.args_type  = "device:s,force:b?",
+.mhandler.cmd_new = qmp_marshal_blockdev_open_tray,
+},
+
+SQMP
+blockdev-open-tray
+--
+
+Opens a block device's tray. If there is a block driver state tree inserted as 
a
+medium, it will become inaccessible to the guest (but it will remain associated
+to the block device, so closing the tray will make it accessible again).
+
+If the tray was already open before, this will be a no-op.
+
+Arguments:
+
+- "device": block device name (json-string)
+- "force": if false (the default), an eject request will be sent to the guest 
if
+   it has locked the tray (and the tray will not be opened 
immediately);
+   if true, the tray will be opened regardless of whether it is locked
+   (json-bool, optional)
+
+Example:
+
+-> { "execute": "blockdev-open-tray",
+ "arguments": { "device": "ide1-cd0" } }
+
+<- { "timestamp": { "seconds": 1418751016,
+"microseconds": 716996 },
+ "event": "DEVICE_TRAY_MOVED",
+ "data": { "device": "ide1-cd0",
+   "tray-open": true } }
+
+<- { "return": {} }
+
+EQMP
+
+{
 .name   = "query-named-block-nodes",
 .args_type  = "",
 .mhandler.cmd_new = qmp_marshal_query_named_block_nodes,
-- 
2.6.1