Skip to contentSkip to conversation

allow REPORT LUN scanning even for LUN 0 PQ of 3

PATCH In review Patrick Mansfield 3 participants 8 messages Aug 30, 2005 – Sep 23, 2005 SCSI

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 allow REPORT LUN scanning even for LUN 0 PQ of 3

James -

Can you ack or comment?

Allow REPORT LUN scanning even if LUN 0 reports a peripheral qualifier of 3 (effectively no storage is available on LUN 0).

Similar patch I previously posted:

http://marc.theaimsgroup.com/?l=linux-scsi&m=110297733824960&w=2

There is also Hannes patch, it leaves LUN 0 available for use via user space, for compatibility it likely requires that it stay visible in the future:

http://marc.theaimsgroup.com/?l=linux-scsi&m=111692497200507&w=2

Hannes' patch also causes a lot of extra vSCSI devices to show up on some platforms :)

drivers/scsi/scsi_scan.c +36 -28
diff -uprN -X /home/patman/dontdiff scsi-misc-2.6.git/drivers/scsi/scsi_scan.c lun0-scsi-misc-2.6.git/drivers/scsi/scsi_scan.c
@@ -64,14 +64,14 @@
  * SCSI_SCAN_NO_RESPONSE: no valid response received from the target, this
  * includes allocation or general failures preventing IO from being sent.
  *
- * SCSI_SCAN_TARGET_PRESENT: target responded, but no device is available
+ * SCSI_SCAN_LUN_IGNORED: target responded, but no device is available
  * on the given LUN.
  *
  * SCSI_SCAN_LUN_PRESENT: target responded, and a device is available on a
  * given LUN.
  */
 #define SCSI_SCAN_NO_RESPONSE		0
-#define SCSI_SCAN_TARGET_PRESENT	1
+#define SCSI_SCAN_LUN_IGNORED		1
 #define SCSI_SCAN_LUN_PRESENT		2
 
 static char *scsi_null_device_strs = "nullnullnullnull";
@@ -578,7 +578,7 @@ static void scsi_probe_lun(struct scsi_r
 
 	/*
 	 * The scanning code needs to know the scsi_level, even if no
-	 * device is attached at LUN 0 (SCSI_SCAN_TARGET_PRESENT) so
+	 * device is attached at LUN 0 (SCSI_SCAN_LUN_IGNORED) so
 	 * non-zero LUNs can be scanned.
 	 */
 	sdev->scsi_level = inq_result[2] & 0x07;
@@ -769,6 +769,19 @@ static int scsi_add_lun(struct scsi_devi
 	return SCSI_SCAN_LUN_PRESENT;
 }
 
+/*
+ * scsi_scan_remove: Helper funciton to free up sdev's that are not added
+ * as sys devices. That is, we never call scsi_probe_and_add_lun for these
+ * sdev's.
+ */
+static void scsi_scan_remove(struct scsi_device *sdev)
+{
+	if (sdev->host->hostt->slave_destroy)
+		sdev->host->hostt->slave_destroy(sdev);
+	transport_destroy_device(&sdev->sdev_gendev);
+	put_device(&sdev->sdev_gendev);
+}
+
 /**
  * scsi_probe_and_add_lun - probe a LUN, if a LUN is found add it
  * @starget:	pointer to target device structure
@@ -783,8 +796,10 @@ static int scsi_add_lun(struct scsi_devi
  *
  * Return:
  *     SCSI_SCAN_NO_RESPONSE: could not allocate or setup a Scsi_Device
- *     SCSI_SCAN_TARGET_PRESENT: target responded, but no device is
- *         attached at the LUN
+ *     SCSI_SCAN_LUN_IGNORED: target responded, but no device is
+ *         attached at the LUN. The sdev is never made visible, and must
+ *         be freed by the caller via scsi_scan_remove(). This way, LUN 0
+ *         is avaiable for use with the REPORT LUN command.
  *     SCSI_SCAN_LUN_PRESENT: a new Scsi_Device was allocated and initialized
  **/
 static int scsi_probe_and_add_lun(struct scsi_target *starget,
@@ -853,7 +868,7 @@ static int scsi_probe_and_add_lun(struct
 		SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
 					"scsi scan: peripheral qualifier of 3,"
 					" no device added\n"));
-		res = SCSI_SCAN_TARGET_PRESENT;
+		res = SCSI_SCAN_LUN_IGNORED;
 		goto out_free_result;
 	}
 
@@ -872,17 +887,13 @@ static int scsi_probe_and_add_lun(struct
  out_free_sreq:
 	scsi_release_request(sreq);
  out_free_sdev:
-	if (res == SCSI_SCAN_LUN_PRESENT) {
+	if ((res == SCSI_SCAN_LUN_PRESENT) || (res == SCSI_SCAN_LUN_IGNORED)) {
 		if (sdevp) {
 			scsi_device_get(sdev);
 			*sdevp = sdev;
 		}
-	} else {
-		if (sdev->host->hostt->slave_destroy)
-			sdev->host->hostt->slave_destroy(sdev);
-		transport_destroy_device(&sdev->sdev_gendev);
-		put_device(&sdev->sdev_gendev);
-	}
+	} else
+		scsi_scan_remove(sdev);
  out:
 	return res;
 }
@@ -1228,7 +1239,8 @@ static int scsi_report_lun_scan(struct s
 				       " from %s lun %d while scanning, scan"
 				       " aborted\n", devname, lun);
 				break;
-			}
+			} else if (res == SCSI_SCAN_LUN_IGNORED) 
+				scsi_scan_remove(sdev);
 		}
 	}
 
@@ -1262,6 +1274,8 @@ struct scsi_device *__scsi_add_device(st
 	if (scsi_host_scan_allowed(shost)) {
 		res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1,
 					     hostdata);
+		if (res == SCSI_SCAN_LUN_IGNORED)
+			scsi_scan_remove(sdev);
 		if (res != SCSI_SCAN_LUN_PRESENT)
 			sdev = ERR_PTR(-ENODEV);
 	}
@@ -1334,7 +1348,8 @@ void scsi_scan_target(struct device *par
 		/*
 		 * Scan for a specific host/chan/id/lun.
 		 */
-		scsi_probe_and_add_lun(starget, lun, NULL, NULL, rescan, NULL);
+		res = scsi_probe_and_add_lun(starget, lun, NULL, &sdev,
+					     rescan, NULL);
 		goto out_reap;
 	}
 
@@ -1342,8 +1357,10 @@ void scsi_scan_target(struct device *par
 	 * Scan LUN 0, if there is some response, scan further. Ideally, we
 	 * would not configure LUN 0 until all LUNs are scanned.
 	 */
-	res = scsi_probe_and_add_lun(starget, 0, &bflags, &sdev, rescan, NULL);
-	if (res == SCSI_SCAN_LUN_PRESENT) {
+	res = scsi_probe_and_add_lun(starget, 0, &bflags, &sdev, rescan,
+				     NULL);
+	WARN_ON(!sdev && res != SCSI_SCAN_NO_RESPONSE);
+	if (sdev && res != SCSI_SCAN_NO_RESPONSE)
 		if (scsi_report_lun_scan(sdev, bflags, rescan) != 0)
 			/*
 			 * The REPORT LUN did not scan the target,
@@ -1351,20 +1368,12 @@ void scsi_scan_target(struct device *par
 			 */
 			scsi_sequential_lun_scan(starget, bflags,
 				       	res, sdev->scsi_level, rescan);
-	} 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,
-				SCSI_SCAN_TARGET_PRESENT, SCSI_2, rescan);
-	}
 	if (sdev)
 		scsi_device_put(sdev);
 
  out_reap:
+	if (res == SCSI_SCAN_LUN_IGNORED)
+		scsi_scan_remove(sdev);
 	/* now determine if the target has any children at all
 	 * and if not, nuke it */
 	scsi_target_reap(starget);
@@ -1540,4 +1549,3 @@ void scsi_free_host_dev(struct scsi_devi
 	put_device(&sdev->sdev_gendev);
 }
 EXPORT_SYMBOL(scsi_free_host_dev);
-
#1 lore raw

PATCH 1/1 scsi_debug testing patch, return LUN 0 with PQ 3

Just a patch for scanning with PQ == 3 for LUN 0, only for use in testing previous patch, don't apply.

drivers/scsi/scsi_debug.c +4 -1
diff -uprN -X /home/patman/dontdiff scsi-misc-2.6/drivers/scsi/scsi_debug.c lun0-replun-scsi-misc-2.6/drivers/scsi/scsi_debug.c
@@ -619,7 +619,10 @@ static int resp_inquiry(struct scsi_cmnd
 
 	alloc_len = (cmd[3] << 8) + cmd[4];
 	memset(arr, 0, SDEBUG_MAX_INQ_ARR_SZ);
-	pq_pdt = (scsi_debug_ptype & 0x1f);
+	if (devip->lun == 0)
+		pq_pdt = 0x7f;
+	else
+		pq_pdt = (scsi_debug_ptype & 0x1f);
 	arr[0] = pq_pdt;
 	if (0x2 & cmd[1]) {  /* CMDDT bit set */
 		mk_sense_buffer(devip, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB,
#2 lore raw
In reply to Patrick Mansfield
Quoted 11 lines · Patrick Mansfield

James -

Can you ack or comment?

Allow REPORT LUN scanning even if LUN 0 reports a peripheral qualifier of 3 (effectively no storage is available on LUN 0).

Similar patch I previously posted:

http://marc.theaimsgroup.com/?l=linux-scsi&m=110297733824960&w=2

Quoted 9 lines

There is also Hannes patch, it leaves LUN 0 available for use via user space, for compatibility it likely requires that it stay visible in the future:

http://marc.theaimsgroup.com/?l=linux-scsi&m=111692497200507&w=2

Hannes' patch also causes a lot of extra vSCSI devices to show up on some platforms :)

We definitely need something like this. The current scan algorithms is just plain broken for targets where LUN 0 has no device attached. It is perfectly valid for storage servers to have a LUN 0 with no device attached (say, for control purposes) and let the client figure out the current configuration via REPORT LUN.
Without this patch Linux simply cannot handle such setups.

James, do consider to include this patch into scsi-misc.

Quoted 1 line

Cheers,

Hannes

Signature · 9 lines
Dr. Hannes Reinecke			[email protected]
SuSE Linux Products GmbH		S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#3 lore raw
In reply to Patrick Mansfield
Quoted 11 lines · Patrick Mansfield

James -

Can you ack or comment?

Allow REPORT LUN scanning even if LUN 0 reports a peripheral qualifier of 3 (effectively no storage is available on LUN 0).

Similar patch I previously posted:

http://marc.theaimsgroup.com/?l=linux-scsi&m=110297733824960&w=2

Quoted 8 lines

There is also Hannes patch, it leaves LUN 0 available for use via user space, for compatibility it likely requires that it stay visible in the future:

http://marc.theaimsgroup.com/?l=linux-scsi&m=111692497200507&w=2

Hannes' patch also causes a lot of extra vSCSI devices to show up on some platforms :)

Sorry, I didn't really like either patch.

The problem, essentially is this rather evil return refcounted sdev under certain circumstances. What I didn't like about your patch was that you actually extended it to be under more circumstances.

The correct fix, I think, is to make scsi_report_lun_scan() actually work on the target device; this will also set us up nicely for WLUN scans if they prove necessary. This being well into -rc for linux, I'll resist the strong urge to take the hatchet to the sdev returning code and stomach the duplication of scsi_level and resist the urge to investigate dumping the rescan parameter ... they can all be fixed properly after 2.6.14.

How does the attached look (and more importantly, does it work)?

James

drivers/scsi/scsi_scan.c +49 -45
@@ -587,6 +587,7 @@ static int scsi_probe_lun(struct scsi_de
 	if (sdev->scsi_level >= 2 ||
 	    (sdev->scsi_level == 1 && (inq_result[3] & 0x0f) == 1))
 		sdev->scsi_level++;
+	sdev->sdev_target->scsi_level = sdev->scsi_level;
 
 	return 0;
 }
@@ -771,6 +772,15 @@ static int scsi_add_lun(struct scsi_devi
 	return SCSI_SCAN_LUN_PRESENT;
 }
 
+static inline void scsi_destroy_sdev(struct scsi_device *sdev)
+{
+	if (sdev->host->hostt->slave_destroy)
+		sdev->host->hostt->slave_destroy(sdev);
+	transport_destroy_device(&sdev->sdev_gendev);
+	put_device(&sdev->sdev_gendev);
+}
+
+
 /**
  * scsi_probe_and_add_lun - probe a LUN, if a LUN is found add it
  * @starget:	pointer to target device structure
@@ -803,9 +813,9 @@ static int scsi_probe_and_add_lun(struct
 	 * The rescan flag is used as an optimization, the first scan of a
 	 * host adapter calls into here with rescan == 0.
 	 */
-	if (rescan) {
-		sdev = scsi_device_lookup_by_target(starget, lun);
-		if (sdev) {
+	sdev = scsi_device_lookup_by_target(starget, lun);
+	if (sdev) {
+		if (rescan || sdev->sdev_state != SDEV_CREATED) {
 			SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
 				"scsi scan: device exists on %s\n",
 				sdev->sdev_gendev.bus_id));
@@ -820,12 +830,15 @@ static int scsi_probe_and_add_lun(struct
 								 sdev->model);
 			return SCSI_SCAN_LUN_PRESENT;
 		}
-	}
-
-	sdev = scsi_alloc_sdev(starget, lun, hostdata);
+	} else
+		sdev = scsi_alloc_sdev(starget, lun, hostdata);
 	if (!sdev)
 		goto out;
 
+	BUG_ON(sdev->sdev_state == SDEV_RUNNING ||
+	       sdev->sdev_state == SDEV_QUIESCE ||
+	       sdev->sdev_state == SDEV_BLOCK);
+
 	result = kmalloc(result_len, GFP_ATOMIC |
 			((shost->unchecked_isa_dma) ? __GFP_DMA : 0));
 	if (!result)
@@ -877,12 +890,8 @@ static int scsi_probe_and_add_lun(struct
 				res = SCSI_SCAN_NO_RESPONSE;
 			}
 		}
-	} else {
-		if (sdev->host->hostt->slave_destroy)
-			sdev->host->hostt->slave_destroy(sdev);
-		transport_destroy_device(&sdev->sdev_gendev);
-		put_device(&sdev->sdev_gendev);
-	}
+	} else
+		scsi_destroy_sdev(sdev);
  out:
 	return res;
 }
@@ -1054,7 +1063,7 @@ EXPORT_SYMBOL(int_to_scsilun);
  *     0: scan completed (or no memory, so further scanning is futile)
  *     1: no report lun scan, or not configured
  **/
-static int scsi_report_lun_scan(struct scsi_device *sdev, int bflags,
+static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 				int rescan)
 {
 	char devname[64];
@@ -1067,7 +1076,8 @@ static int scsi_report_lun_scan(struct s
 	struct scsi_lun *lunp, *lun_data;
 	u8 *data;
 	struct scsi_sense_hdr sshdr;
-	struct scsi_target *starget = scsi_target(sdev);
+	struct scsi_device *sdev;
+	struct Scsi_Host *shost = dev_to_shost(&starget->dev);
 
 	/*
 	 * Only support SCSI-3 and up devices if BLIST_NOREPORTLUN is not set.
@@ -1075,15 +1085,23 @@ static int scsi_report_lun_scan(struct s
 	 * support more than 8 LUNs.
 	 */
 	if ((bflags & BLIST_NOREPORTLUN) || 
-	     sdev->scsi_level < SCSI_2 ||
-	    (sdev->scsi_level < SCSI_3 && 
-	     (!(bflags & BLIST_REPORTLUN2) || sdev->host->max_lun <= 8)) )
+	     starget->scsi_level < SCSI_2 ||
+	    (starget->scsi_level < SCSI_3 && 
+	     (!(bflags & BLIST_REPORTLUN2) || shost->max_lun <= 8)) )
 		return 1;
 	if (bflags & BLIST_NOLUN)
 		return 0;
 
+	if (!(sdev = scsi_device_lookup_by_target(starget, 0))) {
+		sdev = scsi_alloc_sdev(starget, 0, NULL);
+		if (!sdev)
+			return 0;
+		if (!scsi_device_get(sdev))
+			return 0;
+	}
+
 	sprintf(devname, "host %d channel %d id %d",
-		sdev->host->host_no, sdev->channel, sdev->id);
+		shost->host_no, sdev->channel, sdev->id);
 
 	/*
 	 * Allocate enough to hold the header (the same size as one scsi_lun)
@@ -1098,8 +1116,10 @@ static int scsi_report_lun_scan(struct s
 	length = (max_scsi_report_luns + 1) * sizeof(struct scsi_lun);
 	lun_data = kmalloc(length, GFP_ATOMIC |
 			   (sdev->host->unchecked_isa_dma ? __GFP_DMA : 0));
-	if (!lun_data)
+	if (!lun_data) {
+		printk(ALLOC_FAILURE_MSG, __FUNCTION__);
 		goto out;
+	}
 
 	scsi_cmd[0] = REPORT_LUNS;
 
@@ -1201,10 +1221,6 @@ static int scsi_report_lun_scan(struct s
 			for (i = 0; i < sizeof(struct scsi_lun); i++)
 				printk("%02x", data[i]);
 			printk(" has a LUN larger than currently supported.\n");
-		} else if (lun == 0) {
-			/*
-			 * LUN 0 has already been scanned.
-			 */
 		} else if (lun > sdev->host->max_lun) {
 			printk(KERN_WARNING "scsi: %s lun%d has a LUN larger"
 			       " than allowed by the host adapter\n",
@@ -1227,13 +1243,13 @@ static int scsi_report_lun_scan(struct s
 	}
 
 	kfree(lun_data);
-	return 0;
-
  out:
-	/*
-	 * We are out of memory, don't try scanning any further.
-	 */
-	printk(ALLOC_FAILURE_MSG, __FUNCTION__);
+	scsi_device_put(sdev);
+	if (sdev->sdev_state == SDEV_CREATED)
+		/*
+		 * the sdev we used didn't appear in the report luns scan
+		 */
+		scsi_destroy_sdev(sdev);
 	return 0;
 }
 
@@ -1326,23 +1342,14 @@ static void __scsi_scan_target(struct de
 	 * would not configure LUN 0 until all LUNs are scanned.
 	 */
 	res = scsi_probe_and_add_lun(starget, 0, &bflags, &sdev, rescan, NULL);
-	if (res == SCSI_SCAN_LUN_PRESENT) {
-		if (scsi_report_lun_scan(sdev, bflags, rescan) != 0)
+	if (res == SCSI_SCAN_LUN_PRESENT || res == SCSI_SCAN_TARGET_PRESENT) {
+		if (scsi_report_lun_scan(starget, bflags, rescan) != 0)
 			/*
 			 * The REPORT LUN did not scan the target,
 			 * do a sequential scan.
 			 */
 			scsi_sequential_lun_scan(starget, bflags,
-				       	res, sdev->scsi_level, rescan);
-	} 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,
-				SCSI_SCAN_TARGET_PRESENT, SCSI_2, rescan);
+				       	res, starget->scsi_level, rescan);
 	}
 	if (sdev)
 		scsi_device_put(sdev);
@@ -1542,10 +1549,7 @@ void scsi_free_host_dev(struct scsi_devi
 {
 	BUG_ON(sdev->id != sdev->host->this_id);
 
-	if (sdev->host->hostt->slave_destroy)
-		sdev->host->hostt->slave_destroy(sdev);
-	transport_destroy_device(&sdev->sdev_gendev);
-	put_device(&sdev->sdev_gendev);
+	scsi_destroy_sdev(sdev);
 }
 EXPORT_SYMBOL(scsi_free_host_dev);
 
include/scsi/scsi_device.h +1 -0
@@ -163,6 +163,7 @@ struct scsi_target {
 	unsigned int		id; /* target id ... replace
 				     * scsi_device.id eventually */
 	unsigned long		create:1; /* signal that it needs to be added */
+	char			scsi_level;
 	void 			*hostdata; /* available to low-level driver */
 	unsigned long		starget_data[0]; /* for the transport */
 	/* starget_data must be the last element!!!! */
#4 lore raw

Hi James -

Quoted 1 line · James Bottomley

Sorry, I didn't really like either patch.

OK :-/

Quoted 3 lines

The problem, essentially is this rather evil return refcounted sdev under certain circumstances. What I didn't like about your patch was that you actually extended it to be under more circumstances.

I don't follow the "certain circumstances", but OK.

Quoted 3 lines

The correct fix, I think, is to make scsi_report_lun_scan() actually work on the target device; this will also set us up nicely for WLUN scans if they prove necessary. This being well into -rc for linux, I'll

Yes, I agree. I was forward porting the patch from some time ago, and didn't think the starget was far enough along.

Quoted 4 lines

resist the strong urge to take the hatchet to the sdev returning code and stomach the duplication of scsi_level and resist the urge to investigate dumping the rescan parameter ... they can all be fixed properly after 2.6.14.

Yes for scsi_level.

With your patch, rescan could go away, but I do not like the lookup for each LU when we first scan a target (I think O(n^2)? i.e. for many LUs, 1000 + 999 + ... + 1 and except for LUN 0, none will match). Long term we could avoid this, maybe still special case LUN 0 in scsi_report_lun_scan(); we could also auto-set the rescan setting (and not externalize it) when there no LU's on a target (or host) at the start

Quoted 1 line

How does the attached look (and more importantly, does it work)?

I tried it on top of scsi-rc-fixes, it oopsed (non-scsi) on ppc/JS20, so I patched on a 2.6.14-rc2 git tree from sept 21, that boots up OK on IDE.

But the module refcount stays non-zero for scsi-debug, probably scsi_device_put/get. I am still debugging ...

Did your try it with the scsi_debug patch? And multiple insmod/rmmod of it?

This chunk (and "insmod scsi_debug.ko max_luns=5" or such):

drivers/scsi/scsi_debug.c +4 -1
@@ -619,7 +619,10 @@ static int resp_inquiry(struct scsi_cmnd
 
 	alloc_len = (cmd[3] << 8) + cmd[4];
 	memset(arr, 0, SDEBUG_MAX_INQ_ARR_SZ);
-	pq_pdt = (scsi_debug_ptype & 0x1f);
+	if (devip->lun == 0)
+		pq_pdt = 0x7f;
+	else
+		pq_pdt = (scsi_debug_ptype & 0x1f);
 	arr[0] = pq_pdt;
 	if (0x2 & cmd[1]) {  /* CMDDT bit set */
 		mk_sense_buffer(devip, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB,

-- Patrick Mansfield

#5 lore raw
Quoted 2 lines · Patrick Mansfield

But the module refcount stays non-zero for scsi-debug, probably scsi_device_put/get. I am still debugging ...

I forgot to mention no scsi_debug devices showed up.

I removed the bad "!" on !scsi_device_get(), and the "insmod ./scsi_debug.ko max_luns=3' found 2 sd's as expected.

But, scsi_debug module count is still at 1. Removing (via delete attr) the 2 scsi_debug devices, and module count is still at 1. So probably still a missing scsi_device_put(). I'm trying to debug a bit more.

-- Patrick Mansfield

#6 lore raw
Quoted 2 lines · Patrick Mansfield

I removed the bad "!" on !scsi_device_get(), and the "insmod ./scsi_debug.ko max_luns=3' found 2 sd's as expected.

Fixed it.

Quoted 3 lines

But, scsi_debug module count is still at 1. Removing (via delete attr) the 2 scsi_debug devices, and module count is still at 1. So probably still a missing scsi_device_put(). I'm trying to debug a bit more.

Found it and fixed that too ... missing a put after the find_device.

James

drivers/scsi/scsi_scan.c +47 -49
@@ -587,6 +587,7 @@ static int scsi_probe_lun(struct scsi_de
 	if (sdev->scsi_level >= 2 ||
 	    (sdev->scsi_level == 1 && (inq_result[3] & 0x0f) == 1))
 		sdev->scsi_level++;
+	sdev->sdev_target->scsi_level = sdev->scsi_level;
 
 	return 0;
 }
@@ -771,6 +772,15 @@ static int scsi_add_lun(struct scsi_devi
 	return SCSI_SCAN_LUN_PRESENT;
 }
 
+static inline void scsi_destroy_sdev(struct scsi_device *sdev)
+{
+	if (sdev->host->hostt->slave_destroy)
+		sdev->host->hostt->slave_destroy(sdev);
+	transport_destroy_device(&sdev->sdev_gendev);
+	put_device(&sdev->sdev_gendev);
+}
+
+
 /**
  * scsi_probe_and_add_lun - probe a LUN, if a LUN is found add it
  * @starget:	pointer to target device structure
@@ -803,9 +813,9 @@ static int scsi_probe_and_add_lun(struct
 	 * The rescan flag is used as an optimization, the first scan of a
 	 * host adapter calls into here with rescan == 0.
 	 */
-	if (rescan) {
-		sdev = scsi_device_lookup_by_target(starget, lun);
-		if (sdev) {
+	sdev = scsi_device_lookup_by_target(starget, lun);
+	if (sdev) {
+		if (rescan || sdev->sdev_state != SDEV_CREATED) {
 			SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
 				"scsi scan: device exists on %s\n",
 				sdev->sdev_gendev.bus_id));
@@ -820,9 +830,9 @@ static int scsi_probe_and_add_lun(struct
 								 sdev->model);
 			return SCSI_SCAN_LUN_PRESENT;
 		}
-	}
-
-	sdev = scsi_alloc_sdev(starget, lun, hostdata);
+		scsi_device_put(sdev);
+	} else
+		sdev = scsi_alloc_sdev(starget, lun, hostdata);
 	if (!sdev)
 		goto out;
 
@@ -877,12 +887,8 @@ static int scsi_probe_and_add_lun(struct
 				res = SCSI_SCAN_NO_RESPONSE;
 			}
 		}
-	} else {
-		if (sdev->host->hostt->slave_destroy)
-			sdev->host->hostt->slave_destroy(sdev);
-		transport_destroy_device(&sdev->sdev_gendev);
-		put_device(&sdev->sdev_gendev);
-	}
+	} else
+		scsi_destroy_sdev(sdev);
  out:
 	return res;
 }
