[PATCH] dm cache: fix issue with background work locking

[PATCH] dm cache: fix issue with background work locking

[Benjamin Marzinski]

dm cache used a rw_semaphore for background_work_lock. Write locks on
rw_semaphores have strict owner semantics, but there was no guarantee
that the process that locked background_work_lock was the same process
that unlocked it. This can be easily seen using a kernel compiled with
CONFIG_DEBUG_RWSEMS. Given a dm cache device <cache>, run: 'dmsetup
suspend <cache> && dmsetup resume <cache>'. This will tigger a kernel
warning:

DEBUG_RWSEMS_WARN_ON((rwsem_owner(sem) != current) && !rwsem_test_oflags(sem, RWSEM_NONSPINNABLE))

triggered by cache_resume(). To fix this, switch from a rw_semaphore to
a spinlock and a wait queue. dm cache already hasd a wait queue and
associated counter, migration_wait and nr_allocated_migrations, that was
getting woken up when background work was getting completed, but wasn't
actually used by anything. This is replaced by the background_work queue
and counter.

Fixes: b29d4986d0da ("dm cache: significant rework to leverage dm-bio-prison-v2")
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 drivers/md/dm-cache-target.c | 55 ++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
index 33dbc71b730f..d8d9c63d2a67 100644
--- a/drivers/md/dm-cache-target.c
+++ b/drivers/md/dm-cache-target.c
@@ -340,8 +340,6 @@ struct cache {
 	struct list_head invalidation_requests;
 
 	sector_t migration_threshold;
-	wait_queue_head_t migration_wait;
-	atomic_t nr_allocated_migrations;
 
 	/*
 	 * The number of in flight migrations that are performing
@@ -397,7 +395,11 @@ struct cache {
 	bool loaded_mappings:1;
 	bool loaded_discards:1;
 
-	struct rw_semaphore background_work_lock;
+	/* background work management */
+	bool background_work_allowed;
+	unsigned background_work_nr;
+	spinlock_t background_work_lock;
+	wait_queue_head_t background_work_wait;
 
 	struct batcher committer;
 	struct work_struct commit_ws;
@@ -488,19 +490,13 @@ static struct dm_cache_migration *alloc_migration(struct cache *cache)
 	memset(mg, 0, sizeof(*mg));
 
 	mg->cache = cache;
-	atomic_inc(&cache->nr_allocated_migrations);
 
 	return mg;
 }
 
 static void free_migration(struct dm_cache_migration *mg)
 {
-	struct cache *cache = mg->cache;
-
-	if (atomic_dec_and_test(&cache->nr_allocated_migrations))
-		wake_up(&cache->migration_wait);
-
-	mempool_free(mg, &cache->migration_pool);
+	mempool_free(mg, &mg->cache->migration_pool);
 }
 
 /*----------------------------------------------------------------*/
@@ -1030,34 +1026,39 @@ static void calc_discard_block_range(struct cache *cache, struct bio *bio,
 
 static void prevent_background_work(struct cache *cache)
 {
-	lockdep_off();
-	down_write(&cache->background_work_lock);
-	lockdep_on();
+	spin_lock_irq(&cache->background_work_lock);
+	cache->background_work_allowed = false;
+	wait_event_lock_irq(cache->background_work_wait,
+			    cache->background_work_nr == 0,
+			    cache->background_work_lock);
+	spin_unlock_irq(&cache->background_work_lock);
 }
 
 static void allow_background_work(struct cache *cache)
 {
-	lockdep_off();
-	up_write(&cache->background_work_lock);
-	lockdep_on();
+	spin_lock_irq(&cache->background_work_lock);
+	cache->background_work_allowed = true;
+	spin_unlock_irq(&cache->background_work_lock);
 }
 
 static bool background_work_begin(struct cache *cache)
 {
 	bool r;
 
-	lockdep_off();
-	r = down_read_trylock(&cache->background_work_lock);
-	lockdep_on();
-
+	spin_lock_irq(&cache->background_work_lock);
+	r = cache->background_work_allowed;
+	if (r)
+		cache->background_work_nr++;
+	spin_unlock_irq(&cache->background_work_lock);
 	return r;
 }
 
 static void background_work_end(struct cache *cache)
 {
-	lockdep_off();
-	up_read(&cache->background_work_lock);
-	lockdep_on();
+	spin_lock_irq(&cache->background_work_lock);
+	if (--cache->background_work_nr == 0)
+		wake_up(&cache->background_work_wait);
+	spin_unlock_irq(&cache->background_work_lock);
 }
 
 /*----------------------------------------------------------------*/
@@ -2507,9 +2508,7 @@ static int cache_create(struct cache_args *ca, struct cache **result)
 
 	spin_lock_init(&cache->lock);
 	bio_list_init(&cache->deferred_bios);
-	atomic_set(&cache->nr_allocated_migrations, 0);
 	atomic_set(&cache->nr_io_migrations, 0);
-	init_waitqueue_head(&cache->migration_wait);
 
 	r = -ENOMEM;
 	atomic_set(&cache->nr_dirty, 0);
@@ -2592,8 +2591,10 @@ static int cache_create(struct cache_args *ca, struct cache **result)
 		     issue_op, cache, cache->wq);
 	dm_iot_init(&cache->tracker);
 
-	init_rwsem(&cache->background_work_lock);
-	prevent_background_work(cache);
+	init_waitqueue_head(&cache->background_work_wait);
+	spin_lock_init(&cache->background_work_lock);
+	cache->background_work_allowed = false;
+	cache->background_work_nr = 0;
 
 	*result = cache;
 	return 0;
-- 
2.53.0

Re: [PATCH] dm cache: fix issue with background work locking

[Matthew Sakai]

On 8/25/26 3:22 PM, Benjamin Marzinski wrote:
> dm cache used a rw_semaphore for background_work_lock. Write locks on
> rw_semaphores have strict owner semantics, but there was no guarantee
> that the process that locked background_work_lock was the same process
> that unlocked it. This can be easily seen using a kernel compiled with
> CONFIG_DEBUG_RWSEMS. Given a dm cache device <cache>, run: 'dmsetup
> suspend <cache> && dmsetup resume <cache>'. This will tigger a kernel

typo: trigger

> warning:
> 
> DEBUG_RWSEMS_WARN_ON((rwsem_owner(sem) != current) && !rwsem_test_oflags(sem, RWSEM_NONSPINNABLE))
> 
> triggered by cache_resume(). To fix this, switch from a rw_semaphore to
> a spinlock and a wait queue. dm cache already hasd a wait queue and

typo: has

> associated counter, migration_wait and nr_allocated_migrations, that was
> getting woken up when background work was getting completed, but wasn't
> actually used by anything. This is replaced by the background_work queue
> and counter.
> 
> Fixes: b29d4986d0da ("dm cache: significant rework to leverage dm-bio-prison-v2")
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Aside from the above, feel free to add

Reviewed-by: Matthew Sakai <msakai@redhat.com>

