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 <[email protected]>
>
> Aside from the above, feel free to add
>
> Reviewed-by: Matthew Sakai <[email protected]>
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;
