Re: [PATCH V2] Btrfs: really fix trim 0 bytes after a device delete

[Martin Steigerwald <Martin@lichtvoll.de>]

Happy new year!

Am Samstag, 3. Januar 2015, 16:30:51 schrieb Lutz Euler:
> Commit 2cac13e41bf5b99ffc426bd28dfd2248df1dfa67, "fix trim 0 bytes after
> a device delete", said:
>   A user reported a bug of btrfs's trim, that is we will trim 0 bytes
>   after a device delete.
> The commit didn't attack the root of the problem so did not fix the bug
> except for a special case.
> 
> For block discard, btrfs_trim_fs directly compares the range passed in
> against the filesystem's objectids. The former is bounded by the sum of
> the sizes of the devices of the filesystem, the latter is a completely
> unrelated set of intervals of 64-bit integers. The bug reported occurred
> as the smallest objectid was larger than the sum of the device sizes.
> The above mentioned commit only fixed the case where the smallest
> objectid is nonzero and the largest objectid less than the sum of the
> device sizes, but it still trims too little if the largest objectid is
> larger than that, and nothing in the reported situation.
> 
> The current mapping between the given range and the objectids is thus
> clearly broken, so, to fix the bug and as a first step towards a
> complete solution, simply ignore the range parameter's start and length
> fields and always trim the whole filesystem. (While this makes it
> impossible to trim a filesystem only partly, due to the broken mapping
> this often didn't work anyway.)
> 
> V2:
> - Rebased onto 3.9. (still applies to and works with 3.19-rc2)
> - Take range->minlen into account.
> 
> Reported-by: Lutz Euler <lutz.euler@freenet.de>
> Signed-off-by: Lutz Euler <lutz.euler@freenet.de>

Is that the patch you send me for testing?

If so, feel free to add:

Reported-and-tested-by: Martin Steigerwald <martin@lichtvoll.de>

If not I can retest with this one.

Thanks,
Martin

> ---
>  fs/btrfs/extent-tree.c |   25 +++++++++++--------------
>  1 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index cfb3cf7..81006c1 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -8824,26 +8824,23 @@ int btrfs_trim_fs(struct btrfs_root *root, struct fstrim_range *range)
>  	u64 start;
>  	u64 end;
>  	u64 trimmed = 0;
> -	u64 total_bytes = btrfs_super_total_bytes(fs_info->super_copy);
>  	int ret = 0;
>  
>  	/*
> -	 * try to trim all FS space, our block group may start from non-zero.
> +	 * The range passed in is a subinterval of the interval from 0
> +	 * to the sum of the sizes of the devices of the filesystem.
> +	 * The objectid's used in the filesystem can span any set of
> +	 * subintervals of the interval from 0 to (u64)-1. As there is
> +	 * neither a simple nor an agreed upon mapping between these
> +	 * two ranges we ignore the range parameter's start and len
> +	 * fields and always trim the whole filesystem (that is, only
> +	 * the free space in allocated chunks).
>  	 */
> -	if (range->len == total_bytes)
> -		cache = btrfs_lookup_first_block_group(fs_info, range->start);
> -	else
> -		cache = btrfs_lookup_block_group(fs_info, range->start);
> +	cache = btrfs_lookup_first_block_group(fs_info, 0);
>  
>  	while (cache) {
> -		if (cache->key.objectid >= (range->start + range->len)) {
> -			btrfs_put_block_group(cache);
> -			break;
> -		}
> -
> -		start = max(range->start, cache->key.objectid);
> -		end = min(range->start + range->len,
> -				cache->key.objectid + cache->key.offset);
> +		start = cache->key.objectid;
> +		end = cache->key.objectid + cache->key.offset;
>  
>  		if (end - start >= range->minlen) {
>  			if (!block_group_cache_done(cache)) {
> 

-- 
Martin 'Helios' Steigerwald - http://www.Lichtvoll.de
GPG: 03B0 0D6C 0040 0710 4AFA  B82F 991B EAAC A599 84C7