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

[Jens Axboe <axboe@kernel.dk>]

On 10/18/21 5:35 PM, Michael Schmitz wrote:
> 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.

I don't remember the details on that front, it's usually very difficult
to get people to test this kind of change, unfortunately. But thanks for
tackling it now!

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

OK, that's good enough then. Was mostly just curious if was actually
being used.

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

You only need to care about bd->last if you have something in the driver
that can make it cheaper to commit more than one request. An example is
a driver that fills in requests, and then has an operation to ring the
submission doorbell to flush them out. The latter is what ->commit_rqs
is for.

For a floppy driver, just ignore bd->last and don't implement
commit_rqs, I don't think we're squeezing a lot of extra efficiency out
of it through that! Think many hundreds of thousands of IOPS or millions
of IOPS, not a handful of IOPS or less.

-- 
Jens Axboe