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

[Heming Zhao <heming.zhao@suse.com>]

You are right. I forgot the radix_tree_remove_prefix().
The radix_tree_remove_prefix() is called both bcache_invalidate_fd & bcache_abort_fd.
So the job will not work as expected in bcache_abort_fd, because the node is already removed.
Please correct me if my thinking is wrong.

On 10/29/19 7:05 PM, Joe Thornber wrote:
> 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
>