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

[John Snow]

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

>> Signed-off-by: John Snow <address@hidden>
>> ---
> 
>> -    int i, first_match, match;
>> +    int i;
>> +    int match, size_match, type_match;
>> +    bool magic = drv->drive == FLOPPY_DRIVE_TYPE_AUTO;
>>  
>>      /* We can only pick a geometry if we have a diskette. */
>>      if ((!drv->media_inserted) || (drv->drive == FLOPPY_DRIVE_TYPE_NONE)) {
>>          return -1;
>>      }
>>  
>> +    /* We need to determine the likely geometry of the inserted medium.
>> +     * In order of preference, we look for:
>> +     * (1) The same drive type and number of sectors,
>> +     * (2) The same diskette size and number of sectors,
>> +     * (3) The same number of sectors,
>> +     * (4) The same drive type.
>> +     *
>> +     * In all cases, matches that occur higher in the drive table will take
>> +     * precedence over matches that occur later in the table.
> 
> Yay - comments!  Makes it somewhat easier to follow.
> 
>> +     */
>>      blk_get_geometry(blk, &nb_sectors);
>> -    match = -1;
>> -    first_match = -1;
>> +    match = size_match = type_match = -1;
>>      for (i = 0; ; i++) {
>>          parse = &fd_formats[i];
>>          if (parse->drive == FLOPPY_DRIVE_TYPE_NONE) {
>>              break;
>>          }
>> -        if (drv->drive == parse->drive ||
>> -            drv->drive == FLOPPY_DRIVE_TYPE_AUTO) {
>> -            size = (parse->max_head + 1) * parse->max_track *
>> -                parse->last_sect;
>> -            if (nb_sectors == size) {
>> -                match = i;
>> -                break;
>> +        size = (parse->max_head + 1) * parse->max_track * parse->last_sect;
>> +        if (nb_sectors == size) {
> 
> To make sure I understand this:
> 
> The first three conditions are reached only when visiting a row where
> nb_sectors matches.
> 

Yes.

>> +            if (magic || parse->drive == drv->drive) {
>> +                /* (1) perfect match */
>> +                goto out;
> 
> The conditional means we get here only if the user specified 'magic'
> (aka the new "auto" behavior of picking the first drive that works) or
> an actual size (144/288/120).  Since the sectors match, we are set; and
> this matches the old behavior (either drive type and sectors match, or
> drive type was unspecified and sectors match).
> 

Yes. This is the old (A), identical to what is now documented as (1).

>> +            } else if (drive_size(parse->drive) == drive_size(drv->drive)) {
>> +                /* (2) physical size match */
>> +                match = (match == -1) ? i : match;
> 
> Here, drive_size(parse->drive) will never be FDRIVE_SIZE_UNKNOWN, but
> drive_size(drv->drive) depends on what the user told us.  If they didn't
> tell us what disk size to target, including if they asked for "auto",
> this rule will never match.  If they DID tell us a disk size
> (144/288/120), then this favors their disk size.  More concretely, if
> they asked for 2880 sectors and a 288 disk, the old algorithm would fail
> (no 288 entry has 2880 sectors), but now succeeds (the 144 entry with
> 2880 sectors has the same 3.5" disk size).  This is a good bug fix.
> 

Yes. This is new behavior, documented as (2).

>> +            } else {
>> +                /* (3) nsectors match only */
> 
> nb_sectors?
> 

Yes, I can update the comment to be less fuzzy.

>> +                size_match = (size_match == -1) ? i : size_match;
> 
> Here, we don't have a drive match.  Either the user did not specify a
> drive type [backwards-compatible behavior - the old loop found the first
> entry with a matching size among every row of the table], or they

The old behavior you are referencing is (A), but that's already taken
care of under (1) above because of the (magic || parse->drive ==
drv->drive) conditional.

If magic is set, the types (or physical sizes) do not need to match,
just the number of sectors -- and the match is so strong, we exit
immediately.

> specified 144/288 while we are visiting entries for 120, or they
> specified 120 while we are visiting entries for 144/288) [new behavior].
> 

New indeed.

> Should I worry about the new behavior?  If the size is one of the four
> ambiguous rows (2880, 3200, 1440, 720), then we are okay - even though
> we set 'size_match' on this iteration, another iteration will also set
> 'match' on the counterpart row, and 'match' takes priority over
> 'size_match'.
> 

You are right to worry. Rationale below.

> BUT what if the user asked for 3360 sectors and a 120 disk?  The old
> code would have set 'first_match' to the 120 15,80,1 row (first row that
> matches the drive type, since the search is limited to just 120 rows and
> there was no size match); but your code looks for 3360 across ALL rows,
> and ends up setting 'size_match' to the 144 21,80,1 row.  And since your
> code never sets 'match' (none of the 120 rows match in size), you end up
> _changing_ the disk format compared to the old code.
> 
> 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."

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

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

>>              }
>>          }
>>      }
>> +
>>      if (match == -1) {
>> -        if (first_match == -1) {
>> -            error_setg(&error_abort, "No candidate geometries present in 
>> table "
>> -                       " for floppy drive type '%s'",
>> -                       FloppyDriveType_lookup[drv->drive]);
>> -        } else {
>> -            match = first_match;
>> -        }
>> -        parse = &fd_formats[match];
>> +        match = (size_match != -1) ? size_match : type_match;
> 
> Only reached for (2), (3), and (4).  If (2) fired, 'match' is set and we
> take that.  Otherwise if (3) fired, 'size_match' is set and we take that
> (but see my claim that (3) can be buggy).  Otherwise, (4) better have
> fired, because if not...
> 

Should be no way for it not to!

>> +    }
>> +
>> +    if (match == -1) {
>> +        error_setg(&error_abort, "No candidate geometries present in table "
>> +                   " for floppy drive type '%s'",
>> +                   FloppyDriveType_lookup[drv->drive]);
> 
> ...we abort.  The abort looks sane.
> 
>>      }
>>  
>> +    parse = &(fd_formats[match]);
>> +
>> + out:
> 
> And the label is what lets us handle (1) as higher priority than
> anything else.
> 
>>      if (parse->max_head == 0) {
>>          drv->flags &= ~FDISK_DBL_SIDES;
>>      } else {
>>
> 
> 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
> (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.

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

--js