xen-blkback: bug fixes

xen-blkback: bug fixes

[Roger Pau Monne]

blkback bug fixes for memory leaks (patches 1 and 2) and a race 
(patch 3).

[PATCH 1/3] xen-blkback: fix memory leak when persistent grants are used

[Roger Pau Monne]

From: Matt Rushton <mrushton@amazon.com>

Currently shrink_free_pagepool() is called before the pages used for
persistent grants are released via free_persistent_gnts(). This
results in a memory leak when a VBD that uses persistent grants is
torn down.

Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: linux-kernel@vger.kernel.org
Cc: xen-devel@lists.xen.org
Cc: Anthony Liguori <aliguori@amazon.com>
Signed-off-by: Matt Rushton <mrushton@amazon.com>
Signed-off-by: Matt Wilson <msw@amazon.com>
---
 drivers/block/xen-blkback/blkback.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 6620b73..30ef7b3 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -625,9 +625,6 @@ purge_gnt_list:
 			print_stats(blkif);
 	}
 
-	/* Since we are shutting down remove all pages from the buffer */
-	shrink_free_pagepool(blkif, 0 /* All */);
-
 	/* Free all persistent grant pages */
 	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
 		free_persistent_gnts(blkif, &blkif->persistent_gnts,
@@ -636,6 +633,9 @@ purge_gnt_list:
 	BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts));
 	blkif->persistent_gnt_c = 0;
 
+	/* Since we are shutting down remove all pages from the buffer */
+	shrink_free_pagepool(blkif, 0 /* All */);
+
 	if (log_stats)
 		print_stats(blkif);
 
-- 
1.7.7.5 (Apple Git-26)

[PATCH 2/3] xen-blkback: fix memory leaks

[Roger Pau Monne]

I've at least identified two possible memory leaks in blkback, both
related to the shutdown path of a VBD:

- blkback doesn't wait for any pending purge work to finish before
  cleaning the list of free_pages. The purge work will call
  put_free_pages and thus we might end up with pages being added to
  the free_pages list after we have emptied it. Fix this by making
  sure there's no pending purge work before exiting
  xen_blkif_schedule, and moving the free_page cleanup code to
  xen_blkif_free.
- blkback doesn't wait for pending requests to end before cleaning
  persistent grants and the list of free_pages. Again this can add
  pages to the free_pages list or persistent grants to the
  persistent_gnts red-black tree. Fixed by moving the persistent
  grants and free_pages cleanup code to xen_blkif_free.

Also, add some checks in xen_blkif_free to make sure we are cleaning
everything.

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>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Matt Rushton <mrushton@amazon.com>
Cc: Matt Wilson <msw@amazon.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
---
 drivers/block/xen-blkback/blkback.c |   27 ++++++++++++++++++---------
 drivers/block/xen-blkback/common.h  |    1 +
 drivers/block/xen-blkback/xenbus.c  |   12 ++++++++++++
 3 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 30ef7b3..dcfe49f 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -375,7 +375,7 @@ static void purge_persistent_gnt(struct xen_blkif *blkif)
 
 	pr_debug(DRV_PFX "Going to purge %u persistent grants\n", num_clean);
 
-	INIT_LIST_HEAD(&blkif->persistent_purge_list);
+	BUG_ON(!list_empty(&blkif->persistent_purge_list));
 	root = &blkif->persistent_gnts;
 purge_list:
 	foreach_grant_safe(persistent_gnt, n, root, node) {
@@ -625,6 +625,23 @@ purge_gnt_list:
 			print_stats(blkif);
 	}
 
