Re: [PATCH RFC] block - ataflop.c: fix breakage introduced at blk-mq refactoring

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Jens,

On 19/10/21 11:30, Jens Axboe wrote:
> Was going to ask if this driver was used by anyone, since it's taken 3

Can't honestly say - I'm not following any other user forum than 
linux-m68k (and that's not really a user forum either).

> years for the breakage to be spotted... In all fairness, it was pretty
> horribly broken before the change too (like waiting in request_fn, under
> a lock).

In all fairness, it was a pretty broken design, but it did at least 
work. I concede that it was unmaintainable in its old form, and still 
largely is, just surprised that I didn't see a call for testing on 
linux-m68k, considering the committer realized it probably wouldn't work.

> So I'm curious, are you actively using it, or was it just an exercise in
> curiosity?

I've used it quite a bit in the past, but not for many years. For legacy 
hardware, floppies are often the only way to get data on or off the 
device, and I consider this driver an important fallback option should 
my network adapter (which is a pretty horrible kludge to use an old ISA 
NE2000 card on the ROM cartridge port) fail.

But then, any use of this legacy hardware is an exercise in curiosity 
mostly.

>
>> Testing this change, I've only ever seen single sector requests with the
>> 'last' flag set. If there is a way to send requests to the driver
>> without that flag set, I'd appreciate a hint. As it now stands,
>> the driver won't release the ST-DMA lock on requests that don't have
>> this flag set, but won't accept further requests because the attempt
>> to acquire the already-held lock once more will fail.
>
> 'last' is set if it's the last of a sequence of ->queue_rq() calls. If
> you just do sync IO, then last is always set, as there is no sequence.
> It's not hard to generate sequences, but on a floppy with basically no
> queue depth the most you'd ever get is 2. You could try and set:
>
> /sys/block/<dev>/queue/max_sectors_kb
>
> to 4 for example, and then do something that generates a larger than 4k
> write or read. Ideally that should give you more than 1.

Thanks, tried that - that does indeed cause multiple requests queued to 
the driver (which rejects them promptly).

Now fails because ataflop_commit_rqs() unconditionally calls 
finish_fdc() right after the first request started processing- and 
promptly wipes it again.

What is the purpose of .commit_rqs? The PC legacy floppy driver doesn't 
use it ...

Cheers,

	Michael