[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.