> ---
>   drivers/md/dm-cache-target.c | 55 ++++++++++++++++++------------------
>   1 file changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
> index 33dbc71b730f..d8d9c63d2a67 100644
> --- a/drivers/md/dm-cache-target.c
> +++ b/drivers/md/dm-cache-target.c
> @@ -340,8 +340,6 @@ struct cache {
>   	struct list_head invalidation_requests;
>   
>   	sector_t migration_threshold;
> -	wait_queue_head_t migration_wait;
> -	atomic_t nr_allocated_migrations;
>   
>   	/*
>   	 * The number of in flight migrations that are performing
> @@ -397,7 +395,11 @@ struct cache {
>   	bool loaded_mappings:1;
>   	bool loaded_discards:1;
>   
> -	struct rw_semaphore background_work_lock;
> +	/* background work management */
> +	bool background_work_allowed;
> +	unsigned background_work_nr;
> +	spinlock_t background_work_lock;
> +	wait_queue_head_t background_work_wait;
>   
>   	struct batcher committer;
>   	struct work_struct commit_ws;
> @@ -488,19 +490,13 @@ static struct dm_cache_migration *alloc_migration(struct cache *cache)
>   	memset(mg, 0, sizeof(*mg));
>   
>   	mg->cache = cache;
> -	atomic_inc(&cache->nr_allocated_migrations);
>   
>   	return mg;
>   }
>   
>   static void free_migration(struct dm_cache_migration *mg)
>   {
> -	struct cache *cache = mg->cache;
> -
> -	if (atomic_dec_and_test(&cache->nr_allocated_migrations))
> -		wake_up(&cache->migration_wait);
> -
> -	mempool_free(mg, &cache->migration_pool);
> +	mempool_free(mg, &mg->cache->migration_pool);
>   }
>   
>   /*----------------------------------------------------------------*/
> @@ -1030,34 +1026,39 @@ static void calc_discard_block_range(struct cache *cache, struct bio *bio,
>   
>   static void prevent_background_work(struct cache *cache)
>   {
> -	lockdep_off();
> -	down_write(&cache->background_work_lock);
> -	lockdep_on();
> +	spin_lock_irq(&cache->background_work_lock);
> +	cache->background_work_allowed = false;
> +	wait_event_lock_irq(cache->background_work_wait,
> +			    cache->background_work_nr == 0,
> +			    cache->background_work_lock);
> +	spin_unlock_irq(&cache->background_work_lock);
>   }
>   
>   static void allow_background_work(struct cache *cache)
>   {
> -	lockdep_off();
> -	up_write(&cache->background_work_lock);
> -	lockdep_on();
> +	spin_lock_irq(&cache->background_work_lock);
> +	cache->background_work_allowed = true;
> +	spin_unlock_irq(&cache->background_work_lock);
>   }
>   
>   static bool background_work_begin(struct cache *cache)
>   {
>   	bool r;
>   
> -	lockdep_off();
> -	r = down_read_trylock(&cache->background_work_lock);
> -	lockdep_on();
> -
> +	spin_lock_irq(&cache->background_work_lock);
> +	r = cache->background_work_allowed;
> +	if (r)
> +		cache->background_work_nr++;
> +	spin_unlock_irq(&cache->background_work_lock);
>   	return r;
>   }
>   
>   static void background_work_end(struct cache *cache)
>   {
> -	lockdep_off();
> -	up_read(&cache->background_work_lock);
> -	lockdep_on();
> +	spin_lock_irq(&cache->background_work_lock);
> +	if (--cache->background_work_nr == 0)
> +		wake_up(&cache->background_work_wait);
> +	spin_unlock_irq(&cache->background_work_lock);
>   }
>   
>   /*----------------------------------------------------------------*/
> @@ -2507,9 +2508,7 @@ static int cache_create(struct cache_args *ca, struct cache **result)
>   
>   	spin_lock_init(&cache->lock);
>   	bio_list_init(&cache->deferred_bios);
> -	atomic_set(&cache->nr_allocated_migrations, 0);
>   	atomic_set(&cache->nr_io_migrations, 0);
> -	init_waitqueue_head(&cache->migration_wait);
>   
>   	r = -ENOMEM;
>   	atomic_set(&cache->nr_dirty, 0);
> @@ -2592,8 +2591,10 @@ static int cache_create(struct cache_args *ca, struct cache **result)
>   		     issue_op, cache, cache->wq);
>   	dm_iot_init(&cache->tracker);
>   
> -	init_rwsem(&cache->background_work_lock);
> -	prevent_background_work(cache);
> +	init_waitqueue_head(&cache->background_work_wait);
> +	spin_lock_init(&cache->background_work_lock);
> +	cache->background_work_allowed = false;
> +	cache->background_work_nr = 0;
>   
>   	*result = cache;
>   	return 0;

Re: [PATCH] dm cache: fix issue with background work locking

[Benjamin Marzinski]

