Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/1] block-backend: allow flush on devices with open tray

[Markus Armbruster]

John Snow <address@hidden> writes:

> On 09/05/2016 05:15 AM, Markus Armbruster wrote:
>> John Snow <address@hidden> writes:
>>
>>> On 09/02/2016 01:44 AM, Markus Armbruster wrote:
>>>> John Snow <address@hidden> 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 <address@hidden>
>>>>> ---
>>>>>  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.