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

[Jan Kiszka]

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

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux