[PATCH 0/3] xen: fixes for gnt and block

[PATCH 0/3] xen: fixes for gnt and block

[Roger Pau Monne]

This series contain a small bugfix for the grant table code (patch 1) 
and a couple of improvements to blkfront (patches 2 and 3) to make it 
work better if there's a shortage on available free grants.

[PATCH 1/3] xen-gnt: prevent adding duplicate gnt callbacks

[Roger Pau Monne]

With the current implementation, the callback in the tail of the list
can be added twice, because the check done in
gnttab_request_free_callback is bogus, callback->next can be NULL if
it is the last callback in the list. If we add the same callback twice
we end up with an infinite loop, were callback == callback->next.

Replace this check with a proper one that iterates over the list to
see if the callback has already been added.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
This patch should be backported to stable trees
---
 drivers/xen/grant-table.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 04c1b2d..d5418c1 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -729,9 +729,18 @@ void gnttab_request_free_callback(struct gnttab_free_callback *callback,
 				  void (*fn)(void *), void *arg, u16 count)
 {
 	unsigned long flags;
+	struct gnttab_free_callback *cb;
+
 	spin_lock_irqsave(&gnttab_list_lock, flags);
-	if (callback->next)
-		goto out;
+
+	/* Check if the callback is already on the list */
+	cb = gnttab_free_callback_list;
+	while (cb) {
+		if (cb == callback)
+			goto out;
+		cb = cb->next;
+	}
+
 	callback->fn = fn;
 	callback->arg = arg;
 	callback->count = count;
-- 
1.7.7.5 (Apple Git-26)

Re: [PATCH 1/3] xen-gnt: prevent adding duplicate gnt callbacks

[Matt Wilson]

On Wed, Jul 31, 2013 at 05:00:42PM +0200, Roger Pau Monne wrote:
> With the current implementation, the callback in the tail of the list
> can be added twice, because the check done in
> gnttab_request_free_callback is bogus, callback->next can be NULL if
> it is the last callback in the list. If we add the same callback twice
> we end up with an infinite loop, were callback == callback->next.
> 
> Replace this check with a proper one that iterates over the list to
> see if the callback has already been added.

Acked-by: Matt Wilson <msw@amazon.com>

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> ---
> This patch should be backported to stable trees
> ---
>  drivers/xen/grant-table.c |   13 +++++++++++--
>  1 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 04c1b2d..d5418c1 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -729,9 +729,18 @@ void gnttab_request_free_callback(struct gnttab_free_callback *callback,
>  				  void (*fn)(void *), void *arg, u16 count)
>  {
>  	unsigned long flags;
> +	struct gnttab_free_callback *cb;
> +
>  	spin_lock_irqsave(&gnttab_list_lock, flags);
> -	if (callback->next)
> -		goto out;
> +
> +	/* Check if the callback is already on the list */
> +	cb = gnttab_free_callback_list;
> +	while (cb) {
> +		if (cb == callback)
> +			goto out;
> +		cb = cb->next;
> +	}
> +
>  	callback->fn = fn;
>  	callback->arg = arg;
>  	callback->count = count;

Re: [PATCH 1/3] xen-gnt: prevent adding duplicate gnt callbacks

[Matt Wilson]

On Wed, Jul 31, 2013 at 05:00:42PM +0200, Roger Pau Monne wrote:
> With the current implementation, the callback in the tail of the list
> can be added twice, because the check done in
> gnttab_request_free_callback is bogus, callback->next can be NULL if
> it is the last callback in the list. If we add the same callback twice
> we end up with an infinite loop, were callback == callback->next.
> 
> Replace this check with a proper one that iterates over the list to
> see if the callback has already been added.

Acked-by: Matt Wilson <msw@amazon.com>

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> ---
> This patch should be backported to stable trees
> ---
>  drivers/xen/grant-table.c |   13 +++++++++++--
>  1 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 04c1b2d..d5418c1 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -729,9 +729,18 @@ void gnttab_request_free_callback(struct gnttab_free_callback *callback,
>  				  void (*fn)(void *), void *arg, u16 count)
>  {
>  	unsigned long flags;
> +	struct gnttab_free_callback *cb;
> +
>  	spin_lock_irqsave(&gnttab_list_lock, flags);
> -	if (callback->next)
> -		goto out;
> +
> +	/* Check if the callback is already on the list */
> +	cb = gnttab_free_callback_list;
> +	while (cb) {
> +		if (cb == callback)
> +			goto out;
> +		cb = cb->next;
> +	}
> +
>  	callback->fn = fn;
>  	callback->arg = arg;
>  	callback->count = count;

Re: [PATCH 1/3] xen-gnt: prevent adding duplicate gnt callbacks

[David Vrabel]

On 31/07/13 16:00, Roger Pau Monne wrote:
> With the current implementation, the callback in the tail of the list
> can be added twice, because the check done in
> gnttab_request_free_callback is bogus, callback->next can be NULL if
> it is the last callback in the list. If we add the same callback twice
> we end up with an infinite loop, were callback == callback->next.
> 
> Replace this check with a proper one that iterates over the list to
> see if the callback has already been added.

As a minimal fix suitable for stable,

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [PATCH 1/3] xen-gnt: prevent adding duplicate gnt callbacks

[David Vrabel]

On 31/07/13 16:00, Roger Pau Monne wrote:
> With the current implementation, the callback in the tail of the list
> can be added twice, because the check done in
> gnttab_request_free_callback is bogus, callback->next can be NULL if
> it is the last callback in the list. If we add the same callback twice
> we end up with an infinite loop, were callback == callback->next.
> 
> Replace this check with a proper one that iterates over the list to
> see if the callback has already been added.

As a minimal fix suitable for stable,

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

[PATCH 1/3] xen-gnt: prevent adding duplicate gnt callbacks

[Roger Pau Monne]

With the current implementation, the callback in the tail of the list
can be added twice, because the check done in
gnttab_request_free_callback is bogus, callback->next can be NULL if
it is the last callback in the list. If we add the same callback twice
we end up with an infinite loop, were callback == callback->next.

Replace this check with a proper one that iterates over the list to
see if the callback has already been added.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
This patch should be backported to stable trees
---
 drivers/xen/grant-table.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 04c1b2d..d5418c1 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -729,9 +729,18 @@ void gnttab_request_free_callback(struct gnttab_free_callback *callback,
 				  void (*fn)(void *), void *arg, u16 count)
 {
 	unsigned long flags;
+	struct gnttab_free_callback *cb;
+
 	spin_lock_irqsave(&gnttab_list_lock, flags);
-	if (callback->next)
-		goto out;
+
+	/* Check if the callback is already on the list */
+	cb = gnttab_free_callback_list;
+	while (cb) {
+		if (cb == callback)
+			goto out;
+		cb = cb->next;
+	}
+
 	callback->fn = fn;
 	callback->arg = arg;
 	callback->count = count;
-- 
1.7.7.5 (Apple Git-26)


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

[PATCH 2/3] xen-blkfront: improve aproximation of required grants per request

[Roger Pau Monne]

Improve the calculation of required grants to process a request by
using nr_phys_segments instead of always assuming a request is going
to use all posible segments.

nr_phys_segments contains the number of scatter-gather DMA addr+len
pairs, which is basically what we put at every granted page.
for_each_sg iterates over the DMA addr+len pairs and uses a grant
page for each of them.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
 drivers/block/xen-blkfront.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 2e1ee34..187a437 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -400,10 +400,13 @@ static int blkif_queue_request(struct request *req)
 	if (unlikely(info->connected != BLKIF_STATE_CONNECTED))
 		return 1;
 
-	max_grefs = info->max_indirect_segments ?
-		    info->max_indirect_segments +
-		    INDIRECT_GREFS(info->max_indirect_segments) :
-		    BLKIF_MAX_SEGMENTS_PER_REQUEST;
+	max_grefs = req->nr_phys_segments;
+	if (max_grefs > BLKIF_MAX_SEGMENTS_PER_REQUEST)
+		/*
+		 * If we are using indirect segments we need to account
+		 * for the indirect grefs used in the request.
+		 */
+		max_grefs += INDIRECT_GREFS(req->nr_phys_segments);
 
 	/* Check if we have enough grants to allocate a requests */
 	if (info->persistent_gnts_c < max_grefs) {
-- 
1.7.7.5 (Apple Git-26)


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

[PATCH 2/3] xen-blkfront: improve aproximation of required grants per request

[Roger Pau Monne]

Improve the calculation of required grants to process a request by
using nr_phys_segments instead of always assuming a request is going
to use all posible segments.

nr_phys_segments contains the number of scatter-gather DMA addr+len
pairs, which is basically what we put at every granted page.
for_each_sg iterates over the DMA addr+len pairs and uses a grant
page for each of them.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
 drivers/block/xen-blkfront.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 2e1ee34..187a437 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -400,10 +400,13 @@ static int blkif_queue_request(struct request *req)
 	if (unlikely(info->connected != BLKIF_STATE_CONNECTED))
 		return 1;
 
-	max_grefs = info->max_indirect_segments ?
-		    info->max_indirect_segments +
-		    INDIRECT_GREFS(info->max_indirect_segments) :
-		    BLKIF_MAX_SEGMENTS_PER_REQUEST;
+	max_grefs = req->nr_phys_segments;
+	if (max_grefs > BLKIF_MAX_SEGMENTS_PER_REQUEST)
+		/*
+		 * If we are using indirect segments we need to account
+		 * for the indirect grefs used in the request.
+		 */
+		max_grefs += INDIRECT_GREFS(req->nr_phys_segments);
 
 	/* Check if we have enough grants to allocate a requests */
 	if (info->persistent_gnts_c < max_grefs) {
-- 
1.7.7.5 (Apple Git-26)

Re: [Xen-devel] [PATCH 2/3] xen-blkfront: improve aproximation of required grants per request

[David Vrabel]

On 31/07/13 16:00, Roger Pau Monne wrote:
> Improve the calculation of required grants to process a request by
> using nr_phys_segments instead of always assuming a request is going
> to use all posible segments.
> 
> nr_phys_segments contains the number of scatter-gather DMA addr+len
> pairs, which is basically what we put at every granted page.
> for_each_sg iterates over the DMA addr+len pairs and uses a grant
> page for each of them.

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [PATCH 2/3] xen-blkfront: improve aproximation of required grants per request

[David Vrabel]

On 31/07/13 16:00, Roger Pau Monne wrote:
> Improve the calculation of required grants to process a request by
> using nr_phys_segments instead of always assuming a request is going
> to use all posible segments.
> 
> nr_phys_segments contains the number of scatter-gather DMA addr+len
> pairs, which is basically what we put at every granted page.
> for_each_sg iterates over the DMA addr+len pairs and uses a grant
> page for each of them.

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

[PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend

[Roger Pau Monne]

There's no need to keep the foreign access in a grant if it is not
persistently mapped by the backend. This allows us to free grants that
are not mapped by the backend, thus preventing blkfront from hoarding
all grants.

The main effect of this is that blkfront will only persistently map
the same grants as the backend, and it will always try to use grants
that are already mapped by the backend. Also the number of persistent
grants in blkfront is the same as in blkback (and is controlled by the
value in blkback).

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
 drivers/block/xen-blkfront.c |   33 +++++++++++++++++++++++++++++----
 1 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 187a437..bc9fc7d 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info,
 	}
 	/* Add the persistent grant into the list of free grants */
 	for (i = 0; i < nseg; i++) {
-		list_add(&s->grants_used[i]->node, &info->persistent_gnts);
-		info->persistent_gnts_c++;
+		if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
+			/*
+			 * If the grant is still mapped by the backend (the
+			 * backend has chosen to make this grant persistent)
+			 * we add it at the head of the list, so it will be
+			 * reused first.
+			 */
+			list_add(&s->grants_used[i]->node, &info->persistent_gnts);
+			info->persistent_gnts_c++;
+		} else {
+			/*
+			 * If the grant is not mapped by the backend we end the
+			 * foreign access and add it to the tail of the list,
+			 * so it will not be picked again unless we run out of
+			 * persistent grants.
+			 */
+			gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
+			s->grants_used[i]->gref = GRANT_INVALID_REF;
+			list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts);
+		}
 	}
 	if (s->req.operation == BLKIF_OP_INDIRECT) {
 		for (i = 0; i < INDIRECT_GREFS(nseg); i++) {
-			list_add(&s->indirect_grants[i]->node, &info->persistent_gnts);
-			info->persistent_gnts_c++;
+			if (gnttab_query_foreign_access(s->indirect_grants[i]->gref)) {
+				list_add(&s->indirect_grants[i]->node, &info->persistent_gnts);
+				info->persistent_gnts_c++;
+			} else {
+				gnttab_end_foreign_access(s->indirect_grants[i]->gref, 0, 0UL);
+				s->indirect_grants[i]->gref = GRANT_INVALID_REF;
+				list_add_tail(&s->indirect_grants[i]->node,
+				              &info->persistent_gnts);
+			}
 		}
 	}
 }
-- 
1.7.7.5 (Apple Git-26)


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

[PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend

[Roger Pau Monne]

There's no need to keep the foreign access in a grant if it is not
persistently mapped by the backend. This allows us to free grants that
are not mapped by the backend, thus preventing blkfront from hoarding
all grants.

The main effect of this is that blkfront will only persistently map
the same grants as the backend, and it will always try to use grants
that are already mapped by the backend. Also the number of persistent
grants in blkfront is the same as in blkback (and is controlled by the
value in blkback).

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
 drivers/block/xen-blkfront.c |   33 +++++++++++++++++++++++++++++----
 1 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 187a437..bc9fc7d 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info,
 	}
 	/* Add the persistent grant into the list of free grants */
 	for (i = 0; i < nseg; i++) {
-		list_add(&s->grants_used[i]->node, &info->persistent_gnts);
-		info->persistent_gnts_c++;
+		if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
+			/*
+			 * If the grant is still mapped by the backend (the
+			 * backend has chosen to make this grant persistent)
+			 * we add it at the head of the list, so it will be
+			 * reused first.
+			 */
+			list_add(&s->grants_used[i]->node, &info->persistent_gnts);
+			info->persistent_gnts_c++;
+		} else {
+			/*
+			 * If the grant is not mapped by the backend we end the
+			 * foreign access and add it to the tail of the list,
+			 * so it will not be picked again unless we run out of
+			 * persistent grants.
+			 */
+			gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
+			s->grants_used[i]->gref = GRANT_INVALID_REF;
+			list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts);
+		}
 	}
 	if (s->req.operation == BLKIF_OP_INDIRECT) {
 		for (i = 0; i < INDIRECT_GREFS(nseg); i++) {
-			list_add(&s->indirect_grants[i]->node, &info->persistent_gnts);
-			info->persistent_gnts_c++;
+			if (gnttab_query_foreign_access(s->indirect_grants[i]->gref)) {
+				list_add(&s->indirect_grants[i]->node, &info->persistent_gnts);
+				info->persistent_gnts_c++;
+			} else {
+				gnttab_end_foreign_access(s->indirect_grants[i]->gref, 0, 0UL);
+				s->indirect_grants[i]->gref = GRANT_INVALID_REF;
+				list_add_tail(&s->indirect_grants[i]->node,
+				              &info->persistent_gnts);
+			}
 		}
 	}
 }
