Re: [Xen-devel] [PATCH v5 07/10] xen/blkback: pseudo support for multi hardware queues/rings

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

> @@ -113,19 +115,55 @@ static void xen_update_blkif_status(struct xen_blkif 
> *blkif)
>       }
>       invalidate_inode_pages2(blkif->vbd.bdev->bd_inode->i_mapping);
>  
> -     blkif->ring.xenblkd = kthread_run(xen_blkif_schedule, &blkif->ring, 
> "%s", name);
> -     if (IS_ERR(blkif->ring.xenblkd)) {
> -             err = PTR_ERR(blkif->ring.xenblkd);
> -             blkif->ring.xenblkd = NULL;
> -             xenbus_dev_error(blkif->be->dev, err, "start xenblkd");
> -             return;
> +     for (i = 0; i < blkif->nr_rings; i++) {
> +             ring = &blkif->rings[i];
> +             ring->xenblkd = kthread_run(xen_blkif_schedule, ring, "%s-%d", 
> name, i);
> +             if (IS_ERR(ring->xenblkd)) {
> +                     err = PTR_ERR(ring->xenblkd);
> +                     ring->xenblkd = NULL;
> +                     xenbus_dev_fatal(blkif->be->dev, err,
> +                                     "start %s-%d xenblkd", name, i);
> +                     goto out;
> +             }
> +     }
> +     return;
> +
> +out:
> +     while (--i >= 0) {
> +             ring = &blkif->rings[i];
> +             kthread_stop(ring->xenblkd);

That won't work. Imagine us failing at the start of the loop above, 
so i==0. We get here and decrement and unsigned int by one, and loop
back to 0xffffffffff. Naturally 0xffff.. >= 0 so we will just continue
one going over the blkif->rings[0xffffff].. and BOOM!

This worked when 'i' was 'int', but now it is unsigned int.

Let me make it 'int' and then this works, or we can swap
the loop around and use 'i-1' to use the previous entry.

[Fixed it up in my tree]
>       }
> +     return;
> +}
> +
.. snip..
> +static int connect_ring(struct backend_info *be)
> +{
> +     struct xenbus_device *dev = be->dev;
> +     unsigned int pers_grants;
> +     char protocol[64] = "";
> +     int err, i;
> +     char *xspath;
> +     size_t xspathsize;
> +     const size_t xenstore_path_ext_size = 11; /* sufficient for 
> "/queue-NNN" */
> +
> +     pr_debug("%s %s\n", __func__, dev->otherend);
> +
> +     be->blkif->blk_protocol = BLKIF_PROTOCOL_DEFAULT;
> +     err = xenbus_gather(XBT_NIL, dev->otherend, "protocol",
> +                         "%63s", protocol, NULL);
> +     if (err)
> +             strcpy(protocol, "unspecified, assuming default");
> +     else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_NATIVE))
> +             be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
> +     else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_32))
> +             be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_32;
> +     else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_X86_64))
> +             be->blkif->blk_protocol = BLKIF_PROTOCOL_X86_64;
> +     else {
> +             xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol);
> +             return -1;
> +     }
> +     err = xenbus_gather(XBT_NIL, dev->otherend,
> +                         "feature-persistent", "%u",
> +                         &pers_grants, NULL);
> +     if (err)
> +             pers_grants = 0;
> +
> +     be->blkif->vbd.feature_gnt_persistent = pers_grants;
> +     be->blkif->vbd.overflow_max_grants = 0;
> +
> +     pr_info("%s: using %d queues, protocol %d (%s) %s\n", dev->nodename,
> +              be->blkif->nr_rings, be->blkif->blk_protocol, protocol,
> +              pers_grants ? "persistent grants" : "");
> +
> +     if (be->blkif->nr_rings == 1)
> +             return read_per_ring_refs(&be->blkif->rings[0], dev->otherend);
> +     else {
> +             xspathsize = strlen(dev->otherend) + xenstore_path_ext_size;
> +             xspath = kmalloc(xspathsize, GFP_KERNEL);
> +             if (!xspath) {
> +                     xenbus_dev_fatal(dev, -ENOMEM, "reading ring 
> references");
> +                     return -ENOMEM;
> +             }
> +
> +             for (i = 0; i < be->blkif->nr_rings; i++) {
> +                     memset(xspath, 0, xspathsize);
> +                     snprintf(xspath, xspathsize, "%s/queue-%u", 
> dev->otherend, i);
> +                     err = read_per_ring_refs(&be->blkif->rings[i], xspath);

Say nr_rings is 4 and this fails at the last one. That means
be->blkif->rings[0..2].pending_free has a bunch of pages and
also ring->blk_ring are set. We return out of this function
and end back in (frontend_changed):
 752                 err = connect_ring(be);
 753                 if (err)
 754                         break;

Great. So we have a memory leak until the device goes in
XenbusStateConnected (where we end up calling xen_blkif_disconnect
and free ring[0..2]..

But that may take a while if the guest is not nice. Perhaps we should
add in  frontend_changed(..) an call to xen_blkif_disconnect in case
we fail at 'connect_ring' to clear the memory faster. I will prep a
patch for that.

> +                     if (err) {
> +                             kfree(xspath);
> +                             return err;
> +                     }
> +             }
> +             kfree(xspath);
> +     }
> +     return 0;
>  }
>  
>  static const struct xenbus_device_id xen_blkbk_ids[] = {
> -- 
> 1.7.10.4
> 

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