Re: [PATCH 1/3] btrfs-progs: convert: properly handle reserved ranges while iterating files

[Jeff Mahoney <jeffm@suse.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6364 bytes --]

On 7/27/17 12:38 PM, Jeff Mahoney wrote:
> On 7/26/17 9:35 PM, Qu Wenruo wrote:
>>
>>
>> On 2017年07月26日 04:54, jeffm@suse.com wrote:
>>> From: Jeff Mahoney <jeffm@suse.com>
>>>
>>> Commit 522ef705e38 (btrfs-progs: convert: Introduce function to calculate
>>> the available space) changed how we handle migrating file data so that
>>> we never have btrfs space associated with the reserved ranges.  This
>>> works pretty well and when we iterate over the file blocks, the
>>> associations are redirected to the migrated locations.
>>>
>>> This commit missed the case in block_iterate_proc where we just check
>>> for intersection with a superblock location before looking up a block
>>> group.  intersect_with_sb checks to see if the range intersects with
>>> a stripe containing a superblock but, in fact, we've reserved the
>>> full 0-1MB range at the start of the disk.  So a file block located
>>> at e.g. 160kB will fall in the reserved region but won't be excepted
>>> in block_iterate_block.  We ultimately hit a BUG_ON when we fail
>>> to look up the block group for that location.
>>
>> The description of the problem  is indeed correct.
>>
>>>
>>> This is reproducible using convert-tests/003-ext4-basic.
>>
>> Thanks for pointing this out, I also reproduced it.
>>
>> While it would be nicer if you could upload a special crafted image as
>> indicated test case.
>> IIRC the test passed without problem several versions ago, so there may
>> be some factors preventing the bug from being exposed.
>>
>>>
>>> The fix is to have intersect_with_sb and block_iterate_proc understand
>>> the full size of the reserved ranges.  Since we use the range to
>>> determine the boundary for the block iterator, let's just return the
>>> boundary.  0 isn't a valid boundary and means that we proceed normally
>>> with block group lookup.
>>
>> I'm OK with current fix as it indeed fix the bug and has minimal impact
>> on current code.
>>
>> So feel free to add:
>> Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
>>
>> While I think there is a better way to solve it more completely.
>>
>> As when we run into block_iterate_proc(), we have already created
>> ext2_save/image.
>> So we can use the the image as ext2 <-> btrfs position mapping, just as
>> we have already done in record_file_blocks().
>>
>> That's to say, we don't need too much care about the intersection with
>> reserved range, but just letting record_file_blocks() to handle it will
>> be good enough.
>>
>> What do you think about this idea?
> 
> I think you're right.  It should do the mapping already so we don't need
> to do anything special in block_iterate_proc.  I can test that in a bit.

So the idea works and, in fact, we could really get rid of most of
block_iterate_proc and still get correct results.  This code is an
optimization so that we can quickly assemble larger extents and not have
to grow on-disk extents repeatedly.  The code as it is does the right
thing most of the time but the boundary condition triggers the barrier
for every block in a migrated range and record_file_blocks must do the
growing.  The end result works fine, it's just slower than it needs to
be for that 0-1MB range.  I'm not sure I care enough to invest the time
to fix that.

-Jeff

> -Jeff
> 
>> Thanks,
>> Qu
>>
>>>
>>> Cc: Qu Wenruo <quwenruo.btrfs@gmx.com>
>>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>>> ---
>>>   convert/source-fs.c | 25 +++++++++++--------------
>>>   1 file changed, 11 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/convert/source-fs.c b/convert/source-fs.c
>>> index 80e4e41..09f6995 100644
>>> --- a/convert/source-fs.c
>>> +++ b/convert/source-fs.c
>>> @@ -28,18 +28,16 @@ const struct simple_range btrfs_reserved_ranges[3]
>>> = {
>>>       { BTRFS_SB_MIRROR_OFFSET(2), SZ_64K }
>>>   };
>>>   -static int intersect_with_sb(u64 bytenr, u64 num_bytes)
>>> +static u64 intersect_with_reserved(u64 bytenr, u64 num_bytes)
>>>   {
>>>       int i;
>>> -    u64 offset;
>>>   -    for (i = 0; i < BTRFS_SUPER_MIRROR_MAX; i++) {
>>> -        offset = btrfs_sb_offset(i);
>>> -        offset &= ~((u64)BTRFS_STRIPE_LEN - 1);
>>> +    for (i = 0; i < ARRAY_SIZE(btrfs_reserved_ranges); i++) {
>>> +        const struct simple_range *range = &btrfs_reserved_ranges[i];
>>>   -        if (bytenr < offset + BTRFS_STRIPE_LEN &&
>>> -            bytenr + num_bytes > offset)
>>> -            return 1;
>>> +        if (bytenr < range_end(range) &&
>>> +            bytenr + num_bytes >= range->start)
>>> +            return range_end(range);
>>>       }
>>>       return 0;
>>>   }
>>> @@ -64,14 +62,14 @@ int block_iterate_proc(u64 disk_block, u64
>>> file_block,
>>>                         struct blk_iterate_data *idata)
>>>   {
>>>       int ret = 0;
>>> -    int sb_region;
>>> +    u64 reserved_boundary;
>>>       int do_barrier;
>>>       struct btrfs_root *root = idata->root;
>>>       struct btrfs_block_group_cache *cache;
>>>       u64 bytenr = disk_block * root->sectorsize;
>>>   -    sb_region = intersect_with_sb(bytenr, root->sectorsize);
>>> -    do_barrier = sb_region || disk_block >= idata->boundary;
>>> +    reserved_boundary = intersect_with_reserved(bytenr,
>>> root->sectorsize);
>>> +    do_barrier = reserved_boundary || disk_block >= idata->boundary;
>>>       if ((idata->num_blocks > 0 && do_barrier) ||
>>>           (file_block > idata->first_block + idata->num_blocks) ||
>>>           (disk_block != idata->disk_block + idata->num_blocks)) {
>>> @@ -91,9 +89,8 @@ int block_iterate_proc(u64 disk_block, u64 file_block,
>>>                   goto fail;
>>>           }
>>>   -        if (sb_region) {
>>> -            bytenr += BTRFS_STRIPE_LEN - 1;
>>> -            bytenr &= ~((u64)BTRFS_STRIPE_LEN - 1);
>>> +        if (reserved_boundary) {
>>> +            bytenr = reserved_boundary;
>>>           } else {
>>>               cache = btrfs_lookup_block_group(root->fs_info, bytenr);
>>>               BUG_ON(!cache);
>>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> 


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]