Re: [PATCH] ext4: don't report delalloc data as a hole on indirect-mapped inodes

[Jan Kara]

On Wed 26-08-26 14:24:46, Daejun Park wrote:
> When a plain lookup finds no block, ext4_ind_map_blocks() sizes the hole
> it reports by counting the empty subtrees under 'partial' in the on-disk
> indirect tree.  That count knows nothing about delayed allocation, so a
> range holding delalloc data that has not been written back yet is
> reported as a plain hole.  The extent-mapped path does not have this
> problem: ext4_ext_determine_insert_hole() trims the hole it found at the
> first delayed extent before returning it.
> 
> ext4_map_blocks() consults the extent status tree before it calls into
> the mapping layer, so a query that starts exactly on the delayed block
> still finds it.  A query that starts earlier does not, because the hole
> reported for the earlier block already spans the delayed one.  On a
> 4k-block filesystem, lseek(SEEK_DATA) from offset 0 on a file whose only
> data block is at logical block N and is still dirty returns:
> 
>   N = 0..11      direct blocks                 N << 12
>   N = 12         first single-indirect block   N << 12
>   N = 13..1035   inside that indirect block    -1 ENXIO
>   N = 1036       first double-indirect block   N << 12
> 
> Blocks 12 and 1036 survive because the query lands on the delayed block
> itself.  Blocks 13..1035 are swallowed by the 1024-block hole reported
> for block 12.  fiemap loses the same data for the same reason.
> 
> An ext3 filesystem mounted as ext4 hits this during ordinary use: ext3
> inodes stay indirect mapped, while mount -t ext4 turns on delayed
> allocation even though mounting ext3 with -o delalloc is explicitly
> rejected.  install(1) from coreutils uses SEEK_DATA to locate data in
> its source file, so installing a sparse file that has not been written
> back silently produces a destination of the right size holding none of
> the data:
> 
>   truncate -s 1G img && mkfs.ext3 -F img && mount -t ext4 img mnt
>   echo | dd of=mnt/src bs=1 count=1 seek=64K
>   install mnt/src mnt/dst
>   cmp mnt/src mnt/dst        # differ: char 65537
> 
> Trim the hole at the first delayed extent, the way the extent-mapped
> path does.  ext4_es_find_extent_range() does not clip the extent it
> returns to the queried range, so skip one that begins at or before
> m_lblk and clamp the result against the hole already found.
> 
> With this the sweep above returns N << 12 for every N, while the
> nodelalloc and extent-mapped controls are unchanged.  generic/225,
> generic/285, generic/286, generic/436, generic/448 and generic/490 pass
> on ext4 made both with and without the extent feature.
> 
> Fixes: facab4d9711e ("ext4: return hole from ext4_map_blocks()")
> Reported-by: Alexander Monakov <amonakov@xxxxxxxxx>
> Closes: https://lore.kernel.org/linux-ext4/594c17d9-c00f-e485-96fb-cedf27ce3aa3@xxxxxxxxx/
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Daejun Park <daejun7.park@xxxxxxxxxxx>

Thanks for report and the fix! So I think a better fix here is to lift most
of the logic of ext4_ext_determine_insert_hole() into a helper function
like:

ext4_lblk_t ext4_determine_insert_hole(struct inode *inode,
				ext4_lblk_t hole_start, ext4_lblk_t hole_len)

and then use it from both ext4_ext_map_block() and ext4_ind_map_blocks().
That avoids the duplication and also have all the comments regarding
various special cases (like delalloc extent covering map->m_lblk) in one
place.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR