[02/19] btrfs: rename DISCARD opt to DISCARD_SYNC
diff mbox series

Message ID e2c7ca7b48bc3a5a219329f7d086ab1cfd7330a3.1570479299.git.dennis@kernel.org
State New
Headers show
Series
  • btrfs: async discard support
Related show

Commit Message

Dennis Zhou Oct. 7, 2019, 8:17 p.m. UTC
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(-)

Comments

Josef Bacik Oct. 7, 2019, 8:27 p.m. UTC | #1
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
Johannes Thumshirn Oct. 8, 2019, 11:12 a.m. UTC | #2
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Nikolay Borisov Oct. 11, 2019, 9:19 a.m. UTC | #3
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>

Patch
diff mbox series

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");