From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?J=FCrgen_Gro=DF?= Subject: Re: [PATCH][linux 2.6.18] remove pointless error handling in scsiback Date: Wed, 02 Jul 2014 14:13:15 +0200 Message-ID: <53B3F75B.10805@suse.com> References: <1404285980-10817-1-git-send-email-jgross@suse.com> <53B40FC2020000780001F774@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53B40FC2020000780001F774@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 > Signed-off-by: Jan Beulich One minor nit below, but in any case: Reviewed-by: Juergen Gross > > --- 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 >