Re: [linux-lvm] resend patch - bcache may mistakenly write data to another disk when writes error

[Joe Thornber <thornber@redhat.com>]

On Tue, Oct 29, 2019 at 09:46:56AM +0000, Heming Zhao wrote:
> Add another comment.
> 
> The key of commit 2938b4dcc & 6b0d969b is function _invalidate_fd().
> But from my analysis, it looks your codes do not fix this issue.
> 
> _invalidate_fd calls bcache_invalidate_fd & bcache_abort_fd.
> bcache_invalidate_fd work as before, only return true or false to indicate there is or isn't fd in cache->errored.
> Then bcache_abort_fd calls _abort_v, _abort_v calls _unlink_block & _free_block.
> These two functions only delist block from currently cache->errored & cache->free list.
> The data still in radix tree with flags BF_DIRTY.

In _abort_v():

1402│        // We can't remove the block from the radix tree yet because
   1│        // we're in the middle of an iteration.

and then after the iteration:

1416│        radix_tree_remove_prefix(cache->rtree, k.bytes, k.bytes + sizeof(k.parts.fd));

I've added a unit test to demonstrate it does indeed work:

 840│static void test_abort_forces_reread(void *context)
   1│{        
   2│        struct fixture *f = context;
   3│        struct mock_engine *me = f->me;
   4│        struct bcache *cache = f->cache;
   5│        struct block *b;
   6│        int fd = 17;
   7│ 
   8│        _expect_read(me, fd, 0);
   9│        _expect(me, E_WAIT);
  10│        T_ASSERT(bcache_get(cache, fd, 0, GF_DIRTY, &b));
  11│        bcache_put(b);
  12│ 
  13│        bcache_abort_fd(cache, fd);
  14│        T_ASSERT(bcache_flush(cache));
  15│ 
  16│        // Check the block is re-read
  17│        _expect_read(me, fd, 0);
  18│        _expect(me, E_WAIT);
  19│        T_ASSERT(bcache_get(cache, fd, 0, 0, &b));
  20│        bcache_put(b);
  21│}

- Joe