| Message ID | 87c887259bfb49878be9990f4dd559ee90d273ec.1687913519.git.naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: zoned: do not enable async discard | expand |
On Wed, Jun 28, 2023 at 09:53:25AM +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. Agreed. But I find some of the logic here rather confusing. Is there a chance we could just decouple the ZONE_RESET logic from looking at the discard options as they are a very fundamental part of the zoned model and thus we should not rely on otherwise optional mount options? > + if (btrfs_test_opt(info, DISCARD_ASYNC)) { > + btrfs_err(info, "zoned: async discard not supported"); > + return -EINVAL; > + } I'd probably only warn about ignoring the option here and clear it as users might have the option in their fstab by now and you'd break mounting the file system and thus potentially booting entirely.
On 6/28/23 09:53, 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..56baac950f11 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_err(info, "zoned: async discard not supported"); > + return -EINVAL; This is an option, so instead of returning an error, why not clearing it with a warning message only ? > + } > + > return 0; > } >
On Tue, Jun 27, 2023 at 10:04:56PM -0700, Christoph Hellwig wrote: > On Wed, Jun 28, 2023 at 09:53:25AM +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. > > Agreed. But I find some of the logic here rather confusing. Is there > a chance we could just decouple the ZONE_RESET logic from looking at > the discard options as they are a very fundamental part of the zoned > model and thus we should not rely on otherwise optional mount options? The logic is mostly common, so it won't be worth decoupling them. The code is implemented in btrfs_delete_unused_bgs(). Here, we check the DISCARD option and zoned mode to do trimming or zone reset. Then, that block group must be kept in transaction->deleted_bgs while it is discarded or zone reset. So, I don't see much point decoupling them. trimming = btrfs_test_opt(fs_info, DISCARD_SYNC) || btrfs_is_zoned(fs_info); But, with discard=async, all these logic is skipped and the async discard work is queued. > > + if (btrfs_test_opt(info, DISCARD_ASYNC)) { > > + btrfs_err(info, "zoned: async discard not supported"); > > + return -EINVAL; > > + } > > I'd probably only warn about ignoring the option here and clear it > as users might have the option in their fstab by now and you'd > break mounting the file system and thus potentially booting entirely. > Sure. That works.
On Wed, Jun 28, 2023 at 03:03:22PM +0900, Damien Le Moal wrote: > On 6/28/23 09:53, 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..56baac950f11 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_err(info, "zoned: async discard not supported"); > > + return -EINVAL; > > This is an option, so instead of returning an error, why not clearing it with a > warning message only ? Sure. I'll change this to btrfs_clear_and_info() that clear the option and print a info message if the option is enabled.
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..56baac950f11 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_err(info, "zoned: async discard not supported"); + return -EINVAL; + } + 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(-)