Re: [PATCH 2/2] Btrfs: fix fiemap extent SHARED flag error with range clone.

[Nikolay Borisov <nborisov@suse.com>]

On  7.03.2018 13:18, Qu Wenruo wrote:
> 
> 
> On 2018年03月07日 19:01, robbieko wrote:
>> Qu Wenruo 於 2018-03-07 18:42 寫到:
>>> On 2018年03月07日 18:33, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2018年03月07日 16:20, robbieko wrote:
>>>>> From: Robbie Ko <robbieko@synology.com>
>>>>>
>>>>> [BUG]
>>>>> Range clone can cause fiemap to return error result.
>>>>> Like:
>>>>>  # mount /dev/vdb5 /mnt/btrfs
>>>>>  # dd if=/dev/zero bs=16K count=2 oflag=dsync of=/mnt/btrfs/file
>>>>>  # xfs_io -c "fiemap -v" /mnt/btrfs/file
>>>>>  /mnt/btrfs/file:
>>>>>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>>>>>    0: [0..63]:         4241424..4241487    64   0x1
>>>>>
>>>>>  # cloner -s $((16*1024)) /mnt/btrfs/file /mnt/btrfs/file_clone
>>>>>  # xfs_io -c "fiemap -v" /mnt/btrfs/file
>>>>>  /mnt/btrfs/file:
>>>>>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>>>>>    0: [0..63]:         4241424..4241487    64   0x1
>>>>>  If we clone second file extent, we will get error FLAGS,
>>>>>  SHARED bit is not set.
>>>>>
>>>>> [REASON]
>>>>> Btrfs only checks if the first extent in extent map is shared,
>>>>> but extent may merge.
>>>>>
>>>>> [FIX]
>>>>> Here we will check each extent with extent map range,
>>>>> if one of them is shared, extent map is shared.
>>>>>
>>>>> [PATCH RESULT]
>>>>>  # xfs_io -c "fiemap -v" /mnt/btrfs/file
>>>>>  /mnt/btrfs/file:
>>>>>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>>>>>    0: [0..63]:         4241424..4241487    64 0x2001
>>>>>
>>>>> Signed-off-by: Robbie Ko <robbieko@synology.com>
>>>>> ---
>>>>>  fs/btrfs/extent_io.c | 146
>>>>> +++++++++++++++++++++++++++++++++++++++++++++------
>>>>>  1 file changed, 131 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>>>> index 066b6df..5c6dca9 100644
>>>>> --- a/fs/btrfs/extent_io.c
>>>>> +++ b/fs/btrfs/extent_io.c
>>>>> @@ -4394,8 +4394,8 @@ static int emit_fiemap_extent(struct
>>>>> fiemap_extent_info *fieinfo,
>>>>>       */
>>>>>      if (cache->offset + cache->len  == offset &&
>>>>>          cache->phys + cache->len == phys  &&
>>>>> -        (cache->flags & ~FIEMAP_EXTENT_LAST) ==
>>>>> -            (flags & ~FIEMAP_EXTENT_LAST)) {
>>>>> +        (cache->flags & ~(FIEMAP_EXTENT_LAST|FIEMAP_EXTENT_SHARED)) ==
>>>>> +            (flags & ~(FIEMAP_EXTENT_LAST|FIEMAP_EXTENT_SHARED))) {
>>>>>          cache->len += len;
>>>>>          cache->flags |= flags;
>>>>>          goto try_submit_last;
>>>>> @@ -4450,6 +4450,134 @@ static int emit_last_fiemap_cache(struct
>>>>> btrfs_fs_info *fs_info,
>>>>>      return ret;
>>>>>  }
>>>>>
>>>>> +/*
>>>>> + * Helper to check the file range is shared.
>>>>> + *
>>>>> + * Fiemap extent will be combined with many extents, so we need to
>>>>> examine
>>>>> + * each extent, and if shared, the results are shared.
>>>>> + *
>>>>> + * Return: 0 if file range is not shared, 1 if it is shared, < 0 on
>>>>> error.
>>>>> + */
>>>>> +static int extent_map_check_shared(struct inode *inode, u64 start,
>>>>> u64 end)
>>>>> +{
>>>>> +    struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>>>>> +    struct btrfs_root *root = BTRFS_I(inode)->root;
>>>>> +    int ret = 0;
>>>>> +    struct extent_buffer *leaf;
>>>>> +    struct btrfs_path *path;
>>>>> +    struct btrfs_file_extent_item *fi;
>>>>> +    struct btrfs_key found_key;
>>>>> +    int check_prev = 1;
>>>>> +    int extent_type;
>>>>> +    int shared = 0;
>>>>> +    u64 cur_offset;
>>>>> +    u64 extent_end;
>>>>> +    u64 ino = btrfs_ino(BTRFS_I(inode));
>>>>> +    u64 disk_bytenr;
>>>>> +
>>>>> +    path = btrfs_alloc_path();
>>>>> +    if (!path) {
>>>>> +        return -ENOMEM;
>>>>> +    }
>>>>> +
>>>>> +    cur_offset = start;
>>>>> +    while (1) {
>>>>> +        ret = btrfs_lookup_file_extent(NULL, root, path, ino,
>>>>> +                           cur_offset, 0);
>>>>> +        if (ret < 0)
>>>>> +            goto error;
>>>>> +        if (ret > 0 && path->slots[0] > 0 && check_prev) {
>>>>> +            leaf = path->nodes[0];
>>>>> +            btrfs_item_key_to_cpu(leaf, &found_key,
>>>>> +                          path->slots[0] - 1);
>>>>> +            if (found_key.objectid == ino &&
>>>>> +                found_key.type == BTRFS_EXTENT_DATA_KEY)
>>>>> +                path->slots[0]--;
>>>>> +        }
>>>>> +        check_prev = 0;
>>>>> +next_slot:
>>>>> +        leaf = path->nodes[0];
>>>>> +        if (path->slots[0] >= btrfs_header_nritems(leaf)) {
>>>>> +            ret = btrfs_next_leaf(root, path);
>>>>> +            if (ret < 0)
>>>>> +                goto error;
>>>>> +            if (ret > 0)
>>>>> +                break;
>>>>> +            leaf = path->nodes[0];
>>>>> +        }
>>>>> +
>>>>> +        disk_bytenr = 0;
>>>>> +        btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
>>>>> +
>>>>> +        if (found_key.objectid > ino)
>>>>> +            break;
>>>>> +        if (WARN_ON_ONCE(found_key.objectid < ino) ||
>>>>> +            found_key.type < BTRFS_EXTENT_DATA_KEY) {
>>>>> +            path->slots[0]++;
>>>>> +            goto next_slot;
>>>>> +        }
>>>>> +        if (found_key.type > BTRFS_EXTENT_DATA_KEY ||
>>>>> +            found_key.offset > end)
>>>>> +            break;
>>>>> +
>>>>> +        fi = btrfs_item_ptr(leaf, path->slots[0],
>>>>> +                    struct btrfs_file_extent_item);
>>>>> +        extent_type = btrfs_file_extent_type(leaf, fi);
>>>>> +
>>>>> +        if (extent_type == BTRFS_FILE_EXTENT_REG ||
>>>>> +            extent_type == BTRFS_FILE_EXTENT_PREALLOC) {
>>>>> +            disk_bytenr = btrfs_file_extent_disk_bytenr(leaf, fi);
>>>>> +            extent_end = found_key.offset +
>>>>> +                btrfs_file_extent_num_bytes(leaf, fi);
>>>>> +            if (extent_end <= start) {
>>>>> +                path->slots[0]++;
>>>>> +                goto next_slot;
>>>>> +            }
>>>>> +            if (disk_bytenr == 0) {
>>>>> +                path->slots[0]++;
>>>>> +                goto next_slot;
>>>>> +            }
>>>>> +
>>>>> +            btrfs_release_path(path);
>>>>> +
>>>>> +            /*
>>>>> +             * As btrfs supports shared space, this information
>>>>> +             * can be exported to userspace tools via
>>>>> +             * flag FIEMAP_EXTENT_SHARED.  If fi_extents_max == 0
>>>>> +             * then we're just getting a count and we can skip the
>>>>> +             * lookup stuff.
>>>>> +             */
>>>>> +            ret = btrfs_check_shared(root,
>>>>> +                         ino, disk_bytenr);
>>>>> +            if (ret < 0)
>>>>> +                goto error;
>>>>> +            if (ret)
>>>>> +                shared = 1;
>>>>> +            ret = 0;
>>>>> +            if (shared) {
>>>>> +                break;
>>>>> +            }
>>>>> +        } else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
>>>>> +            extent_end = found_key.offset +
>>>>> +                btrfs_file_extent_inline_len(leaf,
>>>>> +                             path->slots[0], fi);
>>>>> +            extent_end = ALIGN(extent_end, fs_info->sectorsize);
>>>>> +            path->slots[0]++;
>>>>> +            goto next_slot;
>>>>> +        } else {
>>>>> +            BUG_ON(1);
>>>>> +        }
>>>>> +        cur_offset = extent_end;
>>>>> +        if (cur_offset > end)
>>>>> +            break;
>>>>> +    }
>>>>> +
>>>>> +    ret = 0;
>>>>> +error:
>>>>> +    btrfs_free_path(path);
>>>>> +    return !ret ? shared : ret;
>>>>> +}
>>>>> +
>>>>>  int extent_fiemap(struct inode *inode, struct fiemap_extent_info
>>>>> *fieinfo,
>>>>>          __u64 start, __u64 len, get_extent_t *get_extent)
>>>>>  {
>>>>> @@ -4587,19 +4715,7 @@ int extent_fiemap(struct inode *inode, struct
>>>>> fiemap_extent_info *fieinfo,
>>>>>              flags |= (FIEMAP_EXTENT_DELALLOC |
>>>>>                    FIEMAP_EXTENT_UNKNOWN);
>>>>>          } else if (fieinfo->fi_extents_max) {
>>>>> -            u64 bytenr = em->block_start -
>>>>> -                (em->start - em->orig_start);
>>>>> -
>>>>> -            /*
>>>>> -             * As btrfs supports shared space, this information
>>>>> -             * can be exported to userspace tools via
>>>>> -             * flag FIEMAP_EXTENT_SHARED.  If fi_extents_max == 0
>>>>> -             * then we're just getting a count and we can skip the
>>>>> -             * lookup stuff.
>>>>> -             */
>>>>> -            ret = btrfs_check_shared(root,
>>>>> -                         btrfs_ino(BTRFS_I(inode)),
>>>>> -                         bytenr);
>>>>
>>>> Since we're going to use the whole extent to determine the SHARED flags,
>>>> what about just passing the extent bytenr into btrfs_check_shared?
>>>>
>>>> In that case I think we could get correct shared flag without using
>>>> another helper function.
>>>> (IIRC it's em->block_start)
>>>
>>> Well, it's not em->block_start, but a little more complex.
>>> (For compressed one it's em->block_start, but for plaintext one, it's
>>> more complex)
>>>
>>> em->block_start = file_extent_disk_bytenr +
>>> file_extent_file_extent_offset
>>>
>>> We need extra calculation to determine the real extent bytenr
>>> (file_extent_disk_bytenr).
>>>
>>> IIRC the correct calculation would be:
>>> file_extent_disk_bytenr = em->block_start - em->start + em->orig_start.
>>>
>>> (Who thought out such anti-human calculation for extent map?!)
>>>
>>> Thanks,
>>> Qu
>>
>> dd if=/dev/zero bs=16K count=4 oflag=dsync of=file
>>
>> btrfs-debugfs -f file
>> (276 0): ram 16384 disk 2171609088 disk_size 16384
>> (276 16384): ram 16384 disk 2171625472 disk_size 16384
>> (276 32768): ram 16384 disk 2171641856 disk_size 16384
>> (276 49152): ram 16384 disk 2171658240 disk_size 16384
>> file: file extents 4 disk size 65536 logical size 65536 ratio 1.00
>>
>> xfs_io -c "fiemap -v" file
>> file:
>>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>>    0: [0..127]:        4241424..4241551   128   0x1
>>
>> My point is that we have only one extent_map,
>> but in fact it can correspond to many extents
>> and each one has the potential to be cloned
>> so we need to examine each one individually.
> 
> Right, I missed this point.
> 
> SHARED flag is determined after extent map merge, so here we can't rely
> on em here.

Shouldn't extent maps correspond to 1:1 disk-state. I.e. they are just
the memory cache of the extent state. So if we merge them, shouldn't we
also merge the on-disk extents as well ?

> 
> So the current helper function looks valid, unless we have method to
> access the original extent map.
> 
> Thanks,
> Qu
> 
>>
>>
>>>>
>>>> Thanks,
>>>> Qu
>>>>
>>>>> +            ret = extent_map_check_shared(inode, em->start,
>>>>> extent_map_end(em) - 1);
>>>>>              if (ret < 0)
>>>>>                  goto out_free;
>>>>>              if (ret)
>>>>> -- 
>>>>> 1.9.1
>>>>>
>>>>> -- 
>>>>> 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
>>>>>
>>>>
>>
>