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

["Jürgen Groß" <jgross@suse.com>]

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
>