Re: [Qemu-devel] 'commit' error for 'all': No medium found

[Stefan Hajnoczi]

On Mon, Feb 25, 2013 at 04:01:42PM +0100, Markus Armbruster wrote:
> Jan Kiszka <address@hidden> writes:
> 
> > On 2013-02-25 15:37, Jeff Cody wrote:
> >> On Mon, Feb 25, 2013 at 03:30:34PM +0100, Markus Armbruster wrote:
> >>> Jan Kiszka <address@hidden> writes:
> >>>
> >>>> On 2013-02-25 14:05, Jeff Cody wrote:
> >>>>> On Sun, Feb 24, 2013 at 07:29:31PM +0100, Jan Kiszka wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> I'm seeing this with git head and 1.4. Apparently, commit on a
> >>>>>> non-populated medium now generates this error instead of ignoring it
> >>>>>> like in the past. As we stop iterating over the block devices while
> >>>>>> doing "all", this may leaving uncommitted data behind.
> >>>>>>
> >>>>>> Didn't test, but I suspect 58513bde83 has something to do with it.
> >>>>>>
> >>>>>> Jan
> >>>>>>
> >>>>>
> >>>>> Hi Jan,
> >>>>>
> >>>>> That commit just affected the reporting on the error - for the "all"
> >>>>> case, bdrv_commit_all() still returned error on the first failure.
> >>>>> When that happened 'commit' may have indicated success rather than
> >>>>> error, depending on the error.
> >>>>
> >>>> Sorry, I just picked on the first commit that jumped into my eyes.
> >>>>
> >>>>>
> >>>>> That would have also left uncommitted data behind, but done so
> >>>>> silently and reported success.
> >>>>>
> >>>>> However, commit e8877497 added error checking to the bdrv_commit()
> >>>>> return value in bdrv_commit_all() - before that bdrv_commit_all()
> >>>>> ignored all error returns of bdrv_commit().
> >>>>>
> >>>>> Do you think there specific error returns that we should ignore then, in
> >>>>> bdrv_commit_all(), such as -ENOMEDIUM?
> >>>>
> >>>> I think commit on a device without a medium should be a nop (as the
> >>>
> >>> Yes.
> >>>
> >>>> commit backlog is empty - provided it's correctly dropped on eject).
> >>>
> >>> Monitor commands eject and change close the backend.  This does not
> >>> collapse ("commit") any COWs into their backing images.  If the backend
> >>> was opened with BDRV_O_SNAPSHOT, the uncommitted updates are lost.
> >>> That's a feature.
> >>>
> >>>>> Also, could you expand on what you mean by non-populated
> >>>>> medium (test case) - is the error being returned "No medium found"?
> >>>>
> >>>> This is a default PC setup: the floppy tends to be not injected, thus
> >>>> "commit all" now reports this error. If there is a blockdev after the
> >>>> floppy in our list, it will not be committed this way. Luckly, the HD is
> >>>> queued before the FD.
> >>>
> >>> Making bdrv_commit_all() check errors was a good idea, we just need to
> >>> fix the regression in the !bs->drv case.
> >>>
> >>> I feel a bit queasy about detecting -ENOMEDIUM, because it's not obvious
> >>> to me that bdrv_commit() can only return that when !bs->drv.
> >> 
> >> Is there any reason to not just check for !bs->drv prior to calling
> >> bdrv_commit() in the FOREACH loop, in bdrv_commit_all()?  That would
> >> seem to be the simplest approach.  
> 
> Yes, that's better.
> 
> >> The behaviour then would be nop on no medium during commit all. But if
> >> you specifically tried to just run commit() on a single device that
> >> had no medium, you would receive ENOMEDIUM.  That seems logical to me.
> >
> > I think, even "commit <blockdev-without-medium>" should not return
> > -ENOMEDIUM. That would be more consistent IMHO.
> 
> "commit FOO" makes sense only when FOO is a COW backend.  When it isn't,
> we can either ignore the non-sensical request silently, or complain.  I
> think both ways are defensible.  We just need to pick one, and stick to
> it consistently.
> 
> "commit all" should obviously apply only to COW backends.  I don't think
> that makes a "commit FOO" that complains inconsistent.  I simply read
> "all" as "all COW backends".
> 
> That said, I don't really care which way we go.  Kevin or Stefan, got a
> preference?

Let's be strict.  The caller can always ignore -ENOTSUP if they want to
call it on arbitrary drives that don't support commit.  The commit code
already checks bs->backing_hd today.

I agree that commit all should mean "all COW backends".

Stefan