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@xxxxxxxxxx> > > 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@xxxxxxx> 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@xxxxxxxxxx> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html