Re: [PATCH] xen-blkback: fix memory leaks

[Matt Wilson]

On Mon, Jan 27, 2014 at 05:19:41PM +0100, Roger Pau Monné wrote:
> On 27/01/14 17:09, Konrad Rzeszutek Wilk wrote:
> > On Mon, Jan 27, 2014 at 11:13:41AM +0100, Roger Pau Monne wrote:
> >> I've at least identified two possible memory leaks in blkback, both
> >> related to the shutdown path of a VBD:
> >>
> >> - We don'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.
> >> - We don'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 lists or persistent grants to the persistent_gnts
> >>   red-black tree.
> >>
> >> Also, add some checks in xen_blkif_free to make sure we are cleaning
> >> everything.
> >>
> >> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> >> Cc: David Vrabel <david.vrabel@xxxxxxxxxx>
> >> Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> >> Cc: Matt Rushton <mrushton@xxxxxxxxxx>
> >> Cc: Matt Wilson <msw@xxxxxxxxxx>
> >> Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> >> ---
> >> This should be applied after the patch:
> >>
> >> xen-blkback: fix memory leak when persistent grants are used
> > 
> > Could you respin the series with the issues below fixed and
> > have said patch as part of the series. That way not only does
> > it have your SoB on it but it makes it easier to apply the patch
> > for lazy^H^H^Hbusy maintainers and makes it clear that you had
> > tested both of them.
> > 
> > Also, please CC Jens Axboe on these patches.
> 
> Ack, will do once the comments below are sorted out.

We'll keep an eye out for the resubmitted series and do some testing.

Thanks for taking this the extra step.

--msw

> >> >From Matt Rushton & Matt Wilson and backported to stable.
> >>
> >> I've been able to create and destroy ~4000 guests while doing heavy IO
> >> operations with this patch on a 512M Dom0 without problems.
> >> ---
> >>  drivers/block/xen-blkback/blkback.c |   29 +++++++++++++++++++----------
> >>  drivers/block/xen-blkback/xenbus.c  |    9 +++++++++
> >>  2 files changed, 28 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> >> index 30ef7b3..19925b7 100644
> >> --- a/drivers/block/xen-blkback/blkback.c
> >> +++ b/drivers/block/xen-blkback/blkback.c
> >> @@ -169,6 +169,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
> >>  				struct pending_req *pending_req);
> >>  static void make_response(struct xen_blkif *blkif, u64 id,
> >>  			  unsigned short op, int st);
> >> +static void xen_blk_drain_io(struct xen_blkif *blkif, bool force);
> >>  
> >>  #define foreach_grant_safe(pos, n, rbtree, node) \
> >>  	for ((pos) = container_of(rb_first((rbtree)), typeof(*(pos)), node), \
> >> @@ -625,6 +626,12 @@ purge_gnt_list:
> >>  			print_stats(blkif);
> >>  	}
> >>  
> >> +	/* Drain pending IO */
> >> +	xen_blk_drain_io(blkif, true);
> >> +
> >> +	/* Drain pending purge work */
> >> +	flush_work(&blkif->persistent_purge_work);
> >> +
> >>  	/* Free all persistent grant pages */
> >>  	if (!RB_EMPTY_ROOT(&blkif->persistent_gnts))
> >>  		free_persistent_gnts(blkif, &blkif->persistent_gnts,
> >> @@ -930,7 +937,7 @@ static int dispatch_other_io(struct xen_blkif *blkif,
> >>  	return -EIO;
> >>  }
> >>  
> >> -static void xen_blk_drain_io(struct xen_blkif *blkif)
> >> +static void xen_blk_drain_io(struct xen_blkif *blkif, bool force)
> >>  {
> >>  	atomic_set(&blkif->drain, 1);
> >>  	do {
> >> @@ -943,7 +950,7 @@ static void xen_blk_drain_io(struct xen_blkif *blkif)
> >>  
> >>  		if (!atomic_read(&blkif->drain))
> >>  			break;
> >> -	} while (!kthread_should_stop());
> >> +	} while (!kthread_should_stop() || force);
> >>  	atomic_set(&blkif->drain, 0);
> >>  }
> >>  
> >> @@ -976,17 +983,19 @@ 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);
> >> +		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);
> >>  	}
> >>  }
> >>  
> >> @@ -1224,7 +1233,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
> >>  	 * issue the WRITE_FLUSH.
> >>  	 */
> >>  	if (drain)
> >> -		xen_blk_drain_io(pending_req->blkif);
> >> +		xen_blk_drain_io(pending_req->blkif, false);
> >>  
> >>  	/*
> >>  	 * If we have failed at this point, we need to undo the M2P override,
> >> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> >> index c2014a0..3c10281 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);
> > 
> > Hm,
> 
> See comment below.
> 
> >>  	blkif->free_pages_num = 0;
> >>  	atomic_set(&blkif->persistent_gnt_in_use, 0);
> >>  
> >> @@ -259,6 +260,14 @@ static void xen_blkif_free(struct xen_blkif *blkif)
> >>  	if (!atomic_dec_and_test(&blkif->refcnt))
> >>  		BUG();
> >>  
> >> +	/* 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));
> > 
> > You don't seem to put anything on this list? Or even declare this?
> > Was there another patch in the series?
> 
> No, the list is already used in current code, but it is initialized only
> before usage, now I need to make sure it's initialized even if not used, or:
> 
> BUG_ON(!list_empty(&blkif->persistent_purge_list));
> 
> Is going to fail.
> 
> I will resend this and replace the other (now useless) initialization
> with a BUG_ON(!list_empty...
> 
> > 
> >> +	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);
> >>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/