[PATCH v2] btrfs: zoned: do not enable async discard

[PATCH v2] btrfs: zoned: do not enable async discard

[Naohiro Aota]

The zoned mode need to reset a zone before using it. We rely on btrfs's
original discard functionality (discarding unused block group range) to do
the resetting.

While the commit 63a7cb130718 ("btrfs: auto enable discard=async when
possible") made the discard done in an async manner, a zoned reset do not
need to be async, as it is fast enough.

Even worth, delaying zone rests prevents using those zones again. So, let's
disable async discard on the zoned mode.

Fixes: 63a7cb130718 ("btrfs: auto enable discard=async when possible")
CC: stable@vger.kernel.org # 6.3+
Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 fs/btrfs/disk-io.c | 7 ++++++-
 fs/btrfs/zoned.c   | 5 +++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 7513388b0567..9b9914e5f03d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3438,11 +3438,16 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 	 * For devices supporting discard turn on discard=async automatically,
 	 * unless it's already set or disabled. This could be turned off by
 	 * nodiscard for the same mount.
+	 *
+	 * The zoned mode piggy backs on the discard functionality for
+	 * resetting a zone. There is no reason to delay the zone reset as it is
+	 * fast enough. So, do not enable async discard for zoned mode.
 	 */
 	if (!(btrfs_test_opt(fs_info, DISCARD_SYNC) ||
 	      btrfs_test_opt(fs_info, DISCARD_ASYNC) ||
 	      btrfs_test_opt(fs_info, NODISCARD)) &&
-	    fs_info->fs_devices->discardable) {
+	    fs_info->fs_devices->discardable &&
+	    !btrfs_is_zoned(fs_info)) {
 		btrfs_set_and_info(fs_info, DISCARD_ASYNC,
 				   "auto enabling async discard");
 	}
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 85b8b332add9..65d17306c2d4 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -805,6 +805,11 @@ int btrfs_check_mountopts_zoned(struct btrfs_fs_info *info)
 		return -EINVAL;
 	}
 
+	if (btrfs_test_opt(info, DISCARD_ASYNC)) {
+		btrfs_warn(info, "zoned: disabling async discard as it is not supported");
+		btrfs_clear_opt(info->mount_opt, DISCARD_ASYNC);
+	}
+
 	return 0;
 }
 
-- 
2.41.0

Re: [PATCH v2] btrfs: zoned: do not enable async discard

[Damien Le Moal]

On 6/29/23 17:37, Naohiro Aota wrote:
> The zoned mode need to reset a zone before using it. We rely on btrfs's
> original discard functionality (discarding unused block group range) to do
> the resetting.
> 
> While the commit 63a7cb130718 ("btrfs: auto enable discard=async when
> possible") made the discard done in an async manner, a zoned reset do not
> need to be async, as it is fast enough.
> 
> Even worth, delaying zone rests prevents using those zones again. So, let's
> disable async discard on the zoned mode.
> 
> Fixes: 63a7cb130718 ("btrfs: auto enable discard=async when possible")
> CC: stable@vger.kernel.org # 6.3+
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>  fs/btrfs/disk-io.c | 7 ++++++-
>  fs/btrfs/zoned.c   | 5 +++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 7513388b0567..9b9914e5f03d 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3438,11 +3438,16 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>  	 * For devices supporting discard turn on discard=async automatically,
>  	 * unless it's already set or disabled. This could be turned off by
>  	 * nodiscard for the same mount.
> +	 *
> +	 * The zoned mode piggy backs on the discard functionality for
> +	 * resetting a zone. There is no reason to delay the zone reset as it is
> +	 * fast enough. So, do not enable async discard for zoned mode.
>  	 */
>  	if (!(btrfs_test_opt(fs_info, DISCARD_SYNC) ||
>  	      btrfs_test_opt(fs_info, DISCARD_ASYNC) ||
>  	      btrfs_test_opt(fs_info, NODISCARD)) &&
> -	    fs_info->fs_devices->discardable) {
> +	    fs_info->fs_devices->discardable &&
> +	    !btrfs_is_zoned(fs_info)) {
>  		btrfs_set_and_info(fs_info, DISCARD_ASYNC,
>  				   "auto enabling async discard");
>  	}
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 85b8b332add9..65d17306c2d4 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -805,6 +805,11 @@ int btrfs_check_mountopts_zoned(struct btrfs_fs_info *info)
>  		return -EINVAL;
>  	}
>  
> +	if (btrfs_test_opt(info, DISCARD_ASYNC)) {
> +		btrfs_warn(info, "zoned: disabling async discard as it is not supported");

The "not supported" mention here kind of imply that we are not finished with
this support yet. So may be a simple: "zoned: ignoring async discard" would
suffice ?

Otherwise, looks good to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> +		btrfs_clear_opt(info->mount_opt, DISCARD_ASYNC);
> +	}
> +
>  	return 0;
>  }
>  

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] btrfs: zoned: do not enable async discard

[Roman Mamedov]

On Thu, 29 Jun 2023 18:15:05 +0900
Damien Le Moal <dlemoal@kernel.org> wrote:

> > +	if (btrfs_test_opt(info, DISCARD_ASYNC)) {
> > +		btrfs_warn(info, "zoned: disabling async discard as it is not supported");
> 
> The "not supported" mention here kind of imply that we are not finished with
> this support yet. So may be a simple: "zoned: ignoring async discard" would
> suffice ?

IMO "not supported" does not imply "not supported yet". To me the message
reads more like "not supported by definition" (of zoned), i.e. no
misunderstanding.

-- 
With respect,
Roman

Re: [PATCH v2] btrfs: zoned: do not enable async discard

[Damien Le Moal]

On 6/29/23 20:25, Roman Mamedov wrote:
> On Thu, 29 Jun 2023 18:15:05 +0900
> Damien Le Moal <dlemoal@kernel.org> wrote:
> 
>>> +	if (btrfs_test_opt(info, DISCARD_ASYNC)) {
>>> +		btrfs_warn(info, "zoned: disabling async discard as it is not supported");
>>
>> The "not supported" mention here kind of imply that we are not finished with
>> this support yet. So may be a simple: "zoned: ignoring async discard" would
>> suffice ?
> 
> IMO "not supported" does not imply "not supported yet". To me the message
> reads more like "not supported by definition" (of zoned), i.e. no
> misunderstanding.

Yeah... I guess it can be understood both ways. I generally prefer to not
"scare" (again, that is very subjective) the user with messages saying "not
supported" if in fact it is not about support but about the feature not being
needed or not appropriate for the setup. But no strong feeling about all this.
Let's keep the message as is. My RB tag stands.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] btrfs: zoned: do not enable async discard

[Christoph Hellwig]

Same as Damien PI'd word it as ignoring instad of disabling.
But otherwise this looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2] btrfs: zoned: do not enable async discard

[David Sterba]

On Thu, Jun 29, 2023 at 05:37:31PM +0900, Naohiro Aota wrote:
> The zoned mode need to reset a zone before using it. We rely on btrfs's
> original discard functionality (discarding unused block group range) to do
> the resetting.
> 
> While the commit 63a7cb130718 ("btrfs: auto enable discard=async when
> possible") made the discard done in an async manner, a zoned reset do not
> need to be async, as it is fast enough.
> 
> Even worth, delaying zone rests prevents using those zones again. So, let's
> disable async discard on the zoned mode.
> 
> Fixes: 63a7cb130718 ("btrfs: auto enable discard=async when possible")
> CC: stable@vger.kernel.org # 6.3+
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>

Added to misc-next, thanks.

> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -805,6 +805,11 @@ int btrfs_check_mountopts_zoned(struct btrfs_fs_info *info)
>  		return -EINVAL;
>  	}
>  
> +	if (btrfs_test_opt(info, DISCARD_ASYNC)) {
> +		btrfs_warn(info, "zoned: disabling async discard as it is not supported");
> +		btrfs_clear_opt(info->mount_opt, DISCARD_ASYNC);
> +	}

I agree with the comments that using 'ignored' is better here, if you
look to the other checks in the function, NOCOW or SPACE_CACHE are not
sukpported and also lead to an error. In case of async discard it's
implemented in a sligly different way but it's not an error. I've also
changed the error level to info, a warning needs attention, in this case
it's not that serious IMHO.