Re: [PATCH 2/2] ext4: track zeroed out blocks of unwritten extents for fast commit

[Jan Kara]

On Wed 07-10-26 09:41:21, Daejun Park via B4 Relay wrote:
> From: Daejun Park <daejun7.park@xxxxxxxxxxx>
> 
> When ext4_ext_convert_to_initialized() converts part of an unwritten
> extent, it may zero out the blocks before and after that range (a side
> only if it fits together with the range in s_extent_max_zeroout_kb,
> 32 KiB by default, and the extent lies within i_size) and convert them
> to written together with it, instead of splitting them off.
> ext4_split_extent() zeroes out and converts the whole extent when a
> split fails with -ENOSPC, -EDQUOT or -ENOMEM. In both cases
> ext4_map_blocks() reports only the mapped range to fast commit, so the
> other converted blocks are not tracked.
> 
> A later write to one of those blocks overwrites a written block in
> place. That changes no mapping and is not tracked either, so the next
> fsync is a fast commit that logs the inode but no range for the block.
> After a crash, replay starts from the last full commit, where the block
> is still part of an unwritten extent, and the fsynced data reads back
> as zeroes. Fast commit tracks one [min, max] range per inode, so this
> shows only when no other block beyond the zeroed ones was tracked in
> the same commit, which makes it easy to miss.
> 
> The first conversion runs when writes allocate through ext4_map_blocks()
> with EXT4_GET_BLOCKS_CREATE, for example with -o nodelalloc,
> -o dioread_lock or DAX. On a 4 KiB block file system made with
> -O fast_commit and mounted with -o nodelalloc,commit=60:
> 
>   fallocate -l 32k f; sync      # blocks 0-7 unwritten
>   write block 0; fsync f        # zeroes out 1-7, tracks only 0
>   write block 2; fsync f        # in place, nothing tracked
>   crash (kill the VM), mount    # fast commit replay
>   block 2 reads back as zeroes
> 
> Track the zeroed out blocks in ext4_zeroout_es(), and the whole extent
> in ext4_split_extent_zeroout() when it converts one to written. Both run
> under i_data_sem, as fast commit tracking already does when
> ext4_ext_dirty() changes an extent kept in the inode (through
> ext4_mark_inode_dirty()).
> 
> Fixes: aa75f4d3daae ("ext4: main fast-commit commit path")
> Cc: stable@xxxxxxxxxxxxxxx # 7.0.x
> Signed-off-by: Daejun Park <daejun7.park@xxxxxxxxxxx>

Thanks for catching this. Some comments below.

> -static void ext4_zeroout_es(struct inode *inode, struct ext4_extent *ex)
> +static void ext4_zeroout_es(handle_t *handle, struct inode *inode,
> +			    struct ext4_extent *ex)
>  {
>  	ext4_lblk_t  ee_block;
>  	ext4_fsblk_t ee_pblock;
> @@ -3164,6 +3165,12 @@ static void ext4_zeroout_es(struct inode *inode, struct ext4_extent *ex)
>  
>  	ext4_es_insert_extent(inode, ee_block, ee_len, ee_pblock,
>  			      EXTENT_STATUS_WRITTEN, false);
> +	/*
> +	 * The zeroed out blocks became written together with the mapped
> +	 * range, but ext4_map_blocks() reports only the mapped range to fast
> +	 * commit.
> +	 */
> +	ext4_fc_track_range(handle, inode, ee_block, ee_block + ee_len - 1);
>  }

It looks a bit odd to hide ext4_fc_track_range() inside a function for
extent status tree tracking. Also see below.

> @@ -3400,6 +3407,14 @@ static int ext4_split_extent_zeroout(handle_t *handle, struct inode *inode,
>  	if (err)
>  		return err;
>  
> +	/*
> +	 * The whole extent is written now, not only the range in @map that
> +	 * ext4_map_blocks() reports to fast commit.
> +	 */
> +	if (flags & EXT4_GET_BLOCKS_CONVERT)
> +		ext4_fc_track_range(handle, inode, ee_block,
> +				    ee_block + ee_len - 1);
> +
>  	return 0;
>  }

Also putting this into ext4_split_extent_zeroout() looks a bit too easy to
miss. In fact I think placing ext4_fc_track_range() into
ext4_issue_zeroout() would make sense because that is where the writing of
"data" really happens. This will fix the use in
ext4_split_extent_zeroout() as well as ext4_ext_convert_to_initialized().
And it will also fix the same class of problem which I think we have in
ext4_alloc_file_blocks()...

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