Jan,

On Tue, Jul 25, 2017 at 2:45 PM, Jan Kara [off-list ref] wrote:

On Thu 29-06-17 06:54:20, Christoph Hellwig wrote:

quoted

@@ -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.

quoted

+                             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