-- 
1.7.7.5 (Apple Git-26)

Re: [PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend

[Matt Wilson]

On Wed, Jul 31, 2013 at 05:00:44PM +0200, Roger Pau Monne wrote:
> There's no need to keep the foreign access in a grant if it is not
> persistently mapped by the backend. This allows us to free grants that
> are not mapped by the backend, thus preventing blkfront from hoarding
> all grants.
> 
> The main effect of this is that blkfront will only persistently map
> the same grants as the backend, and it will always try to use grants
> that are already mapped by the backend. Also the number of persistent
> grants in blkfront is the same as in blkback (and is controlled by the
> value in blkback).
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Matt Wilson <msw@amazon.com>

> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> ---
>  drivers/block/xen-blkfront.c |   33 +++++++++++++++++++++++++++++----
>  1 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 187a437..bc9fc7d 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info,
>  	}
>  	/* Add the persistent grant into the list of free grants */
>  	for (i = 0; i < nseg; i++) {
> -		list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> -		info->persistent_gnts_c++;
> +		if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
> +			/*
> +			 * If the grant is still mapped by the backend (the
> +			 * backend has chosen to make this grant persistent)
> +			 * we add it at the head of the list, so it will be
> +			 * reused first.
> +			 */
> +			list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> +			info->persistent_gnts_c++;
> +		} else {
> +			/*
> +			 * If the grant is not mapped by the backend we end the
> +			 * foreign access and add it to the tail of the list,
> +			 * so it will not be picked again unless we run out of
> +			 * persistent grants.
> +			 */
> +			gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
> +			s->grants_used[i]->gref = GRANT_INVALID_REF;
> +			list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts);
> +		}
>  	}
>  	if (s->req.operation == BLKIF_OP_INDIRECT) {
>  		for (i = 0; i < INDIRECT_GREFS(nseg); i++) {
> -			list_add(&s->indirect_grants[i]->node, &info->persistent_gnts);
> -			info->persistent_gnts_c++;
> +			if (gnttab_query_foreign_access(s->indirect_grants[i]->gref)) {
> +				list_add(&s->indirect_grants[i]->node, &info->persistent_gnts);
> +				info->persistent_gnts_c++;
> +			} else {
> +				gnttab_end_foreign_access(s->indirect_grants[i]->gref, 0, 0UL);
> +				s->indirect_grants[i]->gref = GRANT_INVALID_REF;
> +				list_add_tail(&s->indirect_grants[i]->node,
> +				              &info->persistent_gnts);
> +			}
>  		}
>  	}
>  }

Re: [PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend

[Matt Wilson]

On Wed, Jul 31, 2013 at 05:00:44PM +0200, Roger Pau Monne wrote:
> There's no need to keep the foreign access in a grant if it is not
> persistently mapped by the backend. This allows us to free grants that
> are not mapped by the backend, thus preventing blkfront from hoarding
> all grants.
> 
> The main effect of this is that blkfront will only persistently map
> the same grants as the backend, and it will always try to use grants
> that are already mapped by the backend. Also the number of persistent
> grants in blkfront is the same as in blkback (and is controlled by the
> value in blkback).
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Matt Wilson <msw@amazon.com>

> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> ---
>  drivers/block/xen-blkfront.c |   33 +++++++++++++++++++++++++++++----
>  1 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 187a437..bc9fc7d 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info,
>  	}
>  	/* Add the persistent grant into the list of free grants */
>  	for (i = 0; i < nseg; i++) {
> -		list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> -		info->persistent_gnts_c++;
> +		if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
> +			/*
> +			 * If the grant is still mapped by the backend (the
> +			 * backend has chosen to make this grant persistent)
> +			 * we add it at the head of the list, so it will be
> +			 * reused first.
> +			 */
> +			list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> +			info->persistent_gnts_c++;
> +		} else {
> +			/*
> +			 * If the grant is not mapped by the backend we end the
> +			 * foreign access and add it to the tail of the list,
> +			 * so it will not be picked again unless we run out of
> +			 * persistent grants.
> +			 */
> +			gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
> +			s->grants_used[i]->gref = GRANT_INVALID_REF;
> +			list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts);
> +		}
>  	}
>  	if (s->req.operation == BLKIF_OP_INDIRECT) {
>  		for (i = 0; i < INDIRECT_GREFS(nseg); i++) {
> -			list_add(&s->indirect_grants[i]->node, &info->persistent_gnts);
> -			info->persistent_gnts_c++;
> +			if (gnttab_query_foreign_access(s->indirect_grants[i]->gref)) {
> +				list_add(&s->indirect_grants[i]->node, &info->persistent_gnts);
> +				info->persistent_gnts_c++;
> +			} else {
> +				gnttab_end_foreign_access(s->indirect_grants[i]->gref, 0, 0UL);
> +				s->indirect_grants[i]->gref = GRANT_INVALID_REF;
> +				list_add_tail(&s->indirect_grants[i]->node,
> +				              &info->persistent_gnts);
> +			}
>  		}
>  	}
>  }

Re: [Xen-devel] [PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend

[David Vrabel]

On 31/07/13 16:00, Roger Pau Monne wrote:
> There's no need to keep the foreign access in a grant if it is not
> persistently mapped by the backend. This allows us to free grants that
> are not mapped by the backend, thus preventing blkfront from hoarding
> all grants.
> 
> The main effect of this is that blkfront will only persistently map
> the same grants as the backend, and it will always try to use grants
> that are already mapped by the backend. Also the number of persistent
> grants in blkfront is the same as in blkback (and is controlled by the
> value in blkback).
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

But please see the documentation updates needed below.

> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info,
>  	}
>  	/* Add the persistent grant into the list of free grants */
>  	for (i = 0; i < nseg; i++) {
> -		list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> -		info->persistent_gnts_c++;
> +		if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
> +			/*
> +			 * If the grant is still mapped by the backend (the
> +			 * backend has chosen to make this grant persistent)
> +			 * we add it at the head of the list, so it will be
> +			 * reused first.
> +			 */
> +			list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> +			info->persistent_gnts_c++;
> +		} else {
> +			/*
> +			 * If the grant is not mapped by the backend we end the
> +			 * foreign access and add it to the tail of the list,
> +			 * so it will not be picked again unless we run out of
> +			 * persistent grants.
> +			 */
> +			gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
> +			s->grants_used[i]->gref = GRANT_INVALID_REF;
> +			list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts);
> +		}

Because you only reclaim grants when a request is completed, can you add
text similar to the following to xen/include/public/io/blkif.h (and the
Linux copy).

feature-persistent:
...
   When the backend driver needs to unmap a persistent grant it should
   do so prior to completing a request that used that grant reference.
   If a persistent grant is unmapped at any other time, the frontend
   driver may not notice and may be unable to reclaim the grant
   reference.

Thanks.

David

Re: [PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend

[Konrad Rzeszutek Wilk]

On Thu, Aug 01, 2013 at 11:10:15AM +0100, David Vrabel wrote:
> On 31/07/13 16:00, Roger Pau Monne wrote:
> > There's no need to keep the foreign access in a grant if it is not
> > persistently mapped by the backend. This allows us to free grants that
> > are not mapped by the backend, thus preventing blkfront from hoarding
> > all grants.
> > 
> > The main effect of this is that blkfront will only persistently map
> > the same grants as the backend, and it will always try to use grants
> > that are already mapped by the backend. Also the number of persistent
> > grants in blkfront is the same as in blkback (and is controlled by the
> > value in blkback).
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: David Vrabel <david.vrabel@citrix.com>

Roger,

Could you repost patch #2 and #3 (as #1 is in v3.11-rc4) with the
comments and the Ack from Matt and Roger's Review-by tag addressed?

Thanks.
> 
> But please see the documentation updates needed below.
> 
> > --- a/drivers/block/xen-blkfront.c
> > +++ b/drivers/block/xen-blkfront.c
> > @@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info,
> >  	}
> >  	/* Add the persistent grant into the list of free grants */
> >  	for (i = 0; i < nseg; i++) {
> > -		list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> > -		info->persistent_gnts_c++;
> > +		if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
> > +			/*
> > +			 * If the grant is still mapped by the backend (the
> > +			 * backend has chosen to make this grant persistent)
> > +			 * we add it at the head of the list, so it will be
> > +			 * reused first.
> > +			 */
> > +			list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> > +			info->persistent_gnts_c++;
> > +		} else {
> > +			/*
> > +			 * If the grant is not mapped by the backend we end the
> > +			 * foreign access and add it to the tail of the list,
> > +			 * so it will not be picked again unless we run out of
> > +			 * persistent grants.
> > +			 */
> > +			gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
> > +			s->grants_used[i]->gref = GRANT_INVALID_REF;
> > +			list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts);
> > +		}
> 
> Because you only reclaim grants when a request is completed, can you add
> text similar to the following to xen/include/public/io/blkif.h (and the
> Linux copy).
> 
> feature-persistent:
> ...
>    When the backend driver needs to unmap a persistent grant it should
>    do so prior to completing a request that used that grant reference.
>    If a persistent grant is unmapped at any other time, the frontend
>    driver may not notice and may be unable to reclaim the grant
>    reference.
> 
> Thanks.
> 
> David
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend

[Konrad Rzeszutek Wilk]

On Thu, Aug 01, 2013 at 11:10:15AM +0100, David Vrabel wrote:
> On 31/07/13 16:00, Roger Pau Monne wrote:
> > There's no need to keep the foreign access in a grant if it is not
> > persistently mapped by the backend. This allows us to free grants that
> > are not mapped by the backend, thus preventing blkfront from hoarding
> > all grants.
> > 
> > The main effect of this is that blkfront will only persistently map
> > the same grants as the backend, and it will always try to use grants
> > that are already mapped by the backend. Also the number of persistent
> > grants in blkfront is the same as in blkback (and is controlled by the
> > value in blkback).
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: David Vrabel <david.vrabel@citrix.com>

Roger,

Could you repost patch #2 and #3 (as #1 is in v3.11-rc4) with the
comments and the Ack from Matt and Roger's Review-by tag addressed?

Thanks.
> 
> But please see the documentation updates needed below.
> 
> > --- a/drivers/block/xen-blkfront.c
> > +++ b/drivers/block/xen-blkfront.c
> > @@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info,
> >  	}
> >  	/* Add the persistent grant into the list of free grants */
> >  	for (i = 0; i < nseg; i++) {
> > -		list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> > -		info->persistent_gnts_c++;
> > +		if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
> > +			/*
> > +			 * If the grant is still mapped by the backend (the
> > +			 * backend has chosen to make this grant persistent)
> > +			 * we add it at the head of the list, so it will be
> > +			 * reused first.
> > +			 */
> > +			list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> > +			info->persistent_gnts_c++;
> > +		} else {
> > +			/*
> > +			 * If the grant is not mapped by the backend we end the
> > +			 * foreign access and add it to the tail of the list,
> > +			 * so it will not be picked again unless we run out of
> > +			 * persistent grants.
> > +			 */
> > +			gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
> > +			s->grants_used[i]->gref = GRANT_INVALID_REF;
> > +			list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts);
> > +		}
> 
> Because you only reclaim grants when a request is completed, can you add
> text similar to the following to xen/include/public/io/blkif.h (and the
> Linux copy).
> 
> feature-persistent:
> ...
>    When the backend driver needs to unmap a persistent grant it should
>    do so prior to completing a request that used that grant reference.
>    If a persistent grant is unmapped at any other time, the frontend
>    driver may not notice and may be unable to reclaim the grant
>    reference.
> 
> Thanks.
> 
> David
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend

[Roger Pau Monné]

On 09/08/13 17:08, Konrad Rzeszutek Wilk wrote:
> On Thu, Aug 01, 2013 at 11:10:15AM +0100, David Vrabel wrote:
>> On 31/07/13 16:00, Roger Pau Monne wrote:
>>> There's no need to keep the foreign access in a grant if it is not
>>> persistently mapped by the backend. This allows us to free grants that
>>> are not mapped by the backend, thus preventing blkfront from hoarding
>>> all grants.
>>>
>>> The main effect of this is that blkfront will only persistently map
>>> the same grants as the backend, and it will always try to use grants
>>> that are already mapped by the backend. Also the number of persistent
>>> grants in blkfront is the same as in blkback (and is controlled by the
>>> value in blkback).
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Reviewed-by: David Vrabel <david.vrabel@citrix.com>
> 
> Roger,
> 
> Could you repost patch #2 and #3 (as #1 is in v3.11-rc4) with the
> comments and the Ack from Matt and Roger's Review-by tag addressed?

Done (I guess you meant the Reviewed-by tag from David), see below however.

> Thanks.
>>
>> But please see the documentation updates needed below.
>>
>>> --- a/drivers/block/xen-blkfront.c
>>> +++ b/drivers/block/xen-blkfront.c
>>> @@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info,
>>>  	}
>>>  	/* Add the persistent grant into the list of free grants */
>>>  	for (i = 0; i < nseg; i++) {
>>> -		list_add(&s->grants_used[i]->node, &info->persistent_gnts);
>>> -		info->persistent_gnts_c++;
>>> +		if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
>>> +			/*
>>> +			 * If the grant is still mapped by the backend (the
>>> +			 * backend has chosen to make this grant persistent)
>>> +			 * we add it at the head of the list, so it will be
>>> +			 * reused first.
>>> +			 */
>>> +			list_add(&s->grants_used[i]->node, &info->persistent_gnts);
>>> +			info->persistent_gnts_c++;
>>> +		} else {
>>> +			/*
>>> +			 * If the grant is not mapped by the backend we end the
>>> +			 * foreign access and add it to the tail of the list,
>>> +			 * so it will not be picked again unless we run out of
>>> +			 * persistent grants.
>>> +			 */
>>> +			gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
>>> +			s->grants_used[i]->gref = GRANT_INVALID_REF;
>>> +			list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts);
>>> +		}
>>
>> Because you only reclaim grants when a request is completed, can you add
>> text similar to the following to xen/include/public/io/blkif.h (and the
>> Linux copy).
>>
>> feature-persistent:
>> ...
>>    When the backend driver needs to unmap a persistent grant it should
>>    do so prior to completing a request that used that grant reference.
>>    If a persistent grant is unmapped at any other time, the frontend
>>    driver may not notice and may be unable to reclaim the grant
>>    reference.
>>

I've sent a patch to update the documentation about persistent grants in
Xen source tree, but the Linux copy of blkif.h lacks any information
about the blkif protocol extensions, so I have not added it there. If
nobody else does it first (before I come back from vacation), I will
send a patch to add the documentation about all the blkif extensions to
the Linux copy of blkif.h (and of course add your comment about
persistent grants unmap).

Roger.

Re: [Xen-devel] [PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend

[Roger Pau Monné]

On 09/08/13 17:08, Konrad Rzeszutek Wilk wrote:
> On Thu, Aug 01, 2013 at 11:10:15AM +0100, David Vrabel wrote:
>> On 31/07/13 16:00, Roger Pau Monne wrote:
>>> There's no need to keep the foreign access in a grant if it is not
>>> persistently mapped by the backend. This allows us to free grants that
>>> are not mapped by the backend, thus preventing blkfront from hoarding
>>> all grants.
>>>
>>> The main effect of this is that blkfront will only persistently map
>>> the same grants as the backend, and it will always try to use grants
>>> that are already mapped by the backend. Also the number of persistent
>>> grants in blkfront is the same as in blkback (and is controlled by the
>>> value in blkback).
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Reviewed-by: David Vrabel <david.vrabel@citrix.com>
> 
> Roger,
> 
> Could you repost patch #2 and #3 (as #1 is in v3.11-rc4) with the
> comments and the Ack from Matt and Roger's Review-by tag addressed?

Done (I guess you meant the Reviewed-by tag from David), see below however.

> Thanks.
>>
>> But please see the documentation updates needed below.
>>
>>> --- a/drivers/block/xen-blkfront.c
>>> +++ b/drivers/block/xen-blkfront.c
>>> @@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info,
>>>  	}
>>>  	/* Add the persistent grant into the list of free grants */
>>>  	for (i = 0; i < nseg; i++) {
>>> -		list_add(&s->grants_used[i]->node, &info->persistent_gnts);
>>> -		info->persistent_gnts_c++;
>>> +		if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
>>> +			/*
>>> +			 * If the grant is still mapped by the backend (the
>>> +			 * backend has chosen to make this grant persistent)
>>> +			 * we add it at the head of the list, so it will be
>>> +			 * reused first.
>>> +			 */
>>> +			list_add(&s->grants_used[i]->node, &info->persistent_gnts);
>>> +			info->persistent_gnts_c++;
>>> +		} else {
>>> +			/*
>>> +			 * If the grant is not mapped by the backend we end the
>>> +			 * foreign access and add it to the tail of the list,
>>> +			 * so it will not be picked again unless we run out of
>>> +			 * persistent grants.
>>> +			 */
>>> +			gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
>>> +			s->grants_used[i]->gref = GRANT_INVALID_REF;
>>> +			list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts);
>>> +		}
>>
>> Because you only reclaim grants when a request is completed, can you add
>> text similar to the following to xen/include/public/io/blkif.h (and the
>> Linux copy).
>>
>> feature-persistent:
>> ...
>>    When the backend driver needs to unmap a persistent grant it should
>>    do so prior to completing a request that used that grant reference.
>>    If a persistent grant is unmapped at any other time, the frontend
>>    driver may not notice and may be unable to reclaim the grant
>>    reference.
>>

I've sent a patch to update the documentation about persistent grants in
Xen source tree, but the Linux copy of blkif.h lacks any information
about the blkif protocol extensions, so I have not added it there. If
nobody else does it first (before I come back from vacation), I will
send a patch to add the documentation about all the blkif extensions to
the Linux copy of blkif.h (and of course add your comment about
persistent grants unmap).

Roger.

Re: [Xen-devel] [PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend

[Konrad Rzeszutek Wilk]

On Mon, Aug 12, 2013 at 12:58:39PM +0200, Roger Pau Monné wrote:
> On 09/08/13 17:08, Konrad Rzeszutek Wilk wrote:
> > On Thu, Aug 01, 2013 at 11:10:15AM +0100, David Vrabel wrote:
> >> On 31/07/13 16:00, Roger Pau Monne wrote:
> >>> There's no need to keep the foreign access in a grant if it is not
> >>> persistently mapped by the backend. This allows us to free grants that
> >>> are not mapped by the backend, thus preventing blkfront from hoarding
> >>> all grants.
> >>>
> >>> The main effect of this is that blkfront will only persistently map
> >>> the same grants as the backend, and it will always try to use grants
> >>> that are already mapped by the backend. Also the number of persistent
> >>> grants in blkfront is the same as in blkback (and is controlled by the
> >>> value in blkback).
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Reviewed-by: David Vrabel <david.vrabel@citrix.com>
> > 
> > Roger,
> > 
> > Could you repost patch #2 and #3 (as #1 is in v3.11-rc4) with the
> > comments and the Ack from Matt and Roger's Review-by tag addressed?
> 
> Done (I guess you meant the Reviewed-by tag from David), see below however.
> 
> > Thanks.
> >>
> >> But please see the documentation updates needed below.
> >>
> >>> --- a/drivers/block/xen-blkfront.c
> >>> +++ b/drivers/block/xen-blkfront.c
> >>> @@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info,
> >>>  	}
> >>>  	/* Add the persistent grant into the list of free grants */
> >>>  	for (i = 0; i < nseg; i++) {
> >>> -		list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> >>> -		info->persistent_gnts_c++;
> >>> +		if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
> >>> +			/*
> >>> +			 * If the grant is still mapped by the backend (the
> >>> +			 * backend has chosen to make this grant persistent)
> >>> +			 * we add it at the head of the list, so it will be
> >>> +			 * reused first.
> >>> +			 */
> >>> +			list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> >>> +			info->persistent_gnts_c++;
> >>> +		} else {
> >>> +			/*
> >>> +			 * If the grant is not mapped by the backend we end the
> >>> +			 * foreign access and add it to the tail of the list,
> >>> +			 * so it will not be picked again unless we run out of
> >>> +			 * persistent grants.
> >>> +			 */
> >>> +			gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
> >>> +			s->grants_used[i]->gref = GRANT_INVALID_REF;
> >>> +			list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts);
> >>> +		}
> >>
> >> Because you only reclaim grants when a request is completed, can you add
> >> text similar to the following to xen/include/public/io/blkif.h (and the
> >> Linux copy).
> >>
> >> feature-persistent:
> >> ...
> >>    When the backend driver needs to unmap a persistent grant it should
> >>    do so prior to completing a request that used that grant reference.
> >>    If a persistent grant is unmapped at any other time, the frontend
> >>    driver may not notice and may be unable to reclaim the grant
> >>    reference.
> >>
> 
> I've sent a patch to update the documentation about persistent grants in
> Xen source tree, but the Linux copy of blkif.h lacks any information
> about the blkif protocol extensions, so I have not added it there. If
> nobody else does it first (before I come back from vacation), I will
> send a patch to add the documentation about all the blkif extensions to
> the Linux copy of blkif.h (and of course add your comment about
> persistent grants unmap).

Please do. It would be good to resync that header file. Thanks.

Re: [PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend

[Konrad Rzeszutek Wilk]

On Mon, Aug 12, 2013 at 12:58:39PM +0200, Roger Pau Monné wrote:
> On 09/08/13 17:08, Konrad Rzeszutek Wilk wrote:
> > On Thu, Aug 01, 2013 at 11:10:15AM +0100, David Vrabel wrote:
> >> On 31/07/13 16:00, Roger Pau Monne wrote:
> >>> There's no need to keep the foreign access in a grant if it is not
> >>> persistently mapped by the backend. This allows us to free grants that
> >>> are not mapped by the backend, thus preventing blkfront from hoarding
> >>> all grants.
> >>>
> >>> The main effect of this is that blkfront will only persistently map
> >>> the same grants as the backend, and it will always try to use grants
> >>> that are already mapped by the backend. Also the number of persistent
> >>> grants in blkfront is the same as in blkback (and is controlled by the
> >>> value in blkback).
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Reviewed-by: David Vrabel <david.vrabel@citrix.com>
> > 
> > Roger,
> > 
> > Could you repost patch #2 and #3 (as #1 is in v3.11-rc4) with the
> > comments and the Ack from Matt and Roger's Review-by tag addressed?
> 
> Done (I guess you meant the Reviewed-by tag from David), see below however.
> 
> > Thanks.
> >>
> >> But please see the documentation updates needed below.
> >>
> >>> --- a/drivers/block/xen-blkfront.c
> >>> +++ b/drivers/block/xen-blkfront.c
> >>> @@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info,
> >>>  	}
> >>>  	/* Add the persistent grant into the list of free grants */
> >>>  	for (i = 0; i < nseg; i++) {
> >>> -		list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> >>> -		info->persistent_gnts_c++;
> >>> +		if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
> >>> +			/*
> >>> +			 * If the grant is still mapped by the backend (the
> >>> +			 * backend has chosen to make this grant persistent)
> >>> +			 * we add it at the head of the list, so it will be
> >>> +			 * reused first.
> >>> +			 */
> >>> +			list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> >>> +			info->persistent_gnts_c++;
> >>> +		} else {
> >>> +			/*
> >>> +			 * If the grant is not mapped by the backend we end the
> >>> +			 * foreign access and add it to the tail of the list,
> >>> +			 * so it will not be picked again unless we run out of
> >>> +			 * persistent grants.
> >>> +			 */
> >>> +			gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
> >>> +			s->grants_used[i]->gref = GRANT_INVALID_REF;
> >>> +			list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts);
> >>> +		}
> >>
> >> Because you only reclaim grants when a request is completed, can you add
> >> text similar to the following to xen/include/public/io/blkif.h (and the
> >> Linux copy).
> >>
> >> feature-persistent:
> >> ...
> >>    When the backend driver needs to unmap a persistent grant it should
> >>    do so prior to completing a request that used that grant reference.
> >>    If a persistent grant is unmapped at any other time, the frontend
> >>    driver may not notice and may be unable to reclaim the grant
> >>    reference.
> >>
> 
> I've sent a patch to update the documentation about persistent grants in
> Xen source tree, but the Linux copy of blkif.h lacks any information
> about the blkif protocol extensions, so I have not added it there. If
> nobody else does it first (before I come back from vacation), I will
> send a patch to add the documentation about all the blkif extensions to
> the Linux copy of blkif.h (and of course add your comment about
> persistent grants unmap).

Please do. It would be good to resync that header file. Thanks.

Re: [PATCH 3/3] xen-blkfront: revoke foreign access for grants not mapped by the backend

[David Vrabel]

On 31/07/13 16:00, Roger Pau Monne wrote:
> There's no need to keep the foreign access in a grant if it is not
> persistently mapped by the backend. This allows us to free grants that
> are not mapped by the backend, thus preventing blkfront from hoarding
> all grants.
> 
> The main effect of this is that blkfront will only persistently map
> the same grants as the backend, and it will always try to use grants
> that are already mapped by the backend. Also the number of persistent
> grants in blkfront is the same as in blkback (and is controlled by the
> value in blkback).
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

But please see the documentation updates needed below.

> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1016,13 +1016,38 @@ static void blkif_completion(struct blk_shadow *s, struct blkfront_info *info,
>  	}
>  	/* Add the persistent grant into the list of free grants */
>  	for (i = 0; i < nseg; i++) {
> -		list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> -		info->persistent_gnts_c++;
> +		if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
> +			/*
> +			 * If the grant is still mapped by the backend (the
> +			 * backend has chosen to make this grant persistent)
> +			 * we add it at the head of the list, so it will be
> +			 * reused first.
> +			 */
> +			list_add(&s->grants_used[i]->node, &info->persistent_gnts);
> +			info->persistent_gnts_c++;
> +		} else {
> +			/*
> +			 * If the grant is not mapped by the backend we end the
> +			 * foreign access and add it to the tail of the list,
> +			 * so it will not be picked again unless we run out of
> +			 * persistent grants.
> +			 */
> +			gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
> +			s->grants_used[i]->gref = GRANT_INVALID_REF;
> +			list_add_tail(&s->grants_used[i]->node, &info->persistent_gnts);
> +		}

Because you only reclaim grants when a request is completed, can you add
text similar to the following to xen/include/public/io/blkif.h (and the
Linux copy).

feature-persistent:
...
   When the backend driver needs to unmap a persistent grant it should
   do so prior to completing a request that used that grant reference.
   If a persistent grant is unmapped at any other time, the frontend
   driver may not notice and may be unable to reclaim the grant
   reference.

Thanks.

David

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