lkml.org 
[lkml]   [2014]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] dm cache: fix race causing dirty blocks to be marked as clean
    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
    >


    \
     
     \ /
      Last update: 2014-09-09 12:21    [W:2.271 / U:0.672 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site