On Tue, Aug 25, 2026 at 04:22:01PM -0400, Matthew Sakai wrote:
> 
> 
> On 8/25/26 3:22 PM, Benjamin Marzinski wrote:
> > dm cache used a rw_semaphore for background_work_lock. Write locks on
> > rw_semaphores have strict owner semantics, but there was no guarantee
> > that the process that locked background_work_lock was the same process
> > that unlocked it. This can be easily seen using a kernel compiled with
> > CONFIG_DEBUG_RWSEMS. Given a dm cache device <cache>, run: 'dmsetup
> > suspend <cache> && dmsetup resume <cache>'. This will tigger a kernel
> 
> typo: trigger
> 
> > warning:
> > 
> > DEBUG_RWSEMS_WARN_ON((rwsem_owner(sem) != current) && !rwsem_test_oflags(sem, RWSEM_NONSPINNABLE))
> > 
> > triggered by cache_resume(). To fix this, switch from a rw_semaphore to
> > a spinlock and a wait queue. dm cache already hasd a wait queue and
> 
> typo: has
> 
> > associated counter, migration_wait and nr_allocated_migrations, that was
> > getting woken up when background work was getting completed, but wasn't
> > actually used by anything. This is replaced by the background_work queue
> > and counter.
> > 
> > Fixes: b29d4986d0da ("dm cache: significant rework to leverage dm-bio-prison-v2")
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> 
> Aside from the above, feel free to add
> 
> Reviewed-by: Matthew Sakai <msakai@redhat.com>

Thanks. I'll wait to see if there are more comments first, but I will
respin this to fix the typos.

-Ben

> 
> > ---
> >   drivers/md/dm-cache-target.c | 55 ++++++++++++++++++------------------
> >   1 file changed, 28 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
> > index 33dbc71b730f..d8d9c63d2a67 100644
> > --- a/drivers/md/dm-cache-target.c
> > +++ b/drivers/md/dm-cache-target.c
> > @@ -340,8 +340,6 @@ struct cache {
> >   	struct list_head invalidation_requests;
> >   	sector_t migration_threshold;
> > -	wait_queue_head_t migration_wait;
> > -	atomic_t nr_allocated_migrations;
> >   	/*
> >   	 * The number of in flight migrations that are performing
> > @@ -397,7 +395,11 @@ struct cache {
> >   	bool loaded_mappings:1;
> >   	bool loaded_discards:1;
> > -	struct rw_semaphore background_work_lock;
> > +	/* background work management */
> > +	bool background_work_allowed;
> > +	unsigned background_work_nr;
> > +	spinlock_t background_work_lock;
> > +	wait_queue_head_t background_work_wait;
> >   	struct batcher committer;
> >   	struct work_struct commit_ws;
> > @@ -488,19 +490,13 @@ static struct dm_cache_migration *alloc_migration(struct cache *cache)
> >   	memset(mg, 0, sizeof(*mg));
> >   	mg->cache = cache;
> > -	atomic_inc(&cache->nr_allocated_migrations);
> >   	return mg;
> >   }
> >   static void free_migration(struct dm_cache_migration *mg)
> >   {
> > -	struct cache *cache = mg->cache;
> > -
> > -	if (atomic_dec_and_test(&cache->nr_allocated_migrations))
> > -		wake_up(&cache->migration_wait);
> > -
> > -	mempool_free(mg, &cache->migration_pool);
> > +	mempool_free(mg, &mg->cache->migration_pool);
> >   }
> >   /*----------------------------------------------------------------*/
> > @@ -1030,34 +1026,39 @@ static void calc_discard_block_range(struct cache *cache, struct bio *bio,
> >   static void prevent_background_work(struct cache *cache)
> >   {
> > -	lockdep_off();
> > -	down_write(&cache->background_work_lock);
> > -	lockdep_on();
> > +	spin_lock_irq(&cache->background_work_lock);
> > +	cache->background_work_allowed = false;
> > +	wait_event_lock_irq(cache->background_work_wait,
> > +			    cache->background_work_nr == 0,
> > +			    cache->background_work_lock);
> > +	spin_unlock_irq(&cache->background_work_lock);
> >   }
> >   static void allow_background_work(struct cache *cache)
> >   {
> > -	lockdep_off();
> > -	up_write(&cache->background_work_lock);
> > -	lockdep_on();
> > +	spin_lock_irq(&cache->background_work_lock);
> > +	cache->background_work_allowed = true;
> > +	spin_unlock_irq(&cache->background_work_lock);
> >   }
> >   static bool background_work_begin(struct cache *cache)
> >   {
> >   	bool r;
> > -	lockdep_off();
> > -	r = down_read_trylock(&cache->background_work_lock);
> > -	lockdep_on();
> > -
> > +	spin_lock_irq(&cache->background_work_lock);
> > +	r = cache->background_work_allowed;
> > +	if (r)
> > +		cache->background_work_nr++;
> > +	spin_unlock_irq(&cache->background_work_lock);
> >   	return r;
> >   }
> >   static void background_work_end(struct cache *cache)
> >   {
> > -	lockdep_off();
> > -	up_read(&cache->background_work_lock);
> > -	lockdep_on();
> > +	spin_lock_irq(&cache->background_work_lock);
> > +	if (--cache->background_work_nr == 0)
> > +		wake_up(&cache->background_work_wait);
> > +	spin_unlock_irq(&cache->background_work_lock);
> >   }
> >   /*----------------------------------------------------------------*/
> > @@ -2507,9 +2508,7 @@ static int cache_create(struct cache_args *ca, struct cache **result)
> >   	spin_lock_init(&cache->lock);
> >   	bio_list_init(&cache->deferred_bios);
> > -	atomic_set(&cache->nr_allocated_migrations, 0);
> >   	atomic_set(&cache->nr_io_migrations, 0);
> > -	init_waitqueue_head(&cache->migration_wait);
> >   	r = -ENOMEM;
> >   	atomic_set(&cache->nr_dirty, 0);
> > @@ -2592,8 +2591,10 @@ static int cache_create(struct cache_args *ca, struct cache **result)
> >   		     issue_op, cache, cache->wq);
> >   	dm_iot_init(&cache->tracker);
> > -	init_rwsem(&cache->background_work_lock);
> > -	prevent_background_work(cache);
> > +	init_waitqueue_head(&cache->background_work_wait);
> > +	spin_lock_init(&cache->background_work_lock);
> > +	cache->background_work_allowed = false;
> > +	cache->background_work_nr = 0;
> >   	*result = cache;
> >   	return 0;

Re: [PATCH] dm cache: fix issue with background work locking

[Ming Hung Tsai]

On Wed, Aug 26, 2026 at 3:22 AM Benjamin Marzinski <bmarzins@redhat.com> wrote:
>
> dm cache used a rw_semaphore for background_work_lock. Write locks on
> rw_semaphores have strict owner semantics, but there was no guarantee
> that the process that locked background_work_lock was the same process
> that unlocked it. This can be easily seen using a kernel compiled with
> CONFIG_DEBUG_RWSEMS. Given a dm cache device <cache>, run: 'dmsetup
> suspend <cache> && dmsetup resume <cache>'. This will tigger a kernel
> warning:
>
> DEBUG_RWSEMS_WARN_ON((rwsem_owner(sem) != current) && !rwsem_test_oflags(sem, RWSEM_NONSPINNABLE))
>
> triggered by cache_resume(). To fix this, switch from a rw_semaphore to
> a spinlock and a wait queue. dm cache already hasd a wait queue and
> associated counter, migration_wait and nr_allocated_migrations, that was
> getting woken up when background work was getting completed, but wasn't
> actually used by anything. This is replaced by the background_work queue
> and counter.
>
> Fixes: b29d4986d0da ("dm cache: significant rework to leverage dm-bio-prison-v2")
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---

Thanks Ben. I confirmed the patch fixed it.

Just one minor nit: checkpatch prefers 'unsigned int' for background_work_nr.

