| Message ID | e22f5f69d881de1ec0e381f1be6bfe61b822c064.1688027756.git.naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs: zoned: do not enable async discard | expand |
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; > } >
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.
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.
Same as Damien PI'd word it as ignoring instad of disabling.
But otherwise this looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
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.
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; }
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(-)