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

[Jürgen Groß <jgross@xxxxxxxx>]

On 07/02/2014 01:57 PM, Jan Beulich wrote:
> > > > On 02.07.14 at 09:26, <"jgross@xxxxxxxx".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@xxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
One minor nit below, but in any case:

Reviewed-by: Juergen Gross <jgross@xxxxxxxx>

> --- 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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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