scsi scan bug when peripheral qualifier of 3 is returned

scsi scan bug when peripheral qualifier of 3 is returned

[Mike Christie]

It looks like in previous kernels scsi_scan_target would pass 
scsi_sequential_lun_scan BLIST_SPARSELUN.

         } else if (res == SCSI_SCAN_TARGET_PRESENT) {
                 /*
                  * There's a target here, but lun 0 is offline so we
                  * can't use the report_lun scan.  Fall back to a
                  * sequential lun scan with a bflags of SPARSELUN and
                  * a default scsi level of SCSI_2
                  */
                 scsi_sequential_lun_scan(starget, BLIST_SPARSELUN,


And in the current kernel we pass it bflags.

                         /*
                          * The REPORT LUN did not scan the target,
                          * do a sequential scan.
                          */
                         scsi_sequential_lun_scan(starget, bflags,


But for SCSI_SCAN_TARGET_PRESENT bflags is not set. Is the correct fix 
to move where bflagsp gets set in scsi_probe_and_add_lun so that it gets 
set for the SCSI_SCAN_TARGET_PRESENT case, or should __scsi_scan_target 
be passing scsi_sequential_lun_scan and possibly scsi_report_lun_scan 
some default bflags values?

Re: scsi scan bug when peripheral qualifier of 3 is returned

[Patrick Mansfield]

On Wed, Jan 04, 2006 at 01:11:06AM -0600, Mike Christie wrote:

> But for SCSI_SCAN_TARGET_PRESENT bflags is not set. Is the correct fix 
> to move where bflagsp gets set in scsi_probe_and_add_lun so that it gets 
> set for the SCSI_SCAN_TARGET_PRESENT case, or should __scsi_scan_target 
> be passing scsi_sequential_lun_scan and possibly scsi_report_lun_scan 
> some default bflags values?

It looks OK to me as-is, since bflags is also passed to and set in
scsi_probe_lun(), right?

-- Patrick Mansfield

Re: scsi scan bug when peripheral qualifier of 3 is returned

[Mike Christie]

Patrick Mansfield wrote:
> On Wed, Jan 04, 2006 at 01:11:06AM -0600, Mike Christie wrote:
> 
> 
>>But for SCSI_SCAN_TARGET_PRESENT bflags is not set. Is the correct fix 
>>to move where bflagsp gets set in scsi_probe_and_add_lun so that it gets 
>>set for the SCSI_SCAN_TARGET_PRESENT case, or should __scsi_scan_target 
>>be passing scsi_sequential_lun_scan and possibly scsi_report_lun_scan 
>>some default bflags values?
> 
> 
> It looks OK to me as-is, since bflags is also passed to and set in
> scsi_probe_lun(), right?
> 

A blagfs variable gets set but it is not the same one passed into 
scsi_probe_and_add_lun.

static int scsi_probe_and_add_lun(struct scsi_target *starget,
                                   uint lun, int *bflagsp,


{
         struct scsi_device *sdev;
         unsigned char *result;
         int bflags,


scsi_probe_and_add_lun gets a *bflagsp passed to it as a function arg, 
but then also decalres a bflags variable itself. It then passes 
scsi_probe_lun() the bflags it declared and does this

         res = scsi_add_lun(sdev, result, &bflags);
         if (res == SCSI_SCAN_LUN_PRESENT) {
                 if (bflags & BLIST_KEY) {
                         sdev->lockable = 0;
                         scsi_unlock_floptical(sdev, result);
                 }
                 if (bflagsp)
                         *bflagsp = bflags;
         }

so *bflagsp pointer only gets set if SCSI_SCAN_LUN_PRESENT was returned 
by scsi_add_lun. For SCSI_SCAN_TARGET_PRESENT we do not even get to 
scsi_add_lun, so for this case *bflagsp never gets set and 
__scsi_scan_target gets zero. Previously, __scsi_scan_target would just 
pass scsi_sequential_lun_scan the sparse blist flag, but now it passes zero.

Re: scsi scan bug when peripheral qualifier of 3 is returned

[Patrick Mansfield]

On Wed, Jan 04, 2006 at 12:28:03PM -0600, Mike Christie wrote:
> Patrick Mansfield wrote:
> >On Wed, Jan 04, 2006 at 01:11:06AM -0600, Mike Christie wrote:
> >
> >
> >>But for SCSI_SCAN_TARGET_PRESENT bflags is not set. Is the correct fix 
> >>to move where bflagsp gets set in scsi_probe_and_add_lun so that it gets 
> >>set for the SCSI_SCAN_TARGET_PRESENT case, or should __scsi_scan_target 
> >>be passing scsi_sequential_lun_scan and possibly scsi_report_lun_scan 
> >>some default bflags values?
> >
> >
> >It looks OK to me as-is, since bflags is also passed to and set in
> >scsi_probe_lun(), right?
> >
> 
> A blagfs variable gets set but it is not the same one passed into 
> scsi_probe_and_add_lun.

Oh ... good catch there.

> static int scsi_probe_and_add_lun(struct scsi_target *starget,
>                                   uint lun, int *bflagsp,
> 
> 
> {
>         struct scsi_device *sdev;
>         unsigned char *result;
>         int bflags,
> 
> 
> scsi_probe_and_add_lun gets a *bflagsp passed to it as a function arg, 
> but then also decalres a bflags variable itself. It then passes 
> scsi_probe_lun() the bflags it declared and does this
> 
>         res = scsi_add_lun(sdev, result, &bflags);
>         if (res == SCSI_SCAN_LUN_PRESENT) {
>                 if (bflags & BLIST_KEY) {
>                         sdev->lockable = 0;
>                         scsi_unlock_floptical(sdev, result);
>                 }
>                 if (bflagsp)
>                         *bflagsp = bflags;
>         }
> 
> so *bflagsp pointer only gets set if SCSI_SCAN_LUN_PRESENT was returned 
> by scsi_add_lun. For SCSI_SCAN_TARGET_PRESENT we do not even get to 
> scsi_add_lun, so for this case *bflagsp never gets set and 
> __scsi_scan_target gets zero. Previously, __scsi_scan_target would just 
> pass scsi_sequential_lun_scan the sparse blist flag, but now it passes zero.

So yes we should always set *bflagsp. i.e.:

--- linux-2.6.15/drivers/scsi/orig-scsi_scan.c	2006-01-02 21:52:12.000000000 -0800
+++ linux-2.6.15/drivers/scsi/scsi_scan.c	2006-01-04 10:58:36.000000000 -0800
@@ -891,13 +891,13 @@ static int scsi_probe_and_add_lun(struct
 	}
 
 	res = scsi_add_lun(sdev, result, &bflags);
+	if (bflagsp)
+		*bflagsp = bflags;
 	if (res == SCSI_SCAN_LUN_PRESENT) {
 		if (bflags & BLIST_KEY) {
 			sdev->lockable = 0;
 			scsi_unlock_floptical(sdev, result);
 		}
-		if (bflagsp)
-			*bflagsp = bflags;
 	}
 
  out_free_result:


Also ... it looks like we should just pass bflags not &bflags to
scsi_add_lun(), since scsi_add_lun() does not modifies bflags.

-- Patrick Mansfield

Re: scsi scan bug when peripheral qualifier of 3 is returned

[Mike Christie]

Patrick Mansfield wrote:
> So yes we should always set *bflagsp. i.e.:
> 
> --- linux-2.6.15/drivers/scsi/orig-scsi_scan.c	2006-01-02 21:52:12.000000000 -0800
> +++ linux-2.6.15/drivers/scsi/scsi_scan.c	2006-01-04 10:58:36.000000000 -0800
> @@ -891,13 +891,13 @@ static int scsi_probe_and_add_lun(struct
>  	}
>  
>  	res = scsi_add_lun(sdev, result, &bflags);
> +	if (bflagsp)
> +		*bflagsp = bflags;


For SCSI_SCAN_TARGET_PRESENT, we never get this far. We bail out a 
couple of lines above in the check for PQ=3. Also, for some boxes like 
Clarions we have this in devinfo:

         {"DGC", "RAID", NULL, BLIST_SPARSELUN}, /* Dell PV 650F, 
storage on LUN 0 */
         {"DGC", "DISK", NULL, BLIST_SPARSELUN},


But LUN0 shows up as DGC/LUNZ. When we used to hardcode BLIST_SPARSELUN 
as a argument to scsi_sequential_lun_scan, we could find a least 8 
devices but now we hit this in scsi_sequential_lun_scan:

        /*
          * If not sparse lun and no device attached at LUN 0 do not scan
          * any further.
          */
         if (!sparse_lun && (lun0_res != SCSI_SCAN_LUN_PRESENT))
                 return;


Does IBM's fastt/DS or other boxes have this problem too? For Clarions 
do I just have to add a DGC/LUNZ entry to the devinfo list?

Re: scsi scan bug when peripheral qualifier of 3 is returned

[Patrick Mansfield]

On Wed, Jan 04, 2006 at 02:31:30PM -0600, Mike Christie wrote:
> Patrick Mansfield wrote:
> >So yes we should always set *bflagsp. i.e.:
> >
> >--- linux-2.6.15/drivers/scsi/orig-scsi_scan.c	2006-01-02 
> >21:52:12.000000000 -0800
> >+++ linux-2.6.15/drivers/scsi/scsi_scan.c	2006-01-04 
> >10:58:36.000000000 -0800
> >@@ -891,13 +891,13 @@ static int scsi_probe_and_add_lun(struct
> > 	}
> > 
> > 	res = scsi_add_lun(sdev, result, &bflags);
> >+	if (bflagsp)
> >+		*bflagsp = bflags;
> 
> 
> For SCSI_SCAN_TARGET_PRESENT, we never get this far. We bail out a 
> couple of lines above in the check for PQ=3. Also, for some boxes like 

OK :-/

Then like this?

--- linux-2.6.15/drivers/scsi/orig-scsi_scan.c	2006-01-02 21:52:12.000000000 -0800
+++ linux-2.6.15/drivers/scsi/scsi_scan.c	2006-01-04 13:03:27.000000000 -0800
@@ -868,6 +868,8 @@ static int scsi_probe_and_add_lun(struct
 
 	if (scsi_probe_lun(sdev, result, result_len, &bflags))
 		goto out_free_result;
+	if (bflagsp)
+		*bflagsp = bflags;
 
 	/*
 	 * result contains valid SCSI INQUIRY data.
@@ -896,8 +898,6 @@ static int scsi_probe_and_add_lun(struct
 			sdev->lockable = 0;
 			scsi_unlock_floptical(sdev, result);
 		}
-		if (bflagsp)
-			*bflagsp = bflags;
 	}
 
  out_free_result:


> Clarions we have this in devinfo:
> 
>         {"DGC", "RAID", NULL, BLIST_SPARSELUN}, /* Dell PV 650F, 
> storage on LUN 0 */
>         {"DGC", "DISK", NULL, BLIST_SPARSELUN},
> 
> 
> But LUN0 shows up as DGC/LUNZ. When we used to hardcode BLIST_SPARSELUN 
> as a argument to scsi_sequential_lun_scan, we could find a least 8 
> devices but now we hit this in scsi_sequential_lun_scan:
> 
>        /*
>          * If not sparse lun and no device attached at LUN 0 do not scan
>          * any further.
>          */
>         if (!sparse_lun && (lun0_res != SCSI_SCAN_LUN_PRESENT))
>                 return;
> 
> 
> Does IBM's fastt/DS or other boxes have this problem too? 

For fastt, DS and other newer storage, we (likely) always use REPORT LUN
and so would never hit that code.

> For Clarions 
> do I just have to add a DGC/LUNZ entry to the devinfo list?

If they do not support REPORT LUN, yes (AFAIR/CT, my brain is slow ...).

Also, on some storage boxes, you can conditionaly set the scsi level too.
You could try changing that and/or also add a blist BLIST_REPORTLUN2 for
them to devinfo, or just use the sg report lun to test it.

Don't forget you can dynamically add devinfo values via modparam and
/proc/scsi.

-- Patrick Mansfield

Re: scsi scan bug when peripheral qualifier of 3 is returned

[Mike Christie]

Patrick Mansfield wrote:
> Then like this?

Yep, this works for our setup. Thanks.

> 
> --- linux-2.6.15/drivers/scsi/orig-scsi_scan.c	2006-01-02 21:52:12.000000000 -0800
> +++ linux-2.6.15/drivers/scsi/scsi_scan.c	2006-01-04 13:03:27.000000000 -0800
> @@ -868,6 +868,8 @@ static int scsi_probe_and_add_lun(struct
>  
>  	if (scsi_probe_lun(sdev, result, result_len, &bflags))
>  		goto out_free_result;
> +	if (bflagsp)
> +		*bflagsp = bflags;
>  
>  	/*
>  	 * result contains valid SCSI INQUIRY data.
> @@ -896,8 +898,6 @@ static int scsi_probe_and_add_lun(struct
>  			sdev->lockable = 0;
>  			scsi_unlock_floptical(sdev, result);
>  		}
> -		if (bflagsp)
> -			*bflagsp = bflags;
>  	}
>  
>   out_free_result:
> 

.....

> 
> Don't forget you can dynamically add devinfo values via modparam and
> /proc/scsi.
> 

I did not know this. I was looking for a writable proc file for some 
reason. Thanks.