Re: [PATCH] dm cache: fix race causing dirty blocks to be marked as clean

[Joe Thornber <thornber@redhat.com>]

Ack.  Thanks.

On Fri, Sep 05, 2014 at 03:11:28AM +0300, Anssi Hannula wrote:
> When a writeback or a promotion of a block is completed, the cell of
> that block is removed from the prison, the block is marked as clean, and
> the clear_dirty() callback of the cache policy is called.
> 
> Unfortunately, performing those actions in this order allows an incoming
> new write bio for that block to come in before clearing the dirty status
> is completed and therefore possibly causing one of these two scenarios:
> 
> Scenario A:
> 
> Thread 1                      Thread 2
> cell_defer()                  .
> - cell removed from prison    .
> - detained bios queued        .
> .                             incoming write bio
> .                             remapped to cache
> .                             set_dirty() called,
> .                               but block already dirty
> .                               => it does nothing
> clear_dirty()                 .
> - block marked clean          .
> - policy clear_dirty() called .
> 
> Result: Block is marked clean even though it is actually dirty. No
> writeback will occur.
> 
> Scenario B:
> 
> Thread 1                      Thread 2
> cell_defer()                  .
> - cell removed from prison    .
> - detained bios queued        .
> clear_dirty()                 .
> - block marked clean          .
> .                             incoming write bio
> .                             remapped to cache
> .                             set_dirty() called
> .                             - block marked dirty
> .                             - policy set_dirty() called
> - policy clear_dirty() called .
> 
> Result: Block is properly marked as dirty, but policy thinks it is clean
> and therefore never asks us to writeback it.
> This case is visible in "dmsetup status" dirty block count (which
> normally decreases to 0 on a quiet device).
> 
> Fix these issues by calling clear_dirty() before calling cell_defer().
> Incoming bios for that block will then be detained in the cell and
> released only after clear_dirty() has completed, so the race will not
> occur.
> 
> Found by inspecting the code after noticing spurious dirty counts
> (scenario B).
> 
> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> Cc: Joe Thornber <ejt@redhat.com>
> Cc: stable@vger.kernel.org
> ---
> 
> > Unfortunately it seems there is some other potentially more serious bug
> > still in there...
> 
> After looking through the code that indeed seems to be the case, as
> explained above.
> 
> Unless I'm missing something?
> 
> I can't say with 100% certainty if this fixes the spurious counts I saw
> since those took quite a long time (1-2 weeks?) to appear and the load
> of that system is somewhat irregular.
> 
> 
>  drivers/md/dm-cache-target.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
> index 1af40ee209e2..7130505c2425 100644
> --- a/drivers/md/dm-cache-target.c
> +++ b/drivers/md/dm-cache-target.c
> @@ -895,8 +895,8 @@ static void migration_success_pre_commit(struct dm_cache_migration *mg)
>  	struct cache *cache = mg->cache;
>  
>  	if (mg->writeback) {
> -		cell_defer(cache, mg->old_ocell, false);
>  		clear_dirty(cache, mg->old_oblock, mg->cblock);
> +		cell_defer(cache, mg->old_ocell, false);
>  		cleanup_migration(mg);
>  		return;
>  
> @@ -951,13 +951,13 @@ static void migration_success_post_commit(struct dm_cache_migration *mg)
>  		}
>  
>  	} else {
> +		clear_dirty(cache, mg->new_oblock, mg->cblock);
>  		if (mg->requeue_holder)
>  			cell_defer(cache, mg->new_ocell, true);
>  		else {
>  			bio_endio(mg->new_ocell->holder, 0);
>  			cell_defer(cache, mg->new_ocell, false);
>  		}
> -		clear_dirty(cache, mg->new_oblock, mg->cblock);
>  		cleanup_migration(mg);
>  	}
>  }
> -- 
> 1.8.4.5
>