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.
