Re: [PATCH RFC v3 2/7] ata: libata-scsi: Add ata_internal_queuecommand()

[Hannes Reinecke]

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@xxxxxxx			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer