Re: question about drivers/scsi/scsi_transport_fc.c

[Christof Schmitt <christof.schmitt@de.ibm.com>]

On Thu, Sep 02, 2010 at 03:39:39PM +0200, Hannes Reinecke wrote:
> Christof Schmitt wrote:
> > On Sat, Aug 28, 2010 at 07:11:12PM +0200, Julia Lawall wrote:
> >> The function fc_bsg_goose_queue in the file drivers/scsi/scsi_transport_fc.c
> >> contains the following code:
> >>
> >>         flagset = test_bit(QUEUE_FLAG_REENTER, &rport->rqst_q->queue_flags) &&
> >>                   !test_bit(QUEUE_FLAG_REENTER, &rport->rqst_q->queue_flags);
> >>
> >> I have the impression that this is always 0.  QUEUE_FLAG_REENTER is 
> >> defined with quite a lot of other constants, so I don't really have a 
> >> guess as to what was intended.
> > 
> > I just came across the code in scsi_run_queue in
> > drivers/scsi/scsi_lib.c. It looks like the check of QUEUE_FLAG_REENTER
> > in fc_bsg_goose_queue is modeled after the check in scsi_run_queue.
> > 
> > The check in scsi_run_queue has been introduced with this commit,
> > maybe this helps understanding the code:
> > 
> > commit 04846f25920d4b05d6040c531cc601049260db52
> > Author: Andreas Herrmann <aherrman@de.ibm.com>
> > Date:   Wed Aug 9 17:31:16 2006 +0200
> > 
> >     [SCSI] limit recursion when flushing shost->starved_list
> > 
> Then it's still wrong. The above commit has:
> 
> +               if (test_bit(QUEUE_FLAG_REENTER, &q->queue_flags) &&
> +                   !test_and_set_bit(QUEUE_FLAG_REENTER,
> +                                     &sdev->request_queue->queue_flags)) {
> 
> Which is slightly different than the above. Looks like a copy and past error.

I missed one commit that changed the locking and the flag handling:

commit 75ad23bc0fcb4f992a5d06982bf0857ab1738e9e
Author: Nick Piggin <npiggin@suse.de>
Date:   Tue Apr 29 14:48:33 2008 +0200

    block: make queue flags non-atomic

This one introduced
+               spin_lock(sdev->request_queue->queue_lock);
+               flagset = test_bit(QUEUE_FLAG_REENTER, &q->queue_flags) &&
+                               !test_bit(QUEUE_FLAG_REENTER,
+				&sdev->request_queue->queue_flags);
which does not make sense to me, maybe it would be enough to test the
bit?

> It probably should read
> 
>          flagset = test_bit(QUEUE_FLAG_REENTER, &rport->rqst_q->queue_flags) &&
>                    !test_and_set_bit(QUEUE_FLAG_REENTER, &rport->rqst_q->queue_flags);
> 
> This was introduced by
> 
> commit 9e4f5e29610162fd426366f3b29e3cc6e575b858
> Author: James Smart <James.Smart@Emulex.Com>
> Date:   Thu Mar 26 13:33:19 2009 -0400
> 
>     [SCSI] FC Pass Thru support
> 
> so we should be blaming^Wasking him.

Or could the whole handling of the QUEUE_FLAG_REENTER simply be
removed from the SCSI and FC code? The flag is set before calling
__blk_run_queue, but this function already sets the flag internally to
avoid recursion.

--
Christof