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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Video for Linux]     [Yosemite News]     [Linux S/390]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux