[PATCH][linux 2.6.18] remove pointless error handling in scsiback

[PATCH][linux 2.6.18] remove pointless error handling in scsiback

[jgross]

From: Juergen Gross <jgross@suse.com>

Changeset 610:3bcc901cbd7a added a retry logic for reporting LUNs to the
frontend. The idea was to retry the operation if a LUN was added between
counting the LUNs (needed for calculating the needed buffer size) and
processing them (filling the buffer). The number of retries was limited to
three. Reasoning was "one error is very unlikely, 4 of them are way off".

Unfortunately above changeset was wrong: The retry didn't include recounting
the LUNs, so the buffer size would not be changed. The number of found was not
resetted, so the first found LUN would again trigger the retry logic until the
number of retries would be exhausted.

While one error is really very unlikely (there was no report of failure and
as shown above, the first error would have led to failure at once), even in
case of a correct retry logic following errors are not completely impossible:
If a target with many LUNs is being added to the frontend, the single LUNs are
added one after another. A parallel running "report LUNs" could see a fast
increasing number of LUNs, so even a few retries might fail.

So just remove the retry logic again.

Signed-off-by: Juergen Gross <jgross@suse.com>

diff -r 0251893a7d5a drivers/xen/scsiback/emulate.c
--- a/drivers/xen/scsiback/emulate.c	Mon Jun 16 16:11:33 2014 +0200
+++ b/drivers/xen/scsiback/emulate.c	Wed Jul 02 09:10:49 2014 +0200
@@ -212,7 +212,6 @@ static int __nr_luns_under_host(struct v
 
 /* REPORT LUNS Define*/
 #define VSCSI_REPORT_LUNS_HEADER	8
-#define VSCSI_REPORT_LUNS_RETRY		3
 
 /* quoted scsi_debug.c/resp_report_luns() */
 static void __report_luns(pending_req_t *pending_req, void *data)
@@ -228,7 +227,6 @@ static void __report_luns(pending_req_t 
 	unsigned int alloc_luns = 0;
 	unsigned int req_bufflen = 0;
 	unsigned int actual_len = 0;
-	unsigned int retry_cnt = 0;
 	int select_report = (int)cmd[2];
 	int i, lun_cnt = 0, lun, upper, err = 0;
 	
@@ -245,7 +243,7 @@ static void __report_luns(pending_req_t 
 	alloc_luns = __nr_luns_under_host(info);
 	alloc_len  = sizeof(struct scsi_lun) * alloc_luns
 				+ VSCSI_REPORT_LUNS_HEADER;
-retry:
+
 	if ((buff = kzalloc(alloc_len, GFP_KERNEL)) == NULL) {
 		printk(KERN_ERR "scsiback:%s kmalloc err\n", __FUNCTION__);
 		goto fail;
@@ -261,14 +259,6 @@ retry:
 			if (lun_cnt >= alloc_luns) {
 				spin_unlock_irqrestore(&info->v2p_lock,
 							flags);
-
-				if (retry_cnt < VSCSI_REPORT_LUNS_RETRY) {
-					retry_cnt++;
-					if (buff)
-						kfree(buff);
-					goto retry;
-				}
-
 				goto fail;
 			}

Re: [PATCH][linux 2.6.18] remove pointless error handling in scsiback

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4521 bytes --]

>>> On 02.07.14 at 09:26, <"jgross@suse.com".non-mime.internet> wrote:
> Changeset 610:3bcc901cbd7a added a retry logic for reporting LUNs to the
> frontend. The idea was to retry the operation if a LUN was added between
> counting the LUNs (needed for calculating the needed buffer size) and
> processing them (filling the buffer). The number of retries was limited to
> three. Reasoning was "one error is very unlikely, 4 of them are way off".
> 
> Unfortunately above changeset was wrong: The retry didn't include recounting
> the LUNs, so the buffer size would not be changed. The number of found was not
> resetted, so the first found LUN would again trigger the retry logic until the
> number of retries would be exhausted.
> 
> While one error is really very unlikely (there was no report of failure and
> as shown above, the first error would have led to failure at once), even in
> case of a correct retry logic following errors are not completely impossible:
> If a target with many LUNs is being added to the frontend, the single LUNs are
> added one after another. A parallel running "report LUNs" could see a fast
> increasing number of LUNs, so even a few retries might fail.
> 
> So just remove the retry logic again.

I'm still not convinced of this being the right action: I can see a point
in the original argumentation, so why don't we just fix it instead of
removing it (see below/attached)?

Jan

scsiback: fix retry handling in __report_luns()

Neither did lun_cnt get reset when invoking a retry here, nor did the
intermediate buffer get resized to fit the larger amount. Fix this, at
once
- adding spare extra buffer space growing with the number of retries,
- correct a couple of signed/unsigned issues here and its helper
  function __nr_luns_under_host(),
- drop pointless NULL checks before kfree() invocations (not only does
  kfree() deal fine with NULL, in the first of the two cases it was
  plain impossible for the pointer to be NULL).

Reported-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/drivers/xen/scsiback/emulate.c
+++ b/drivers/xen/scsiback/emulate.c
@@ -193,12 +193,12 @@ static int __maybe_unused __copy_from_sg
 	return 0;
 }
 
-static int __nr_luns_under_host(struct vscsibk_info *info)
+static unsigned int __nr_luns_under_host(struct vscsibk_info *info)
 {
 	struct v2p_entry *entry;
 	struct list_head *head = &(info->v2p_entry_lists);
 	unsigned long flags;
-	int lun_cnt = 0;
+	unsigned int lun_cnt = 0;
 
 	spin_lock_irqsave(&info->v2p_lock, flags);
 	list_for_each_entry(entry, head, l) {
@@ -213,6 +213,7 @@ static int __nr_luns_under_host(struct v
 /* REPORT LUNS Define*/
 #define VSCSI_REPORT_LUNS_HEADER	8
 #define VSCSI_REPORT_LUNS_RETRY		3
+#define VSCSI_REPORT_LUNS_SPARE		3
 
 /* quoted scsi_debug.c/resp_report_luns() */
 static void __report_luns(pending_req_t *pending_req, void *data)
@@ -229,8 +230,8 @@ static void __report_luns(pending_req_t 
 	unsigned int req_bufflen = 0;
 	unsigned int actual_len = 0;
 	unsigned int retry_cnt = 0;
-	int select_report = (int)cmd[2];
-	int i, lun_cnt = 0, lun, upper, err = 0;
+	int err, select_report = (int)cmd[2];
+	unsigned int i, lun_cnt, lun, upper;
 	
 	struct v2p_entry *entry;
 	struct list_head *head = &(info->v2p_entry_lists);
@@ -242,16 +243,18 @@ static void __report_luns(pending_req_t 
 	if ((req_bufflen < 4) || (select_report != 0))
 		goto fail;
 
-	alloc_luns = __nr_luns_under_host(info);
+retry:
+	alloc_luns = __nr_luns_under_host(info)
+		     + retry_cnt * VSCSI_REPORT_LUNS_SPARE;
 	alloc_len  = sizeof(struct scsi_lun) * alloc_luns
 				+ VSCSI_REPORT_LUNS_HEADER;
-retry:
 	if ((buff = kzalloc(alloc_len, GFP_KERNEL)) == NULL) {
 		printk(KERN_ERR "scsiback:%s kmalloc err\n", __FUNCTION__);
 		goto fail;
 	}
 
-	one_lun = (struct scsi_lun *) &buff[8];
+	one_lun = (struct scsi_lun *)&buff[VSCSI_REPORT_LUNS_HEADER];
+	lun_cnt = 0;
 	spin_lock_irqsave(&info->v2p_lock, flags);
 	list_for_each_entry(entry, head, l) {
 		if ((entry->v.chn == channel) &&
@@ -264,8 +267,7 @@ retry:
 
 				if (retry_cnt < VSCSI_REPORT_LUNS_RETRY) {
 					retry_cnt++;
-					if (buff)
-						kfree(buff);
+					kfree(buff);
 					goto retry;
 				}
 
@@ -309,8 +311,7 @@ fail:
 		INVALID_FIELD_IN_CDB, 0);
 	pending_req->rslt  = check_condition_result;
 	pending_req->resid = 0;
-	if (buff)
-		kfree(buff);
+	kfree(buff);
 	return;
 }
 



[-- Attachment #2: xen-scsiback-report-LUNs-retry.patch --]
[-- Type: text/plain, Size: 3072 bytes --]

scsiback: fix retry handling in __report_luns()

Neither did lun_cnt get reset when invoking a retry here, nor did the
intermediate buffer get resized to fit the larger amount. Fix this, at
once
- adding spare extra buffer space growing with the number of retries,
- correct a couple of signed/unsigned issues here and its helper
  function __nr_luns_under_host(),
- drop pointless NULL checks before kfree() invocations (not only does
  kfree() deal fine with NULL, in the first of the two cases it was
  plain impossible for the pointer to be NULL).

Reported-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/drivers/xen/scsiback/emulate.c
+++ b/drivers/xen/scsiback/emulate.c
@@ -193,12 +193,12 @@ static int __maybe_unused __copy_from_sg
 	return 0;
 }
 
-static int __nr_luns_under_host(struct vscsibk_info *info)
+static unsigned int __nr_luns_under_host(struct vscsibk_info *info)
 {
 	struct v2p_entry *entry;
 	struct list_head *head = &(info->v2p_entry_lists);
 	unsigned long flags;
-	int lun_cnt = 0;
+	unsigned int lun_cnt = 0;
 
 	spin_lock_irqsave(&info->v2p_lock, flags);
 	list_for_each_entry(entry, head, l) {
@@ -213,6 +213,7 @@ static int __nr_luns_under_host(struct v
 /* REPORT LUNS Define*/
 #define VSCSI_REPORT_LUNS_HEADER	8
 #define VSCSI_REPORT_LUNS_RETRY		3
+#define VSCSI_REPORT_LUNS_SPARE		3
 
 /* quoted scsi_debug.c/resp_report_luns() */
 static void __report_luns(pending_req_t *pending_req, void *data)
@@ -229,8 +230,8 @@ static void __report_luns(pending_req_t 
 	unsigned int req_bufflen = 0;
 	unsigned int actual_len = 0;
 	unsigned int retry_cnt = 0;
-	int select_report = (int)cmd[2];
-	int i, lun_cnt = 0, lun, upper, err = 0;
+	int err, select_report = (int)cmd[2];
+	unsigned int i, lun_cnt, lun, upper;
 	
 	struct v2p_entry *entry;
 	struct list_head *head = &(info->v2p_entry_lists);
@@ -242,16 +243,18 @@ static void __report_luns(pending_req_t 
 	if ((req_bufflen < 4) || (select_report != 0))
 		goto fail;
 
-	alloc_luns = __nr_luns_under_host(info);
+retry:
+	alloc_luns = __nr_luns_under_host(info)
+		     + retry_cnt * VSCSI_REPORT_LUNS_SPARE;
 	alloc_len  = sizeof(struct scsi_lun) * alloc_luns
 				+ VSCSI_REPORT_LUNS_HEADER;
-retry:
 	if ((buff = kzalloc(alloc_len, GFP_KERNEL)) == NULL) {
 		printk(KERN_ERR "scsiback:%s kmalloc err\n", __FUNCTION__);
 		goto fail;
 	}
 
-	one_lun = (struct scsi_lun *) &buff[8];
+	one_lun = (struct scsi_lun *)&buff[VSCSI_REPORT_LUNS_HEADER];
+	lun_cnt = 0;
 	spin_lock_irqsave(&info->v2p_lock, flags);
 	list_for_each_entry(entry, head, l) {
 		if ((entry->v.chn == channel) &&
@@ -264,8 +267,7 @@ retry:
 
 				if (retry_cnt < VSCSI_REPORT_LUNS_RETRY) {
 					retry_cnt++;
-					if (buff)
-						kfree(buff);
+					kfree(buff);
 					goto retry;
 				}
 
@@ -309,8 +311,7 @@ fail:
 		INVALID_FIELD_IN_CDB, 0);
 	pending_req->rslt  = check_condition_result;
 	pending_req->resid = 0;
-	if (buff)
-		kfree(buff);
+	kfree(buff);
 	return;
 }
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH][linux 2.6.18] remove pointless error handling in scsiback

[Jürgen Groß]

On 07/02/2014 01:57 PM, Jan Beulich wrote:
>>>> On 02.07.14 at 09:26, <"jgross@suse.com".non-mime.internet> wrote:
>> Changeset 610:3bcc901cbd7a added a retry logic for reporting LUNs to the
>> frontend. The idea was to retry the operation if a LUN was added between
>> counting the LUNs (needed for calculating the needed buffer size) and
>> processing them (filling the buffer). The number of retries was limited to
>> three. Reasoning was "one error is very unlikely, 4 of them are way off".
>>
>> Unfortunately above changeset was wrong: The retry didn't include recounting
>> the LUNs, so the buffer size would not be changed. The number of found was not
>> resetted, so the first found LUN would again trigger the retry logic until the
>> number of retries would be exhausted.
>>
>> While one error is really very unlikely (there was no report of failure and
>> as shown above, the first error would have led to failure at once), even in
>> case of a correct retry logic following errors are not completely impossible:
>> If a target with many LUNs is being added to the frontend, the single LUNs are
>> added one after another. A parallel running "report LUNs" could see a fast
>> increasing number of LUNs, so even a few retries might fail.
>>
>> So just remove the retry logic again.
>
> I'm still not convinced of this being the right action: I can see a point
> in the original argumentation, so why don't we just fix it instead of
> removing it (see below/attached)?
>
> Jan
>
> scsiback: fix retry handling in __report_luns()
>
> Neither did lun_cnt get reset when invoking a retry here, nor did the
> intermediate buffer get resized to fit the larger amount. Fix this, at
> once
> - adding spare extra buffer space growing with the number of retries,
> - correct a couple of signed/unsigned issues here and its helper
>    function __nr_luns_under_host(),
> - drop pointless NULL checks before kfree() invocations (not only does
>    kfree() deal fine with NULL, in the first of the two cases it was
>    plain impossible for the pointer to be NULL).
>
> Reported-by: Juergen Gross <jgross@suse.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

One minor nit below, but in any case:

Reviewed-by: Juergen Gross <jgross@suse.com>

>
> --- a/drivers/xen/scsiback/emulate.c
> +++ b/drivers/xen/scsiback/emulate.c
> @@ -193,12 +193,12 @@ static int __maybe_unused __copy_from_sg
>   	return 0;
>   }
>
> -static int __nr_luns_under_host(struct vscsibk_info *info)
> +static unsigned int __nr_luns_under_host(struct vscsibk_info *info)
>   {
>   	struct v2p_entry *entry;
>   	struct list_head *head = &(info->v2p_entry_lists);
>   	unsigned long flags;
> -	int lun_cnt = 0;
> +	unsigned int lun_cnt = 0;
>
>   	spin_lock_irqsave(&info->v2p_lock, flags);
>   	list_for_each_entry(entry, head, l) {
> @@ -213,6 +213,7 @@ static int __nr_luns_under_host(struct v
>   /* REPORT LUNS Define*/
>   #define VSCSI_REPORT_LUNS_HEADER	8
>   #define VSCSI_REPORT_LUNS_RETRY		3
> +#define VSCSI_REPORT_LUNS_SPARE		3
>
>   /* quoted scsi_debug.c/resp_report_luns() */
>   static void __report_luns(pending_req_t *pending_req, void *data)
> @@ -229,8 +230,8 @@ static void __report_luns(pending_req_t
>   	unsigned int req_bufflen = 0;
>   	unsigned int actual_len = 0;
>   	unsigned int retry_cnt = 0;

When you are cleaning up local variables, IMHO you could remove above 3
unneeded initializations, too.

> -	int select_report = (int)cmd[2];
> -	int i, lun_cnt = 0, lun, upper, err = 0;
> +	int err, select_report = (int)cmd[2];
> +	unsigned int i, lun_cnt, lun, upper;
>   	
>   	struct v2p_entry *entry;
>   	struct list_head *head = &(info->v2p_entry_lists);
> @@ -242,16 +243,18 @@ static void __report_luns(pending_req_t
>   	if ((req_bufflen < 4) || (select_report != 0))
>   		goto fail;
>
> -	alloc_luns = __nr_luns_under_host(info);
> +retry:
> +	alloc_luns = __nr_luns_under_host(info)
> +		     + retry_cnt * VSCSI_REPORT_LUNS_SPARE;
>   	alloc_len  = sizeof(struct scsi_lun) * alloc_luns
>   				+ VSCSI_REPORT_LUNS_HEADER;
> -retry:
>   	if ((buff = kzalloc(alloc_len, GFP_KERNEL)) == NULL) {
>   		printk(KERN_ERR "scsiback:%s kmalloc err\n", __FUNCTION__);
>   		goto fail;
>   	}
>
> -	one_lun = (struct scsi_lun *) &buff[8];
> +	one_lun = (struct scsi_lun *)&buff[VSCSI_REPORT_LUNS_HEADER];
> +	lun_cnt = 0;
>   	spin_lock_irqsave(&info->v2p_lock, flags);
>   	list_for_each_entry(entry, head, l) {
>   		if ((entry->v.chn == channel) &&
> @@ -264,8 +267,7 @@ retry:
>
>   				if (retry_cnt < VSCSI_REPORT_LUNS_RETRY) {
>   					retry_cnt++;
> -					if (buff)
> -						kfree(buff);
> +					kfree(buff);
>   					goto retry;
>   				}
>
> @@ -309,8 +311,7 @@ fail:
>   		INVALID_FIELD_IN_CDB, 0);
>   	pending_req->rslt  = check_condition_result;
>   	pending_req->resid = 0;
> -	if (buff)
> -		kfree(buff);
> +	kfree(buff);
>   	return;
>   }
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

Re: [PATCH][linux 2.6.18] remove pointless error handling in scsiback

[Jan Beulich]

>>> On 02.07.14 at 14:13, <JGross@suse.com> wrote:
> On 07/02/2014 01:57 PM, Jan Beulich wrote:
>> @@ -229,8 +230,8 @@ static void __report_luns(pending_req_t
>>   	unsigned int req_bufflen = 0;
>>   	unsigned int actual_len = 0;
>>   	unsigned int retry_cnt = 0;
> 
> When you are cleaning up local variables, IMHO you could remove above 3
> unneeded initializations, too.

Certainly not retry_cnt, but perhaps you meant (apart from the other
two visible here) alloc_luns (immediately prior to the first context line).
Yes, I guess I'll do that.

Jan

Re: [PATCH][linux 2.6.18] remove pointless error handling in scsiback

[Jan Beulich]

>>> On 02.07.14 at 14:27, <JBeulich@suse.com> wrote:
>>>> On 02.07.14 at 14:13, <JGross@suse.com> wrote:
>> On 07/02/2014 01:57 PM, Jan Beulich wrote:
>>> @@ -229,8 +230,8 @@ static void __report_luns(pending_req_t
>>>   	unsigned int req_bufflen = 0;
>>>   	unsigned int actual_len = 0;
>>>   	unsigned int retry_cnt = 0;
>> 
>> When you are cleaning up local variables, IMHO you could remove above 3
>> unneeded initializations, too.
> 
> Certainly not retry_cnt, but perhaps you meant (apart from the other
> two visible here) alloc_luns (immediately prior to the first context line).
> Yes, I guess I'll do that.

And indeed very useful to have this done, as it resulted in me spotting
that alloc_len was of type unsigned char, i.e. setting us up for memory
corruption as soon as the total buffer size needed would exceed 255
bytes. Fixed at once (and I take it that this doesn't invalidate your
Reviewed-by).

Jan

Re: [PATCH][linux 2.6.18] remove pointless error handling in scsiback

[Jürgen Groß]

On 07/02/2014 02:33 PM, Jan Beulich wrote:
>>>> On 02.07.14 at 14:27, <JBeulich@suse.com> wrote:
>>>>> On 02.07.14 at 14:13, <JGross@suse.com> wrote:
>>> On 07/02/2014 01:57 PM, Jan Beulich wrote:
>>>> @@ -229,8 +230,8 @@ static void __report_luns(pending_req_t
>>>>    	unsigned int req_bufflen = 0;
>>>>    	unsigned int actual_len = 0;
>>>>    	unsigned int retry_cnt = 0;
>>>
>>> When you are cleaning up local variables, IMHO you could remove above 3
>>> unneeded initializations, too.
>>
>> Certainly not retry_cnt, but perhaps you meant (apart from the other
>> two visible here) alloc_luns (immediately prior to the first context line).

Sorry, yes.

>> Yes, I guess I'll do that.
>
> And indeed very useful to have this done, as it resulted in me spotting
> that alloc_len was of type unsigned char, i.e. setting us up for memory
> corruption as soon as the total buffer size needed would exceed 255
> bytes. Fixed at once (and I take it that this doesn't invalidate your
> Reviewed-by).

In this case covered by "In any case" :-)

Juergen

[PATCH][linux 2.6.18] remove pointless error handling in scsiback

[jgross]

From: Juergen Gross <jgross@suse.com>

It makes no sense to try repeating traversing the lun list in case of a
possible mismatch between the original counting of luns and the real processing.

To make things worse, the retry itself was wrong as the number of detected
luns was not resetted for the retry leading to the next failing retry at once.

Signed-off-by: Juergen Gross <jgross@suse.com>


diff -r 0251893a7d5a drivers/xen/scsiback/emulate.c
--- a/drivers/xen/scsiback/emulate.c	Mon Jun 16 16:11:33 2014 +0200
+++ b/drivers/xen/scsiback/emulate.c	Tue Jul 01 17:19:27 2014 +0200
@@ -228,7 +228,6 @@ static void __report_luns(pending_req_t 
 	unsigned int alloc_luns = 0;
 	unsigned int req_bufflen = 0;
 	unsigned int actual_len = 0;
-	unsigned int retry_cnt = 0;
 	int select_report = (int)cmd[2];
 	int i, lun_cnt = 0, lun, upper, err = 0;
 	
@@ -245,7 +244,7 @@ static void __report_luns(pending_req_t 
 	alloc_luns = __nr_luns_under_host(info);
 	alloc_len  = sizeof(struct scsi_lun) * alloc_luns
 				+ VSCSI_REPORT_LUNS_HEADER;
-retry:
+
 	if ((buff = kzalloc(alloc_len, GFP_KERNEL)) == NULL) {
 		printk(KERN_ERR "scsiback:%s kmalloc err\n", __FUNCTION__);
 		goto fail;
@@ -261,14 +260,6 @@ retry:
 			if (lun_cnt >= alloc_luns) {
 				spin_unlock_irqrestore(&info->v2p_lock,
 							flags);
-
-				if (retry_cnt < VSCSI_REPORT_LUNS_RETRY) {
-					retry_cnt++;
-					if (buff)
-						kfree(buff);
-					goto retry;
-				}
-
 				goto fail;
 			}

Re: [PATCH][linux 2.6.18] remove pointless error handling in scsiback

[Jan Beulich]

>>> On 01.07.14 at 17:27, <"jgross@suse.com".non-mime.internet> wrote:
> From: Juergen Gross <jgross@suse.com>
> 
> It makes no sense to try repeating traversing the lun list in case of a
> possible mismatch between the original counting of luns and the real 
> processing.

Perhaps the retry logic isn't correct, but with the lock being dropped
between the two loops I'm not sure the retry makes no sense. Can
you explain?

Jan

> To make things worse, the retry itself was wrong as the number of detected
> luns was not resetted for the retry leading to the next failing retry at 
> once.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> 
> diff -r 0251893a7d5a drivers/xen/scsiback/emulate.c
> --- a/drivers/xen/scsiback/emulate.c	Mon Jun 16 16:11:33 2014 +0200
> +++ b/drivers/xen/scsiback/emulate.c	Tue Jul 01 17:19:27 2014 +0200
> @@ -228,7 +228,6 @@ static void __report_luns(pending_req_t 
>  	unsigned int alloc_luns = 0;
>  	unsigned int req_bufflen = 0;
>  	unsigned int actual_len = 0;
> -	unsigned int retry_cnt = 0;
>  	int select_report = (int)cmd[2];
>  	int i, lun_cnt = 0, lun, upper, err = 0;
>  	
> @@ -245,7 +244,7 @@ static void __report_luns(pending_req_t 
>  	alloc_luns = __nr_luns_under_host(info);
>  	alloc_len  = sizeof(struct scsi_lun) * alloc_luns
>  				+ VSCSI_REPORT_LUNS_HEADER;
> -retry:
> +
>  	if ((buff = kzalloc(alloc_len, GFP_KERNEL)) == NULL) {
>  		printk(KERN_ERR "scsiback:%s kmalloc err\n", __FUNCTION__);
>  		goto fail;
> @@ -261,14 +260,6 @@ retry:
>  			if (lun_cnt >= alloc_luns) {
>  				spin_unlock_irqrestore(&info->v2p_lock,
>  							flags);
> -
> -				if (retry_cnt < VSCSI_REPORT_LUNS_RETRY) {
> -					retry_cnt++;
> -					if (buff)
> -						kfree(buff);
> -					goto retry;
> -				}
> -
>  				goto fail;
>  			}
>  

Re: [PATCH][linux 2.6.18] remove pointless error handling in scsiback

[Jürgen Groß]

On 07/01/2014 05:39 PM, Jan Beulich wrote:
>>>> On 01.07.14 at 17:27, <"jgross@suse.com".non-mime.internet> wrote:
>> From: Juergen Gross <jgross@suse.com>
>>
>> It makes no sense to try repeating traversing the lun list in case of a
>> possible mismatch between the original counting of luns and the real
>> processing.
>
> Perhaps the retry logic isn't correct, but with the lock being dropped
> between the two loops I'm not sure the retry makes no sense. Can
> you explain?

In theory the number of LUNs could have just changed between
counting them and evaluating them. The counting isn't redone,
so what's the point of retrying? What are the chances a matching
LUN will appear and disappear in just the correct timing?

You could argue the counting should be included in the retry loop.
But then doing a limited number of retries is only decreasing the
chance of a problem, not eliminating it.

The real fix would be to process the LUNs in the first run without
using an intermediate buffer by filling the domU's buffer directly.
This would avoid all race conditions.


Juergen

>
> Jan
>
>> To make things worse, the retry itself was wrong as the number of detected
>> luns was not resetted for the retry leading to the next failing retry at
>> once.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>>
>> diff -r 0251893a7d5a drivers/xen/scsiback/emulate.c
>> --- a/drivers/xen/scsiback/emulate.c	Mon Jun 16 16:11:33 2014 +0200
>> +++ b/drivers/xen/scsiback/emulate.c	Tue Jul 01 17:19:27 2014 +0200
>> @@ -228,7 +228,6 @@ static void __report_luns(pending_req_t
>>   	unsigned int alloc_luns = 0;
>>   	unsigned int req_bufflen = 0;
>>   	unsigned int actual_len = 0;
>> -	unsigned int retry_cnt = 0;
>>   	int select_report = (int)cmd[2];
>>   	int i, lun_cnt = 0, lun, upper, err = 0;
>>   	
>> @@ -245,7 +244,7 @@ static void __report_luns(pending_req_t
>>   	alloc_luns = __nr_luns_under_host(info);
>>   	alloc_len  = sizeof(struct scsi_lun) * alloc_luns
>>   				+ VSCSI_REPORT_LUNS_HEADER;
>> -retry:
>> +
>>   	if ((buff = kzalloc(alloc_len, GFP_KERNEL)) == NULL) {
>>   		printk(KERN_ERR "scsiback:%s kmalloc err\n", __FUNCTION__);
>>   		goto fail;
>> @@ -261,14 +260,6 @@ retry:
>>   			if (lun_cnt >= alloc_luns) {
>>   				spin_unlock_irqrestore(&info->v2p_lock,
>>   							flags);
>> -
>> -				if (retry_cnt < VSCSI_REPORT_LUNS_RETRY) {
>> -					retry_cnt++;
>> -					if (buff)
>> -						kfree(buff);
>> -					goto retry;
>> -				}
>> -
>>   				goto fail;
>>   			}
>>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

Re: [PATCH][linux 2.6.18] remove pointless error handling in scsiback

[Jan Beulich]

>>> On 01.07.14 at 20:51, <JGross@suse.com> wrote:
> On 07/01/2014 05:39 PM, Jan Beulich wrote:
>>>>> On 01.07.14 at 17:27, <"jgross@suse.com".non-mime.internet> wrote:
>>> From: Juergen Gross <jgross@suse.com>
>>>
>>> It makes no sense to try repeating traversing the lun list in case of a
>>> possible mismatch between the original counting of luns and the real
>>> processing.
>>
>> Perhaps the retry logic isn't correct, but with the lock being dropped
>> between the two loops I'm not sure the retry makes no sense. Can
>> you explain?
> 
> In theory the number of LUNs could have just changed between
> counting them and evaluating them. The counting isn't redone,
> so what's the point of retrying? What are the chances a matching
> LUN will appear and disappear in just the correct timing?
> 
> You could argue the counting should be included in the retry loop.
> But then doing a limited number of retries is only decreasing the
> chance of a problem, not eliminating it.
> 
> The real fix would be to process the LUNs in the first run without
> using an intermediate buffer by filling the domU's buffer directly.
> This would avoid all race conditions.

I did a little archaeology and managed to spot where the retry (which
was added after the initial introduction of pvSCSI) came from. See
http://lists.xenproject.org/archives/html/xen-devel/2008-07/msg00915.html
- effectively you're requesting to revert changeset 610:3bcc901cbd7a,
i.e. you would need to (a) say so in the description and (b) explain
why the change (and the reasoning having led to it) was wrong. And
_if_ we indeed decide to undo this, the file should then also be pruned
of the VSCSI_REPORT_LUNS_RETRY definition.

Jan

Re: [PATCH][linux 2.6.18] remove pointless error handling in scsiback

[Juergen Gross]

On 07/02/2014 08:52 AM, Jan Beulich wrote:
>>>> On 01.07.14 at 20:51, <JGross@suse.com> wrote:
>> On 07/01/2014 05:39 PM, Jan Beulich wrote:
>>>>>> On 01.07.14 at 17:27, <"jgross@suse.com".non-mime.internet> wrote:
>>>> From: Juergen Gross <jgross@suse.com>
>>>>
>>>> It makes no sense to try repeating traversing the lun list in case of a
>>>> possible mismatch between the original counting of luns and the real
>>>> processing.
>>>
>>> Perhaps the retry logic isn't correct, but with the lock being dropped
>>> between the two loops I'm not sure the retry makes no sense. Can
>>> you explain?
>>
>> In theory the number of LUNs could have just changed between
>> counting them and evaluating them. The counting isn't redone,
>> so what's the point of retrying? What are the chances a matching
>> LUN will appear and disappear in just the correct timing?
>>
>> You could argue the counting should be included in the retry loop.
>> But then doing a limited number of retries is only decreasing the
>> chance of a problem, not eliminating it.
>>
>> The real fix would be to process the LUNs in the first run without
>> using an intermediate buffer by filling the domU's buffer directly.
>> This would avoid all race conditions.
>
> I did a little archaeology and managed to spot where the retry (which
> was added after the initial introduction of pvSCSI) came from. See
> http://lists.xenproject.org/archives/html/xen-devel/2008-07/msg00915.html
> - effectively you're requesting to revert changeset 610:3bcc901cbd7a,
> i.e. you would need to (a) say so in the description and (b) explain
> why the change (and the reasoning having led to it) was wrong. And
> _if_ we indeed decide to undo this, the file should then also be pruned
> of the VSCSI_REPORT_LUNS_RETRY definition.

Okay. I think the reasoning was wrong indeed. After thinking about this
some more time I came to the conclusion that multiple failures are not
completely unlikely when a target with many LUNs is added while the domU
is trying to probe the target (e.g. when the first LUN was detected).
The single LUNs will be added to the backend one after another, so the
count of matching LUNs will increase rather fast.

I'll respin the patch with a proper description.

Juergen