Re: [PATCH] btrfs: populate extent_map::generation when reading from disk

[Filipe Manana <fdmanana@kernel.org>]

On Sat, Feb 05, 2022 at 04:55:47PM +0800, Qu Wenruo wrote:
> [WEIRD BEHAVIOR]
> 
> When btrfs_get_extent() tries to get some file extent from disk, it
> never populates extent_map::generation , leaving the value to be 0.
> 
> On the other hand, for extent map generated by IO, it will get its
> generation properly set at finish_ordered_io()
> 
>  finish_ordered_io()
>  |- unpin_extent_cache(gen = trans->transid)
>     |- em->generation = gen;
> 
> [REGRESSION?]
> I have no idea when such behavior is introduced, but at least in v5.15
> this incorrect behavior is already there.

The extent map generation is basically only used by the fsync code, but
as it deals only with modified extents, it always sees non-zero generation.

> 
> [AFFECT]
> Not really sure if there is any behavior really get affected.

affect -> effect

No, I don't think it affects anything.

> 
> Sure there are locations like extent map merging, but there is no value
> smaller than 0 for u64, thus it won't really cause a difference.
> 
> For autodefrag, although it's checking em->generation to determine if we
> need to defrag a range, but that @new_than value is always from IO, thus

This is confusing.
You mean the minimum generation threshold for autodefrag. Referring to
a function parameter (and it's named "newer_than") out of context, is
hard to follow.


> all those extent maps with 0 generation will just be skipped, and that's
> the expected behavior anyway.
> 
> For manual defrag, @newer_than is 0, and our check is to skip generation
> smaller than @newer_than, thus it still makes no difference.

Same here, saying the minimum generation threshold for defrag is more
informative than referring to the name of a function parameter. A function
that is not even touched by the patch makes it hard to understand.

> 
> [FIX]
> To make things less weird, let us populate extent_map::generation in
> btrfs_extent_item_to_extent_map().

Looks good.
Though I don't think this fixes anything. As I pointed out in the other
thread, the extent map generation is basically only I used by fsync, which
doesn't use extent maps that are not the in the list of modified extents
(and those always have a generation > 0).

Thnaks.

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/file-item.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index 90c5c38836ab..9a3de652ada8 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -1211,6 +1211,7 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
>  	extent_start = key.offset;
>  	extent_end = btrfs_file_extent_end(path);
>  	em->ram_bytes = btrfs_file_extent_ram_bytes(leaf, fi);
> +	em->generation = btrfs_file_extent_generation(leaf, fi);
>  	if (type == BTRFS_FILE_EXTENT_REG ||
>  	    type == BTRFS_FILE_EXTENT_PREALLOC) {
>  		em->start = extent_start;
> -- 
> 2.35.0
>