[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/01/2014 05:39 PM, Jan Beulich wrote:
On 01.07.14 at 17:27, <"jgross@xxxxxxxx".non-mime.internet> wrote:
From: Juergen Gross <jgross@xxxxxxxx>

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

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.



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

Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

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;
        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) {
-                               if (retry_cnt < VSCSI_REPORT_LUNS_RETRY) {
-                                       retry_cnt++;
-                                       if (buff)
-                                               kfree(buff);
-                                       goto retry;
-                               }
                                goto fail;

Xen-devel mailing list

Xen-devel mailing list



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