Re: [PATCH 3/3] fnic: check for started requests in fnic_wq_copy_cleanup_handler()

[Ming Lei <ming.lei@redhat.com>]

On Tue, May 04, 2021 at 09:49:12AM +0200, Martin Wilck wrote:
> On Thu, 2021-04-29 at 19:28 +0200, Hannes Reinecke wrote:
> > On 4/29/21 4:34 PM, Ming Lei wrote:
> > > On Thu, Apr 29, 2021 at 02:25:17PM +0200, Hannes Reinecke wrote:
> > > > fnic_wq_copy_cleanup_handler() is using scsi_host_find_tag() to
> > > > map id to a scsi command. However, as per discussion on the
> > > > mailinglist
> > > > scsi_host_find_tag() might return a non-started request, so we
> > > > need
> > > > to check the returned command with blk_mq_request_started() to
> > > > avoid
> > > > the function tripping over a non-initialized command.
> > > > 
> > > > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > > > ---
> > > >   drivers/scsi/fnic/fnic_scsi.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/scsi/fnic/fnic_scsi.c
> > > > b/drivers/scsi/fnic/fnic_scsi.c
> > > > index 762cc8bd2653..b9fd3d87416b 100644
> > > > --- a/drivers/scsi/fnic/fnic_scsi.c
> > > > +++ b/drivers/scsi/fnic/fnic_scsi.c
> > > > @@ -1466,7 +1466,7 @@ void fnic_wq_copy_cleanup_handler(struct
> > > > vnic_wq_copy *wq,
> > > >                 return;
> > > >   
> > > >         sc = scsi_host_find_tag(fnic->lport->host, id);
> > > > -       if (!sc)
> > > > +       if (!sc || !blk_mq_request_started(sc->request))
> > > >                 return;
> > > 
> > > scsi_host_find_tag() has covered blk_mq_request_started check
> > > already, so
> > > this patch isn't necessary.
> > > 
> > Right. So drop it, then.
> 
> While you are at this, could you please re-consider e73a5e8e8003
> ("scsi: core: Only return started requests from scsi_host_find_tag()")
> in general?
> 
> I have come to think that commit is incorrect. It was created as an
> attempt to fix the observed fnic crashes, but it was ineffective. The
> crashes were eventually fixed by patch 2/3 of this series. 
> 
> IMO scsi_host_find_tag() should return a request if there is one,
> regardless whether or not it's started, and leave the decision to
> ignore pending requests to the caller, like it used to be until v5.8.

Can you share the cases in which SCSI needs to deal with non in-flight
requests via scsi_host_find_tag()? which is supposed to be used for retrieving
request via one active tag in scsi io completion path.


Thanks,
Ming