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

[Markus Armbruster]

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?