| Message ID | e2c7ca7b48bc3a5a219329f7d086ab1cfd7330a3.1570479299.git.dennis@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: async discard support | expand |
On Mon, Oct 07, 2019 at 04:17:33PM -0400, 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 <dennis@kernel.org> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
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 <dennis@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>
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); btrfs_add_free_space(cache, start, len); btrfs_free_reserved_bytes(cache, len, delalloc); diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 1b151af25772..a02fece949cb 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -695,11 +695,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, info->metadata_ratio); break; case Opt_discard: - btrfs_set_and_info(info, DISCARD, - "turning on discard"); + btrfs_set_and_info(info, DISCARD_SYNC, + "turning on sync discard"); break; case Opt_nodiscard: - btrfs_clear_and_info(info, DISCARD, + btrfs_clear_and_info(info, DISCARD_SYNC, "turning off discard"); break; case Opt_space_cache: @@ -1322,7 +1322,7 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry) seq_puts(seq, ",nologreplay"); if (btrfs_test_opt(info, FLUSHONCOMMIT)) seq_puts(seq, ",flushoncommit"); - if (btrfs_test_opt(info, DISCARD)) + if (btrfs_test_opt(info, DISCARD_SYNC)) seq_puts(seq, ",discard"); if (!(info->sb->s_flags & SB_POSIXACL)) seq_puts(seq, ",noacl");
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 <dennis@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(-)