* [PATCH] xen-blk: persistent-grants fixes
@ 2012-11-02 15:43 Roger Pau Monne
2012-11-02 17:05 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 3+ messages in thread
From: Roger Pau Monne @ 2012-11-02 15:43 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, konrad.wilk, linux-kernel
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.
Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
Cc: <konrad.wilk@oracle.com>
Cc: <linux-kernel@vger.kernel.org>
---
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(
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)
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] xen-blk: persistent-grants fixes
2012-11-02 15:43 [PATCH] xen-blk: persistent-grants fixes Roger Pau Monne
@ 2012-11-02 17:05 ` Konrad Rzeszutek Wilk
2012-11-02 20:13 ` Roger Pau Monné
0 siblings, 1 reply; 3+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-11-02 17:05 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, konrad.wilk, linux-kernel
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@citrix.com>
> Cc: <konrad.wilk@oracle.com>
> Cc: <linux-kernel@vger.kernel.org>
> ---
> 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] xen-blk: persistent-grants fixes
2012-11-02 17:05 ` Konrad Rzeszutek Wilk
@ 2012-11-02 20:13 ` Roger Pau Monné
0 siblings, 0 replies; 3+ messages in thread
From: Roger Pau Monné @ 2012-11-02 20:13 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: xen-devel@lists.xen.org, konrad.wilk@oracle.com,
linux-kernel@vger.kernel.org
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@citrix.com>
>> Cc: <konrad.wilk@oracle.com>
>> Cc: <linux-kernel@vger.kernel.org>
>> ---
>> 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@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-11-02 20:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-02 15:43 [PATCH] xen-blk: persistent-grants fixes Roger Pau Monne
2012-11-02 17:05 ` Konrad Rzeszutek Wilk
2012-11-02 20:13 ` Roger Pau Monné
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox