[PATCH RESEND 1/2] xen/blkback: safely unmap purge persistent grants

[PATCH RESEND 1/2] xen/blkback: safely unmap purge persistent grants

[Bob Liu]

Commit c43cf3ea8385 ("xen-blkback: safely unmap grants in case they are still
in use") use gnttab_unmap_refs_async() to wait until the mapped pages are no
longer in use before unmapping them, but that commit missed the persistent case.
Purge persistent pages can't be unmapped either unless no longer in use.

Signed-off-by: Bob Liu <bob.liu@oracle.com>
---
 drivers/block/xen-blkback/blkback.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 2a04d34..f59d7c3 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -327,8 +327,18 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
 	struct gnttab_unmap_grant_ref unmap[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 	struct page *pages[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 	struct persistent_gnt *persistent_gnt;
-	int ret, segs_to_unmap = 0;
+	int segs_to_unmap = 0;
 	struct xen_blkif *blkif = container_of(work, typeof(*blkif), persistent_purge_work);
+	struct gntab_unmap_queue_data unmap_data;
+	struct completion unmap_completion;
+
+	init_completion(&unmap_completion);
+
+	unmap_data.data = &unmap_completion;
+	unmap_data.done = &free_persistent_gnts_unmap_callback;
+	unmap_data.pages = pages;
+	unmap_data.unmap_ops = unmap;
+	unmap_data.kunmap_ops = NULL;
 
 	while(!list_empty(&blkif->persistent_purge_list)) {
 		persistent_gnt = list_first_entry(&blkif->persistent_purge_list,
@@ -344,17 +354,19 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
 		pages[segs_to_unmap] = persistent_gnt->page;
 
 		if (++segs_to_unmap == BLKIF_MAX_SEGMENTS_PER_REQUEST) {
-			ret = gnttab_unmap_refs(unmap, NULL, pages,
-				segs_to_unmap);
-			BUG_ON(ret);
+			unmap_data.count = segs_to_unmap;
+			gnttab_unmap_refs_async(&unmap_data);
+			wait_for_completion(&unmap_completion);
+
 			put_free_pages(blkif, pages, segs_to_unmap);
 			segs_to_unmap = 0;
 		}
 		kfree(persistent_gnt);
 	}
 	if (segs_to_unmap > 0) {
-		ret = gnttab_unmap_refs(unmap, NULL, pages, segs_to_unmap);
-		BUG_ON(ret);
+		unmap_data.count = segs_to_unmap;
+		gnttab_unmap_refs_async(&unmap_data);
+		wait_for_completion(&unmap_completion);
 		put_free_pages(blkif, pages, segs_to_unmap);
 	}
 }
-- 
1.8.3.1

[PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[Bob Liu]

There are several place using gnttab async unmap and wait for
completion, so move the common code to a function
gnttab_unmap_refs_async_wait_completion().

Signed-off-by: Bob Liu <bob.liu@oracle.com>
---
 drivers/block/xen-blkback/blkback.c | 31 +++----------------------------
 drivers/xen/gntdev.c                | 28 +++-------------------------
 drivers/xen/grant-table.c           | 29 +++++++++++++++++++++++++++++
 include/xen/grant_table.h           |  1 +
 4 files changed, 36 insertions(+), 53 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index f59d7c3..0c8da82 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -263,17 +263,6 @@ static void put_persistent_gnt(struct xen_blkif *blkif,
 	atomic_dec(&blkif->persistent_gnt_in_use);
 }
 
-static void free_persistent_gnts_unmap_callback(int result,
-						struct gntab_unmap_queue_data *data)
-{
-	struct completion *c = data->data;
-
-	/* BUG_ON used to reproduce existing behaviour,
-	   but is this the best way to deal with this? */
-	BUG_ON(result);
-	complete(c);
-}
-
 static void free_persistent_gnts(struct xen_blkif *blkif, struct rb_root *root,
                                  unsigned int num)
 {
@@ -283,12 +272,7 @@ static void free_persistent_gnts(struct xen_blkif *blkif, struct rb_root *root,
 	struct rb_node *n;
 	int segs_to_unmap = 0;
 	struct gntab_unmap_queue_data unmap_data;
-	struct completion unmap_completion;
-
-	init_completion(&unmap_completion);
 
-	unmap_data.data = &unmap_completion;
-	unmap_data.done = &free_persistent_gnts_unmap_callback;
 	unmap_data.pages = pages;
 	unmap_data.unmap_ops = unmap;
 	unmap_data.kunmap_ops = NULL;
@@ -308,8 +292,7 @@ static void free_persistent_gnts(struct xen_blkif *blkif, struct rb_root *root,
 			!rb_next(&persistent_gnt->node)) {
 
 			unmap_data.count = segs_to_unmap;
-			gnttab_unmap_refs_async(&unmap_data);
-			wait_for_completion(&unmap_completion);
+			BUG_ON(gnttab_unmap_refs_async_wait_completion(&unmap_data));
 
 			put_free_pages(blkif, pages, segs_to_unmap);
 			segs_to_unmap = 0;
@@ -330,12 +313,7 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
 	int segs_to_unmap = 0;
 	struct xen_blkif *blkif = container_of(work, typeof(*blkif), persistent_purge_work);
 	struct gntab_unmap_queue_data unmap_data;
-	struct completion unmap_completion;
 
-	init_completion(&unmap_completion);
-
-	unmap_data.data = &unmap_completion;
-	unmap_data.done = &free_persistent_gnts_unmap_callback;
 	unmap_data.pages = pages;
 	unmap_data.unmap_ops = unmap;
 	unmap_data.kunmap_ops = NULL;
@@ -355,9 +333,7 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
 
 		if (++segs_to_unmap == BLKIF_MAX_SEGMENTS_PER_REQUEST) {
 			unmap_data.count = segs_to_unmap;
-			gnttab_unmap_refs_async(&unmap_data);
-			wait_for_completion(&unmap_completion);
-
+			BUG_ON(gnttab_unmap_refs_async_wait_completion(&unmap_data));
 			put_free_pages(blkif, pages, segs_to_unmap);
 			segs_to_unmap = 0;
 		}
@@ -365,8 +341,7 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
 	}
 	if (segs_to_unmap > 0) {
 		unmap_data.count = segs_to_unmap;
-		gnttab_unmap_refs_async(&unmap_data);
-		wait_for_completion(&unmap_completion);
+		BUG_ON(gnttab_unmap_refs_async_wait_completion(&unmap_data));
 		put_free_pages(blkif, pages, segs_to_unmap);
 	}
 }
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index d5bb1a3..7a88524 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -327,30 +327,10 @@ static int map_grant_pages(struct grant_map *map)
 	return err;
 }
 
-struct unmap_grant_pages_callback_data
-{
-	struct completion completion;
-	int result;
-};
-
-static void unmap_grant_callback(int result,
-				 struct gntab_unmap_queue_data *data)
-{
-	struct unmap_grant_pages_callback_data* d = data->data;
-
-	d->result = result;
-	complete(&d->completion);
-}
-
 static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
 {
 	int i, err = 0;
 	struct gntab_unmap_queue_data unmap_data;
-	struct unmap_grant_pages_callback_data data;
-
-	init_completion(&data.completion);
-	unmap_data.data = &data;
-	unmap_data.done= &unmap_grant_callback;
 
 	if (map->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) {
 		int pgno = (map->notify.addr >> PAGE_SHIFT);
@@ -367,11 +347,9 @@ static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
 	unmap_data.pages = map->pages + offset;
 	unmap_data.count = pages;
 
-	gnttab_unmap_refs_async(&unmap_data);
-
-	wait_for_completion(&data.completion);
-	if (data.result)
-		return data.result;
+	err = gnttab_unmap_refs_async_wait_completion(&unmap_data);
+	if (err)
+		return err;
 
 	for (i = 0; i < pages; i++) {
 		if (map->unmap_ops[offset+i].status)
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 17972fb..1dce9bc 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -123,6 +123,12 @@ struct gnttab_ops {
 	int (*query_foreign_access)(grant_ref_t ref);
 };
 
+struct unmap_refs_async_callback_data
+{
+	struct completion completion;
+	int result;
+};
+
 static struct gnttab_ops *gnttab_interface;
 
 static int grant_table_version;
@@ -863,6 +869,29 @@ void gnttab_unmap_refs_async(struct gntab_unmap_queue_data* item)
 }
 EXPORT_SYMBOL_GPL(gnttab_unmap_refs_async);
 
+static void unmap_refs_async_callback(int result,
+		struct gntab_unmap_queue_data *data)
+{
+	struct unmap_refs_async_callback_data* d = data->data;
+
+	d->result = result;
+	complete(&d->completion);
+}
+
+int gnttab_unmap_refs_async_wait_completion(struct gntab_unmap_queue_data* item)
+{
+	struct unmap_refs_async_callback_data data;
+
+	init_completion(&data.completion);
+	item->data = &data;
+	item->done= &unmap_refs_async_callback;
+	gnttab_unmap_refs_async(item);
+	wait_for_completion(&data.completion);
+
+	return data.result;
+}
+EXPORT_SYMBOL_GPL(gnttab_unmap_refs_async_wait_completion);
+
 static int gnttab_map_frames_v1(xen_pfn_t *frames, unsigned int nr_gframes)
 {
 	int rc;
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index 143ca5f..46bad05 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -191,6 +191,7 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
 		      struct gnttab_unmap_grant_ref *kunmap_ops,
 		      struct page **pages, unsigned int count);
 void gnttab_unmap_refs_async(struct gntab_unmap_queue_data* item);
+int gnttab_unmap_refs_async_wait_completion(struct gntab_unmap_queue_data* item);
 
 
 /* Perform a batch of grant map/copy operations. Retry every batch slot
-- 
1.8.3.1

[PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[Bob Liu]

There are several place using gnttab async unmap and wait for
completion, so move the common code to a function
gnttab_unmap_refs_async_wait_completion().

Signed-off-by: Bob Liu <bob.liu@oracle.com>
---
 drivers/block/xen-blkback/blkback.c | 31 +++----------------------------
 drivers/xen/gntdev.c                | 28 +++-------------------------
 drivers/xen/grant-table.c           | 29 +++++++++++++++++++++++++++++
 include/xen/grant_table.h           |  1 +
 4 files changed, 36 insertions(+), 53 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index f59d7c3..0c8da82 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -263,17 +263,6 @@ static void put_persistent_gnt(struct xen_blkif *blkif,
 	atomic_dec(&blkif->persistent_gnt_in_use);
 }
 
-static void free_persistent_gnts_unmap_callback(int result,
-						struct gntab_unmap_queue_data *data)
-{
-	struct completion *c = data->data;
-
-	/* BUG_ON used to reproduce existing behaviour,
-	   but is this the best way to deal with this? */
-	BUG_ON(result);
-	complete(c);
-}
-
 static void free_persistent_gnts(struct xen_blkif *blkif, struct rb_root *root,
                                  unsigned int num)
 {
@@ -283,12 +272,7 @@ static void free_persistent_gnts(struct xen_blkif *blkif, struct rb_root *root,
 	struct rb_node *n;
 	int segs_to_unmap = 0;
 	struct gntab_unmap_queue_data unmap_data;
-	struct completion unmap_completion;
-
-	init_completion(&unmap_completion);
 
-	unmap_data.data = &unmap_completion;
-	unmap_data.done = &free_persistent_gnts_unmap_callback;
 	unmap_data.pages = pages;
 	unmap_data.unmap_ops = unmap;
 	unmap_data.kunmap_ops = NULL;
@@ -308,8 +292,7 @@ static void free_persistent_gnts(struct xen_blkif *blkif, struct rb_root *root,
 			!rb_next(&persistent_gnt->node)) {
 
 			unmap_data.count = segs_to_unmap;
-			gnttab_unmap_refs_async(&unmap_data);
-			wait_for_completion(&unmap_completion);
+			BUG_ON(gnttab_unmap_refs_async_wait_completion(&unmap_data));
 
 			put_free_pages(blkif, pages, segs_to_unmap);
 			segs_to_unmap = 0;
@@ -330,12 +313,7 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
 	int segs_to_unmap = 0;
 	struct xen_blkif *blkif = container_of(work, typeof(*blkif), persistent_purge_work);
 	struct gntab_unmap_queue_data unmap_data;
-	struct completion unmap_completion;
 
-	init_completion(&unmap_completion);
-
-	unmap_data.data = &unmap_completion;
-	unmap_data.done = &free_persistent_gnts_unmap_callback;
 	unmap_data.pages = pages;
 	unmap_data.unmap_ops = unmap;
 	unmap_data.kunmap_ops = NULL;
@@ -355,9 +333,7 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
 
 		if (++segs_to_unmap == BLKIF_MAX_SEGMENTS_PER_REQUEST) {
 			unmap_data.count = segs_to_unmap;
-			gnttab_unmap_refs_async(&unmap_data);
-			wait_for_completion(&unmap_completion);
-
+			BUG_ON(gnttab_unmap_refs_async_wait_completion(&unmap_data));
 			put_free_pages(blkif, pages, segs_to_unmap);
 			segs_to_unmap = 0;
 		}
@@ -365,8 +341,7 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
 	}
 	if (segs_to_unmap > 0) {
 		unmap_data.count = segs_to_unmap;
-		gnttab_unmap_refs_async(&unmap_data);
-		wait_for_completion(&unmap_completion);
+		BUG_ON(gnttab_unmap_refs_async_wait_completion(&unmap_data));
 		put_free_pages(blkif, pages, segs_to_unmap);
 	}
 }
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index d5bb1a3..7a88524 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -327,30 +327,10 @@ static int map_grant_pages(struct grant_map *map)
 	return err;
 }
 
-struct unmap_grant_pages_callback_data
-{
-	struct completion completion;
-	int result;
-};
-
-static void unmap_grant_callback(int result,
-				 struct gntab_unmap_queue_data *data)
-{
-	struct unmap_grant_pages_callback_data* d = data->data;
-
-	d->result = result;
-	complete(&d->completion);
-}
-
 static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
 {
 	int i, err = 0;
 	struct gntab_unmap_queue_data unmap_data;
-	struct unmap_grant_pages_callback_data data;
-
-	init_completion(&data.completion);
-	unmap_data.data = &data;
-	unmap_data.done= &unmap_grant_callback;
 
 	if (map->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) {
 		int pgno = (map->notify.addr >> PAGE_SHIFT);
@@ -367,11 +347,9 @@ static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
 	unmap_data.pages = map->pages + offset;
 	unmap_data.count = pages;
 
-	gnttab_unmap_refs_async(&unmap_data);
-
-	wait_for_completion(&data.completion);
-	if (data.result)
-		return data.result;
+	err = gnttab_unmap_refs_async_wait_completion(&unmap_data);
+	if (err)
+		return err;
 
 	for (i = 0; i < pages; i++) {
 		if (map->unmap_ops[offset+i].status)
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 17972fb..1dce9bc 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -123,6 +123,12 @@ struct gnttab_ops {
 	int (*query_foreign_access)(grant_ref_t ref);
 };
 
+struct unmap_refs_async_callback_data
+{
+	struct completion completion;
+	int result;
+};
+
 static struct gnttab_ops *gnttab_interface;
 
 static int grant_table_version;
@@ -863,6 +869,29 @@ void gnttab_unmap_refs_async(struct gntab_unmap_queue_data* item)
 }
 EXPORT_SYMBOL_GPL(gnttab_unmap_refs_async);
 
+static void unmap_refs_async_callback(int result,
+		struct gntab_unmap_queue_data *data)
+{
+	struct unmap_refs_async_callback_data* d = data->data;
+
+	d->result = result;
+	complete(&d->completion);
+}
+
+int gnttab_unmap_refs_async_wait_completion(struct gntab_unmap_queue_data* item)
+{
+	struct unmap_refs_async_callback_data data;
+
+	init_completion(&data.completion);
+	item->data = &data;
+	item->done= &unmap_refs_async_callback;
+	gnttab_unmap_refs_async(item);
+	wait_for_completion(&data.completion);
+
+	return data.result;
+}
+EXPORT_SYMBOL_GPL(gnttab_unmap_refs_async_wait_completion);
+
 static int gnttab_map_frames_v1(xen_pfn_t *frames, unsigned int nr_gframes)
 {
 	int rc;
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index 143ca5f..46bad05 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -191,6 +191,7 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
 		      struct gnttab_unmap_grant_ref *kunmap_ops,
 		      struct page **pages, unsigned int count);
 void gnttab_unmap_refs_async(struct gntab_unmap_queue_data* item);
+int gnttab_unmap_refs_async_wait_completion(struct gntab_unmap_queue_data* item);
 
 
 /* Perform a batch of grant map/copy operations. Retry every batch slot
-- 
1.8.3.1

Re: [PATCH RESEND 1/2] xen/blkback: safely unmap purge persistent grants

[Roger Pau Monné]

El 26/03/15 a les 13.16, Bob Liu ha escrit:
> Commit c43cf3ea8385 ("xen-blkback: safely unmap grants in case they are still
> in use") use gnttab_unmap_refs_async() to wait until the mapped pages are no
> longer in use before unmapping them, but that commit missed the persistent case.
> Purge persistent pages can't be unmapped either unless no longer in use.
> 
> Signed-off-by: Bob Liu <bob.liu@oracle.com>

AFAICT since this is basically the same patch as what I've sent:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Re: [PATCH RESEND 1/2] xen/blkback: safely unmap purge persistent grants

[Roger Pau Monné]

El 26/03/15 a les 13.16, Bob Liu ha escrit:
> Commit c43cf3ea8385 ("xen-blkback: safely unmap grants in case they are still
> in use") use gnttab_unmap_refs_async() to wait until the mapped pages are no
> longer in use before unmapping them, but that commit missed the persistent case.
> Purge persistent pages can't be unmapped either unless no longer in use.
> 
> Signed-off-by: Bob Liu <bob.liu@oracle.com>

AFAICT since this is basically the same patch as what I've sent:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Re: [PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[Roger Pau Monné]

El 26/03/15 a les 13.16, Bob Liu ha escrit:
> There are several place using gnttab async unmap and wait for
> completion, so move the common code to a function
> gnttab_unmap_refs_async_wait_completion().
> 
> Signed-off-by: Bob Liu <bob.liu@oracle.com>

For the blkback parts:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Although, as David already said, I think we should do better than BUG_ON
on error.

Re: [PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[Roger Pau Monné]

El 26/03/15 a les 13.16, Bob Liu ha escrit:
> There are several place using gnttab async unmap and wait for
> completion, so move the common code to a function
> gnttab_unmap_refs_async_wait_completion().
> 
> Signed-off-by: Bob Liu <bob.liu@oracle.com>

For the blkback parts:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Although, as David already said, I think we should do better than BUG_ON
on error.

Re: [PATCH RESEND 1/2] xen/blkback: safely unmap purge persistent grants

[Konrad Rzeszutek Wilk]

On Thu, Mar 26, 2015 at 04:23:27PM +0100, Roger Pau Monné wrote:
> El 26/03/15 a les 13.16, Bob Liu ha escrit:
> > Commit c43cf3ea8385 ("xen-blkback: safely unmap grants in case they are still
> > in use") use gnttab_unmap_refs_async() to wait until the mapped pages are no
> > longer in use before unmapping them, but that commit missed the persistent case.
> > Purge persistent pages can't be unmapped either unless no longer in use.
> > 
> > Signed-off-by: Bob Liu <bob.liu@oracle.com>
> 
> AFAICT since this is basically the same patch as what I've sent:
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>

Hey Jens,

These patches are on top of patches that add this framework in the Xen tree.
Would you be OK with this patch (and the other which I will CC shortly)
to go through the Xen tree?

Thanks. 

Re: [PATCH RESEND 1/2] xen/blkback: safely unmap purge persistent grants

[Konrad Rzeszutek Wilk]

On Thu, Mar 26, 2015 at 04:23:27PM +0100, Roger Pau Monné wrote:
> El 26/03/15 a les 13.16, Bob Liu ha escrit:
> > Commit c43cf3ea8385 ("xen-blkback: safely unmap grants in case they are still
> > in use") use gnttab_unmap_refs_async() to wait until the mapped pages are no
> > longer in use before unmapping them, but that commit missed the persistent case.
> > Purge persistent pages can't be unmapped either unless no longer in use.
> > 
> > Signed-off-by: Bob Liu <bob.liu@oracle.com>
> 
> AFAICT since this is basically the same patch as what I've sent:
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>

Hey Jens,

These patches are on top of patches that add this framework in the Xen tree.
Would you be OK with this patch (and the other which I will CC shortly)
to go through the Xen tree?

Thanks. 

Re: [PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[Konrad Rzeszutek Wilk]

On Thu, Mar 26, 2015 at 08:16:01PM +0800, Bob Liu wrote:
> There are several place using gnttab async unmap and wait for
> completion, so move the common code to a function
> gnttab_unmap_refs_async_wait_completion().
> 
> Signed-off-by: Bob Liu <bob.liu@oracle.com>

Jens, and would you be OK if this patch went through the Xen tree?

Thanks.
> ---
>  drivers/block/xen-blkback/blkback.c | 31 +++----------------------------
>  drivers/xen/gntdev.c                | 28 +++-------------------------
>  drivers/xen/grant-table.c           | 29 +++++++++++++++++++++++++++++
>  include/xen/grant_table.h           |  1 +
>  4 files changed, 36 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> index f59d7c3..0c8da82 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -263,17 +263,6 @@ static void put_persistent_gnt(struct xen_blkif *blkif,
>  	atomic_dec(&blkif->persistent_gnt_in_use);
>  }
>  
> -static void free_persistent_gnts_unmap_callback(int result,
> -						struct gntab_unmap_queue_data *data)
> -{
> -	struct completion *c = data->data;
> -
> -	/* BUG_ON used to reproduce existing behaviour,
> -	   but is this the best way to deal with this? */
> -	BUG_ON(result);
> -	complete(c);
> -}
> -
>  static void free_persistent_gnts(struct xen_blkif *blkif, struct rb_root *root,
>                                   unsigned int num)
>  {
> @@ -283,12 +272,7 @@ static void free_persistent_gnts(struct xen_blkif *blkif, struct rb_root *root,
>  	struct rb_node *n;
>  	int segs_to_unmap = 0;
>  	struct gntab_unmap_queue_data unmap_data;
> -	struct completion unmap_completion;
> -
> -	init_completion(&unmap_completion);
>  
> -	unmap_data.data = &unmap_completion;
> -	unmap_data.done = &free_persistent_gnts_unmap_callback;
>  	unmap_data.pages = pages;
>  	unmap_data.unmap_ops = unmap;
>  	unmap_data.kunmap_ops = NULL;
> @@ -308,8 +292,7 @@ static void free_persistent_gnts(struct xen_blkif *blkif, struct rb_root *root,
>  			!rb_next(&persistent_gnt->node)) {
>  
>  			unmap_data.count = segs_to_unmap;
> -			gnttab_unmap_refs_async(&unmap_data);
> -			wait_for_completion(&unmap_completion);
> +			BUG_ON(gnttab_unmap_refs_async_wait_completion(&unmap_data));
>  
>  			put_free_pages(blkif, pages, segs_to_unmap);
>  			segs_to_unmap = 0;
> @@ -330,12 +313,7 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
>  	int segs_to_unmap = 0;
>  	struct xen_blkif *blkif = container_of(work, typeof(*blkif), persistent_purge_work);
>  	struct gntab_unmap_queue_data unmap_data;
> -	struct completion unmap_completion;
>  
> -	init_completion(&unmap_completion);
> -
> -	unmap_data.data = &unmap_completion;
> -	unmap_data.done = &free_persistent_gnts_unmap_callback;
>  	unmap_data.pages = pages;
>  	unmap_data.unmap_ops = unmap;
>  	unmap_data.kunmap_ops = NULL;
> @@ -355,9 +333,7 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
>  
>  		if (++segs_to_unmap == BLKIF_MAX_SEGMENTS_PER_REQUEST) {
>  			unmap_data.count = segs_to_unmap;
> -			gnttab_unmap_refs_async(&unmap_data);
> -			wait_for_completion(&unmap_completion);
> -
> +			BUG_ON(gnttab_unmap_refs_async_wait_completion(&unmap_data));
>  			put_free_pages(blkif, pages, segs_to_unmap);
>  			segs_to_unmap = 0;
>  		}
> @@ -365,8 +341,7 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
>  	}
>  	if (segs_to_unmap > 0) {
>  		unmap_data.count = segs_to_unmap;
> -		gnttab_unmap_refs_async(&unmap_data);
> -		wait_for_completion(&unmap_completion);
> +		BUG_ON(gnttab_unmap_refs_async_wait_completion(&unmap_data));
>  		put_free_pages(blkif, pages, segs_to_unmap);
>  	}
>  }
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index d5bb1a3..7a88524 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -327,30 +327,10 @@ static int map_grant_pages(struct grant_map *map)
>  	return err;
>  }
>  
> -struct unmap_grant_pages_callback_data
> -{
> -	struct completion completion;
> -	int result;
> -};
> -
> -static void unmap_grant_callback(int result,
> -				 struct gntab_unmap_queue_data *data)
> -{
> -	struct unmap_grant_pages_callback_data* d = data->data;
> -
> -	d->result = result;
> -	complete(&d->completion);
> -}
> -
>  static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
>  {
>  	int i, err = 0;
>  	struct gntab_unmap_queue_data unmap_data;
> -	struct unmap_grant_pages_callback_data data;
> -
> -	init_completion(&data.completion);
> -	unmap_data.data = &data;
> -	unmap_data.done= &unmap_grant_callback;
>  
>  	if (map->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) {
>  		int pgno = (map->notify.addr >> PAGE_SHIFT);
> @@ -367,11 +347,9 @@ static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
>  	unmap_data.pages = map->pages + offset;
>  	unmap_data.count = pages;
>  
> -	gnttab_unmap_refs_async(&unmap_data);
> -
> -	wait_for_completion(&data.completion);
> -	if (data.result)
> -		return data.result;
> +	err = gnttab_unmap_refs_async_wait_completion(&unmap_data);
> +	if (err)
> +		return err;
>  
>  	for (i = 0; i < pages; i++) {
>  		if (map->unmap_ops[offset+i].status)
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 17972fb..1dce9bc 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -123,6 +123,12 @@ struct gnttab_ops {
>  	int (*query_foreign_access)(grant_ref_t ref);
>  };
>  
> +struct unmap_refs_async_callback_data
> +{
> +	struct completion completion;
> +	int result;
> +};
> +
>  static struct gnttab_ops *gnttab_interface;
>  
>  static int grant_table_version;
> @@ -863,6 +869,29 @@ void gnttab_unmap_refs_async(struct gntab_unmap_queue_data* item)
>  }
>  EXPORT_SYMBOL_GPL(gnttab_unmap_refs_async);
>  
> +static void unmap_refs_async_callback(int result,
> +		struct gntab_unmap_queue_data *data)
> +{
> +	struct unmap_refs_async_callback_data* d = data->data;
> +
> +	d->result = result;
> +	complete(&d->completion);
> +}
> +
> +int gnttab_unmap_refs_async_wait_completion(struct gntab_unmap_queue_data* item)
> +{
> +	struct unmap_refs_async_callback_data data;
> +
> +	init_completion(&data.completion);
> +	item->data = &data;
> +	item->done= &unmap_refs_async_callback;
> +	gnttab_unmap_refs_async(item);
> +	wait_for_completion(&data.completion);
> +
> +	return data.result;
> +}
> +EXPORT_SYMBOL_GPL(gnttab_unmap_refs_async_wait_completion);
> +
>  static int gnttab_map_frames_v1(xen_pfn_t *frames, unsigned int nr_gframes)
>  {
>  	int rc;
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index 143ca5f..46bad05 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -191,6 +191,7 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>  		      struct gnttab_unmap_grant_ref *kunmap_ops,
>  		      struct page **pages, unsigned int count);
>  void gnttab_unmap_refs_async(struct gntab_unmap_queue_data* item);
> +int gnttab_unmap_refs_async_wait_completion(struct gntab_unmap_queue_data* item);
>  
>  
>  /* Perform a batch of grant map/copy operations. Retry every batch slot
> -- 
> 1.8.3.1
> 

Re: [PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[Konrad Rzeszutek Wilk]

On Thu, Mar 26, 2015 at 08:16:01PM +0800, Bob Liu wrote:
> There are several place using gnttab async unmap and wait for
> completion, so move the common code to a function
> gnttab_unmap_refs_async_wait_completion().
> 
> Signed-off-by: Bob Liu <bob.liu@oracle.com>

Jens, and would you be OK if this patch went through the Xen tree?

Thanks.
> ---
>  drivers/block/xen-blkback/blkback.c | 31 +++----------------------------
>  drivers/xen/gntdev.c                | 28 +++-------------------------
>  drivers/xen/grant-table.c           | 29 +++++++++++++++++++++++++++++
>  include/xen/grant_table.h           |  1 +
>  4 files changed, 36 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> index f59d7c3..0c8da82 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -263,17 +263,6 @@ static void put_persistent_gnt(struct xen_blkif *blkif,
>  	atomic_dec(&blkif->persistent_gnt_in_use);
>  }
>  
> -static void free_persistent_gnts_unmap_callback(int result,
> -						struct gntab_unmap_queue_data *data)
> -{
> -	struct completion *c = data->data;
> -
> -	/* BUG_ON used to reproduce existing behaviour,
> -	   but is this the best way to deal with this? */
> -	BUG_ON(result);
> -	complete(c);
> -}
> -
>  static void free_persistent_gnts(struct xen_blkif *blkif, struct rb_root *root,
>                                   unsigned int num)
>  {
> @@ -283,12 +272,7 @@ static void free_persistent_gnts(struct xen_blkif *blkif, struct rb_root *root,
>  	struct rb_node *n;
>  	int segs_to_unmap = 0;
>  	struct gntab_unmap_queue_data unmap_data;
> -	struct completion unmap_completion;
> -
> -	init_completion(&unmap_completion);
>  
> -	unmap_data.data = &unmap_completion;
> -	unmap_data.done = &free_persistent_gnts_unmap_callback;
>  	unmap_data.pages = pages;
>  	unmap_data.unmap_ops = unmap;
>  	unmap_data.kunmap_ops = NULL;
> @@ -308,8 +292,7 @@ static void free_persistent_gnts(struct xen_blkif *blkif, struct rb_root *root,
>  			!rb_next(&persistent_gnt->node)) {
>  
>  			unmap_data.count = segs_to_unmap;
> -			gnttab_unmap_refs_async(&unmap_data);
> -			wait_for_completion(&unmap_completion);
> +			BUG_ON(gnttab_unmap_refs_async_wait_completion(&unmap_data));
>  
>  			put_free_pages(blkif, pages, segs_to_unmap);
>  			segs_to_unmap = 0;
> @@ -330,12 +313,7 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
>  	int segs_to_unmap = 0;
>  	struct xen_blkif *blkif = container_of(work, typeof(*blkif), persistent_purge_work);
>  	struct gntab_unmap_queue_data unmap_data;
> -	struct completion unmap_completion;
>  
> -	init_completion(&unmap_completion);
> -
> -	unmap_data.data = &unmap_completion;
> -	unmap_data.done = &free_persistent_gnts_unmap_callback;
>  	unmap_data.pages = pages;
>  	unmap_data.unmap_ops = unmap;
>  	unmap_data.kunmap_ops = NULL;
> @@ -355,9 +333,7 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
>  
>  		if (++segs_to_unmap == BLKIF_MAX_SEGMENTS_PER_REQUEST) {
>  			unmap_data.count = segs_to_unmap;
> -			gnttab_unmap_refs_async(&unmap_data);
> -			wait_for_completion(&unmap_completion);
> -
> +			BUG_ON(gnttab_unmap_refs_async_wait_completion(&unmap_data));
>  			put_free_pages(blkif, pages, segs_to_unmap);
>  			segs_to_unmap = 0;
>  		}
> @@ -365,8 +341,7 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
>  	}
>  	if (segs_to_unmap > 0) {
>  		unmap_data.count = segs_to_unmap;
> -		gnttab_unmap_refs_async(&unmap_data);
> -		wait_for_completion(&unmap_completion);
> +		BUG_ON(gnttab_unmap_refs_async_wait_completion(&unmap_data));
>  		put_free_pages(blkif, pages, segs_to_unmap);
>  	}
>  }
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index d5bb1a3..7a88524 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -327,30 +327,10 @@ static int map_grant_pages(struct grant_map *map)
>  	return err;
>  }
>  
> -struct unmap_grant_pages_callback_data
> -{
> -	struct completion completion;
> -	int result;
> -};
> -
> -static void unmap_grant_callback(int result,
> -				 struct gntab_unmap_queue_data *data)
> -{
> -	struct unmap_grant_pages_callback_data* d = data->data;
> -
> -	d->result = result;
> -	complete(&d->completion);
> -}
> -
>  static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
>  {
>  	int i, err = 0;
>  	struct gntab_unmap_queue_data unmap_data;
> -	struct unmap_grant_pages_callback_data data;
> -
> -	init_completion(&data.completion);
> -	unmap_data.data = &data;
> -	unmap_data.done= &unmap_grant_callback;
>  
>  	if (map->notify.flags & UNMAP_NOTIFY_CLEAR_BYTE) {
>  		int pgno = (map->notify.addr >> PAGE_SHIFT);
> @@ -367,11 +347,9 @@ static int __unmap_grant_pages(struct grant_map *map, int offset, int pages)
>  	unmap_data.pages = map->pages + offset;
>  	unmap_data.count = pages;
>  
> -	gnttab_unmap_refs_async(&unmap_data);
> -
> -	wait_for_completion(&data.completion);
> -	if (data.result)
> -		return data.result;
> +	err = gnttab_unmap_refs_async_wait_completion(&unmap_data);
> +	if (err)
> +		return err;
>  
>  	for (i = 0; i < pages; i++) {
>  		if (map->unmap_ops[offset+i].status)
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 17972fb..1dce9bc 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -123,6 +123,12 @@ struct gnttab_ops {
>  	int (*query_foreign_access)(grant_ref_t ref);
>  };
>  
> +struct unmap_refs_async_callback_data
> +{
> +	struct completion completion;
> +	int result;
> +};
> +
>  static struct gnttab_ops *gnttab_interface;
>  
>  static int grant_table_version;
> @@ -863,6 +869,29 @@ void gnttab_unmap_refs_async(struct gntab_unmap_queue_data* item)
>  }
>  EXPORT_SYMBOL_GPL(gnttab_unmap_refs_async);
>  
> +static void unmap_refs_async_callback(int result,
> +		struct gntab_unmap_queue_data *data)
> +{
> +	struct unmap_refs_async_callback_data* d = data->data;
> +
> +	d->result = result;
> +	complete(&d->completion);
> +}
> +
> +int gnttab_unmap_refs_async_wait_completion(struct gntab_unmap_queue_data* item)
> +{
> +	struct unmap_refs_async_callback_data data;
> +
> +	init_completion(&data.completion);
> +	item->data = &data;
> +	item->done= &unmap_refs_async_callback;
> +	gnttab_unmap_refs_async(item);
> +	wait_for_completion(&data.completion);
> +
> +	return data.result;
> +}
> +EXPORT_SYMBOL_GPL(gnttab_unmap_refs_async_wait_completion);
> +
>  static int gnttab_map_frames_v1(xen_pfn_t *frames, unsigned int nr_gframes)
>  {
>  	int rc;
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index 143ca5f..46bad05 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -191,6 +191,7 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
>  		      struct gnttab_unmap_grant_ref *kunmap_ops,
>  		      struct page **pages, unsigned int count);
>  void gnttab_unmap_refs_async(struct gntab_unmap_queue_data* item);
> +int gnttab_unmap_refs_async_wait_completion(struct gntab_unmap_queue_data* item);
>  
>  
>  /* Perform a batch of grant map/copy operations. Retry every batch slot
> -- 
> 1.8.3.1
> 

Re: [PATCH RESEND 1/2] xen/blkback: safely unmap purge persistent grants

[Jens Axboe]

On 03/26/2015 01:01 PM, Konrad Rzeszutek Wilk wrote:
> On Thu, Mar 26, 2015 at 04:23:27PM +0100, Roger Pau Monné wrote:
>> El 26/03/15 a les 13.16, Bob Liu ha escrit:
>>> Commit c43cf3ea8385 ("xen-blkback: safely unmap grants in case they are still
>>> in use") use gnttab_unmap_refs_async() to wait until the mapped pages are no
>>> longer in use before unmapping them, but that commit missed the persistent case.
>>> Purge persistent pages can't be unmapped either unless no longer in use.
>>>
>>> Signed-off-by: Bob Liu <bob.liu@oracle.com>
>>
>> AFAICT since this is basically the same patch as what I've sent:
>>
>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>
> 
> Hey Jens,
> 
> These patches are on top of patches that add this framework in the Xen tree.
> Would you be OK with this patch (and the other which I will CC shortly)
> to go through the Xen tree?

Sure that's fine.

-- 
Jens Axboe

Re: [PATCH RESEND 1/2] xen/blkback: safely unmap purge persistent grants

[Jens Axboe]

On 03/26/2015 01:01 PM, Konrad Rzeszutek Wilk wrote:
> On Thu, Mar 26, 2015 at 04:23:27PM +0100, Roger Pau Monné wrote:
>> El 26/03/15 a les 13.16, Bob Liu ha escrit:
>>> Commit c43cf3ea8385 ("xen-blkback: safely unmap grants in case they are still
>>> in use") use gnttab_unmap_refs_async() to wait until the mapped pages are no
>>> longer in use before unmapping them, but that commit missed the persistent case.
>>> Purge persistent pages can't be unmapped either unless no longer in use.
>>>
>>> Signed-off-by: Bob Liu <bob.liu@oracle.com>
>>
>> AFAICT since this is basically the same patch as what I've sent:
>>
>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>
> 
> Hey Jens,
> 
> These patches are on top of patches that add this framework in the Xen tree.
> Would you be OK with this patch (and the other which I will CC shortly)
> to go through the Xen tree?

Sure that's fine.

-- 
Jens Axboe

Re: [PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[Konrad Rzeszutek Wilk]

On Thu, Mar 26, 2015 at 04:32:45PM +0100, Roger Pau Monné wrote:
> El 26/03/15 a les 13.16, Bob Liu ha escrit:
> > There are several place using gnttab async unmap and wait for
> > completion, so move the common code to a function
> > gnttab_unmap_refs_async_wait_completion().
> > 
> > Signed-off-by: Bob Liu <bob.liu@oracle.com>
> 
> For the blkback parts:
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Although, as David already said, I think we should do better than BUG_ON
> on error.

Have an 'stuck deferred pages' list and an timer to occasionally
flush them out? Something similar to 569ca5b3f94cd0b3295ec5943aa457cf4a4f6a3a
"xen/gnttab: add deferred freeing logic" ?
> 

Re: [PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[Konrad Rzeszutek Wilk]

On Thu, Mar 26, 2015 at 04:32:45PM +0100, Roger Pau Monné wrote:
> El 26/03/15 a les 13.16, Bob Liu ha escrit:
> > There are several place using gnttab async unmap and wait for
> > completion, so move the common code to a function
> > gnttab_unmap_refs_async_wait_completion().
> > 
> > Signed-off-by: Bob Liu <bob.liu@oracle.com>
> 
> For the blkback parts:
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Although, as David already said, I think we should do better than BUG_ON
> on error.

Have an 'stuck deferred pages' list and an timer to occasionally
flush them out? Something similar to 569ca5b3f94cd0b3295ec5943aa457cf4a4f6a3a
"xen/gnttab: add deferred freeing logic" ?
> 

Re: [PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[Bob Liu]

On 03/28/2015 08:44 AM, Konrad Rzeszutek Wilk wrote:
> On Thu, Mar 26, 2015 at 04:32:45PM +0100, Roger Pau Monné wrote:
>> El 26/03/15 a les 13.16, Bob Liu ha escrit:
>>> There are several place using gnttab async unmap and wait for
>>> completion, so move the common code to a function
>>> gnttab_unmap_refs_async_wait_completion().
>>>
>>> Signed-off-by: Bob Liu <bob.liu@oracle.com>
>>
>> For the blkback parts:
>>
>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Although, as David already said, I think we should do better than BUG_ON
>> on error.
> 

This patch only keeps the existing behaviour the same as before. I
prefer to make new patches(based on this one) if want to change the
behaviour.

> Have an 'stuck deferred pages' list and an timer to occasionally
> flush them out? Something similar to 569ca5b3f94cd0b3295ec5943aa457cf4a4f6a3a
> "xen/gnttab: add deferred freeing logic" ?

Sounds like a good idea.

-- 
Regards,
-Bob

Re: [PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[Bob Liu]

On 03/28/2015 08:44 AM, Konrad Rzeszutek Wilk wrote:
> On Thu, Mar 26, 2015 at 04:32:45PM +0100, Roger Pau Monné wrote:
>> El 26/03/15 a les 13.16, Bob Liu ha escrit:
>>> There are several place using gnttab async unmap and wait for
>>> completion, so move the common code to a function
>>> gnttab_unmap_refs_async_wait_completion().
>>>
>>> Signed-off-by: Bob Liu <bob.liu@oracle.com>
>>
>> For the blkback parts:
>>
>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Although, as David already said, I think we should do better than BUG_ON
>> on error.
> 

This patch only keeps the existing behaviour the same as before. I
prefer to make new patches(based on this one) if want to change the
behaviour.

> Have an 'stuck deferred pages' list and an timer to occasionally
> flush them out? Something similar to 569ca5b3f94cd0b3295ec5943aa457cf4a4f6a3a
> "xen/gnttab: add deferred freeing logic" ?

Sounds like a good idea.

-- 
Regards,
-Bob

Re: [Xen-devel] [PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[David Vrabel]

On 30/03/15 01:03, Bob Liu wrote:
> 
> On 03/28/2015 08:44 AM, Konrad Rzeszutek Wilk wrote:
>> On Thu, Mar 26, 2015 at 04:32:45PM +0100, Roger Pau Monné wrote:
>>> El 26/03/15 a les 13.16, Bob Liu ha escrit:
>>>> There are several place using gnttab async unmap and wait for
>>>> completion, so move the common code to a function
>>>> gnttab_unmap_refs_async_wait_completion().
>>>>
>>>> Signed-off-by: Bob Liu <bob.liu@oracle.com>
>>>
>>> For the blkback parts:
>>>
>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> Although, as David already said, I think we should do better than BUG_ON
>>> on error.
>>
> 
> This patch only keeps the existing behaviour the same as before. I
> prefer to make new patches(based on this one) if want to change the
> behaviour.
> 
>> Have an 'stuck deferred pages' list and an timer to occasionally
>> flush them out? Something similar to 569ca5b3f94cd0b3295ec5943aa457cf4a4f6a3a
>> "xen/gnttab: add deferred freeing logic" ?
> 
> Sounds like a good idea.

If it failed the first time I don't see how it would succeed later.  No
one has actually hit this particular BUG_ON() so I would leave it as is.

David

Re: [PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[David Vrabel]

On 30/03/15 01:03, Bob Liu wrote:
> 
> On 03/28/2015 08:44 AM, Konrad Rzeszutek Wilk wrote:
>> On Thu, Mar 26, 2015 at 04:32:45PM +0100, Roger Pau Monné wrote:
>>> El 26/03/15 a les 13.16, Bob Liu ha escrit:
>>>> There are several place using gnttab async unmap and wait for
>>>> completion, so move the common code to a function
>>>> gnttab_unmap_refs_async_wait_completion().
>>>>
>>>> Signed-off-by: Bob Liu <bob.liu@oracle.com>
>>>
>>> For the blkback parts:
>>>
>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> Although, as David already said, I think we should do better than BUG_ON
>>> on error.
>>
> 
> This patch only keeps the existing behaviour the same as before. I
> prefer to make new patches(based on this one) if want to change the
> behaviour.
> 
>> Have an 'stuck deferred pages' list and an timer to occasionally
>> flush them out? Something similar to 569ca5b3f94cd0b3295ec5943aa457cf4a4f6a3a
>> "xen/gnttab: add deferred freeing logic" ?
> 
> Sounds like a good idea.

If it failed the first time I don't see how it would succeed later.  No
one has actually hit this particular BUG_ON() so I would leave it as is.

David

Re: [Xen-devel] [PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[David Vrabel]

On 26/03/15 12:16, Bob Liu wrote:
> There are several place using gnttab async unmap and wait for
> completion, so move the common code to a function
> gnttab_unmap_refs_async_wait_completion().
> 
[...]
> +
> +int gnttab_unmap_refs_async_wait_completion(struct gntab_unmap_queue_data* item)

This name is a bit wordy.  Can you rename it to:
gnttab_unmap_refs_sync() gnttab_unmap_refs_wait(), or
gnttab_unmap_refs_safe().

(I can't decide which is the best name.).

David

Re: [PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[David Vrabel]

On 26/03/15 12:16, Bob Liu wrote:
> There are several place using gnttab async unmap and wait for
> completion, so move the common code to a function
> gnttab_unmap_refs_async_wait_completion().
> 
[...]
> +
> +int gnttab_unmap_refs_async_wait_completion(struct gntab_unmap_queue_data* item)

This name is a bit wordy.  Can you rename it to:
gnttab_unmap_refs_sync() gnttab_unmap_refs_wait(), or
gnttab_unmap_refs_safe().

(I can't decide which is the best name.).

David

Re: [Xen-devel] [PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[Bob Liu]

On 04/01/2015 06:18 PM, David Vrabel wrote:
> On 26/03/15 12:16, Bob Liu wrote:
>> There are several place using gnttab async unmap and wait for
>> completion, so move the common code to a function
>> gnttab_unmap_refs_async_wait_completion().
>>
> [...]
>> +
>> +int gnttab_unmap_refs_async_wait_completion(struct gntab_unmap_queue_data* item)
> 
> This name is a bit wordy.  Can you rename it to:
> gnttab_unmap_refs_sync() gnttab_unmap_refs_wait(), or

Sure, I think gnttab_unmap_refs_sync() is fine.
Will be updated.

> gnttab_unmap_refs_safe().
> 
> (I can't decide which is the best name.).
> 
> David
> 

-- 
Regards,
-Bob

Re: [PATCH 2/2] xen/grant: introduce func gnttab_unmap_refs_async_wait_completion()

[Bob Liu]

On 04/01/2015 06:18 PM, David Vrabel wrote:
> On 26/03/15 12:16, Bob Liu wrote:
>> There are several place using gnttab async unmap and wait for
>> completion, so move the common code to a function
>> gnttab_unmap_refs_async_wait_completion().
>>
> [...]
>> +
>> +int gnttab_unmap_refs_async_wait_completion(struct gntab_unmap_queue_data* item)
> 
> This name is a bit wordy.  Can you rename it to:
> gnttab_unmap_refs_sync() gnttab_unmap_refs_wait(), or

Sure, I think gnttab_unmap_refs_sync() is fine.
Will be updated.

> gnttab_unmap_refs_safe().
> 
> (I can't decide which is the best name.).
> 
> David
> 

-- 
Regards,
-Bob

[PATCH RESEND 1/2] xen/blkback: safely unmap purge persistent grants

[Bob Liu]

Commit c43cf3ea8385 ("xen-blkback: safely unmap grants in case they are still
in use") use gnttab_unmap_refs_async() to wait until the mapped pages are no
longer in use before unmapping them, but that commit missed the persistent case.
Purge persistent pages can't be unmapped either unless no longer in use.

Signed-off-by: Bob Liu <bob.liu@oracle.com>
---
 drivers/block/xen-blkback/blkback.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 2a04d34..f59d7c3 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -327,8 +327,18 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
 	struct gnttab_unmap_grant_ref unmap[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 	struct page *pages[BLKIF_MAX_SEGMENTS_PER_REQUEST];
 	struct persistent_gnt *persistent_gnt;
-	int ret, segs_to_unmap = 0;
+	int segs_to_unmap = 0;
 	struct xen_blkif *blkif = container_of(work, typeof(*blkif), persistent_purge_work);
+	struct gntab_unmap_queue_data unmap_data;
+	struct completion unmap_completion;
+
+	init_completion(&unmap_completion);
+
+	unmap_data.data = &unmap_completion;
+	unmap_data.done = &free_persistent_gnts_unmap_callback;
+	unmap_data.pages = pages;
+	unmap_data.unmap_ops = unmap;
+	unmap_data.kunmap_ops = NULL;
 
 	while(!list_empty(&blkif->persistent_purge_list)) {
 		persistent_gnt = list_first_entry(&blkif->persistent_purge_list,
@@ -344,17 +354,19 @@ void xen_blkbk_unmap_purged_grants(struct work_struct *work)
 		pages[segs_to_unmap] = persistent_gnt->page;
 
 		if (++segs_to_unmap == BLKIF_MAX_SEGMENTS_PER_REQUEST) {
-			ret = gnttab_unmap_refs(unmap, NULL, pages,
-				segs_to_unmap);
-			BUG_ON(ret);
+			unmap_data.count = segs_to_unmap;
+			gnttab_unmap_refs_async(&unmap_data);
+			wait_for_completion(&unmap_completion);
+
 			put_free_pages(blkif, pages, segs_to_unmap);
 			segs_to_unmap = 0;
 		}
 		kfree(persistent_gnt);
 	}
 	if (segs_to_unmap > 0) {
-		ret = gnttab_unmap_refs(unmap, NULL, pages, segs_to_unmap);
-		BUG_ON(ret);
+		unmap_data.count = segs_to_unmap;
+		gnttab_unmap_refs_async(&unmap_data);
+		wait_for_completion(&unmap_completion);
 		put_free_pages(blkif, pages, segs_to_unmap);
 	}
 }
-- 
1.8.3.1