Re: [Xen-devel] [PATCH] xen-blk: persistent-grants fixes

[Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx>]

On Fri, Nov 02, 2012 at 04:43:04PM +0100, Roger Pau Monne wrote:
> This patch contains fixes for persistent grants implementation v2:
> 
>  * handle == 0 is a valid handle, so initialize grants in blkback
>    setting the handle to BLKBACK_INVALID_HANDLE instead of 0. Reported
>    by Konrad Rzeszutek Wilk.
> 
>  * new_map is a boolean, use "true" or "false" instead of 1 and 0.
>    Reported by Konrad Rzeszutek Wilk.
> 
>  * blkfront announces the persistent-grants feature as
>    feature-persistent-grants, use feature-persistent instead which is
>    consistent with blkback and the public Xen headers.
> 
>  * Add a consistency check in blkfront to make sure we don't try to
>    access segments that have not been set.

Looks good.. but
> 
> Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Cc: <konrad.wilk@xxxxxxxxxx>
> Cc: <linux-kernel@xxxxxxxxxxxxxxx>
> ---
>  drivers/block/xen-blkback/blkback.c |   15 +++++++++------
>  drivers/block/xen-blkback/xenbus.c  |    2 +-
>  drivers/block/xen-blkfront.c        |    3 ++-
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/blkback.c 
> b/drivers/block/xen-blkback/blkback.c
> index 663d42d..a059616 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -503,7 +503,7 @@ static int xen_blkbk_map(struct blkif_request *req,
>                        * We are using persistent grants and
>                        * the grant is already mapped
>                        */
> -                     new_map = 0;
> +                     new_map = false;
>               } else if (use_persistent_gnts &&
>                          blkif->persistent_gnt_c <
>                          max_mapped_grant_pages(blkif->blk_protocol)) {
> @@ -511,8 +511,8 @@ static int xen_blkbk_map(struct blkif_request *req,
>                        * We are using persistent grants, the grant is
>                        * not mapped but we have room for it
>                        */
> -                     new_map = 1;
> -                     persistent_gnt = kzalloc(
> +                     new_map = true;
> +                     persistent_gnt = kmalloc(

Why do we want to use kmalloc instead of kzalloc?

>                               sizeof(struct persistent_gnt),
>                               GFP_KERNEL);
>                       if (!persistent_gnt)
> @@ -523,6 +523,7 @@ static int xen_blkbk_map(struct blkif_request *req,
>                               return -ENOMEM;
>                       }
>                       persistent_gnt->gnt = req->u.rw.seg[i].gref;
> +                     persistent_gnt->handle = BLKBACK_INVALID_HANDLE;
>  
>                       pages_to_gnt[segs_to_map] =
>                               persistent_gnt->page;
> @@ -547,7 +548,7 @@ static int xen_blkbk_map(struct blkif_request *req,
>                               pr_alert(DRV_PFX " domain %u, device %#x is 
> using maximum number of persistent grants\n",
>                                        blkif->domid, blkif->vbd.handle);
>                       }
> -                     new_map = 1;
> +                     new_map = true;
>                       pages[i] = blkbk->pending_page(pending_req, i);
>                       addr = vaddr(pending_req, i);
>                       pages_to_gnt[segs_to_map] =
> @@ -584,7 +585,8 @@ static int xen_blkbk_map(struct blkif_request *req,
>        */
>       bitmap_zero(pending_req->unmap_seg, BLKIF_MAX_SEGMENTS_PER_REQUEST);
>       for (i = 0, j = 0; i < nseg; i++) {
> -             if (!persistent_gnts[i] || !persistent_gnts[i]->handle) {
> +             if (!persistent_gnts[i] ||
> +                 persistent_gnts[i]->handle == BLKBACK_INVALID_HANDLE) {
>                       /* This is a newly mapped grant */
>                       BUG_ON(j >= segs_to_map);
>                       if (unlikely(map[j].status != 0)) {
> @@ -601,7 +603,8 @@ static int xen_blkbk_map(struct blkif_request *req,
>                       }
>               }
>               if (persistent_gnts[i]) {
> -                     if (!persistent_gnts[i]->handle) {
> +                     if (persistent_gnts[i]->handle ==
> +                         BLKBACK_INVALID_HANDLE) {
>                               /*
>                                * If this is a new persistent grant
>                                * save the handler
> diff --git a/drivers/block/xen-blkback/xenbus.c 
> b/drivers/block/xen-blkback/xenbus.c
> index b225026..a03ecbb 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -760,7 +760,7 @@ static int connect_ring(struct backend_info *be)
>               return -1;
>       }
>       err = xenbus_gather(XBT_NIL, dev->otherend,
> -                         "feature-persistent-grants", "%u",
> +                         "feature-persistent", "%u",
>                           &pers_grants, NULL);
>       if (err)
>               pers_grants = 0;
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 911d733..f1de806 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -852,6 +852,7 @@ static void blkif_completion(struct blk_shadow *s, struct 
> blkfront_info *info,
>               rq_for_each_segment(bvec, s->request, iter) {
>                       BUG_ON((bvec->bv_offset + bvec->bv_len) > PAGE_SIZE);
>                       i = offset >> PAGE_SHIFT;
> +                     BUG_ON(i >= s->req.u.rw.nr_segments);
>                       shared_data = kmap_atomic(
>                               pfn_to_page(s->grants_used[i]->pfn));
>                       bvec_data = bvec_kmap_irq(bvec, &flags);
> @@ -1069,7 +1070,7 @@ again:
>               goto abort_transaction;
>       }
>       err = xenbus_printf(xbt, dev->nodename,
> -                         "feature-persistent-grants", "%u", 1);
> +                         "feature-persistent", "%u", 1);
>       if (err)
>               dev_warn(&dev->dev,
>                        "writing persistent grants feature to xenbus");
> -- 
> 1.7.7.5 (Apple Git-26)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

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