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

[Jens Axboe <axboe@kernel.dk>]

On 10/18/21 6:42 PM, Michael Schmitz wrote:
> Hi Jens,
> 
> On Tue, Oct 19, 2021 at 12:40 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> '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.
> 
> OK, that's indeed a no-op for our floppy driver, which can queue
> exactly one request.

Right, and the only reason the depth is set to 2 is to allow one for
merging purposes.

>> 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.
> 
> I'm not averse to using bd->last to close down only after the last
> request in a sequence if it can be done safely (i.e. the requests that
> had been rejected are then promptly requeued). But complexity is the
> enemy of maintainability, so the nice and easy fix should be enough.

With just 2 requests, any sequence is going to be pretty limited :-).
My recommendation would be to just ignore bd->last and treat any
request as a standalone unit. Should make for easier code too, and
you won't have two different cases to handle.

> I'll respin and send another version shortly.

Great, thanks.

-- 
Jens Axboe