On 7.10.19 г. 23:17 ч., Dennis Zhou wrote:
> This series introduces async discard which will use the flag
> DISCARD_ASYNC, so rename the original flag to DISCARD_SYNC as it is
> synchronously done in transaction commit.
> 
> Signed-off-by: Dennis Zhou <den...@kernel.org>
> ---
>  fs/btrfs/block-group.c | 2 +-
>  fs/btrfs/ctree.h       | 2 +-
>  fs/btrfs/extent-tree.c | 4 ++--
>  fs/btrfs/super.c       | 8 ++++----
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index bf7e3f23bba7..afe86028246a 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -1365,7 +1365,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info 
> *fs_info)
>               spin_unlock(&space_info->lock);
>  
>               /* DISCARD can flip during remount */
> -             trimming = btrfs_test_opt(fs_info, DISCARD);
> +             trimming = btrfs_test_opt(fs_info, DISCARD_SYNC);
>  
>               /* Implicit trim during transaction commit. */
>               if (trimming)
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 19d669d12ca1..1877586576aa 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1171,7 +1171,7 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const struct 
> btrfs_fs_info *info)
>  #define BTRFS_MOUNT_FLUSHONCOMMIT       (1 << 7)
>  #define BTRFS_MOUNT_SSD_SPREAD               (1 << 8)
>  #define BTRFS_MOUNT_NOSSD            (1 << 9)
> -#define BTRFS_MOUNT_DISCARD          (1 << 10)
> +#define BTRFS_MOUNT_DISCARD_SYNC     (1 << 10)
>  #define BTRFS_MOUNT_FORCE_COMPRESS      (1 << 11)
>  #define BTRFS_MOUNT_SPACE_CACHE              (1 << 12)
>  #define BTRFS_MOUNT_CLEAR_CACHE              (1 << 13)
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 49cb26fa7c63..77a5904756c5 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2903,7 +2903,7 @@ int btrfs_finish_extent_commit(struct 
> btrfs_trans_handle *trans)
>                       break;
>               }
>  
> -             if (btrfs_test_opt(fs_info, DISCARD))
> +             if (btrfs_test_opt(fs_info, DISCARD_SYNC))
>                       ret = btrfs_discard_extent(fs_info, start,
>                                                  end + 1 - start, NULL);
>  
> @@ -4146,7 +4146,7 @@ static int __btrfs_free_reserved_extent(struct 
> btrfs_fs_info *fs_info,
>       if (pin)
>               pin_down_extent(cache, start, len, 1);
>       else {
> -             if (btrfs_test_opt(fs_info, DISCARD))
> +             if (btrfs_test_opt(fs_info, DISCARD_SYNC))
>                       ret = btrfs_discard_extent(fs_info, start, len, NULL);

Is discard even needed in that function? All but one call of
btrfs_free_reserved_extent( it calls __btrfs_Free_Reserved_extent with
pin 0) happen in cleanup code when an extent has just been allocated,
not written to and potentially it has been discarded.

In cow_file_range that function is called only if create_io_em fails or
btrfs_add_ordered_extent fail, both of which happen _before_ any io is
submitted to the newly reserved range hence I think this can be removed.

In submit_compressed_extents the code flow is similar - out_free_reserve
can be called only before btrfs_submit_compressed_write

btrfs_new_extent_direct - again, called in case extent_map creation
fails, before any io happens.

__btrfs_prealloc_file_range - called as a cleanup for a prealloc extent.

btrfs_alloc_tree_block - the metadata extent is allocated but not
written to yet

btrfs_finish_ordered_io - here it seems it can be called for an extent
which could have had some data written to it on disk so discard seems
like necessary. On the other hand the code contradicts the comment:

"We only free the extent in the truncated case if we didn't write out
the extent at all. "

Yet the 'if' does:

 if ((ret || !logical_len) &&
     clear_reserved_extent &&
     !test_bit(BTRFS_ORDERED_NOCOW, &ordered_extent->flags) &&
     !test_bit(BTRFS_ORDERED_PREALLOC, &ordered_extent->flags))

So even if we have ret non 0 (meaning error) we could still free the
extent so long it's not before insert_reserved_file_extent returns
success (the clear_reserved_extent check). This logic is messy, Josef do
you have any idea what should be the correct behavior?

My point is that if btrfs_free_reserved_extent should only be called in
finish_ordered_io for a truncated extent, which hasn't been written at
all then this renders the btrfs_discard_extent call in
btrfs_free_reserved_extent redundant and can be removed, provided my
analysis is correct.

What do you think ?



<snip>

Reply via email to