@@ -1054,7 +1060,7 @@ EXPORT_SYMBOL(int_to_scsilun);
  *     0: scan completed (or no memory, so further scanning is futile)
  *     1: no report lun scan, or not configured
  **/
-static int scsi_report_lun_scan(struct scsi_device *sdev, int bflags,
+static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 				int rescan)
 {
 	char devname[64];
@@ -1067,7 +1073,8 @@ static int scsi_report_lun_scan(struct s
 	struct scsi_lun *lunp, *lun_data;
 	u8 *data;
 	struct scsi_sense_hdr sshdr;
-	struct scsi_target *starget = scsi_target(sdev);
+	struct scsi_device *sdev;
+	struct Scsi_Host *shost = dev_to_shost(&starget->dev);
 
 	/*
 	 * Only support SCSI-3 and up devices if BLIST_NOREPORTLUN is not set.
@@ -1075,15 +1082,23 @@ static int scsi_report_lun_scan(struct s
 	 * support more than 8 LUNs.
 	 */
 	if ((bflags & BLIST_NOREPORTLUN) || 
-	     sdev->scsi_level < SCSI_2 ||
-	    (sdev->scsi_level < SCSI_3 && 
-	     (!(bflags & BLIST_REPORTLUN2) || sdev->host->max_lun <= 8)) )
+	     starget->scsi_level < SCSI_2 ||
+	    (starget->scsi_level < SCSI_3 && 
+	     (!(bflags & BLIST_REPORTLUN2) || shost->max_lun <= 8)) )
 		return 1;
 	if (bflags & BLIST_NOLUN)
 		return 0;
 
+	if (!(sdev = scsi_device_lookup_by_target(starget, 0))) {
+		sdev = scsi_alloc_sdev(starget, 0, NULL);
+		if (!sdev)
+			return 0;
+		if (scsi_device_get(sdev))
+			return 0;
+	}
+
 	sprintf(devname, "host %d channel %d id %d",
-		sdev->host->host_no, sdev->channel, sdev->id);
+		shost->host_no, sdev->channel, sdev->id);
 
 	/*
 	 * Allocate enough to hold the header (the same size as one scsi_lun)
@@ -1098,8 +1113,10 @@ static int scsi_report_lun_scan(struct s
 	length = (max_scsi_report_luns + 1) * sizeof(struct scsi_lun);
 	lun_data = kmalloc(length, GFP_ATOMIC |
 			   (sdev->host->unchecked_isa_dma ? __GFP_DMA : 0));
-	if (!lun_data)
+	if (!lun_data) {
+		printk(ALLOC_FAILURE_MSG, __FUNCTION__);
 		goto out;
+	}
 
 	scsi_cmd[0] = REPORT_LUNS;
 
@@ -1201,10 +1218,6 @@ static int scsi_report_lun_scan(struct s
 			for (i = 0; i < sizeof(struct scsi_lun); i++)
 				printk("%02x", data[i]);
 			printk(" has a LUN larger than currently supported.\n");
-		} else if (lun == 0) {
-			/*
-			 * LUN 0 has already been scanned.
-			 */
 		} else if (lun > sdev->host->max_lun) {
 			printk(KERN_WARNING "scsi: %s lun%d has a LUN larger"
 			       " than allowed by the host adapter\n",
@@ -1227,13 +1240,13 @@ static int scsi_report_lun_scan(struct s
 	}
 
 	kfree(lun_data);
-	return 0;
-
  out:
-	/*
-	 * We are out of memory, don't try scanning any further.
-	 */
-	printk(ALLOC_FAILURE_MSG, __FUNCTION__);
+	scsi_device_put(sdev);
+	if (sdev->sdev_state == SDEV_CREATED)
+		/*
+		 * the sdev we used didn't appear in the report luns scan
+		 */
+		scsi_destroy_sdev(sdev);
 	return 0;
 }
 
@@ -1299,7 +1312,6 @@ static void __scsi_scan_target(struct de
 	struct Scsi_Host *shost = dev_to_shost(parent);
 	int bflags = 0;
 	int res;
-	struct scsi_device *sdev = NULL;
 	struct scsi_target *starget;
 
 	if (shost->this_id == id)
@@ -1325,27 +1337,16 @@ static void __scsi_scan_target(struct de
 	 * Scan LUN 0, if there is some response, scan further. Ideally, we
 	 * would not configure LUN 0 until all LUNs are scanned.
 	 */
-	res = scsi_probe_and_add_lun(starget, 0, &bflags, &sdev, rescan, NULL);
-	if (res == SCSI_SCAN_LUN_PRESENT) {
-		if (scsi_report_lun_scan(sdev, bflags, rescan) != 0)
+	res = scsi_probe_and_add_lun(starget, 0, &bflags, NULL, rescan, NULL);
+	if (res == SCSI_SCAN_LUN_PRESENT || res == SCSI_SCAN_TARGET_PRESENT) {
+		if (scsi_report_lun_scan(starget, bflags, rescan) != 0)
 			/*
 			 * The REPORT LUN did not scan the target,
 			 * do a sequential scan.
 			 */
 			scsi_sequential_lun_scan(starget, bflags,
-				       	res, sdev->scsi_level, rescan);
-	} 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,
-				SCSI_SCAN_TARGET_PRESENT, SCSI_2, rescan);
+				       	res, starget->scsi_level, rescan);
 	}
-	if (sdev)
-		scsi_device_put(sdev);
 
  out_reap:
 	/* now determine if the target has any children at all
@@ -1542,10 +1543,7 @@ void scsi_free_host_dev(struct scsi_devi
 {
 	BUG_ON(sdev->id != sdev->host->this_id);
 
-	if (sdev->host->hostt->slave_destroy)
-		sdev->host->hostt->slave_destroy(sdev);
-	transport_destroy_device(&sdev->sdev_gendev);
-	put_device(&sdev->sdev_gendev);
+	scsi_destroy_sdev(sdev);
 }
 EXPORT_SYMBOL(scsi_free_host_dev);
 
include/scsi/scsi_device.h +1 -0
@@ -163,6 +163,7 @@ struct scsi_target {
 	unsigned int		id; /* target id ... replace
 				     * scsi_device.id eventually */
 	unsigned long		create:1; /* signal that it needs to be added */
+	char			scsi_level;
 	void 			*hostdata; /* available to low-level driver */
 	unsigned long		starget_data[0]; /* for the transport */
 	/* starget_data must be the last element!!!! */
#7 lore raw
Quoted 11 lines · James Bottomley
Quoted 2 lines · Patrick Mansfield

I removed the bad "!" on !scsi_device_get(), and the "insmod ./scsi_debug.ko max_luns=3' found 2 sd's as expected.

Fixed it.

Quoted 3 lines

But, scsi_debug module count is still at 1. Removing (via delete attr) the 2 scsi_debug devices, and module count is still at 1. So probably still a missing scsi_device_put(). I'm trying to debug a bit more.

Found it and fixed that too ... missing a put after the find_device.

OK, works fine now ...

Review

Quoted 6 lines
(quoted diff) +6 -0
+static inline void scsi_destroy_sdev(struct scsi_device *sdev)
+{
+	if (sdev->host->hostt->slave_destroy)
+		sdev->host->hostt->slave_destroy(sdev);
+	transport_destroy_device(&sdev->sdev_gendev);
+	put_device(&sdev->sdev_gendev);

One nit ... get rid of the inline, as the code path is not performance critical.

Also, note that most devices won't behave as scsi_debug configured as previosly patched: it had PQ !=0 for LUN 0, but then put LUN 0 in REPORT LUN output.

So, we hit the already existing sdev case in James' patch.

But it was a good test case :)

For the record, here's a scsi_debug patch to not have LUN 0 show up, and not have it in REPORT LUNS output.

drivers/scsi/scsi_debug.c +9 -6
@@ -619,7 +619,10 @@ static int resp_inquiry(struct scsi_cmnd
 
 	alloc_len = (cmd[3] << 8) + cmd[4];
 	memset(arr, 0, SDEBUG_MAX_INQ_ARR_SZ);
-	pq_pdt = (scsi_debug_ptype & 0x1f);
+	if (devip->lun == 0)
+		pq_pdt = 0x7f; /* XXX hack no LUN 0 */
+	else
+		pq_pdt = (scsi_debug_ptype & 0x1f);
 	arr[0] = pq_pdt;
 	if (0x2 & cmd[1]) {  /* CMDDT bit set */
 		mk_sense_buffer(devip, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB,
@@ -946,7 +949,7 @@ static int resp_report_luns(struct scsi_
 			    struct sdebug_dev_info * devip)
 {
 	unsigned int alloc_len;
-	int lun_cnt, i, upper;
+	int lun_cnt, i, upper, lun;
 	unsigned char *cmd = (unsigned char *)scp->cmnd;
 	int select_report = (int)cmd[2];
 	struct scsi_lun *one_lun;
@@ -960,18 +963,18 @@ static int resp_report_luns(struct scsi_
 	}
 	/* can produce response with up to 16k luns (lun 0 to lun 16383) */
 	memset(arr, 0, SDEBUG_RLUN_ARR_SZ);
-	lun_cnt = scsi_debug_max_luns;
+	lun_cnt = scsi_debug_max_luns - 1;
 	arr[2] = ((sizeof(struct scsi_lun) * lun_cnt) >> 8) & 0xff;
 	arr[3] = (sizeof(struct scsi_lun) * lun_cnt) & 0xff;
 	lun_cnt = min((int)((SDEBUG_RLUN_ARR_SZ - 8) /
 			    sizeof(struct scsi_lun)), lun_cnt);
 	one_lun = (struct scsi_lun *) &arr[8];
-	for (i = 0; i < lun_cnt; i++) {
-		upper = (i >> 8) & 0x3f;
+	for (i = 0, lun = 1; i < lun_cnt; i++, lun++) {
+		upper = (lun >> 8) & 0x3f;
 		if (upper)
 			one_lun[i].scsi_lun[0] =
 			    (upper | (SAM2_LUN_ADDRESS_METHOD << 6));
-		one_lun[i].scsi_lun[1] = i & 0xff;
+		one_lun[i].scsi_lun[1] = lun & 0xff;
 	}
 	return fill_from_dev_buffer(scp, arr,
 				    min((int)alloc_len, SDEBUG_RLUN_ARR_SZ));

-- Patrick Mansfield