Worth noting before the commit b29d4986d0da ("dm cache: significant
rework to leverage dm-bio-prison-v2"), dm cache waited for migrations
via wait_for_migrations(), which did a wait_event() on migration_wait
and nr_allocated_migrations. That commit replaced it with
rw_semaphores and used lockdep_off() to suppress warnings, before
DEBUG_RWSEMS existed.

Reviewed-by: Ming-Hung Tsai <mtsai@redhat.com>

>  drivers/md/dm-cache-target.c | 55 ++++++++++++++++++------------------
>  1 file changed, 28 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
> index 33dbc71b730f..d8d9c63d2a67 100644
> --- a/drivers/md/dm-cache-target.c
> +++ b/drivers/md/dm-cache-target.c
> @@ -340,8 +340,6 @@ struct cache {
>         struct list_head invalidation_requests;
>
>         sector_t migration_threshold;
> -       wait_queue_head_t migration_wait;
> -       atomic_t nr_allocated_migrations;
>
>         /*
>          * The number of in flight migrations that are performing
> @@ -397,7 +395,11 @@ struct cache {
>         bool loaded_mappings:1;
>         bool loaded_discards:1;
>
> -       struct rw_semaphore background_work_lock;
> +       /* background work management */
> +       bool background_work_allowed;
> +       unsigned background_work_nr;
> +       spinlock_t background_work_lock;
> +       wait_queue_head_t background_work_wait;
>
>         struct batcher committer;
>         struct work_struct commit_ws;
> @@ -488,19 +490,13 @@ static struct dm_cache_migration *alloc_migration(struct cache *cache)
>         memset(mg, 0, sizeof(*mg));
>
>         mg->cache = cache;
> -       atomic_inc(&cache->nr_allocated_migrations);
>
>         return mg;
>  }
>
>  static void free_migration(struct dm_cache_migration *mg)
>  {
> -       struct cache *cache = mg->cache;
> -
> -       if (atomic_dec_and_test(&cache->nr_allocated_migrations))
> -               wake_up(&cache->migration_wait);
> -
> -       mempool_free(mg, &cache->migration_pool);
> +       mempool_free(mg, &mg->cache->migration_pool);
>  }
>
>  /*----------------------------------------------------------------*/
> @@ -1030,34 +1026,39 @@ static void calc_discard_block_range(struct cache *cache, struct bio *bio,
>
>  static void prevent_background_work(struct cache *cache)
>  {
> -       lockdep_off();
> -       down_write(&cache->background_work_lock);
> -       lockdep_on();
> +       spin_lock_irq(&cache->background_work_lock);
> +       cache->background_work_allowed = false;
> +       wait_event_lock_irq(cache->background_work_wait,
> +                           cache->background_work_nr == 0,
> +                           cache->background_work_lock);
> +       spin_unlock_irq(&cache->background_work_lock);
>  }
>
>  static void allow_background_work(struct cache *cache)
>  {
> -       lockdep_off();
> -       up_write(&cache->background_work_lock);
> -       lockdep_on();
> +       spin_lock_irq(&cache->background_work_lock);
> +       cache->background_work_allowed = true;
> +       spin_unlock_irq(&cache->background_work_lock);
>  }
>
>  static bool background_work_begin(struct cache *cache)
>  {
>         bool r;
>
> -       lockdep_off();
> -       r = down_read_trylock(&cache->background_work_lock);
> -       lockdep_on();
> -
> +       spin_lock_irq(&cache->background_work_lock);
> +       r = cache->background_work_allowed;
> +       if (r)
> +               cache->background_work_nr++;
> +       spin_unlock_irq(&cache->background_work_lock);
>         return r;
>  }
>
>  static void background_work_end(struct cache *cache)
>  {
> -       lockdep_off();
> -       up_read(&cache->background_work_lock);
> -       lockdep_on();
> +       spin_lock_irq(&cache->background_work_lock);
> +       if (--cache->background_work_nr == 0)
> +               wake_up(&cache->background_work_wait);
> +       spin_unlock_irq(&cache->background_work_lock);
>  }
>
>  /*----------------------------------------------------------------*/
> @@ -2507,9 +2508,7 @@ static int cache_create(struct cache_args *ca, struct cache **result)
>
>         spin_lock_init(&cache->lock);
>         bio_list_init(&cache->deferred_bios);
> -       atomic_set(&cache->nr_allocated_migrations, 0);
>         atomic_set(&cache->nr_io_migrations, 0);
> -       init_waitqueue_head(&cache->migration_wait);
>
>         r = -ENOMEM;
>         atomic_set(&cache->nr_dirty, 0);
> @@ -2592,8 +2591,10 @@ static int cache_create(struct cache_args *ca, struct cache **result)
>                      issue_op, cache, cache->wq);
>         dm_iot_init(&cache->tracker);
>
> -       init_rwsem(&cache->background_work_lock);
> -       prevent_background_work(cache);
> +       init_waitqueue_head(&cache->background_work_wait);
> +       spin_lock_init(&cache->background_work_lock);
> +       cache->background_work_allowed = false;
> +       cache->background_work_nr = 0;
>
>         *result = cache;
>         return 0;
> --
> 2.53.0
>

Re: [PATCH] dm cache: fix issue with background work locking

[Mikulas Patocka]

[-- Attachment #1: Type: text/plain, Size: 6751 bytes --]



On Thu, 27 Aug 2026, Ming Hung Tsai wrote:

> On Wed, Aug 26, 2026 at 3:22 AM Benjamin Marzinski <bmarzins@redhat.com> wrote:
> >
> > dm cache used a rw_semaphore for background_work_lock. Write locks on
> > rw_semaphores have strict owner semantics, but there was no guarantee
> > that the process that locked background_work_lock was the same process
> > that unlocked it. This can be easily seen using a kernel compiled with
> > CONFIG_DEBUG_RWSEMS. Given a dm cache device <cache>, run: 'dmsetup
> > suspend <cache> && dmsetup resume <cache>'. This will tigger a kernel
> > warning:
> >
> > DEBUG_RWSEMS_WARN_ON((rwsem_owner(sem) != current) && !rwsem_test_oflags(sem, RWSEM_NONSPINNABLE))
> >
> > triggered by cache_resume(). To fix this, switch from a rw_semaphore to
> > a spinlock and a wait queue. dm cache already hasd a wait queue and
> > associated counter, migration_wait and nr_allocated_migrations, that was
> > getting woken up when background work was getting completed, but wasn't
> > actually used by anything. This is replaced by the background_work queue
> > and counter.
> >
> > Fixes: b29d4986d0da ("dm cache: significant rework to leverage dm-bio-prison-v2")
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> 
> Thanks Ben. I confirmed the patch fixed it.
> 
> Just one minor nit: checkpatch prefers 'unsigned int' for background_work_nr.

Hi

Personally, I prefer 'unsigned'. There is no reason why to type more 
tokens if we don't have to.

Mikulas

> Worth noting before the commit b29d4986d0da ("dm cache: significant
> rework to leverage dm-bio-prison-v2"), dm cache waited for migrations
> via wait_for_migrations(), which did a wait_event() on migration_wait
> and nr_allocated_migrations. That commit replaced it with
> rw_semaphores and used lockdep_off() to suppress warnings, before
> DEBUG_RWSEMS existed.
> 
> Reviewed-by: Ming-Hung Tsai <mtsai@redhat.com>
> 
> >  drivers/md/dm-cache-target.c | 55 ++++++++++++++++++------------------
> >  1 file changed, 28 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
> > index 33dbc71b730f..d8d9c63d2a67 100644
> > --- a/drivers/md/dm-cache-target.c
> > +++ b/drivers/md/dm-cache-target.c
> > @@ -340,8 +340,6 @@ struct cache {
> >         struct list_head invalidation_requests;
> >
> >         sector_t migration_threshold;
> > -       wait_queue_head_t migration_wait;
> > -       atomic_t nr_allocated_migrations;
> >
> >         /*
> >          * The number of in flight migrations that are performing
> > @@ -397,7 +395,11 @@ struct cache {
> >         bool loaded_mappings:1;
> >         bool loaded_discards:1;
> >
> > -       struct rw_semaphore background_work_lock;
> > +       /* background work management */
> > +       bool background_work_allowed;
> > +       unsigned background_work_nr;
> > +       spinlock_t background_work_lock;
> > +       wait_queue_head_t background_work_wait;
> >
> >         struct batcher committer;
> >         struct work_struct commit_ws;
> > @@ -488,19 +490,13 @@ static struct dm_cache_migration *alloc_migration(struct cache *cache)
> >         memset(mg, 0, sizeof(*mg));
> >
> >         mg->cache = cache;
> > -       atomic_inc(&cache->nr_allocated_migrations);
> >
> >         return mg;
> >  }
> >
> >  static void free_migration(struct dm_cache_migration *mg)
> >  {
> > -       struct cache *cache = mg->cache;
> > -
> > -       if (atomic_dec_and_test(&cache->nr_allocated_migrations))
> > -               wake_up(&cache->migration_wait);
> > -
> > -       mempool_free(mg, &cache->migration_pool);
> > +       mempool_free(mg, &mg->cache->migration_pool);
> >  }
> >
> >  /*----------------------------------------------------------------*/
> > @@ -1030,34 +1026,39 @@ static void calc_discard_block_range(struct cache *cache, struct bio *bio,
> >
> >  static void prevent_background_work(struct cache *cache)
> >  {
> > -       lockdep_off();
> > -       down_write(&cache->background_work_lock);
> > -       lockdep_on();
> > +       spin_lock_irq(&cache->background_work_lock);
> > +       cache->background_work_allowed = false;
> > +       wait_event_lock_irq(cache->background_work_wait,
> > +                           cache->background_work_nr == 0,
> > +                           cache->background_work_lock);
> > +       spin_unlock_irq(&cache->background_work_lock);
> >  }
> >
> >  static void allow_background_work(struct cache *cache)
> >  {
> > -       lockdep_off();
> > -       up_write(&cache->background_work_lock);
> > -       lockdep_on();
> > +       spin_lock_irq(&cache->background_work_lock);
> > +       cache->background_work_allowed = true;
> > +       spin_unlock_irq(&cache->background_work_lock);
> >  }
> >
> >  static bool background_work_begin(struct cache *cache)
> >  {
> >         bool r;
> >
> > -       lockdep_off();
> > -       r = down_read_trylock(&cache->background_work_lock);
> > -       lockdep_on();
> > -
> > +       spin_lock_irq(&cache->background_work_lock);
> > +       r = cache->background_work_allowed;
> > +       if (r)
> > +               cache->background_work_nr++;
> > +       spin_unlock_irq(&cache->background_work_lock);
> >         return r;
> >  }
> >
> >  static void background_work_end(struct cache *cache)
> >  {
> > -       lockdep_off();
> > -       up_read(&cache->background_work_lock);
> > -       lockdep_on();
> > +       spin_lock_irq(&cache->background_work_lock);
> > +       if (--cache->background_work_nr == 0)
> > +               wake_up(&cache->background_work_wait);
> > +       spin_unlock_irq(&cache->background_work_lock);
> >  }
> >
> >  /*----------------------------------------------------------------*/
> > @@ -2507,9 +2508,7 @@ static int cache_create(struct cache_args *ca, struct cache **result)
> >
> >         spin_lock_init(&cache->lock);
> >         bio_list_init(&cache->deferred_bios);
> > -       atomic_set(&cache->nr_allocated_migrations, 0);
> >         atomic_set(&cache->nr_io_migrations, 0);
> > -       init_waitqueue_head(&cache->migration_wait);
> >
> >         r = -ENOMEM;
> >         atomic_set(&cache->nr_dirty, 0);
> > @@ -2592,8 +2591,10 @@ static int cache_create(struct cache_args *ca, struct cache **result)
> >                      issue_op, cache, cache->wq);
> >         dm_iot_init(&cache->tracker);
> >
> > -       init_rwsem(&cache->background_work_lock);
> > -       prevent_background_work(cache);
> > +       init_waitqueue_head(&cache->background_work_wait);
> > +       spin_lock_init(&cache->background_work_lock);
> > +       cache->background_work_allowed = false;
> > +       cache->background_work_nr = 0;
> >
> >         *result = cache;
> >         return 0;
> > --
> > 2.53.0
> >
>