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

Jens Axboe
2021-10-18T23:40:22Z

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