Messages in this thread | | | Date | Mon, 7 Nov 2022 15:34:31 +0100 | From | Hannes Reinecke <> | Subject | Re: [PATCH RFC v3 2/7] ata: libata-scsi: Add ata_internal_queuecommand() |
| |
On 11/7/22 14:29, Damien Le Moal wrote: > On 11/7/22 19:12, Hannes Reinecke wrote: >> On 11/2/22 12:25, Damien Le Moal wrote: >>> On 11/2/22 20:12, Hannes Reinecke wrote: >>>> On 11/2/22 11:07, Damien Le Moal wrote: >>>>> On 11/2/22 18:52, John Garry wrote: >>>>>> Hi Damien, >>>>>> >>>> [ .. ] >> So we only need to find a way of 're-using' that tag, then we won't have >>>> to set aside a reserved tag and everything would be dandy... >>> >>> I tried that. It is very ugly... Problem is that integration with EH in >>> case a real NCQ error happens when all that read-log-complete dance is >>> happening is hard. And don't get me started with the need to save/restore >>> the scsi command context of the command we are reusing the tag from. >>> >>> And given that the code is changing to use regular submission path for >>> internal commands, right now, we need a reserved tag. Or a way to "borrow" >>> the tag from a request that we need to check. Which means we need some >>> additional api to not always try to allocate a tag. >>> >>>> >>>> Maybe we can stop processing when we receive an error (should be doing >>>> that anyway as otherwise the log might be overwritten), then we should >>>> be having a pretty good chance of getting that tag. >>> >>> Hmmm.... that would be no better than using EH which does stop processing >>> until the internal house keeping is done. >>> >>>> Or, precisely, getting _any_ tag as at least one tag is free at that point. >>>> Hmm? >>> >>> See above. Not free, but usable as far as the device is concerned since we >>> have at least on command we need to check completed at the device level >>> (but not yet completed from scsi/block layer point of view). >>> >> So, having had an entire weekend pondering this issue why don't we >> allocate an _additional_ set of requests? >> After all, we had been very generous with allocating queues and requests >> (what with us doing a full provisioning of the requests for all queues >> already for the non-shared tag case). >> >> Idea would be to keep the single tag bitmap, but add eg a new rq state >> MQ_RQ_ERROR. Once that flag is set we'll fetch the error request instead >> of the normal one: >> >> @@ -761,6 +763,8 @@ static inline struct request >> *blk_mq_tag_to_rq(struct blk_mq_tags *tags, >> { >> if (tag < tags->nr_tags) { >> prefetch(tags->rqs[tag]); >> + if (unlikely(blk_mq_request_error(tags->rqs[tag]))) >> + return tags->error_rqs[tag]; >> return tags->rqs[tag]; >> } >> >> and, of course, we would need to provision the error request first. >> >> Rationale here is that this will be primarily for devices with a low >> number of tags, so doubling the number of request isn't much of an >> overhead (as we'll be doing it essentially anyway in the error case as >> we'll have to save the original request _somewhere_), and that it would >> remove quite some cruft from the subsystem; look at SCSI EH trying to >> store the original request contents and then after EH restoring them again. > > Interesting idea. I like it. It is essentially a set of reserved requests > without reserved tags: the tag to use for these requests would be provided > "manually" by the user. Right ? > Yes. Upon failure one would be calling something like 'blk_mq_get_error_rq(rq)', which would set the error flag in the original request, fetch the matching request from ->static_rqs, and return that one.
Just figured, we could simply enlarge 'static_rqs' to have double the size; then we can easily get the appropriate request from 'static_rqs' by just adding the queue size. Making things even easier ...
> That should allow simplifying any processing that needs to reuse a tag, > and currently its request. That is, CDL, but also usb-scsi, scsi EH and > the few scsi LLDs using scsi_eh_prep_cmnd()+scsi_eh_restore_cmnd(). > Ideally, these 2 functions could go away too. > Which was precisely the idea. We have quite some drivers/infrastructure which already require a similar functionality, and basically all of them cover devices with a really low tag space (32/31 in the libata NCQ case, 16 in the SCSI TCQ case, or even _1_ in the SCSI parallel case :-) So a request duplication wouldn't matter _that_ much here.
Drivers with a higher queue depth typically can do 'real' TMFs, where you need to allocate a new tag anyway, and so the whole operation doesn't apply here.
Cheers,
Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
| |