blkback bug fixes for memory leaks (patches 1 and 2) and a race
(patch 3).
From: Matt Rushton <[email protected]>
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 <[email protected]>
Cc: "Roger Pau Monné" <[email protected]>
Cc: Ian Campbell <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Anthony Liguori <[email protected]>
Signed-off-by: Matt Rushton <[email protected]>
Signed-off-by: Matt Wilson <[email protected]>
---
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)
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é <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: Matt Rushton <[email protected]>
Cc: Matt Wilson <[email protected]>
Cc: Ian Campbell <[email protected]>
---
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)
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é <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Cc: David Vrabel <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: Matt Rushton <[email protected]>
Cc: Matt Wilson <[email protected]>
Cc: Ian Campbell <[email protected]>
---
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)
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
> [email protected]
> http://lists.xen.org/xen-devel
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
>>> On 28.01.14 at 18:43, Roger Pau Monne <[email protected]> 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
On 29/01/14 08:52, Jan Beulich wrote:
>>>> On 28.01.14 at 18:43, Roger Pau Monne <[email protected]> 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.
On 29/01/14 09:52, Jan Beulich wrote:
>>>> On 28.01.14 at 18:43, Roger Pau Monne <[email protected]> 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);
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?
>>> On 03.02.14 at 17:58, Roger Pau Monné<[email protected]> wrote:
> On 29/01/14 09:52, Jan Beulich wrote:
>>>>> On 28.01.14 at 18:43, Roger Pau Monne <[email protected]> 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
On 04/02/14 09:02, Jan Beulich wrote:
>>>> On 03.02.14 at 17:58, Roger Pau Monné<[email protected]> wrote:
>> On 29/01/14 09:52, Jan Beulich wrote:
>>>>>> On 28.01.14 at 18:43, Roger Pau Monne <[email protected]> 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.
>>> On 04.02.14 at 09:16, Roger Pau Monné<[email protected]> wrote:
> On 04/02/14 09:02, Jan Beulich wrote:
>>>>> On 03.02.14 at 17:58, Roger Pau Monné<[email protected]> wrote:
>>> On 29/01/14 09:52, Jan Beulich wrote:
>>>>>>> On 28.01.14 at 18:43, Roger Pau Monne <[email protected]> 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
On 04/02/14 09:31, Jan Beulich wrote:
>>>> On 04.02.14 at 09:16, Roger Pau Monné<[email protected]> wrote:
>> On 04/02/14 09:02, Jan Beulich wrote:
>>>>>> On 03.02.14 at 17:58, Roger Pau Monné<[email protected]> wrote:
>>>> On 29/01/14 09:52, Jan Beulich wrote:
>>>>>>>> On 28.01.14 at 18:43, Roger Pau Monne <[email protected]> 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.
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
> [email protected]
> http://lists.xen.org/xen-devel