Re: [PATCH] 7/7 fix single_lun code for per-scsi_device queue_lock

[Patrick Mansfield <patmans@us.ibm.com>]

On Tue, Mar 25, 2003 at 04:23:41PM -0500, Luben Tuikov wrote:
> Patrick Mansfield wrote:
> > Fix single_lun code for per-scsi_device queue_lock
> > 
> > diff -purN -X /home/patman/dontdiff lksplit-25/drivers/scsi/scsi.h sl_lksplit-25/drivers/scsi/scsi.h
> > --- lksplit-25/drivers/scsi/scsi.h	Mon Mar 24 12:15:10 2003
> > +++ sl_lksplit-25/drivers/scsi/scsi.h	Mon Mar 24 12:15:26 2003
> > @@ -531,6 +531,15 @@ extern struct list_head scsi_dev_info_li
> >  extern int scsi_dev_info_list_add_str(char *);
> >  
> >  /*
> > + * scsi_target: representation of a scsi target, for now, this is only
> > + * used for single_lun devices.
> > + */
> > +struct scsi_target {
> > +	unsigned int starget_busy;
> > +	unsigned int starget_refcnt;
> > +};
> 
> Yes ok, but let's do it right.
> First, I abhore all this letter playing to devise
> names like: starget_busy and starget_refcnt.

> starget_busy could be an atomic_t which is equal
> to the sum of all LU's pending_q_counts, i.e.
> it better, else we're losing commands, and also
> with a different name (more descriptive).
> Something like active_cmds, or active_cmd_count,
> but NOT ``..._cnt''!
> 
> Also, moving all the necessary variables into scsi_target, like
> say single_lun to be renamed single_lu.
> 
> Also having a list of devices:
> 
> 	struct list_head lu_list;
> 
> etc, etc.

> > +	struct scsi_target      *sdev_target;   /* used only for single_lun */
> 
> 
> Should probably be used for all devices.  Let's do it right.

I would like to have a scsi_target that always shows up, I don't have time
now to code it. So for now the klugey single_lun code.

> > +	spin_lock_irqsave(current_sdev->request_queue->queue_lock, flags2);
> > +	spin_lock_irqsave(current_sdev->host->host_lock, flags);
> 
> No, you do NOT need flags and flags2.  Look at other kernel code
> to see how this is done.  scsi_put_command() gives
> an example of doing double locks and irq state -- it's a nice one.

OK

> you MUST absolutely mention this somewhere with boldface red
> letters comment that the queue lock is always obtained FIRST
> and the host lock SECOND (and, trivially, released in opposite
> order).

OK will add comments, though undecided on whether to have both a
target lock and host_lock, or just go with host_lock (per comments sent
reply to James).

> > +	__blk_run_queue(current_sdev->request_queue);
> > +	spin_unlock_irqrestore(current_sdev->request_queue->queue_lock, flags2);
> 
> Now if you make device_busy an atomic_t type, then you DO not
> need these double locking acrobatics.

We should defer such changes until we clean up IO completion, per other
email comments.

> > +	if (sdev->single_lun) {
> > +		spin_lock_irqsave(sdev->host->host_lock, flags);
> > +		sdev->sdev_target->starget_refcnt--;
> > +		if (sdev->sdev_target->starget_refcnt == 0)
> > +			kfree(sdev->sdev_target);
> 
> 
> Have you seen ``atomic_dec_and_test()''?

OK

-- Patrick Mansfield