Re: [PATCH 1/3] btrfs: Don't discard unwritten extents

[Nikolay Borisov <nborisov@suse.com>]

On 27.11.19 г. 18:15 ч., David Sterba wrote:
> On Wed, Nov 27, 2019 at 05:06:00PM +0100, David Sterba wrote:
>> On Thu, Nov 21, 2019 at 02:03:29PM +0200, Nikolay Borisov wrote:
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -4169,8 +4169,6 @@ static int __btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
>>>  		if (ret)
>>>  			goto out;
>>>  	} else {
>>> -		if (btrfs_test_opt(fs_info, DISCARD))
>>> -			ret = btrfs_discard_extent(fs_info, start, len, NULL);
>>>  		btrfs_add_free_space(cache, start, len);
>>>  		btrfs_free_reserved_bytes(cache, len, delalloc);
>>>  		trace_btrfs_reserved_extent_free(fs_info, start, len);
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index 0ac0f5b33003..5d80fe030e79 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -3250,10 +3250,15 @@ static int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent)
>>>  		if ((ret || !logical_len) &&
>>>  		    clear_reserved_extent &&
>>>  		    !test_bit(BTRFS_ORDERED_NOCOW, &ordered_extent->flags) &&
>>> -		    !test_bit(BTRFS_ORDERED_PREALLOC, &ordered_extent->flags))
>>> +		    !test_bit(BTRFS_ORDERED_PREALLOC, &ordered_extent->flags)) {
>>>  			btrfs_free_reserved_extent(fs_info,
>>>  						   ordered_extent->start,
>>>  						   ordered_extent->disk_len, 1);
>>> +			if (ret && btrfs_test_opt(fs_info, DISCARD))
>>> +				btrfs_discard_extent(fs_info,
>>> +				ordered_extent->start, ordered_extent->disk_len,
>>> +				NULL);
>>
>> This brings back vague memories of misplaced discard (in
>> finish_ordered_io), that was quite hard to catch. I can't find the fix
>> though. Filipe, is it the same issue?
> 
> Closest to what I thought are dcc82f4783ad9 "Btrfs: fix log tree
> corruption when fs mounted with -o discard" or 678886bdc6378 "Btrfs: fix
> fs corruption on transaction abort if device supports discard".
> 

Pinned extents are discarded when btrfs_finish_extent_commit is called
from transaction commit. If you look closer you'd see
btrfs_Free_Reserved_extent here means we call into
__btrfs_Free_Reserved_extent with pin equal to false, so the original
code would have gone to the else {} clause where the discard was. By
predicating on (ret) I only handle the case where an ordered extent
(which only applies to data) is potentially partially written.