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

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

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
> > processing.
> 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.


Juergen

> Jan
>
> > 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
> > once.
> >
> > 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;
> > -retry:
> > +
> >         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) {
> >                                 spin_unlock_irqrestore(&info->v2p_lock,
> >                                                         flags);
> > -
> > -                               if (retry_cnt < VSCSI_REPORT_LUNS_RETRY) {
> > -                                       retry_cnt++;
> > -                                       if (buff)
> > -                                               kfree(buff);
> > -                                       goto retry;
> > -                               }
> > -
> >                                 goto fail;
> >                         }
>
>
>
> _______________________________________________
> 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