lkml.org 
[lkml]   [2022]   [Oct]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 6.0 121/862] btrfs: fix missed extent on fsync after dropping extent maps
    Date
    From: Filipe Manana <fdmanana@suse.com>

    commit cef7820d6abf8d61f8e1db411eae3c712f6d72a2 upstream.

    When dropping extent maps for a range, through btrfs_drop_extent_cache(),
    if we find an extent map that starts before our target range and/or ends
    before the target range, and we are not able to allocate extent maps for
    splitting that extent map, then we don't fail and simply remove the entire
    extent map from the inode's extent map tree.

    This is generally fine, because in case anyone needs to access the extent
    map, it can just load it again later from the respective file extent
    item(s) in the subvolume btree. However, if that extent map is new and is
    in the list of modified extents, then a fast fsync will miss the parts of
    the extent that were outside our range (that needed to be split),
    therefore not logging them. Fix that by marking the inode for a full
    fsync. This issue was introduced after removing BUG_ON()s triggered when
    the split extent map allocations failed, done by commit 7014cdb49305ed
    ("Btrfs: btrfs_drop_extent_cache should never fail"), back in 2012, and
    the fast fsync path already existed but was very recent.

    Also, in the case where we could allocate extent maps for the split
    operations but then fail to add a split extent map to the tree, mark the
    inode for a full fsync as well. This is not supposed to ever fail, and we
    assert that, but in case assertions are disabled (CONFIG_BTRFS_ASSERT is
    not set), it's the correct thing to do to make sure a fast fsync will not
    miss a new extent.

    CC: stable@vger.kernel.org # 5.15+
    Reviewed-by: Anand Jain <anand.jain@oracle.com>
    Signed-off-by: Filipe Manana <fdmanana@suse.com>
    Signed-off-by: David Sterba <dsterba@suse.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    ---
    fs/btrfs/file.c | 58 ++++++++++++++++++++++++++++++++++++++++++++------------
    1 file changed, 46 insertions(+), 12 deletions(-)

    --- a/fs/btrfs/file.c
    +++ b/fs/btrfs/file.c
    @@ -523,6 +523,7 @@ void btrfs_drop_extent_cache(struct btrf
    testend = 0;
    }
    while (1) {
    + bool ends_after_range = false;
    int no_splits = 0;

    modified = false;
    @@ -539,10 +540,12 @@ void btrfs_drop_extent_cache(struct btrf
    write_unlock(&em_tree->lock);
    break;
    }
    + if (testend && em->start + em->len > start + len)
    + ends_after_range = true;
    flags = em->flags;
    gen = em->generation;
    if (skip_pinned && test_bit(EXTENT_FLAG_PINNED, &em->flags)) {
    - if (testend && em->start + em->len >= start + len) {
    + if (ends_after_range) {
    free_extent_map(em);
    write_unlock(&em_tree->lock);
    break;
    @@ -592,7 +595,7 @@ void btrfs_drop_extent_cache(struct btrf
    split = split2;
    split2 = NULL;
    }
    - if (testend && em->start + em->len > start + len) {
    + if (ends_after_range) {
    u64 diff = start + len - em->start;

    split->start = start + len;
    @@ -630,14 +633,42 @@ void btrfs_drop_extent_cache(struct btrf
    } else {
    ret = add_extent_mapping(em_tree, split,
    modified);
    - ASSERT(ret == 0); /* Logic error */
    + /* Logic error, shouldn't happen. */
    + ASSERT(ret == 0);
    + if (WARN_ON(ret != 0) && modified)
    + btrfs_set_inode_full_sync(inode);
    }
    free_extent_map(split);
    split = NULL;
    }
    next:
    - if (extent_map_in_tree(em))
    + if (extent_map_in_tree(em)) {
    + /*
    + * If the extent map is still in the tree it means that
    + * either of the following is true:
    + *
    + * 1) It fits entirely in our range (doesn't end beyond
    + * it or starts before it);
    + *
    + * 2) It starts before our range and/or ends after our
    + * range, and we were not able to allocate the extent
    + * maps for split operations, @split and @split2.
    + *
    + * If we are at case 2) then we just remove the entire
    + * extent map - this is fine since if anyone needs it to
    + * access the subranges outside our range, will just
    + * load it again from the subvolume tree's file extent
    + * item. However if the extent map was in the list of
    + * modified extents, then we must mark the inode for a
    + * full fsync, otherwise a fast fsync will miss this
    + * extent if it's new and needs to be logged.
    + */
    + if ((em->start < start || ends_after_range) && modified) {
    + ASSERT(no_splits);
    + btrfs_set_inode_full_sync(inode);
    + }
    remove_extent_mapping(em_tree, em);
    + }
    write_unlock(&em_tree->lock);

    /* once for us */
    @@ -2201,14 +2232,6 @@ int btrfs_sync_file(struct file *file, l
    atomic_inc(&root->log_batch);

    /*
    - * Always check for the full sync flag while holding the inode's lock,
    - * to avoid races with other tasks. The flag must be either set all the
    - * time during logging or always off all the time while logging.
    - */
    - full_sync = test_bit(BTRFS_INODE_NEEDS_FULL_SYNC,
    - &BTRFS_I(inode)->runtime_flags);
    -
    - /*
    * Before we acquired the inode's lock and the mmap lock, someone may
    * have dirtied more pages in the target range. We need to make sure
    * that writeback for any such pages does not start while we are logging
    @@ -2233,6 +2256,17 @@ int btrfs_sync_file(struct file *file, l
    }

    /*
    + * Always check for the full sync flag while holding the inode's lock,
    + * to avoid races with other tasks. The flag must be either set all the
    + * time during logging or always off all the time while logging.
    + * We check the flag here after starting delalloc above, because when
    + * running delalloc the full sync flag may be set if we need to drop
    + * extra extent map ranges due to temporary memory allocation failures.
    + */
    + full_sync = test_bit(BTRFS_INODE_NEEDS_FULL_SYNC,
    + &BTRFS_I(inode)->runtime_flags);
    +
    + /*
    * We have to do this here to avoid the priority inversion of waiting on
    * IO of a lower priority task while holding a transaction open.
    *

    \
     
     \ /
      Last update: 2022-10-19 10:47    [W:4.138 / U:0.560 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site