Re: [Qemu-devel] [PATCH v4 10/12] fdc: rework pick_geometry

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6727 bytes --]

On 01/21/2016 01:14 PM, John Snow wrote:
> 
> 
> On 01/20/2016 06:45 PM, Eric Blake wrote:
>> On 01/19/2016 11:51 PM, John Snow wrote:
>>> This one is the crazy one.
>>>
>>> fd_revalidate currently uses pick_geometry to tell if the diskette
>>> geometry has changed upon an eject/insert event, but it won't allow us
>>> to insert a 1.44MB diskette into a 2.88MB drive. This is inflexible.
>>>
>>> The new algorithm applies a new heuristic to guessing disk geometries
>>> that allows us to switch diskette types as long as the physical size
>>> matches before falling back to the old heuristic.
>>>
>>> The old one is roughly:
>>>  - If the size (sectors) and type matches, choose it.
>>>  - Fall back to the first geometry that matched our type.
>>>
> 
> For sake of review, we'll call these choices (A) and (B).
> 
>>> The new one is:
>>>  - If the size (sectors) and type matches, choose it.
>>>  - If the size (sectors) and physical size match, choose it.
>>>  - If the size (sectors) matches at all, choose it begrudgingly.
>>
>> This is the one I worry about; details below.
>>
>>>  - Fall back to the first geometry that matched our type.
>>>
> 
> And these choices will be (1) through (4) as they are annotated in the
> comment below in this way as well.
> 
> (1) is identical to (A), and (4) is identical to (B).

Agreed.

> That leaves (2) and (3) are new behaviors.
> 
> There is strong rationale for (2).
> (3) was invented as a last-ditch effort before (4), see below!
> 

>>
>> Thus, I think you still have a bug. :(
>>
> 
> It's definitely new behavior. In either case, I think we can agree that
> QEMU is choosing a garbage format as a last-ditch effort to see if
> something works.
> 
> The old behavior of "Choose a wrong geometry of the right type as a
> last-chance effort" was something I thought that maybe I could "improve"
> by doing:
> 
> "Choose a wrong geometry of the right type as a last-chance effort,
> unless we found something that is the correct number of sectors, in
> which case maybe use that as a backup strategy because it might work
> better."

I don't think the geometry for a 120 row is going to work right for a
guest expecting 144/288 behavior.

> 
> It is decidedly "new" behavior, but it is similarly to the old strategy
> a recourse action when faced with the absence of a more precise match
> (right size and type (1) or right size and correct physical size (2))
> 
> The thought was basically: "I bet the floppy drive code would cope
> better by setting a geometry that is at the very least the right number
> of sectors over one that's clearly wrong."

I think the point of this function is:

If the user told us a disk type and gave us an image with an exact
number of sectors, use the geometry associated with that size.  Behavior
(2) refines this to further pick sizes that are compatible with the
current disk type (with a 288 drive, the exact geometry for 2880 sectors
visiting a 144 disk is probably better than the behavior (B) choice for
the first 288 disk).

If the user did not tell us a disk type, favor a geometry that matches
the number of sectors.  If the size can occur from more than one disk
type, we used to favor the disk type that occurred first in the list
(2880 sectors maps to 3.5", not 5.25"; while 720 sectors favors 5.25");
due to behavior (2) the new code may instead favor the disk size that
matches the default fallback of "auto" (if auto falls back to 144 or
288, 720 sectors will now select that size rather than 5.25").

If the user told us a disk type but we don't have a sector match, then
pick the first geometry associated with that disk type (hopefully the
host image is smaller than that choice, and we pad out the host file to
pretend that it matches the most common use of that disk type from the
guest's view).

Finally, the user told us nothing and we have no size heuristic to fall
back on, so pick a (hopefully sane) default of the first table entry
(old behavior) or the "auto" fallback (new behavior).

> 
>>>              }
>>> -            if (first_match == -1) {
>>> -                first_match = i;
>>> +        } else if (type_match == -1) {
>>
>> Here, we are visiting a row where sectors doesn't match.
>>
> 
> Yes, the old "hail mary" behavior present from 2003-2016, what I
> annotated as type (B) in my reply above. Now documented as behavior (4).
> 
>>> +            if ((parse->drive == drv->drive) ||
>>> +                (magic && (parse->drive == get_fallback_drive_type(drv)))) {
>>> +                /* (4) type matches, or type matches the autodetect default if
>>> +                 *     we are using the autodetect mechanism. */
>>> +                type_match = i;
>>
>> In which case, if the user told us a size, or the default for the "auto"
>> size matches, we set 'type_match' (in effect, picking the first
>> 144/288/120 row that matches their explicit or default type).
>>
> 
> Right, this is the old behavior. "We couldn't find the right geometry,
> so we're just going to pick one that is of the type you specified and
> hope you know what you're doing."
> 
> The nugget of new behavior here is that if the user did NOT specify a
> type, and we STILL managed to not find a precise matching geometry,
> we'll choose the first geometry that belongs to the fallback type.

Which seems like a good change (if you don't tell us a disk type, then
we'll default to giving you a 144/288 drive UNLESS hueristics prove you
were probably trying to open a 120 format based on sector size).

>> I think you can salvage the patch, though.  The condition for (3) is
>> currently 'else [if (1)]'.  But what if it is instead 'else if

I meant that as 'if (true)', not 'if (condition identical to (1) in the
discussion above)'.

>> (drive_size(drv->drive) == FDRIVE_SIZE_UKNOWN)'?  Then we are only
>> looking for a sector match on rows where we don't know what drive the
>> user wants, and leaving 'size_match' unchanged on rows that differ in
>> disk size from an explicit user's request.
>>
> 
> Should be unnecessary -- (1) should handle nb_sectors match adequately
> when the user did not specify a drive type.

After (re-)reading the use of 'magic', I concur.

> 
> If (3) is undesired, it can be scrapped outright.

I think that's the best approach then - drop condition (3) outright, and
only do an early exit (1), set 'match' (2), or set 'type_match' (4).

Tricky patch, but I think with that change, I'll be on board for your v5
patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]