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

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



On 02/11/12 18:05, Konrad Rzeszutek Wilk wrote:
> 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?

In the previous patch I relied on handle being 0 when not set (that's
why I was using kzalloc), but this was wrong (0 is a valid handle), so
now I explicitly initialize it to BLKBACK_INVALID_HANDLE. I replaced the
kzalloc with kmalloc because it should be faster, and there's no need to
zero it anymore.

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


 


Rackspace

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