On Wed, Aug 26, 2026 at 3:22 AM Benjamin Marzinski <[email protected]> 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 <[email protected]>
> ---
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 <[email protected]>
> 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
>
