(unknown),

(unknown),

[realrichardsharpe]

This patch is being sent for feedback and review.

When I got AEN working in scst_local I noticed that deleting LUNs on 
the target (Linux SCSI) so I tracked it down to what looks like failure 
to take into account that LUNs might have been deleted in 
drivers/scsi/scsi_scan.c, in particular in scsi_report_lun_scan.

The changes work in the sense that if I delete LUNs in a variety of 
orders (starting at 0 and working up, starting at the largest and 
working backward, starting in the middle), but I wonder if I have 
missed anything.

[PATCH 1/1] Remove LUNs that no longer exist when we scan a target with REPORT LUNS.

[realrichardsharpe]

From: Richard Sharpe <realrichardsharpe@gmail.com>

If the target returns logical_unit_not_supported when we send REPORT LUNS
it means that it supports REPORT LUNS but there really are no LUNs there.
Delete LUN 0 in that case.

Also, when parsing the LUNs reported, remove any LUNs that used to exist
in the gaps, and remove LUNs beyond the end of those reported. They no
longer exist.

Also don't scan a target where the ID is too large or the channel is
too large.

Tested by adding four LUNs with scst_local and then deleting them in
various combinations, including deleting from LUN 0, deleting from last
LUN and deleting in the middle out.

Signed-off-by: Richard Sharpe <realrichardsharpe@gmail.com>
---
 drivers/scsi/scsi_scan.c |   65 +++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 1c027a9..d0f5d30 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1309,7 +1309,7 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 	char devname[64];
 	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
 	unsigned int length;
-	unsigned int lun;
+	unsigned int lun, old_lun = 0;
 	unsigned int num_luns;
 	unsigned int retries;
 	int result;
@@ -1410,6 +1410,22 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 		if (result == 0)
 			break;
 		else if (scsi_sense_valid(&sshdr)) {
+			/* 
+			 * This is awful. We should use a named constant!
+			 * If the target returns logical_unit_not_supported
+			 * then there really are zero LUNs there, so delete
+			 * LUN 0 if it exists. 
+			 */
+			if (sshdr.asc == 0x25) {
+				struct scsi_device *sdev = NULL;
+				sdev = scsi_device_lookup_by_target(starget, 0);
+				if (sdev) {
+					scsi_device_put(sdev);
+					__scsi_remove_device(sdev);
+					SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
+						"Removing LUN 0\n"));
+				}
+			}
 			if (sshdr.sense_key != UNIT_ATTENTION)
 				break;
 		}
@@ -1473,6 +1489,27 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 		} else {
 			int res;
 
+			/*
+			 * First, check for and remove any missing LUNs, as
+			 * they have been removed on the target.
+			 */
+			if (lun > old_lun + 1) {
+				unsigned int i;
+				for (i = old_lun + 1; i < lun; i++) {
+					struct scsi_device *sdev;
+					sdev = scsi_device_lookup_by_target(
+							starget, i);
+					if (sdev) {
+						scsi_device_put(sdev);
+						__scsi_remove_device(sdev);
+						SCSI_LOG_SCAN_BUS(3, 
+							printk(KERN_INFO
+							"Removing LUN %d\n", 
+							i));
+					}
+				}
+			}
+
 			res = scsi_probe_and_add_lun(starget,
 				lun, NULL, NULL, rescan, NULL);
 			if (res == SCSI_SCAN_NO_RESPONSE) {
@@ -1485,6 +1522,26 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
 				        " aborted\n", lun);
 				break;
 			}
+
+			old_lun = lun;
+		}
+	}
+
+	/*
+	 * Remove any device that no longer exist. Does not work for LUN 0.
+	 * See above.
+	 */
+	if (lun < shost->max_lun) {
+		int i = 0;
+		for (i = lun + 1; i < shost->max_lun; i++) {
+			struct scsi_device *sdev;
+			sdev = scsi_device_lookup_by_target(starget, i);
+			if (sdev) {
+				scsi_device_put(sdev);
+				__scsi_remove_device(sdev);
+				SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
+					"Removed LUN %d\n", i));
+			}
 		}
 	}
 
@@ -1565,9 +1622,11 @@ static void __scsi_scan_target(struct device *parent, unsigned int channel,
 	int res;
 	struct scsi_target *starget;
 
-	if (shost->this_id == id)
+	if (shost->this_id == id ||
+		id > shost->max_id ||
+		channel > shost->max_channel)
 		/*
-		 * Don't scan the host adapter
+		 * Don't scan the host adapter or dis-allowed ones.
 		 */
 		return;
 
-- 
1.6.6.1

Re: [PATCH 1/1] Remove LUNs that no longer exist when we scan a target with REPORT LUNS.

[Konrad Rzeszutek Wilk]

On Sunday 15 August 2010 20:08:33 realrichardsharpe@gmail.com wrote:
> From: Richard Sharpe <realrichardsharpe@gmail.com>
>
> If the target returns logical_unit_not_supported when we send REPORT LUNS
> it means that it supports REPORT LUNS but there really are no LUNs there.
> Delete LUN 0 in that case.
>
> Also, when parsing the LUNs reported, remove any LUNs that used to exist
> in the gaps, and remove LUNs beyond the end of those reported. They no
> longer exist.
>
> Also don't scan a target where the ID is too large or the channel is
> too large.
>
> Tested by adding four LUNs with scst_local and then deleting them in
> various combinations, including deleting from LUN 0, deleting from last
> LUN and deleting in the middle out.

What happens if you stack the device? Say you got multipath on top of this or 
device-mapper? Does this work properly ? For multipath I would expect it to 
work OK, and multipath device would queue the I/O until a LUN with the same 
SCSI_ID would reappear.
>
> Signed-off-by: Richard Sharpe <realrichardsharpe@gmail.com>
> ---
>  drivers/scsi/scsi_scan.c |   65
> +++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 62
> insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 1c027a9..d0f5d30 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1309,7 +1309,7 @@ static int scsi_report_lun_scan(struct scsi_target
> *starget, int bflags, char devname[64];
>  	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
>  	unsigned int length;
> -	unsigned int lun;
> +	unsigned int lun, old_lun = 0;
>  	unsigned int num_luns;
>  	unsigned int retries;
>  	int result;
> @@ -1410,6 +1410,22 @@ static int scsi_report_lun_scan(struct scsi_target
> *starget, int bflags, if (result == 0)
>  			break;
>  		else if (scsi_sense_valid(&sshdr)) {
> +			/*
> +			 * This is awful. We should use a named constant!
> +			 * If the target returns logical_unit_not_supported
> +			 * then there really are zero LUNs there, so delete
> +			 * LUN 0 if it exists.
> +			 */
> +			if (sshdr.asc == 0x25) {
> +				struct scsi_device *sdev = NULL;
> +				sdev = scsi_device_lookup_by_target(starget, 0);
> +				if (sdev) {
> +					scsi_device_put(sdev);
> +					__scsi_remove_device(sdev);
> +					SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
> +						"Removing LUN 0\n"));
> +				}
> +			}
>  			if (sshdr.sense_key != UNIT_ATTENTION)
>  				break;
>  		}
> @@ -1473,6 +1489,27 @@ static int scsi_report_lun_scan(struct scsi_target
> *starget, int bflags, } else {
>  			int res;
>
> +			/*
> +			 * First, check for and remove any missing LUNs, as
> +			 * they have been removed on the target.
> +			 */
> +			if (lun > old_lun + 1) {
> +				unsigned int i;
> +				for (i = old_lun + 1; i < lun; i++) {
> +					struct scsi_device *sdev;
> +					sdev = scsi_device_lookup_by_target(
> +							starget, i);
> +					if (sdev) {
> +						scsi_device_put(sdev);
> +						__scsi_remove_device(sdev);
> +						SCSI_LOG_SCAN_BUS(3,
> +							printk(KERN_INFO
> +							"Removing LUN %d\n",
> +							i));
> +					}
> +				}
> +			}
> +
>  			res = scsi_probe_and_add_lun(starget,
>  				lun, NULL, NULL, rescan, NULL);
>  			if (res == SCSI_SCAN_NO_RESPONSE) {
> @@ -1485,6 +1522,26 @@ static int scsi_report_lun_scan(struct scsi_target
> *starget, int bflags, " aborted\n", lun);
>  				break;
>  			}
> +
> +			old_lun = lun;
> +		}
> +	}
> +
> +	/*
> +	 * Remove any device that no longer exist. Does not work for LUN 0.
> +	 * See above.
> +	 */
> +	if (lun < shost->max_lun) {
> +		int i = 0;
> +		for (i = lun + 1; i < shost->max_lun; i++) {
> +			struct scsi_device *sdev;
> +			sdev = scsi_device_lookup_by_target(starget, i);
> +			if (sdev) {
> +				scsi_device_put(sdev);
> +				__scsi_remove_device(sdev);
> +				SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
> +					"Removed LUN %d\n", i));
> +			}
>  		}
>  	}
>
> @@ -1565,9 +1622,11 @@ static void __scsi_scan_target(struct device
> *parent, unsigned int channel, int res;
>  	struct scsi_target *starget;
>
> -	if (shost->this_id == id)
> +	if (shost->this_id == id ||
> +		id > shost->max_id ||
> +		channel > shost->max_channel)
>  		/*
> -		 * Don't scan the host adapter
> +		 * Don't scan the host adapter or dis-allowed ones.
>  		 */
>  		return;

Re: [PATCH 1/1] Remove LUNs that no longer exist when we scan a target with REPORT LUNS.

[Richard Sharpe]

On Mon, Aug 16, 2010 at 6:45 AM, Konrad Rzeszutek Wilk
<konrad@darnok.org> wrote:
> On Sunday 15 August 2010 20:08:33 realrichardsharpe@gmail.com wrote:
>> From: Richard Sharpe <realrichardsharpe@gmail.com>
>>
>> If the target returns logical_unit_not_supported when we send REPORT LUNS
>> it means that it supports REPORT LUNS but there really are no LUNs there.
>> Delete LUN 0 in that case.
>>
>> Also, when parsing the LUNs reported, remove any LUNs that used to exist
>> in the gaps, and remove LUNs beyond the end of those reported. They no
>> longer exist.
>>
>> Also don't scan a target where the ID is too large or the channel is
>> too large.
>>
>> Tested by adding four LUNs with scst_local and then deleting them in
>> various combinations, including deleting from LUN 0, deleting from last
>> LUN and deleting in the middle out.
>
> What happens if you stack the device? Say you got multipath on top of this or
> device-mapper? Does this work properly ? For multipath I would expect it to
> work OK, and multipath device would queue the I/O until a LUN with the same
> SCSI_ID would reappear.

I do not know :-)

Let me test this situation to find out.

-- 
Regards,
Richard Sharpe

Re: [PATCH 1/1] Remove LUNs that no longer exist when we scan a target with REPORT LUNS.

[Konrad Rzeszutek Wilk]

> >> Tested by adding four LUNs with scst_local and then deleting them in
> >> various combinations, including deleting from LUN 0, deleting from last
> >> LUN and deleting in the middle out.
> >
> > What happens if you stack the device? Say you got multipath on top of
> > this or device-mapper? Does this work properly ? For multipath I would
> > expect it to work OK, and multipath device would queue the I/O until a
> > LUN with the same SCSI_ID would reappear.
>
> I do not know :-)
>
> Let me test this situation to find out.

Cool. Thanks.

Re: [PATCH 1/1] Remove LUNs that no longer exist when we scan a target with REPORT LUNS.

[Richard Sharpe]

On Sun, Aug 15, 2010 at 5:08 PM,  <realrichardsharpe@gmail.com> wrote:
> From: Richard Sharpe <realrichardsharpe@gmail.com>
>
> If the target returns logical_unit_not_supported when we send REPORT LUNS
> it means that it supports REPORT LUNS but there really are no LUNs there.
> Delete LUN 0 in that case.
>
> Also, when parsing the LUNs reported, remove any LUNs that used to exist
> in the gaps, and remove LUNs beyond the end of those reported. They no
> longer exist.
>
> Also don't scan a target where the ID is too large or the channel is
> too large.
>
> Tested by adding four LUNs with scst_local and then deleting them in
> various combinations, including deleting from LUN 0, deleting from last
> LUN and deleting in the middle out.

Hmmm, before James responds, it turns out that I did not test as
carefully as I thought, and deleting LUNs in the order 0, 2, ... does
not produce the results I expected, so I will have to rework this.

-- 
Regards,
Richard Sharpe

Re: [PATCH 1/1] Remove LUNs that no longer exist when we scan a target with REPORT LUNS.

[James Bottomley]

On Wed, 2010-08-18 at 08:10 -0700, Richard Sharpe wrote:
> On Sun, Aug 15, 2010 at 5:08 PM,  <realrichardsharpe@gmail.com> wrote:
> > From: Richard Sharpe <realrichardsharpe@gmail.com>
> >
> > If the target returns logical_unit_not_supported when we send REPORT LUNS
> > it means that it supports REPORT LUNS but there really are no LUNs there.
> > Delete LUN 0 in that case.
> >
> > Also, when parsing the LUNs reported, remove any LUNs that used to exist
> > in the gaps, and remove LUNs beyond the end of those reported. They no
> > longer exist.
> >
> > Also don't scan a target where the ID is too large or the channel is
> > too large.
> >
> > Tested by adding four LUNs with scst_local and then deleting them in
> > various combinations, including deleting from LUN 0, deleting from last
> > LUN and deleting in the middle out.
> 
> Hmmm, before James responds, it turns out that I did not test as
> carefully as I thought, and deleting LUNs in the order 0, 2, ... does
> not produce the results I expected, so I will have to rework this.

OK, so I think what I'd really like is an AEN notification
infrastructure based on the unit attentions that bubbles this up to user
space for a decision.  That way if user space does the removal, we're
not going to get into locking or other problems.

James

RE: [PATCH 1/1] Remove LUNs that no longer exist when we scan a target with REPORT LUNS.

[Loke, Chetan]

Hello Richard,

I know you just sent an email stating that you will be reworking the logic but I've a question.

I haven't looked at the call-flow in detail but consider the following case:

1) IO is streaming to LUN 'x'.
2) LUN 'x' was unmapped from a target. Trust me there are arrays that will let you do that ;).
3) Initiator issued a 'rescan'.
4) LUN 'x' is missing from the response.
   4.1) Now we can't just yank that missing LUN 'x'.

This is why:
A) Outstanding IO's will need some time to abort. Example: FC EDTOV etc might be ~30s.
B) SCSI-mid should know that it should NOT retry the aborted IO because the device is scheduled for deletion.
   Otherwise it will be easy to induce an abort storm.

Will __scsi_remove_device take care of all the scsi_device state transition?


Chetan Loke


> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Konrad Rzeszutek Wilk
> Sent: August 16, 2010 9:45 AM
> To: realrichardsharpe@gmail.com
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 1/1] Remove LUNs that no longer exist when we scan
> a target with REPORT LUNS.
> 
> On Sunday 15 August 2010 20:08:33 realrichardsharpe@gmail.com wrote:
> > From: Richard Sharpe <realrichardsharpe@gmail.com>
> >
> > If the target returns logical_unit_not_supported when we send REPORT
> LUNS
> > it means that it supports REPORT LUNS but there really are no LUNs
> there.
> > Delete LUN 0 in that case.
> >
> > Also, when parsing the LUNs reported, remove any LUNs that used to
> exist
> > in the gaps, and remove LUNs beyond the end of those reported. They
> no
> > longer exist.
> >
> > Also don't scan a target where the ID is too large or the channel is
> > too large.
> >
> > Tested by adding four LUNs with scst_local and then deleting them in
> > various combinations, including deleting from LUN 0, deleting from
> last
> > LUN and deleting in the middle out.
> 
> What happens if you stack the device? Say you got multipath on top of
> this or
> device-mapper? Does this work properly ? For multipath I would expect
> it to
> work OK, and multipath device would queue the I/O until a LUN with the
> same
> SCSI_ID would reappear.
> >
> > Signed-off-by: Richard Sharpe <realrichardsharpe@gmail.com>
> > ---
> >  drivers/scsi/scsi_scan.c |   65
> > +++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 62
> > insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> > index 1c027a9..d0f5d30 100644
> > --- a/drivers/scsi/scsi_scan.c
> > +++ b/drivers/scsi/scsi_scan.c
> > @@ -1309,7 +1309,7 @@ static int scsi_report_lun_scan(struct
> scsi_target
> > *starget, int bflags, char devname[64];
> >  	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
> >  	unsigned int length;
> > -	unsigned int lun;
> > +	unsigned int lun, old_lun = 0;
> >  	unsigned int num_luns;
> >  	unsigned int retries;
> >  	int result;
> > @@ -1410,6 +1410,22 @@ static int scsi_report_lun_scan(struct
> scsi_target
> > *starget, int bflags, if (result == 0)
> >  			break;
> >  		else if (scsi_sense_valid(&sshdr)) {
> > +			/*
> > +			 * This is awful. We should use a named constant!
> > +			 * If the target returns logical_unit_not_supported
> > +			 * then there really are zero LUNs there, so delete
> > +			 * LUN 0 if it exists.
> > +			 */
> > +			if (sshdr.asc == 0x25) {
> > +				struct scsi_device *sdev = NULL;
> > +				sdev = scsi_device_lookup_by_target(starget,
> 0);
> > +				if (sdev) {
> > +					scsi_device_put(sdev);
> > +					__scsi_remove_device(sdev);
> > +					SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
> > +						"Removing LUN 0\n"));
> > +				}
> > +			}
> >  			if (sshdr.sense_key != UNIT_ATTENTION)
> >  				break;
> >  		}
> > @@ -1473,6 +1489,27 @@ static int scsi_report_lun_scan(struct
> scsi_target
> > *starget, int bflags, } else {
> >  			int res;
> >
> > +			/*
> > +			 * First, check for and remove any missing LUNs, as
> > +			 * they have been removed on the target.
> > +			 */
> > +			if (lun > old_lun + 1) {
> > +				unsigned int i;
> > +				for (i = old_lun + 1; i < lun; i++) {
> > +					struct scsi_device *sdev;
> > +					sdev = scsi_device_lookup_by_target(
> > +							starget, i);
> > +					if (sdev) {
> > +						scsi_device_put(sdev);
> > +						__scsi_remove_device(sdev);
> > +						SCSI_LOG_SCAN_BUS(3,
> > +							printk(KERN_INFO
> > +							"Removing LUN %d\n",
> > +							i));
> > +					}
> > +				}
> > +			}
> > +
> >  			res = scsi_probe_and_add_lun(starget,
> >  				lun, NULL, NULL, rescan, NULL);
> >  			if (res == SCSI_SCAN_NO_RESPONSE) {
> > @@ -1485,6 +1522,26 @@ static int scsi_report_lun_scan(struct
> scsi_target
> > *starget, int bflags, " aborted\n", lun);
> >  				break;
> >  			}
> > +
> > +			old_lun = lun;
> > +		}
> > +	}
> > +
> > +	/*
> > +	 * Remove any device that no longer exist. Does not work for LUN
> 0.
> > +	 * See above.
> > +	 */
> > +	if (lun < shost->max_lun) {
> > +		int i = 0;
> > +		for (i = lun + 1; i < shost->max_lun; i++) {
> > +			struct scsi_device *sdev;
> > +			sdev = scsi_device_lookup_by_target(starget, i);
> > +			if (sdev) {
> > +				scsi_device_put(sdev);
> > +				__scsi_remove_device(sdev);
> > +				SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
> > +					"Removed LUN %d\n", i));
> > +			}
> >  		}
> >  	}
> >
> > @@ -1565,9 +1622,11 @@ static void __scsi_scan_target(struct device
> > *parent, unsigned int channel, int res;
> >  	struct scsi_target *starget;
> >
> > -	if (shost->this_id == id)
> > +	if (shost->this_id == id ||
> > +		id > shost->max_id ||
> > +		channel > shost->max_channel)
> >  		/*
> > -		 * Don't scan the host adapter
> > +		 * Don't scan the host adapter or dis-allowed ones.
> >  		 */
> >  		return;
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] Remove LUNs that no longer exist when we scan a target with REPORT LUNS.

[Richard Sharpe]

On Wed, Aug 18, 2010 at 8:13 AM, James Bottomley
<James.Bottomley@suse.de> wrote:
> On Wed, 2010-08-18 at 08:10 -0700, Richard Sharpe wrote:
>> On Sun, Aug 15, 2010 at 5:08 PM,  <realrichardsharpe@gmail.com> wrote:
>> > From: Richard Sharpe <realrichardsharpe@gmail.com>
>> >
>> > If the target returns logical_unit_not_supported when we send REPORT LUNS
>> > it means that it supports REPORT LUNS but there really are no LUNs there.
>> > Delete LUN 0 in that case.
>> >
>> > Also, when parsing the LUNs reported, remove any LUNs that used to exist
>> > in the gaps, and remove LUNs beyond the end of those reported. They no
>> > longer exist.
>> >
>> > Also don't scan a target where the ID is too large or the channel is
>> > too large.
>> >
>> > Tested by adding four LUNs with scst_local and then deleting them in
>> > various combinations, including deleting from LUN 0, deleting from last
>> > LUN and deleting in the middle out.
>>
>> Hmmm, before James responds, it turns out that I did not test as
>> carefully as I thought, and deleting LUNs in the order 0, 2, ... does
>> not produce the results I expected, so I will have to rework this.
>
> OK, so I think what I'd really like is an AEN notification
> infrastructure based on the unit attentions that bubbles this up to user
> space for a decision.  That way if user space does the removal, we're
> not going to get into locking or other problems.

Having looked at this some more, I think that I chose the wrong way to
go about this. What I think should happen is that the target (in my
case scst_local, which is both a SCSI LLD and a SCSI target driver
that hooks into SCST) should establish a UNIT ATTENTION condition with
an ASC of REPORTED LUNS DATA HAS CHANGED (ASC = 3Fh ASCQ = 0Eh).

This should be detected by the SCSI stack which should then do a
REPORT LUNS scan and do the appropriate things (like cancel any
submitted requests for LUNs that no longer exist.

Does this sound reasonable?

-- 
Regards,
Richard Sharpe
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html