From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755187AbaIIJzM (ORCPT ); Tue, 9 Sep 2014 05:55:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:1599 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751627AbaIIJzK (ORCPT ); Tue, 9 Sep 2014 05:55:10 -0400 Date: Tue, 9 Sep 2014 10:54:59 +0100 From: Joe Thornber To: Anssi Hannula Cc: Alasdair G Kergon , Joe Thornber , dm-devel@redhat.com, linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH] dm cache: fix race causing dirty blocks to be marked as clean Message-ID: <20140909095457.GA2967@debian> Mail-Followup-To: Anssi Hannula , Alasdair G Kergon , Joe Thornber , dm-devel@redhat.com, linux-kernel@vger.kernel.org, stable@vger.kernel.org References: <53F10F80.6080204@iki.fi> <1409875888-1255-1-git-send-email-anssi.hannula@iki.fi> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1409875888-1255-1-git-send-email-anssi.hannula@iki.fi> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Cc: Joe Thornber > 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 >