On Mon, Feb 25, 2013 at 03:30:34PM +0100, Markus Armbruster wrote:
> Jan Kiszka <jan.kis...@siemens.com> 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.
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.
Jeff
