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

[John Snow]

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 -- 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