On 7/26/17 9:35 PM, Qu Wenruo wrote:
>
>
> On 2017年07月26日 04:54, jeffm@xxxxxxxx wrote:
>> From: Jeff Mahoney <jeffm@xxxxxxxx>
>>
>> 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@xxxxxxx>
>
> 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.
-Jeff
> Thanks,
> Qu
>
>>
>> Cc: Qu Wenruo <quwenruo.btrfs@xxxxxxx>
>> Signed-off-by: Jeff Mahoney <jeffm@xxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Jeff Mahoney
SUSE Labs
Attachment:
signature.asc
Description: OpenPGP digital signature
