Re: [PATCH 5/5] ext4: Switch to iomap for SEEK_HOLE / SEEK_DATA

[Andreas Gruenbacher <agruenba@redhat.com>]

Jan,

On Tue, Jul 25, 2017 at 2:45 PM, Jan Kara <jack@suse.cz> wrote:
> On Thu 29-06-17 06:54:20, Christoph Hellwig wrote:
>> @@ -3359,6 +3358,7 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>>       unsigned long first_block = offset >> blkbits;
>>       unsigned long last_block = (offset + length - 1) >> blkbits;
>>       struct ext4_map_blocks map;
>> +     bool delalloc = false;
>>       int ret;
>>
>>       if (WARN_ON_ONCE(ext4_has_inline_data(inode)))
>> @@ -3369,6 +3369,27 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>>
>>       if (!(flags & IOMAP_WRITE)) {
>>               ret = ext4_map_blocks(NULL, inode, &map, 0);
>> +             if (!ret) {
>> +                     struct extent_status es = {};
>> +
>> +                     ext4_es_find_delayed_extent_range(inode, map.m_lblk,
>> +                                     map.m_lblk + map.m_len - 1, &es);
>> +                     /* Is delalloc data before next block in extent tree? */
>> +                     if (es.es_len && es.es_lblk < map.m_lblk + map.m_len) {
>> +                             ext4_lblk_t offset = 0;
>> +
>> +                             if (es.es_lblk < map.m_lblk)
>> +                                     offset = map.m_lblk - es.es_lblk;
>> +                             map.m_lblk = es.es_lblk + offset;
>> +                             map.m_pblk = ext4_es_pblock(&es) + offset;
>
> This is wrong for delalloc extent - adding offset to some magic block
> value...

Right.

>> +                             map.m_len = es.es_len - offset;
>> +                             if (ext4_es_status(&es) & EXTENT_STATUS_UNWRITTEN)
>> +                                     map.m_flags |= EXT4_MAP_UNWRITTEN;
>> +                             if (ext4_es_status(&es) & EXTENT_STATUS_DELAYED)
>> +                                     delalloc = true;
>> +                             ret = 1;
>> +                     }
>> +             }
>
> Also the delalloc handling seems to be wrong that if we have file as:
>
> HD
>
> where H is hole, D is delalloc block, and iomap is called at offset 0, we
> should be getting back "hole at 0" back but instead you will return
> "delalloc at 1".
>
> We deal with a similar situation in ext4_da_map_blocks() so it would be
> good if we could reuse that one rather than reimplementing it here. But
> factoring the necessary functionality out isn't that easy.

I've tried to figure out where the issue might be, and I really don't see any.

It may be confusing that ext4_iomap_begin doesn't split up
IOMAP_UNWRITTEN maps into holes and data as ext4_find_unwritten_pgoff
did. This splitting is instead done generically in
iomap_seek_{hole,data} for IOMAP_UNWRITTEN maps using
page_cache_seek_hole_data.

I'll post an improved version of this patch queue shortly.

Thanks,
Andreas