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