+	/* Drain pending purge work */
+	flush_work(&blkif->persistent_purge_work);
+
+	if (log_stats)
+		print_stats(blkif);
+
+	blkif->xenblkd = NULL;
+	xen_blkif_put(blkif);
+
+	return 0;
+}
+
+/*
+ * Remove persistent grants and empty the pool of free pages
+ */
+void xen_blkbk_free_caches(struct xen_blkif *blkif)
+{
 	/* Free all persistent grant pages */
 	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
 		free_persistent_gnts(blkif, &blkif->persistent_gnts,
@@ -635,14 +652,6 @@ purge_gnt_list:
 
 	/* Since we are shutting down remove all pages from the buffer */
 	shrink_free_pagepool(blkif, 0 /* All */);
-
-	if (log_stats)
-		print_stats(blkif);
-
-	blkif->xenblkd = NULL;
-	xen_blkif_put(blkif);
-
-	return 0;
 }
 
 /*
diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
index 8d88075..f733d76 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -376,6 +376,7 @@ int xen_blkif_xenbus_init(void);
 irqreturn_t xen_blkif_be_int(int irq, void *dev_id);
 int xen_blkif_schedule(void *arg);
 int xen_blkif_purge_persistent(void *arg);
+void xen_blkbk_free_caches(struct xen_blkif *blkif);
 
 int xen_blkbk_flush_diskcache(struct xenbus_transaction xbt,
 			      struct backend_info *be, int state);
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index c2014a0..8afef67 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -125,6 +125,7 @@ static struct xen_blkif *xen_blkif_alloc(domid_t domid)
 	blkif->persistent_gnts.rb_node = NULL;
 	spin_lock_init(&blkif->free_pages_lock);
 	INIT_LIST_HEAD(&blkif->free_pages);
+	INIT_LIST_HEAD(&blkif->persistent_purge_list);
 	blkif->free_pages_num = 0;
 	atomic_set(&blkif->persistent_gnt_in_use, 0);
 
@@ -259,6 +260,17 @@ static void xen_blkif_free(struct xen_blkif *blkif)
 	if (!atomic_dec_and_test(&blkif->refcnt))
 		BUG();
 
+	/* Remove all persistent grants and the cache of ballooned pages. */
+	xen_blkbk_free_caches(blkif);
+
+	/* Make sure everything is drained before shutting down */
+	BUG_ON(blkif->persistent_gnt_c != 0);
+	BUG_ON(atomic_read(&blkif->persistent_gnt_in_use) != 0);
+	BUG_ON(blkif->free_pages_num != 0);
+	BUG_ON(!list_empty(&blkif->persistent_purge_list));
+	BUG_ON(!list_empty(&blkif->free_pages));
+	BUG_ON(!RB_EMPTY_ROOT(&blkif->persistent_gnts));
+
 	/* Check that there is no request in use */
 	list_for_each_entry_safe(req, n, &blkif->pending_free, free_list) {
 		list_del(&req->free_list);
-- 
1.7.7.5 (Apple Git-26)

[PATCH 3/3] xen-blkback: fix shutdown race

[Roger Pau Monne]

Move the call to xen_blkif_put after we have freed the request,
otherwise we have a race between the release of the request and the
cleanup done in xen_blkif_free.

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>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Matt Rushton <mrushton@amazon.com>
Cc: Matt Wilson <msw@amazon.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
---
 drivers/block/xen-blkback/blkback.c |   28 +++++++++++++++++++++-------
 1 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index dcfe49f..8200aa0 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -985,17 +985,31 @@ static void __end_block_io_op(struct pending_req *pending_req, int error)
 	 * the proper response on the ring.
 	 */
 	if (atomic_dec_and_test(&pending_req->pendcnt)) {
-		xen_blkbk_unmap(pending_req->blkif,
+		struct xen_blkif *blkif = pending_req->blkif;
+
+		xen_blkbk_unmap(blkif,
 		                pending_req->segments,
 		                pending_req->nr_pages);
-		make_response(pending_req->blkif, pending_req->id,
+		make_response(blkif, pending_req->id,
 			      pending_req->operation, pending_req->status);
-		xen_blkif_put(pending_req->blkif);
-		if (atomic_read(&pending_req->blkif->refcnt) <= 2) {
-			if (atomic_read(&pending_req->blkif->drain))
-				complete(&pending_req->blkif->drain_complete);
+		free_req(blkif, pending_req);
+		/*
+		 * Make sure the request is freed before releasing blkif,
+		 * or there could be a race between free_req and the
+		 * cleanup done in xen_blkif_free during shutdown.
+		 *
+		 * NB: The fact that we might try to wake up pending_free_wq
+		 * before drain_complete (in case there's a drain going on)
+		 * it's not a problem with our current implementation
+		 * because we can assure there's no thread waiting on
+		 * pending_free_wq if there's a drain going on, but it has
+		 * to be taken into account if the current model is changed.
+		 */
+		xen_blkif_put(blkif);
+		if (atomic_read(&blkif->refcnt) <= 2) {
+			if (atomic_read(&blkif->drain))
+				complete(&blkif->drain_complete);
 		}
-		free_req(pending_req->blkif, pending_req);
 	}
 }
 
-- 
1.7.7.5 (Apple Git-26)

Re: [Xen-devel] xen-blkback: bug fixes

[Konrad Rzeszutek Wilk]

On Tue, Jan 28, 2014 at 06:43:32PM +0100, Roger Pau Monne wrote:
> blkback bug fixes for memory leaks (patches 1 and 2) and a race 
> (patch 3).

They all look OK to me. I've stuck them in my 'stable/for-jens-3.14'
branch and are testing them now (hadn't pushed it yet).

Matt and Matt,

Could you take a look at the other two patches as well?

David, Boris,

Are you OK with pushing those patches out to Jens Axboe if nobody
gives an NACK by Friday?

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

Re: [Xen-devel] xen-blkback: bug fixes

[Boris Ostrovsky]

On 01/28/2014 02:38 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, Jan 28, 2014 at 06:43:32PM +0100, Roger Pau Monne wrote:
>> blkback bug fixes for memory leaks (patches 1 and 2) and a race
>> (patch 3).
> They all look OK to me. I've stuck them in my 'stable/for-jens-3.14'
> branch and are testing them now (hadn't pushed it yet).
>
> Matt and Matt,
>
> Could you take a look at the other two patches as well?
>
> David, Boris,
>
> Are you OK with pushing those patches out to Jens Axboe if nobody
> gives an NACK by Friday?

The patches look reasonable to me so I don't have any objections (but
I am not particularly familiar with this code.)

-boris

Re: [Xen-devel] [PATCH 3/3] xen-blkback: fix shutdown race

[Jan Beulich]

>>> On 28.01.14 at 18:43, Roger Pau Monne <roger.pau@citrix.com> wrote:
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -985,17 +985,31 @@ static void __end_block_io_op(struct pending_req 
> *pending_req, int error)
>  	 * the proper response on the ring.
>  	 */
>  	if (atomic_dec_and_test(&pending_req->pendcnt)) {
> -		xen_blkbk_unmap(pending_req->blkif,
> +		struct xen_blkif *blkif = pending_req->blkif;
> +
> +		xen_blkbk_unmap(blkif,
>  		                pending_req->segments,
>  		                pending_req->nr_pages);
> -		make_response(pending_req->blkif, pending_req->id,
> +		make_response(blkif, pending_req->id,
>  			      pending_req->operation, pending_req->status);
> -		xen_blkif_put(pending_req->blkif);
> -		if (atomic_read(&pending_req->blkif->refcnt) <= 2) {
> -			if (atomic_read(&pending_req->blkif->drain))
> -				complete(&pending_req->blkif->drain_complete);
> +		free_req(blkif, pending_req);
> +		/*
> +		 * Make sure the request is freed before releasing blkif,
> +		 * or there could be a race between free_req and the
> +		 * cleanup done in xen_blkif_free during shutdown.
> +		 *
> +		 * NB: The fact that we might try to wake up pending_free_wq
> +		 * before drain_complete (in case there's a drain going on)
> +		 * it's not a problem with our current implementation
> +		 * because we can assure there's no thread waiting on
> +		 * pending_free_wq if there's a drain going on, but it has
> +		 * to be taken into account if the current model is changed.
> +		 */
> +		xen_blkif_put(blkif);
> +		if (atomic_read(&blkif->refcnt) <= 2) {
> +			if (atomic_read(&blkif->drain))
> +				complete(&blkif->drain_complete);
>  		}
> -		free_req(pending_req->blkif, pending_req);
>  	}
>  }

The put is still too early imo - you're explicitly accessing field in the
structure immediately afterwards. This may not be an issue at
present, but I think it's at least a latent one.

Apart from that, the two if()s would - at least to me - be more
clear if combined into one.

Jan

Re: [Xen-devel] [PATCH 3/3] xen-blkback: fix shutdown race

[Roger Pau Monné]

On 29/01/14 08:52, Jan Beulich wrote:
>>>> On 28.01.14 at 18:43, Roger Pau Monne <roger.pau@citrix.com> wrote:
>> --- a/drivers/block/xen-blkback/blkback.c
>> +++ b/drivers/block/xen-blkback/blkback.c
>> @@ -985,17 +985,31 @@ static void __end_block_io_op(struct pending_req 
>> *pending_req, int error)
>>  	 * the proper response on the ring.
>>  	 */
>>  	if (atomic_dec_and_test(&pending_req->pendcnt)) {
>> -		xen_blkbk_unmap(pending_req->blkif,
>> +		struct xen_blkif *blkif = pending_req->blkif;
>> +
>> +		xen_blkbk_unmap(blkif,
>>  		                pending_req->segments,
>>  		                pending_req->nr_pages);
>> -		make_response(pending_req->blkif, pending_req->id,
>> +		make_response(blkif, pending_req->id,
>>  			      pending_req->operation, pending_req->status);
>> -		xen_blkif_put(pending_req->blkif);
>> -		if (atomic_read(&pending_req->blkif->refcnt) <= 2) {
>> -			if (atomic_read(&pending_req->blkif->drain))
>> -				complete(&pending_req->blkif->drain_complete);
>> +		free_req(blkif, pending_req);
>> +		/*
>> +		 * Make sure the request is freed before releasing blkif,
>> +		 * or there could be a race between free_req and the
>> +		 * cleanup done in xen_blkif_free during shutdown.
>> +		 *
>> +		 * NB: The fact that we might try to wake up pending_free_wq
>> +		 * before drain_complete (in case there's a drain going on)
>> +		 * it's not a problem with our current implementation
>> +		 * because we can assure there's no thread waiting on
>> +		 * pending_free_wq if there's a drain going on, but it has
>> +		 * to be taken into account if the current model is changed.
>> +		 */
>> +		xen_blkif_put(blkif);
>> +		if (atomic_read(&blkif->refcnt) <= 2) {
>> +			if (atomic_read(&blkif->drain))
>> +				complete(&blkif->drain_complete);
>>  		}
>> -		free_req(pending_req->blkif, pending_req);
>>  	}
>>  }
> 
> The put is still too early imo - you're explicitly accessing field in the
> structure immediately afterwards. This may not be an issue at
> present, but I think it's at least a latent one.

Yes, thanks for catching that one, it's an issue that we should solve
now on this patch or else I would just be solving a race by introducing
a new one.

> Apart from that, the two if()s would - at least to me - be more
> clear if combined into one.

Ack, will see how the patch ends up looking after getting rid of the new
race.

Roger.

Re: [Xen-devel] [PATCH 3/3] xen-blkback: fix shutdown race

[Roger Pau Monné]

On 29/01/14 09:52, Jan Beulich wrote:
>>>> On 28.01.14 at 18:43, Roger Pau Monne <roger.pau@citrix.com> wrote:
>> --- a/drivers/block/xen-blkback/blkback.c
>> +++ b/drivers/block/xen-blkback/blkback.c
>> @@ -985,17 +985,31 @@ static void __end_block_io_op(struct pending_req 
>> *pending_req, int error)
>>  	 * the proper response on the ring.
>>  	 */
>>  	if (atomic_dec_and_test(&pending_req->pendcnt)) {
>> -		xen_blkbk_unmap(pending_req->blkif,
>> +		struct xen_blkif *blkif = pending_req->blkif;
>> +
>> +		xen_blkbk_unmap(blkif,
>>  		                pending_req->segments,
>>  		                pending_req->nr_pages);
>> -		make_response(pending_req->blkif, pending_req->id,
>> +		make_response(blkif, pending_req->id,
>>  			      pending_req->operation, pending_req->status);
>> -		xen_blkif_put(pending_req->blkif);
>> -		if (atomic_read(&pending_req->blkif->refcnt) <= 2) {
>> -			if (atomic_read(&pending_req->blkif->drain))
>> -				complete(&pending_req->blkif->drain_complete);
>> +		free_req(blkif, pending_req);
>> +		/*
>> +		 * Make sure the request is freed before releasing blkif,
>> +		 * or there could be a race between free_req and the
>> +		 * cleanup done in xen_blkif_free during shutdown.
>> +		 *
>> +		 * NB: The fact that we might try to wake up pending_free_wq
>> +		 * before drain_complete (in case there's a drain going on)
>> +		 * it's not a problem with our current implementation
>> +		 * because we can assure there's no thread waiting on
>> +		 * pending_free_wq if there's a drain going on, but it has
>> +		 * to be taken into account if the current model is changed.
>> +		 */
>> +		xen_blkif_put(blkif);
>> +		if (atomic_read(&blkif->refcnt) <= 2) {
>> +			if (atomic_read(&blkif->drain))
>> +				complete(&blkif->drain_complete);
>>  		}
>> -		free_req(pending_req->blkif, pending_req);
>>  	}
>>  }
> 
> The put is still too early imo - you're explicitly accessing field in the
> structure immediately afterwards. This may not be an issue at
> present, but I think it's at least a latent one.
> 
> Apart from that, the two if()s would - at least to me - be more
> clear if combined into one.

In order to get rid of the race I had to introduce yet another atomic_t 
in xen_blkif struct, which is something I don't really like, but I 
could not see any other way to solve this. If that's fine I will resend 
the series, here is the reworked patch:

---
diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index dcfe49f..d9b8cd3 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -945,7 +945,7 @@ static void xen_blk_drain_io(struct xen_blkif *blkif)
 	do {
 		/* The initial value is one, and one refcnt taken at the
 		 * start of the xen_blkif_schedule thread. */
-		if (atomic_read(&blkif->refcnt) <= 2)
+		if (atomic_read(&blkif->inflight) == 0)
 			break;
 		wait_for_completion_interruptible_timeout(
 				&blkif->drain_complete, HZ);
@@ -985,17 +985,30 @@ static void __end_block_io_op(struct pending_req *pending_req, int error)
 	 * the proper response on the ring.
 	 */
 	if (atomic_dec_and_test(&pending_req->pendcnt)) {
-		xen_blkbk_unmap(pending_req->blkif,
+		struct xen_blkif *blkif = pending_req->blkif;
+
+		xen_blkbk_unmap(blkif,
 		                pending_req->segments,
 		                pending_req->nr_pages);
-		make_response(pending_req->blkif, pending_req->id,
+		make_response(blkif, pending_req->id,
 			      pending_req->operation, pending_req->status);
-		xen_blkif_put(pending_req->blkif);
-		if (atomic_read(&pending_req->blkif->refcnt) <= 2) {
-			if (atomic_read(&pending_req->blkif->drain))
-				complete(&pending_req->blkif->drain_complete);
+		free_req(blkif, pending_req);
+		/*
+		 * Make sure the request is freed before releasing blkif,
+		 * or there could be a race between free_req and the
+		 * cleanup done in xen_blkif_free during shutdown.
+		 *
+		 * NB: The fact that we might try to wake up pending_free_wq
+		 * before drain_complete (in case there's a drain going on)
+		 * it's not a problem with our current implementation
+		 * because we can assure there's no thread waiting on
+		 * pending_free_wq if there's a drain going on, but it has
+		 * to be taken into account if the current model is changed.
+		 */
+		if (atomic_dec_and_test(&blkif->inflight) && atomic_read(&blkif->drain)) {
+			complete(&blkif->drain_complete);
 		}
-		free_req(pending_req->blkif, pending_req);
+		xen_blkif_put(blkif);
 	}
 }
 
@@ -1249,6 +1262,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
 	 * below (in "!bio") if we are handling a BLKIF_OP_DISCARD.
 	 */
 	xen_blkif_get(blkif);
+	atomic_inc(&blkif->inflight);
 
 	for (i = 0; i < nseg; i++) {
 		while ((bio == NULL) ||
diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
index f733d76..e40326a 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -278,6 +278,7 @@ struct xen_blkif {
 	/* for barrier (drain) requests */
 	struct completion	drain_complete;
 	atomic_t		drain;
+	atomic_t		inflight;
 	/* One thread per one blkif. */
 	struct task_struct	*xenblkd;
 	unsigned int		waiting_reqs;
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index 8afef67..84973c6 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -128,6 +128,7 @@ static struct xen_blkif *xen_blkif_alloc(domid_t domid)
 	INIT_LIST_HEAD(&blkif->persistent_purge_list);
 	blkif->free_pages_num = 0;
 	atomic_set(&blkif->persistent_gnt_in_use, 0);
+	atomic_set(&blkif->inflight, 0);
 
 	INIT_LIST_HEAD(&blkif->pending_free);
 

Re: [Xen-devel] xen-blkback: bug fixes

[Matt Wilson]

On Tue, Jan 28, 2014 at 03:38:37PM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Jan 28, 2014 at 06:43:32PM +0100, Roger Pau Monne wrote:
> > blkback bug fixes for memory leaks (patches 1 and 2) and a race 
> > (patch 3).
> 
> They all look OK to me. I've stuck them in my 'stable/for-jens-3.14'
> branch and are testing them now (hadn't pushed it yet).
> 
> Matt and Matt,
> 
> Could you take a look at the other two patches as well?

Sure, though somehow you didn't address your message to us, so I
didn't see it until today.

Matt Rushton did some review and testing on an earlier version that
came out fine. We'll give the final series a test since there was
still a bit of rework.

--msw

> David, Boris,
> 
> Are you OK with pushing those patches out to Jens Axboe if nobody
> gives an NACK by Friday?

Re: [Xen-devel] [PATCH 3/3] xen-blkback: fix shutdown race

[Jan Beulich]

>>> On 03.02.14 at 17:58, Roger Pau Monné<roger.pau@citrix.com> wrote:
> On 29/01/14 09:52, Jan Beulich wrote:
>>>>> On 28.01.14 at 18:43, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>> +		free_req(blkif, pending_req);
>>> +		/*
>>> +		 * Make sure the request is freed before releasing blkif,
>>> +		 * or there could be a race between free_req and the
>>> +		 * cleanup done in xen_blkif_free during shutdown.
>>> +		 *
>>> +		 * NB: The fact that we might try to wake up pending_free_wq
>>> +		 * before drain_complete (in case there's a drain going on)
>>> +		 * it's not a problem with our current implementation
>>> +		 * because we can assure there's no thread waiting on
>>> +		 * pending_free_wq if there's a drain going on, but it has
>>> +		 * to be taken into account if the current model is changed.
>>> +		 */
>>> +		xen_blkif_put(blkif);
>>> +		if (atomic_read(&blkif->refcnt) <= 2) {
>>> +			if (atomic_read(&blkif->drain))
>>> +				complete(&blkif->drain_complete);
>>>  		}
>>> -		free_req(pending_req->blkif, pending_req);
>>>  	}
>>>  }
>> 
>> The put is still too early imo - you're explicitly accessing field in the
>> structure immediately afterwards. This may not be an issue at
>> present, but I think it's at least a latent one.
>> 
>> Apart from that, the two if()s would - at least to me - be more
>> clear if combined into one.
> 
> In order to get rid of the race I had to introduce yet another atomic_t 
> in xen_blkif struct, which is something I don't really like, but I 
> could not see any other way to solve this. If that's fine I will resend 
> the series, here is the reworked patch:

Mind explaining why you can't simply move the xen_blkif_put()
down between the if() and the free_ref().

Jan

Re: [Xen-devel] [PATCH 3/3] xen-blkback: fix shutdown race

[Roger Pau Monné]

On 04/02/14 09:02, Jan Beulich wrote:
>>>> On 03.02.14 at 17:58, Roger Pau Monné<roger.pau@citrix.com> wrote:
>> On 29/01/14 09:52, Jan Beulich wrote:
>>>>>> On 28.01.14 at 18:43, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>>> +		free_req(blkif, pending_req);
>>>> +		/*
>>>> +		 * Make sure the request is freed before releasing blkif,
>>>> +		 * or there could be a race between free_req and the
>>>> +		 * cleanup done in xen_blkif_free during shutdown.
>>>> +		 *
>>>> +		 * NB: The fact that we might try to wake up pending_free_wq
>>>> +		 * before drain_complete (in case there's a drain going on)
>>>> +		 * it's not a problem with our current implementation
>>>> +		 * because we can assure there's no thread waiting on
>>>> +		 * pending_free_wq if there's a drain going on, but it has
>>>> +		 * to be taken into account if the current model is changed.
>>>> +		 */
>>>> +		xen_blkif_put(blkif);
>>>> +		if (atomic_read(&blkif->refcnt) <= 2) {
>>>> +			if (atomic_read(&blkif->drain))
>>>> +				complete(&blkif->drain_complete);
>>>>  		}
>>>> -		free_req(pending_req->blkif, pending_req);
>>>>  	}
>>>>  }
>>>
>>> The put is still too early imo - you're explicitly accessing field in the
>>> structure immediately afterwards. This may not be an issue at
>>> present, but I think it's at least a latent one.
>>>
>>> Apart from that, the two if()s would - at least to me - be more
>>> clear if combined into one.
>>
>> In order to get rid of the race I had to introduce yet another atomic_t 
>> in xen_blkif struct, which is something I don't really like, but I 
>> could not see any other way to solve this. If that's fine I will resend 
>> the series, here is the reworked patch:
> 
> Mind explaining why you can't simply move the xen_blkif_put()
> down between the if() and the free_ref().

You mean doing something like:

if (atomic_read(&blkif->refcnt) <= 3) {
	if (atomic_read(&blkif->drain))
		complete(&blkif->drain_complete);
}
xen_blkif_put(blkif);
free_req(blkif, pending_req);

This would not be safe, because we are comparing refcnt before
decrementing it, and also we are not doing it atomically (the dec and
compare). If for example we have two inflight requests, both could
perform the atomic_read of refcnt, see there's still another inflight
request, and then both decrement, without anyone calling complete.

There's also the issue that with this approach as we are freeing the
request after we have put blkif, which is a race with the cleanup being
done in xen_blkif_free.

Roger.

Re: [Xen-devel] [PATCH 3/3] xen-blkback: fix shutdown race

[Jan Beulich]

>>> On 04.02.14 at 09:16, Roger Pau Monné<roger.pau@citrix.com> wrote:
> On 04/02/14 09:02, Jan Beulich wrote:
>>>>> On 03.02.14 at 17:58, Roger Pau Monné<roger.pau@citrix.com> wrote:
>>> On 29/01/14 09:52, Jan Beulich wrote:
>>>>>>> On 28.01.14 at 18:43, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>>>> +		free_req(blkif, pending_req);
>>>>> +		/*
>>>>> +		 * Make sure the request is freed before releasing blkif,
>>>>> +		 * or there could be a race between free_req and the
>>>>> +		 * cleanup done in xen_blkif_free during shutdown.
>>>>> +		 *
>>>>> +		 * NB: The fact that we might try to wake up pending_free_wq
>>>>> +		 * before drain_complete (in case there's a drain going on)
>>>>> +		 * it's not a problem with our current implementation
>>>>> +		 * because we can assure there's no thread waiting on
>>>>> +		 * pending_free_wq if there's a drain going on, but it has
>>>>> +		 * to be taken into account if the current model is changed.
>>>>> +		 */
>>>>> +		xen_blkif_put(blkif);
>>>>> +		if (atomic_read(&blkif->refcnt) <= 2) {
>>>>> +			if (atomic_read(&blkif->drain))
>>>>> +				complete(&blkif->drain_complete);
>>>>>  		}
>>>>> -		free_req(pending_req->blkif, pending_req);
>>>>>  	}
>>>>>  }
>>>>
>>>> The put is still too early imo - you're explicitly accessing field in the
>>>> structure immediately afterwards. This may not be an issue at
>>>> present, but I think it's at least a latent one.
>>>>
>>>> Apart from that, the two if()s would - at least to me - be more
>>>> clear if combined into one.
>>>
>>> In order to get rid of the race I had to introduce yet another atomic_t 
>>> in xen_blkif struct, which is something I don't really like, but I 
>>> could not see any other way to solve this. If that's fine I will resend 
>>> the series, here is the reworked patch:
>> 
>> Mind explaining why you can't simply move the xen_blkif_put()
>> down between the if() and the free_ref().
> 
> You mean doing something like:
> 
> if (atomic_read(&blkif->refcnt) <= 3) {
> 	if (atomic_read(&blkif->drain))
> 		complete(&blkif->drain_complete);
> }
> xen_blkif_put(blkif);
> free_req(blkif, pending_req);

Actually, I got the description wrong. I really meant

free_req();
if (atomic_read ...)
	complete();
xen_blkif_put();

Jan

Re: [Xen-devel] [PATCH 3/3] xen-blkback: fix shutdown race

[Roger Pau Monné]

On 04/02/14 09:31, Jan Beulich wrote:
>>>> On 04.02.14 at 09:16, Roger Pau Monné<roger.pau@citrix.com> wrote:
>> On 04/02/14 09:02, Jan Beulich wrote:
>>>>>> On 03.02.14 at 17:58, Roger Pau Monné<roger.pau@citrix.com> wrote:
>>>> On 29/01/14 09:52, Jan Beulich wrote:
>>>>>>>> On 28.01.14 at 18:43, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>>>>> +		free_req(blkif, pending_req);
>>>>>> +		/*
>>>>>> +		 * Make sure the request is freed before releasing blkif,
>>>>>> +		 * or there could be a race between free_req and the
>>>>>> +		 * cleanup done in xen_blkif_free during shutdown.
>>>>>> +		 *
>>>>>> +		 * NB: The fact that we might try to wake up pending_free_wq
>>>>>> +		 * before drain_complete (in case there's a drain going on)
>>>>>> +		 * it's not a problem with our current implementation
>>>>>> +		 * because we can assure there's no thread waiting on
>>>>>> +		 * pending_free_wq if there's a drain going on, but it has
>>>>>> +		 * to be taken into account if the current model is changed.
>>>>>> +		 */
>>>>>> +		xen_blkif_put(blkif);
>>>>>> +		if (atomic_read(&blkif->refcnt) <= 2) {
>>>>>> +			if (atomic_read(&blkif->drain))
>>>>>> +				complete(&blkif->drain_complete);
>>>>>>  		}
>>>>>> -		free_req(pending_req->blkif, pending_req);
>>>>>>  	}
>>>>>>  }
>>>>>
>>>>> The put is still too early imo - you're explicitly accessing field in the
>>>>> structure immediately afterwards. This may not be an issue at
>>>>> present, but I think it's at least a latent one.
>>>>>
>>>>> Apart from that, the two if()s would - at least to me - be more
>>>>> clear if combined into one.
>>>>
>>>> In order to get rid of the race I had to introduce yet another atomic_t 
>>>> in xen_blkif struct, which is something I don't really like, but I 
>>>> could not see any other way to solve this. If that's fine I will resend 
>>>> the series, here is the reworked patch:
>>>
>>> Mind explaining why you can't simply move the xen_blkif_put()
>>> down between the if() and the free_ref().
>>
>> You mean doing something like:
>>
>> if (atomic_read(&blkif->refcnt) <= 3) {
>> 	if (atomic_read(&blkif->drain))
>> 		complete(&blkif->drain_complete);
>> }
>> xen_blkif_put(blkif);
>> free_req(blkif, pending_req);
> 
> Actually, I got the description wrong. I really meant
> 
> free_req();
> if (atomic_read ...)
> 	complete();
> xen_blkif_put();

IMHO this is still a race, since we evaluate refcnt before decrementing
it. If we have for example 2 in flight requests, both could read refcnt,
both could see it's greater than 3 (so no one would call complete), and
then both will decrement it, without anyone actually calling complete.

Roger.

Re: [Xen-devel] xen-blkback: bug fixes

[Konrad Rzeszutek Wilk]

On Mon, Feb 03, 2014 at 10:31:40PM -0800, Matt Wilson wrote:
> On Tue, Jan 28, 2014 at 03:38:37PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Tue, Jan 28, 2014 at 06:43:32PM +0100, Roger Pau Monne wrote:
> > > blkback bug fixes for memory leaks (patches 1 and 2) and a race 
> > > (patch 3).
> > 
> > They all look OK to me. I've stuck them in my 'stable/for-jens-3.14'
> > branch and are testing them now (hadn't pushed it yet).
> > 
> > Matt and Matt,
> > 
> > Could you take a look at the other two patches as well?
> 
> Sure, though somehow you didn't address your message to us, so I
> didn't see it until today.

Duh!
> 
> Matt Rushton did some review and testing on an earlier version that
> came out fine. We'll give the final series a test since there was
> still a bit of rework.

<nods>
> 
> --msw
> 
> > David, Boris,
> > 
> > Are you OK with pushing those patches out to Jens Axboe if nobody
> > gives an NACK by Friday?
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel