Re: [PATCH] dm-crypt: limit the number of allocated pages

From: Mikulas Patocka <mpatocka@redhat.com>
To: John Stoffel <john@stoffel.org>
Cc: Mike Snitzer <msnitzer@redhat.com>, Tom Yan <tom.ty89@gmail.com>,
	dm-devel@redhat.com, Ondrej Kozina <okozina@redhat.com>,
	hch@lst.de, Milan Broz <mbroz@redhat.com>
Subject: Re: [PATCH] dm-crypt: limit the number of allocated pages
Date: Mon, 14 Aug 2017 20:07:30 -0400 (EDT)	[thread overview]
Message-ID: <alpine.LRH.2.02.1708142001390.24994@file01.intranet.prod.int.rdu2.redhat.com> (raw)
In-Reply-To: <22930.1510.64868.411152@quad.stoffel.home>

On Mon, 14 Aug 2017, John Stoffel wrote:

> >>>>> "Mikulas" == Mikulas Patocka <mpatocka@redhat.com> writes:
> 
> Mikulas> dm-crypt consumes excessive amount memory when the user attempts to zero
> Mikulas> a dm-crypt device with "blkdiscard -z". The command "blkdiscard -z" calls
> Mikulas> the BLKZEROOUT ioctl, it goes to the function __blkdev_issue_zeroout,
> Mikulas> __blkdev_issue_zeroout sends large amount of write bios that contain the
> Mikulas> zero page as their payload.
> 
> Mikulas> For each incoming page, dm-crypt allocates another page that holds the
> Mikulas> encrypted data, so when processing "blkdiscard -z", dm-crypt tries to
> Mikulas> allocate the amount of memory that is equal to the size of the device.
> Mikulas> This can trigger OOM killer or cause system crash.
> 
> Mikulas> This patch fixes the bug by limiting the amount of memory that dm-crypt
> Mikulas> allocates to 2% of total system memory.
> 
> Is this limit per-device or system wide?  it's not clear to me if the
> minimum is just one page, or more so it might be nice to clarify
> that.

The limit is system-wide. The limit is divided by the number of active 
dm-crypt devices and each device receives an equal share.

There is a lower bound of BIO_MAX_PAGES * 16. The per-device limit is 
never lower than this.

> Also, for large memory systems, that's still alot of pages.  Maybe it
> should be more exponential in it's clamping as memory sizes go up?  2%
> of 2T is 4G, which is pretty darn big...

The more we restrict it, the less parallelism is used in dm-crypt, so it 
makes sense to have a big value on machines with many cores.

> Mikulas> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Mikulas> Cc: stable@vger.kernel.org
> 
> Mikulas> ---
> Mikulas>  drivers/md/dm-crypt.c |   60 +++++++++++++++++++++++++++++++++++++++++++++++++-
> Mikulas>  1 file changed, 59 insertions(+), 1 deletion(-)
> 
> Mikulas> Index: linux-2.6/drivers/md/dm-crypt.c
> Mikulas> ===================================================================
> Mikulas> --- linux-2.6.orig/drivers/md/dm-crypt.c
> Mikulas> +++ linux-2.6/drivers/md/dm-crypt.c
> Mikulas> @@ -148,6 +148,8 @@ struct crypt_config {
> Mikulas>  	mempool_t *tag_pool;
> Mikulas>  	unsigned tag_pool_max_sectors;
>  
> Mikulas> +	struct percpu_counter n_allocated_pages;
> Mikulas> +
> Mikulas>  	struct bio_set *bs;
> Mikulas>  	struct mutex bio_alloc_lock;
>  
> Mikulas> @@ -219,6 +221,12 @@ struct crypt_config {
> Mikulas>  #define MAX_TAG_SIZE	480
> Mikulas>  #define POOL_ENTRY_SIZE	512
>  
> Mikulas> +static DEFINE_SPINLOCK(dm_crypt_clients_lock);
> Mikulas> +static unsigned dm_crypt_clients_n = 0;
> Mikulas> +static volatile unsigned long dm_crypt_pages_per_client;
> Mikulas> +#define DM_CRYPT_MEMORY_PERCENT			2
> Mikulas> +#define DM_CRYPT_MIN_PAGES_PER_CLIENT		(BIO_MAX_PAGES * 16)
> Mikulas> +
> Mikulas>  static void clone_init(struct dm_crypt_io *, struct bio *);
> Mikulas>  static void kcryptd_queue_crypt(struct dm_crypt_io *io);
> Mikulas>  static struct scatterlist *crypt_get_sg_data(struct crypt_config *cc,
> Mikulas> @@ -2158,6 +2166,37 @@ static int crypt_wipe_key(struct crypt_c
> Mikulas>  	return r;
> Mikulas>  }
>  
> Mikulas> +static void crypt_calculate_pages_per_client(void)
> Mikulas> +{
> Mikulas> +	unsigned long pages = (totalram_pages - totalhigh_pages) * DM_CRYPT_MEMORY_PERCENT / 100;
> Mikulas> +	if (!dm_crypt_clients_n)
> Mikulas> +		return;
> Mikulas> +	pages /= dm_crypt_clients_n;
> 
> Would it make sense to use a shift here, or does this only run once on
> setup?  And shouldn't you return a value here, if only to make it
> clear what you're defaulting to?  

This piece of code is executed only when a dm-crypt device is created or 
deactivated, so performance doesn't matter.

> Mikulas> +	if (pages < DM_CRYPT_MIN_PAGES_PER_CLIENT)
> Mikulas> +		pages = DM_CRYPT_MIN_PAGES_PER_CLIENT;
> Mikulas> +	dm_crypt_pages_per_client = pages;
> Mikulas> +}
> Mikulas> +
> Mikulas> +static void *crypt_page_alloc(gfp_t gfp_mask, void *pool_data)
> Mikulas> +{
> Mikulas> +	struct crypt_config *cc = pool_data;
> Mikulas> +	struct page *page;
> Mikulas> +	if (unlikely(percpu_counter_compare(&cc->n_allocated_pages, dm_crypt_pages_per_client) >= 0) &&
> Mikulas> +	    likely(gfp_mask & __GFP_NORETRY))
> Mikulas> +		return NULL;
> Mikulas> +	page = alloc_page(gfp_mask);
> Mikulas> +	if (likely(page != NULL))
> Mikulas> +		percpu_counter_add(&cc->n_allocated_pages, 1);
> Mikulas> +	return page;
> Mikulas> +}
> Mikulas> +
> Mikulas> +static void crypt_page_free(void *page, void *pool_data)
> Mikulas> +{
> Mikulas> +	struct crypt_config *cc = pool_data;
> Mikulas> +	__free_page(page);
> Mikulas> +	percpu_counter_sub(&cc->n_allocated_pages, 1);
> Mikulas> +}
> Mikulas> +
> Mikulas>  static void crypt_dtr(struct dm_target *ti)
> Mikulas>  {
> Mikulas>  	struct crypt_config *cc = ti->private;
> Mikulas> @@ -2184,6 +2223,10 @@ static void crypt_dtr(struct dm_target *
> Mikulas>  	mempool_destroy(cc->req_pool);
> Mikulas>  	mempool_destroy(cc->tag_pool);
>  
> Mikulas> +	if (cc->page_pool)
> Mikulas> +		WARN_ON(percpu_counter_sum(&cc->n_allocated_pages) != 0);
> Mikulas> +	percpu_counter_destroy(&cc->n_allocated_pages);
> Mikulas> +
> Mikulas>  	if (cc->iv_gen_ops && cc->iv_gen_ops->dtr)
> cc-> iv_gen_ops->dtr(cc);
>  
> Mikulas> @@ -2198,6 +2241,12 @@ static void crypt_dtr(struct dm_target *
>  
> Mikulas>  	/* Must zero key material before freeing */
> Mikulas>  	kzfree(cc);
> Mikulas> +
> Mikulas> +	spin_lock(&dm_crypt_clients_lock);
> Mikulas> +	WARN_ON(!dm_crypt_clients_n);
> Mikulas> +	dm_crypt_clients_n--;
> Mikulas> +	crypt_calculate_pages_per_client();
> Mikulas> +	spin_unlock(&dm_crypt_clients_lock);
> Mikulas>  }
>  
> Mikulas>  static int crypt_ctr_ivmode(struct dm_target *ti, const char *ivmode)
> Mikulas> @@ -2636,6 +2685,15 @@ static int crypt_ctr(struct dm_target *t
>  
> ti-> private = cc;
>  
> Mikulas> +	spin_lock(&dm_crypt_clients_lock);
> Mikulas> +	dm_crypt_clients_n++;
> Mikulas> +	crypt_calculate_pages_per_client();
> Mikulas> +	spin_unlock(&dm_crypt_clients_lock);
> Mikulas> +
> Mikulas> +	ret = percpu_counter_init(&cc->n_allocated_pages, 0, GFP_KERNEL);
> Mikulas> +	if (ret < 0)
> Mikulas> +		goto bad;
> Mikulas> +
> Mikulas>  	/* Optional parameters need to be read before cipher constructor */
> Mikulas>  	if (argc > 5) {
> Mikulas>  		ret = crypt_ctr_optional(ti, argc - 5, &argv[5]);
> Mikulas> @@ -2690,7 +2748,7 @@ static int crypt_ctr(struct dm_target *t
> Mikulas>  		ALIGN(sizeof(struct dm_crypt_io) + cc->dmreq_start + additional_req_size,
> Mikulas>  		      ARCH_KMALLOC_MINALIGN);
>  
> Mikulas> -	cc->page_pool = mempool_create_page_pool(BIO_MAX_PAGES, 0);
> Mikulas> +	cc->page_pool = mempool_create(BIO_MAX_PAGES, crypt_page_alloc, crypt_page_free, cc);
> Mikulas>  	if (!cc->page_pool) {
> ti-> error = "Cannot allocate page mempool";
> Mikulas>  		goto bad;
> 
> Mikulas> --
> Mikulas> dm-devel mailing list
> Mikulas> dm-devel@redhat.com
> Mikulas> https://www.redhat.com/mailman/listinfo/dm-devel
> 