* [Qemu-devel] [PATCH v2 0/1] block-backend: allow flush on devices with open tray
@ 2016-09-01 21:56 John Snow
2016-09-01 21:56 ` [Qemu-devel] [PATCH v2 1/1] " John Snow
0 siblings, 1 reply; 11+ messages in thread
From: John Snow @ 2016-09-01 21:56 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, qemu-stable, mreitz, John Snow
This one fell through the cracks. This is therefore for 2.7.1
and the 2.8 development tree instead.
I have not written a test for this just yet, does anyone have
a suggestion on where it belongs?
John Snow (1):
block-backend: allow flush on devices with open tray
block/block-backend.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v2 1/1] block-backend: allow flush on devices with open tray
2016-09-01 21:56 [Qemu-devel] [PATCH v2 0/1] block-backend: allow flush on devices with open tray John Snow
@ 2016-09-01 21:56 ` John Snow
2016-09-01 22:11 ` Eric Blake
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: John Snow @ 2016-09-01 21:56 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, qemu-stable, mreitz, John Snow
If a device still has an attached BDS because the medium has not yet
been removed, we will be unable to migrate to a new host because
blk_flush will return an error for that backend.
Replace the call to blk_is_available to blk_is_inserted to weaken
the check and allow flushes from the backend to work, while still
disallowing flushes from the frontend/device model to work.
This fixes a regression present in 2.6.0 caused by the following commit:
fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
block: Move some bdrv_*_all() functions to BB
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/block-backend.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index effa038..36a32c3 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1058,7 +1058,7 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, int64_t offset,
BlockAIOCB *blk_aio_flush(BlockBackend *blk,
BlockCompletionFunc *cb, void *opaque)
{
- if (!blk_is_available(blk)) {
+ if (!blk_is_inserted(blk)) {
return blk_abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
}
@@ -1118,7 +1118,7 @@ int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int count)
int blk_co_flush(BlockBackend *blk)
{
- if (!blk_is_available(blk)) {
+ if (!blk_is_inserted(blk)) {
return -ENOMEDIUM;
}
@@ -1127,7 +1127,7 @@ int blk_co_flush(BlockBackend *blk)
int blk_flush(BlockBackend *blk)
{
- if (!blk_is_available(blk)) {
+ if (!blk_is_inserted(blk)) {
return -ENOMEDIUM;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] block-backend: allow flush on devices with open tray
2016-09-01 21:56 ` [Qemu-devel] [PATCH v2 1/1] " John Snow
@ 2016-09-01 22:11 ` Eric Blake
2016-09-01 22:17 ` John Snow
2016-09-01 23:48 ` [Qemu-devel] [Qemu-block] " Jeff Cody
2016-09-02 5:44 ` [Qemu-devel] " Markus Armbruster
2 siblings, 1 reply; 11+ messages in thread
From: Eric Blake @ 2016-09-01 22:11 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: kwolf, mreitz, qemu-devel, qemu-stable
[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]
On 09/01/2016 04:56 PM, John Snow wrote:
> If a device still has an attached BDS because the medium has not yet
> been removed, we will be unable to migrate to a new host because
> blk_flush will return an error for that backend.
>
> Replace the call to blk_is_available to blk_is_inserted to weaken
> the check and allow flushes from the backend to work, while still
> disallowing flushes from the frontend/device model to work.
Sounds slightly nicer if you stop short: s/ to work//
>
> This fixes a regression present in 2.6.0 caused by the following commit:
> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
> block: Move some bdrv_*_all() functions to BB
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/block-backend.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
However, I don't have an offhand suggestion on how best to test it.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] block-backend: allow flush on devices with open tray
2016-09-01 22:11 ` Eric Blake
@ 2016-09-01 22:17 ` John Snow
0 siblings, 0 replies; 11+ messages in thread
From: John Snow @ 2016-09-01 22:17 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: kwolf, mreitz, qemu-devel, qemu-stable
On 09/01/2016 06:11 PM, Eric Blake wrote:
> Sounds slightly nicer if you stop short: s/ to work//
I'm happy with this edit if a maintainer would like to make it.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/1] block-backend: allow flush on devices with open tray
2016-09-01 21:56 ` [Qemu-devel] [PATCH v2 1/1] " John Snow
2016-09-01 22:11 ` Eric Blake
@ 2016-09-01 23:48 ` Jeff Cody
2016-09-02 5:44 ` [Qemu-devel] " Markus Armbruster
2 siblings, 0 replies; 11+ messages in thread
From: Jeff Cody @ 2016-09-01 23:48 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, kwolf, mreitz, qemu-devel, qemu-stable
On Thu, Sep 01, 2016 at 05:56:52PM -0400, John Snow wrote:
> If a device still has an attached BDS because the medium has not yet
> been removed, we will be unable to migrate to a new host because
> blk_flush will return an error for that backend.
>
> Replace the call to blk_is_available to blk_is_inserted to weaken
> the check and allow flushes from the backend to work, while still
> disallowing flushes from the frontend/device model to work.
>
> This fixes a regression present in 2.6.0 caused by the following commit:
> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
> block: Move some bdrv_*_all() functions to BB
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
I don't have any real suggestions for a test either, except to point you to
test 091 which does a live migration test (which is not very helpful, I
know).
> ---
> block/block-backend.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index effa038..36a32c3 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1058,7 +1058,7 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, int64_t offset,
> BlockAIOCB *blk_aio_flush(BlockBackend *blk,
> BlockCompletionFunc *cb, void *opaque)
> {
> - if (!blk_is_available(blk)) {
> + if (!blk_is_inserted(blk)) {
> return blk_abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
> }
>
> @@ -1118,7 +1118,7 @@ int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int count)
>
> int blk_co_flush(BlockBackend *blk)
> {
> - if (!blk_is_available(blk)) {
> + if (!blk_is_inserted(blk)) {
> return -ENOMEDIUM;
> }
>
> @@ -1127,7 +1127,7 @@ int blk_co_flush(BlockBackend *blk)
>
> int blk_flush(BlockBackend *blk)
> {
> - if (!blk_is_available(blk)) {
> + if (!blk_is_inserted(blk)) {
> return -ENOMEDIUM;
> }
>
> --
> 2.7.4
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] block-backend: allow flush on devices with open tray
2016-09-01 21:56 ` [Qemu-devel] [PATCH v2 1/1] " John Snow
2016-09-01 22:11 ` Eric Blake
2016-09-01 23:48 ` [Qemu-devel] [Qemu-block] " Jeff Cody
@ 2016-09-02 5:44 ` Markus Armbruster
2016-09-02 16:05 ` John Snow
2 siblings, 1 reply; 11+ messages in thread
From: Markus Armbruster @ 2016-09-02 5:44 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, kwolf, mreitz, qemu-devel, qemu-stable
John Snow <jsnow@redhat.com> writes:
> If a device still has an attached BDS because the medium has not yet
> been removed, we will be unable to migrate to a new host because
> blk_flush will return an error for that backend.
>
> Replace the call to blk_is_available to blk_is_inserted to weaken
> the check and allow flushes from the backend to work, while still
> disallowing flushes from the frontend/device model to work.
>
> This fixes a regression present in 2.6.0 caused by the following commit:
> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
> block: Move some bdrv_*_all() functions to BB
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/block-backend.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index effa038..36a32c3 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1058,7 +1058,7 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, int64_t offset,
> BlockAIOCB *blk_aio_flush(BlockBackend *blk,
> BlockCompletionFunc *cb, void *opaque)
> {
> - if (!blk_is_available(blk)) {
> + if (!blk_is_inserted(blk)) {
> return blk_abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
> }
>
> @@ -1118,7 +1118,7 @@ int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int count)
>
> int blk_co_flush(BlockBackend *blk)
> {
> - if (!blk_is_available(blk)) {
> + if (!blk_is_inserted(blk)) {
> return -ENOMEDIUM;
> }
>
> @@ -1127,7 +1127,7 @@ int blk_co_flush(BlockBackend *blk)
>
> int blk_flush(BlockBackend *blk)
> {
> - if (!blk_is_available(blk)) {
> + if (!blk_is_inserted(blk)) {
> return -ENOMEDIUM;
> }
Naive question: should we flush before we open the tray?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] block-backend: allow flush on devices with open tray
2016-09-02 5:44 ` [Qemu-devel] " Markus Armbruster
@ 2016-09-02 16:05 ` John Snow
2016-09-05 9:15 ` Markus Armbruster
0 siblings, 1 reply; 11+ messages in thread
From: John Snow @ 2016-09-02 16:05 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-block, kwolf, mreitz, qemu-devel, qemu-stable
On 09/02/2016 01:44 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> If a device still has an attached BDS because the medium has not yet
>> been removed, we will be unable to migrate to a new host because
>> blk_flush will return an error for that backend.
>>
>> Replace the call to blk_is_available to blk_is_inserted to weaken
>> the check and allow flushes from the backend to work, while still
>> disallowing flushes from the frontend/device model to work.
>>
>> This fixes a regression present in 2.6.0 caused by the following commit:
>> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
>> block: Move some bdrv_*_all() functions to BB
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block/block-backend.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index effa038..36a32c3 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -1058,7 +1058,7 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, int64_t offset,
>> BlockAIOCB *blk_aio_flush(BlockBackend *blk,
>> BlockCompletionFunc *cb, void *opaque)
>> {
>> - if (!blk_is_available(blk)) {
>> + if (!blk_is_inserted(blk)) {
>> return blk_abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
>> }
>>
>> @@ -1118,7 +1118,7 @@ int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int count)
>>
>> int blk_co_flush(BlockBackend *blk)
>> {
>> - if (!blk_is_available(blk)) {
>> + if (!blk_is_inserted(blk)) {
>> return -ENOMEDIUM;
>> }
>>
>> @@ -1127,7 +1127,7 @@ int blk_co_flush(BlockBackend *blk)
>>
>> int blk_flush(BlockBackend *blk)
>> {
>> - if (!blk_is_available(blk)) {
>> + if (!blk_is_inserted(blk)) {
>> return -ENOMEDIUM;
>> }
>
> Naive question: should we flush before we open the tray?
>
Naive, but long-winded answer:
There are two types of flushes to me, conceptually:
Frontend and Backend.
Frontend would be a request from the quest to flush. If the medium in
question is absent, this should rightly fail. I do expect this to be
handled at the device layer.
Backend is a request from QEMU for various reasons, like needing to
drain the queue for migration or savevm.
What's happening here is that we have a backend request to flush a
medium that is inaccessible to the guest -- The flush all routine is
ignorant of this fact. So we get a migration request with an open tray
(because the user had opened it some time prior perhaps, and left it
open unwittingly) and it fails because its inaccessible to the guest.
Migration fails as a result.
That seems wrong to me. A few ways to fix it:
(1) Have internal flush requests be aware of the fact that there's
nothing to flush here: this is a read-only media and we could skip it.
(2) Allow flush to fail in a non-fatal way for cases where we need to
flush, but cannot (-ENOMEDIUM) and where it would rightly fail on a real
machine.
(3) Just allow flushes to work on devices not visible to the guest,
which is what I've done here. Internal requests should work, Guest
requests should fail.
I was briefly concerned about "What if this lets us flush something we
shouldn't?" and Max's take was "That doesn't seem so bad. CDROMs are RO
anyway."
So I went with the easiest option here.
To answer your question more directly, we aren't choosing to
eject-then-flush, the user is. I can't move the flush before the eject
unless we flush-on-eject internally, then mark the device explicitly as
not needing to be flushed anymore, but that's essentially (1) up there.
--js
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] block-backend: allow flush on devices with open tray
2016-09-02 16:05 ` John Snow
@ 2016-09-05 9:15 ` Markus Armbruster
2016-09-06 21:44 ` John Snow
0 siblings, 1 reply; 11+ messages in thread
From: Markus Armbruster @ 2016-09-05 9:15 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, qemu-stable, qemu-devel, qemu-block, mreitz
John Snow <jsnow@redhat.com> writes:
> On 09/02/2016 01:44 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> If a device still has an attached BDS because the medium has not yet
>>> been removed, we will be unable to migrate to a new host because
>>> blk_flush will return an error for that backend.
>>>
>>> Replace the call to blk_is_available to blk_is_inserted to weaken
>>> the check and allow flushes from the backend to work, while still
>>> disallowing flushes from the frontend/device model to work.
>>>
>>> This fixes a regression present in 2.6.0 caused by the following commit:
>>> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
>>> block: Move some bdrv_*_all() functions to BB
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> block/block-backend.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block/block-backend.c b/block/block-backend.c
>>> index effa038..36a32c3 100644
>>> --- a/block/block-backend.c
>>> +++ b/block/block-backend.c
>>> @@ -1058,7 +1058,7 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, int64_t offset,
>>> BlockAIOCB *blk_aio_flush(BlockBackend *blk,
>>> BlockCompletionFunc *cb, void *opaque)
>>> {
>>> - if (!blk_is_available(blk)) {
>>> + if (!blk_is_inserted(blk)) {
>>> return blk_abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
>>> }
>>>
>>> @@ -1118,7 +1118,7 @@ int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int count)
>>>
>>> int blk_co_flush(BlockBackend *blk)
>>> {
>>> - if (!blk_is_available(blk)) {
>>> + if (!blk_is_inserted(blk)) {
>>> return -ENOMEDIUM;
>>> }
>>>
>>> @@ -1127,7 +1127,7 @@ int blk_co_flush(BlockBackend *blk)
>>>
>>> int blk_flush(BlockBackend *blk)
>>> {
>>> - if (!blk_is_available(blk)) {
>>> + if (!blk_is_inserted(blk)) {
>>> return -ENOMEDIUM;
>>> }
>>
>> Naive question: should we flush before we open the tray?
>>
>
> Naive, but long-winded answer:
>
> There are two types of flushes to me, conceptually:
>
> Frontend and Backend.
>
> Frontend would be a request from the quest to flush. If the medium in
> question is absent, this should rightly fail. I do expect this to be
> handled at the device layer.
>
> Backend is a request from QEMU for various reasons, like needing to
> drain the queue for migration or savevm.
>
> What's happening here is that we have a backend request to flush a
> medium that is inaccessible to the guest
Assuming we caught frontend requests at the device layer already.
> -- The flush all routine is
> ignorant of this fact. So we get a migration request with an open tray
> (because the user had opened it some time prior perhaps, and left it
> open unwittingly) and it fails because its inaccessible to the
> guest. Migration fails as a result.
>
> That seems wrong to me. A few ways to fix it:
>
> (1) Have internal flush requests be aware of the fact that there's
> nothing to flush here: this is a read-only media and we could skip it.
Returning successfully because there's nothing to flush makes sense to
me.
> (2) Allow flush to fail in a non-fatal way for cases where we need to
> flush, but cannot (-ENOMEDIUM) and where it would rightly fail on a
> real machine.
I don't like -ENOMEDIUM when there's nothing to flush.
> (3) Just allow flushes to work on devices not visible to the guest,
> which is what I've done here. Internal requests should work, Guest
> requests should fail.
I guess that's okay, ...
> I was briefly concerned about "What if this lets us flush something we
> shouldn't?" and Max's take was "That doesn't seem so bad. CDROMs are
> RO anyway."
... even though I don't buy Max's reason. Writable removable media
exist. The argument I can buy is that internal requests are
fundamentally different from external requests.
> So I went with the easiest option here.
>
> To answer your question more directly, we aren't choosing to
> eject-then-flush, the user is. I can't move the flush before the eject
> unless we flush-on-eject internally, then mark the device explicitly
> as not needing to be flushed anymore, but that's essentially (1) up
> there.
>
> --js
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] block-backend: allow flush on devices with open tray
2016-09-05 9:15 ` Markus Armbruster
@ 2016-09-06 21:44 ` John Snow
2016-09-07 6:51 ` Markus Armbruster
2016-09-07 8:18 ` Kevin Wolf
0 siblings, 2 replies; 11+ messages in thread
From: John Snow @ 2016-09-06 21:44 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-stable, qemu-devel, qemu-block, mreitz
On 09/05/2016 05:15 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 09/02/2016 01:44 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> If a device still has an attached BDS because the medium has not yet
>>>> been removed, we will be unable to migrate to a new host because
>>>> blk_flush will return an error for that backend.
>>>>
>>>> Replace the call to blk_is_available to blk_is_inserted to weaken
>>>> the check and allow flushes from the backend to work, while still
>>>> disallowing flushes from the frontend/device model to work.
>>>>
>>>> This fixes a regression present in 2.6.0 caused by the following commit:
>>>> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
>>>> block: Move some bdrv_*_all() functions to BB
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> block/block-backend.c | 6 +++---
>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/block/block-backend.c b/block/block-backend.c
>>>> index effa038..36a32c3 100644
>>>> --- a/block/block-backend.c
>>>> +++ b/block/block-backend.c
>>>> @@ -1058,7 +1058,7 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, int64_t offset,
>>>> BlockAIOCB *blk_aio_flush(BlockBackend *blk,
>>>> BlockCompletionFunc *cb, void *opaque)
>>>> {
>>>> - if (!blk_is_available(blk)) {
>>>> + if (!blk_is_inserted(blk)) {
>>>> return blk_abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
>>>> }
>>>>
>>>> @@ -1118,7 +1118,7 @@ int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int count)
>>>>
>>>> int blk_co_flush(BlockBackend *blk)
>>>> {
>>>> - if (!blk_is_available(blk)) {
>>>> + if (!blk_is_inserted(blk)) {
>>>> return -ENOMEDIUM;
>>>> }
>>>>
>>>> @@ -1127,7 +1127,7 @@ int blk_co_flush(BlockBackend *blk)
>>>>
>>>> int blk_flush(BlockBackend *blk)
>>>> {
>>>> - if (!blk_is_available(blk)) {
>>>> + if (!blk_is_inserted(blk)) {
>>>> return -ENOMEDIUM;
>>>> }
>>>
>>> Naive question: should we flush before we open the tray?
>>>
>>
>> Naive, but long-winded answer:
>>
>> There are two types of flushes to me, conceptually:
>>
>> Frontend and Backend.
>>
>> Frontend would be a request from the quest to flush. If the medium in
>> question is absent, this should rightly fail. I do expect this to be
>> handled at the device layer.
>>
>> Backend is a request from QEMU for various reasons, like needing to
>> drain the queue for migration or savevm.
>>
>> What's happening here is that we have a backend request to flush a
>> medium that is inaccessible to the guest
>
> Assuming we caught frontend requests at the device layer already.
>
>> -- The flush all routine is
>> ignorant of this fact. So we get a migration request with an open tray
>> (because the user had opened it some time prior perhaps, and left it
>> open unwittingly) and it fails because its inaccessible to the
>> guest. Migration fails as a result.
>>
>> That seems wrong to me. A few ways to fix it:
>>
>> (1) Have internal flush requests be aware of the fact that there's
>> nothing to flush here: this is a read-only media and we could skip it.
>
> Returning successfully because there's nothing to flush makes sense to
> me.
>
Only slightly more involved. At the time I wrote the first patch, I
don't think Denis' improvements had gone in yet, and I still haven't
really looked at them to see how easy they are to co-opt here.
>> (2) Allow flush to fail in a non-fatal way for cases where we need to
>> flush, but cannot (-ENOMEDIUM) and where it would rightly fail on a
>> real machine.
>
> I don't like -ENOMEDIUM when there's nothing to flush.
>
>> (3) Just allow flushes to work on devices not visible to the guest,
>> which is what I've done here. Internal requests should work, Guest
>> requests should fail.
>
> I guess that's okay, ...
>
Conceptually, QEMU should be allowed to flush things to its internal
drive abstractions regardless of what it looks like to the guest
whenever it decides it is necessary to.
In practice, I don't know how clean we are about separating out who is
requesting the flush to know which to deny.
>> I was briefly concerned about "What if this lets us flush something we
>> shouldn't?" and Max's take was "That doesn't seem so bad. CDROMs are
>> RO anyway."
>
> ... even though I don't buy Max's reason. Writable removable media
> exist. The argument I can buy is that internal requests are
> fundamentally different from external requests.
>
Yeah, I think any such flush request should be denied by the device model.
>> So I went with the easiest option here.
>>
>> To answer your question more directly, we aren't choosing to
>> eject-then-flush, the user is. I can't move the flush before the eject
>> unless we flush-on-eject internally, then mark the device explicitly
>> as not needing to be flushed anymore, but that's essentially (1) up
>> there.
>>
>> --js
I am at the moment inclined to stick with the existing solution "for
now" and add hardening against flush requests from the guest later
within the 2.8 window if needed.
--js
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] block-backend: allow flush on devices with open tray
2016-09-06 21:44 ` John Snow
@ 2016-09-07 6:51 ` Markus Armbruster
2016-09-07 8:18 ` Kevin Wolf
1 sibling, 0 replies; 11+ messages in thread
From: Markus Armbruster @ 2016-09-07 6:51 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, mreitz, qemu-stable, qemu-block, qemu-devel
John Snow <jsnow@redhat.com> writes:
> On 09/05/2016 05:15 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 09/02/2016 01:44 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> If a device still has an attached BDS because the medium has not yet
>>>>> been removed, we will be unable to migrate to a new host because
>>>>> blk_flush will return an error for that backend.
>>>>>
>>>>> Replace the call to blk_is_available to blk_is_inserted to weaken
>>>>> the check and allow flushes from the backend to work, while still
>>>>> disallowing flushes from the frontend/device model to work.
>>>>>
>>>>> This fixes a regression present in 2.6.0 caused by the following commit:
>>>>> fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
>>>>> block: Move some bdrv_*_all() functions to BB
>>>>>
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>> ---
>>>>> block/block-backend.c | 6 +++---
>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/block/block-backend.c b/block/block-backend.c
>>>>> index effa038..36a32c3 100644
>>>>> --- a/block/block-backend.c
>>>>> +++ b/block/block-backend.c
>>>>> @@ -1058,7 +1058,7 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, int64_t offset,
>>>>> BlockAIOCB *blk_aio_flush(BlockBackend *blk,
>>>>> BlockCompletionFunc *cb, void *opaque)
>>>>> {
>>>>> - if (!blk_is_available(blk)) {
>>>>> + if (!blk_is_inserted(blk)) {
>>>>> return blk_abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
>>>>> }
>>>>>
>>>>> @@ -1118,7 +1118,7 @@ int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int count)
>>>>>
>>>>> int blk_co_flush(BlockBackend *blk)
>>>>> {
>>>>> - if (!blk_is_available(blk)) {
>>>>> + if (!blk_is_inserted(blk)) {
>>>>> return -ENOMEDIUM;
>>>>> }
>>>>>
>>>>> @@ -1127,7 +1127,7 @@ int blk_co_flush(BlockBackend *blk)
>>>>>
>>>>> int blk_flush(BlockBackend *blk)
>>>>> {
>>>>> - if (!blk_is_available(blk)) {
>>>>> + if (!blk_is_inserted(blk)) {
>>>>> return -ENOMEDIUM;
>>>>> }
>>>>
>>>> Naive question: should we flush before we open the tray?
>>>>
>>>
>>> Naive, but long-winded answer:
>>>
>>> There are two types of flushes to me, conceptually:
>>>
>>> Frontend and Backend.
>>>
>>> Frontend would be a request from the quest to flush. If the medium in
>>> question is absent, this should rightly fail. I do expect this to be
>>> handled at the device layer.
>>>
>>> Backend is a request from QEMU for various reasons, like needing to
>>> drain the queue for migration or savevm.
>>>
>>> What's happening here is that we have a backend request to flush a
>>> medium that is inaccessible to the guest
>>
>> Assuming we caught frontend requests at the device layer already.
>>
>>> -- The flush all routine is
>>> ignorant of this fact. So we get a migration request with an open tray
>>> (because the user had opened it some time prior perhaps, and left it
>>> open unwittingly) and it fails because its inaccessible to the
>>> guest. Migration fails as a result.
>>>
>>> That seems wrong to me. A few ways to fix it:
>>>
>>> (1) Have internal flush requests be aware of the fact that there's
>>> nothing to flush here: this is a read-only media and we could skip it.
>>
>> Returning successfully because there's nothing to flush makes sense to
>> me.
>>
>
> Only slightly more involved. At the time I wrote the first patch, I
> don't think Denis' improvements had gone in yet, and I still haven't
> really looked at them to see how easy they are to co-opt here.
>
>>> (2) Allow flush to fail in a non-fatal way for cases where we need to
>>> flush, but cannot (-ENOMEDIUM) and where it would rightly fail on a
>>> real machine.
>>
>> I don't like -ENOMEDIUM when there's nothing to flush.
>>
>>> (3) Just allow flushes to work on devices not visible to the guest,
>>> which is what I've done here. Internal requests should work, Guest
>>> requests should fail.
>>
>> I guess that's okay, ...
>>
>
> Conceptually, QEMU should be allowed to flush things to its internal
> drive abstractions regardless of what it looks like to the guest
> whenever it decides it is necessary to.
>
> In practice, I don't know how clean we are about separating out who is
> requesting the flush to know which to deny.
Exactly.
>>> I was briefly concerned about "What if this lets us flush something we
>>> shouldn't?" and Max's take was "That doesn't seem so bad. CDROMs are
>>> RO anyway."
>>
>> ... even though I don't buy Max's reason. Writable removable media
>> exist. The argument I can buy is that internal requests are
>> fundamentally different from external requests.
>>
>
> Yeah, I think any such flush request should be denied by the device model.
>
>>> So I went with the easiest option here.
>>>
>>> To answer your question more directly, we aren't choosing to
>>> eject-then-flush, the user is. I can't move the flush before the eject
>>> unless we flush-on-eject internally, then mark the device explicitly
>>> as not needing to be flushed anymore, but that's essentially (1) up
>>> there.
>>>
>>> --js
>
> I am at the moment inclined to stick with the existing solution "for
> now" and add hardening against flush requests from the guest later
> within the 2.8 window if needed.
Okay.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] block-backend: allow flush on devices with open tray
2016-09-06 21:44 ` John Snow
2016-09-07 6:51 ` Markus Armbruster
@ 2016-09-07 8:18 ` Kevin Wolf
1 sibling, 0 replies; 11+ messages in thread
From: Kevin Wolf @ 2016-09-07 8:18 UTC (permalink / raw)
To: John Snow; +Cc: Markus Armbruster, qemu-stable, qemu-devel, qemu-block, mreitz
Am 06.09.2016 um 23:44 hat John Snow geschrieben:
> On 09/05/2016 05:15 AM, Markus Armbruster wrote:
> >John Snow <jsnow@redhat.com> writes:
> >>(3) Just allow flushes to work on devices not visible to the guest,
> >>which is what I've done here. Internal requests should work, Guest
> >>requests should fail.
> >
> >I guess that's okay, ...
> >
>
> Conceptually, QEMU should be allowed to flush things to its internal
> drive abstractions regardless of what it looks like to the guest
> whenever it decides it is necessary to.
>
> In practice, I don't know how clean we are about separating out who
> is requesting the flush to know which to deny.
Maybe the move from bdrv_flush_all() to blk_flush_all() was wrong. If
you have a BDS that isn't currently attached to a guest device or at
least not accessible from the guest device's BB because of things like
an open tray, but the BDS has previously been written to, you still want
to flush it for migration.
The only other clean option I see is that we flush BDSes when they are
detached from the BB, or otherwise become unaccessible (e.g. the tray is
opened). And that second part suggests that it might be too easy to get
this one wrong, so maybe bdrv_flush_all() was the best option after all.
Kevin
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-09-07 8:18 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-01 21:56 [Qemu-devel] [PATCH v2 0/1] block-backend: allow flush on devices with open tray John Snow
2016-09-01 21:56 ` [Qemu-devel] [PATCH v2 1/1] " John Snow
2016-09-01 22:11 ` Eric Blake
2016-09-01 22:17 ` John Snow
2016-09-01 23:48 ` [Qemu-devel] [Qemu-block] " Jeff Cody
2016-09-02 5:44 ` [Qemu-devel] " Markus Armbruster
2016-09-02 16:05 ` John Snow
2016-09-05 9:15 ` Markus Armbruster
2016-09-06 21:44 ` John Snow
2016-09-07 6:51 ` Markus Armbruster
2016-09-07 8:18 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.