Re: [PATCH 3/7] dm-crypt: avoid deadlock in mempools

[Mike Snitzer <snitzer@redhat.com>]

On Fri, Feb 13 2015 at  8:24P -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> This patch fixes a theoretical deadlock introduced in the previous patch.
> 
> The function crypt_alloc_buffer may be called concurrently. If we allocate
> from the mempool concurrently, there is a possibility of deadlock.
> For example, if we have mempool of 256 pages, two processes, each wanting 256,
> pages allocate from the mempool concurrently, it may deadlock in a situation
> where both processes have allocated 128 pages and the mempool is exhausted.
> 
> In order to avoid this scenarios, we allocate the pages under a mutex.
> 
> In order to not degrade performance with excessive locking, we try
> non-blocking allocations without a mutex first and if it fails, we fallback
> to a blocking allocation with a mutex.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/md/dm-crypt.c |   41 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 5 deletions(-)
> 
> Index: linux-3.18-rc1/drivers/md/dm-crypt.c
> ===================================================================
> --- linux-3.18-rc1.orig/drivers/md/dm-crypt.c	2014-10-21 00:49:31.000000000 +0200
> +++ linux-3.18-rc1/drivers/md/dm-crypt.c	2014-10-21 00:49:34.000000000 +0200
> @@ -124,6 +124,7 @@ struct crypt_config {
>  	mempool_t *req_pool;
>  	mempool_t *page_pool;
>  	struct bio_set *bs;
> +	struct mutex bio_alloc_lock;
>  
>  	struct workqueue_struct *io_queue;
>  	struct workqueue_struct *crypt_queue;
> @@ -949,27 +950,51 @@ static void crypt_free_buffer_pages(stru
>  /*
>   * Generate a new unfragmented bio with the given size
>   * This should never violate the device limitations
> + *
> + * This function may be called concurrently. If we allocate from the mempool
> + * concurrently, there is a possibility of deadlock. For example, if we have
> + * mempool of 256 pages, two processes, each wanting 256, pages allocate from
> + * the mempool concurrently, it may deadlock in a situation where both processes
> + * have allocated 128 pages and the mempool is exhausted.
> + *
> + * In order to avoid this scenarios, we allocate the pages under a mutex.
> + *
> + * In order to not degrade performance with excessive locking, we try
> + * non-blocking allocations without a mutex first and if it fails, we fallback
> + * to a blocking allocation with a mutex.
>   */

I folded this in:

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index cb075a7..6fc655d 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -968,10 +968,10 @@ static void crypt_free_buffer_pages(struct crypt_config *cc, struct bio *clone);
  * the mempool concurrently, it may deadlock in a situation where both processes
  * have allocated 128 pages and the mempool is exhausted.
  *
- * In order to avoid this scenarios, we allocate the pages under a mutex.
+ * In order to avoid this scenario we allocate the pages under a mutex.
  *
  * In order to not degrade performance with excessive locking, we try
- * non-blocking allocations without a mutex first and if it fails, we fallback
+ * non-blocking allocation without a mutex first and if it fails, we fallback
  * to a blocking allocation with a mutex.
  */
 static struct bio *crypt_alloc_buffer(struct dm_crypt_io *io, unsigned size)