Using async discard by default with SSDs?

Using async discard by default with SSDs?

[Neal Gompa]

Hey all,

I was talking with Chris Mason today at a Fedora Hatch event about
async discards (as we were thinking about doing this in Fedora some
time ago[1]), and he seemed to consider it reasonable to make it so
Btrfs uses async discards by default when being formatted on SSDs.

He and I couldn't think of a reason why not to, other than the
potential lack of "discard=none" option to turn off discards if the
user wanted it to. Do we already have this option? Are there any other
reasons not to do this? Or is this something we should have changed in
Btrfs so everyone gets async discards by default going forward?

Thanks in advance and best regards,
Neal

[1]: https://pagure.io/fedora-btrfs/project/issue/6

--
真実はいつも一つ！/ Always, there's only one truth!

Re: Using async discard by default with SSDs?

[Roman Mamedov]

On Thu, 21 Jul 2022 14:43:16 -0400
Neal Gompa <ngompa13@gmail.com> wrote:

> potential lack of "discard=none" option to turn off discards if the
> user wanted it to. Do we already have this option?

Like on Ext4, XFS and others, the "nodiscard" mount option is available:
https://btrfs.readthedocs.io/en/latest/btrfs-man5.html

-- 
With respect,
Roman

Re: Using async discard by default with SSDs?

[David Sterba]

On Thu, Jul 21, 2022 at 02:43:16PM -0400, Neal Gompa wrote:
> Hey all,
> 
> I was talking with Chris Mason today at a Fedora Hatch event about
> async discards (as we were thinking about doing this in Fedora some
> time ago[1]), and he seemed to consider it reasonable to make it so
> Btrfs uses async discards by default when being formatted on SSDs.
> 
> He and I couldn't think of a reason why not to, other than the
> potential lack of "discard=none" option to turn off discards if the
> user wanted it to. Do we already have this option? Are there any other
> reasons not to do this? Or is this something we should have changed in
> Btrfs so everyone gets async discards by default going forward?

I think it's safe to use by default, with the usual question who could
be affected by that negatively. The async triggers, timeouts and
thresholds are preset conservatively and so far there have been no
complaints.

The tunables have been hidden under debug, but there are also some stats
(like how many bytes could be discarded in the next round), so it would
be good to also publish that when it's on by default.

Re: Using async discard by default with SSDs?

[Chris Murphy]

On Mon, Jul 25, 2022, at 3:08 PM, David Sterba wrote:

> I think it's safe to use by default, with the usual question who could
> be affected by that negatively. The async triggers, timeouts and
> thresholds are preset conservatively and so far there have been no
> complaints.
>
> The tunables have been hidden under debug, but there are also some stats
> (like how many bytes could be discarded in the next round), so it would
> be good to also publish that when it's on by default.

In my testing, discard=async rather quickly submits recently freed metadata blocks for gc, blocks referred by the backup roots in the super. Is this a concern for recovery? Is there a way to exclude the most recent ~5 generations of metadata from gc, and could it improve the usefulness of backup roots for recovery?

-- 
Chris Murphy

Re: Using async discard by default with SSDs?

[David Sterba]

On Tue, Jul 26, 2022 at 04:00:42PM -0400, Chris Murphy wrote:
> 
> 
> On Mon, Jul 25, 2022, at 3:08 PM, David Sterba wrote:
> 
> > I think it's safe to use by default, with the usual question who could
> > be affected by that negatively. The async triggers, timeouts and
> > thresholds are preset conservatively and so far there have been no
> > complaints.
> >
> > The tunables have been hidden under debug, but there are also some stats
> > (like how many bytes could be discarded in the next round), so it would
> > be good to also publish that when it's on by default.
> 
> In my testing, discard=async rather quickly submits recently freed
> metadata blocks for gc, blocks referred by the backup roots in the
> super. Is this a concern for recovery? Is there a way to exclude the
> most recent ~5 generations of metadata from gc, and could it improve
> the usefulness of backup roots for recovery?

What is quickly here? The default timeout is set to 2 minutes and that's
what I've seen when testing that on a fresh filesystem. There's some
additional logic that's adaptive and derived from the latency increase
caused by the discard, but this could be observed on some specific
delete heavy workloads.

There's no logic that would take into account generation so it could
interfere with the backup root feature, but it's been unreliable already
and there are no guarantees to keep the recently freed blocks unallocated.
I've heared some feedback that the feature should be removed or
improved, I'm personally in the "removed" camp.

The block groups eligible for discard are evaluated based on time and
size criteria so I think adding a generation is straightforward, rough
guess is to btrfs_discard_check_filter(). This affects overall
performance though, FB guys wanted faster discard for their workload so
adding delay based on generation could make it worse. They now use the
default settings, though there are tunables, and I'd like to keep
defaults working for most users. If somebody evaluates the effects of
additional generational we can change it.

Re: Using async discard by default with SSDs?

[Chris Murphy]

On Tue, Jul 26, 2022, at 5:36 PM, David Sterba wrote:
> On Tue, Jul 26, 2022 at 04:00:42PM -0400, Chris Murphy wrote:
>> 
>> 
>> On Mon, Jul 25, 2022, at 3:08 PM, David Sterba wrote:
>> 
>> > I think it's safe to use by default, with the usual question who could
>> > be affected by that negatively. The async triggers, timeouts and
>> > thresholds are preset conservatively and so far there have been no
>> > complaints.
>> >
>> > The tunables have been hidden under debug, but there are also some stats
>> > (like how many bytes could be discarded in the next round), so it would
>> > be good to also publish that when it's on by default.
>> 
>> In my testing, discard=async rather quickly submits recently freed
>> metadata blocks for gc, blocks referred by the backup roots in the
>> super. Is this a concern for recovery? Is there a way to exclude the
>> most recent ~5 generations of metadata from gc, and could it improve
>> the usefulness of backup roots for recovery?
>
> What is quickly here?

Seconds, less than a minute, all the blocks pointed to by backup roots are empty (zeros).

 The default timeout is set to 2 minutes and that's
> what I've seen when testing that on a fresh filesystem. 

Ok I'll retest with 5.19 series. The last time I tested time to drive gc backup rootswas circa 5.8.

There's some
> additional logic that's adaptive and derived from the latency increase
> caused by the discard, but this could be observed on some specific
> delete heavy workloads.
>
> There's no logic that would take into account generation so it could
> interfere with the backup root feature, but it's been unreliable already
> and there are no guarantees to keep the recently freed blocks unallocated.
> I've heared some feedback that the feature should be removed or
> improved, I'm personally in the "removed" camp.

I have no strong opinion. If it very rarely works, I'm less concerned. But if it can sometimes work a significant minority of the time, some folks occasionally benefit in a pretty significant way.

--
Chris Murphy

Re: Using async discard by default with SSDs?

[Chris Murphy]

On Tue, Jul 26, 2022, at 6:10 PM, Chris Murphy wrote:
> On Tue, Jul 26, 2022, at 5:36 PM, David Sterba wrote:

>> What is quickly here?
>
> Seconds, less than a minute, all the blocks pointed to by backup roots 
> are empty (zeros).
>
>  The default timeout is set to 2 minutes and that's
>> what I've seen when testing that on a fresh filesystem. 
>
> Ok I'll retest with 5.19 series. The last time I tested time to drive 
> gc backup rootswas circa 5.8.

With 5.19, backup roots remain available for a surprisingly long time, definitely more than 2 minutes. It's not an exhaustive test, just a dozen samples over a half hour, but I never once saw zeros returned. Two out of those dozen, I saw the block already overwritten with a leaf of a newer generation and a completely different owner (e.g. csum tree written at the block address for the backup tree root)  - which would make that backup root useless.

What is a likely target kernel version to make discard=async the default? The merge window for 5.20 closes August 14. Is 5.21 a practical target?

-- 
Chris Murphy

Re: Using async discard by default with SSDs?

[David Sterba]

On Wed, Jul 27, 2022 at 10:50:01AM -0400, Chris Murphy wrote:
> On Tue, Jul 26, 2022, at 6:10 PM, Chris Murphy wrote:
> > On Tue, Jul 26, 2022, at 5:36 PM, David Sterba wrote:
> >> What is quickly here?
> >
> > Seconds, less than a minute, all the blocks pointed to by backup roots 
> > are empty (zeros).
> >
> >  The default timeout is set to 2 minutes and that's
> >> what I've seen when testing that on a fresh filesystem. 
> >
> > Ok I'll retest with 5.19 series. The last time I tested time to drive 
> > gc backup rootswas circa 5.8.
> 
> With 5.19, backup roots remain available for a surprisingly long time,
> definitely more than 2 minutes. It's not an exhaustive test, just a
> dozen samples over a half hour, but I never once saw zeros returned.
> Two out of those dozen, I saw the block already overwritten with a
> leaf of a newer generation and a completely different owner (e.g. csum
> tree written at the block address for the backup tree root)  - which
> would make that backup root useless.
> 
> What is a likely target kernel version to make discard=async the
> default? The merge window for 5.20 closes August 14. Is 5.21 a
> practical target?

The changes for the next merge window are supposed to be done a week or
two before it opens, but as this is a simple change I think I can
squeeze it in.

Re: Using async discard by default with SSDs?

[Chris Murphy]

On Wed, Jul 27, 2022, at 10:56 AM, David Sterba wrote:
> On Wed, Jul 27, 2022 at 10:50:01AM -0400, Chris Murphy wrote:
>> What is a likely target kernel version to make discard=async the
>> default? The merge window for 5.20 closes August 14. Is 5.21 a
>> practical target?
>
> The changes for the next merge window are supposed to be done a week or
> two before it opens, but as this is a simple change I think I can
> squeeze it in.

For 5.20?

I'm not aware of any conflict with fstrim. But I wonder if there's a preference to coordinate the change with util-linux folks? 

Currently, util-linux provides fstrim.timer which runs fstrim.service every Monday at 00:00 local time. The command is:

ExecStart=/usr/sbin/fstrim --listed-in /etc/fstab:/proc/self/mountinfo --verbose --quiet-unsupported

I'm not sure how they'd go about implementing an exception for Btrfs, either entirely or only if a discard mount option is detected. But I can ask?

-- 
Chris Murphy

Re: Using async discard by default with SSDs?

[Neal Gompa]

On Wed, Jul 27, 2022 at 8:01 AM David Sterba <dsterba@suse.cz> wrote:
>
> On Wed, Jul 27, 2022 at 10:50:01AM -0400, Chris Murphy wrote:
> > On Tue, Jul 26, 2022, at 6:10 PM, Chris Murphy wrote:
> > > On Tue, Jul 26, 2022, at 5:36 PM, David Sterba wrote:
> > >> What is quickly here?
> > >
> > > Seconds, less than a minute, all the blocks pointed to by backup roots
> > > are empty (zeros).
> > >
> > >  The default timeout is set to 2 minutes and that's
> > >> what I've seen when testing that on a fresh filesystem.
> > >
> > > Ok I'll retest with 5.19 series. The last time I tested time to drive
> > > gc backup rootswas circa 5.8.
> >
> > With 5.19, backup roots remain available for a surprisingly long time,
> > definitely more than 2 minutes. It's not an exhaustive test, just a
> > dozen samples over a half hour, but I never once saw zeros returned.
> > Two out of those dozen, I saw the block already overwritten with a
> > leaf of a newer generation and a completely different owner (e.g. csum
> > tree written at the block address for the backup tree root)  - which
> > would make that backup root useless.
> >
> > What is a likely target kernel version to make discard=async the
> > default? The merge window for 5.20 closes August 14. Is 5.21 a
> > practical target?
>
> The changes for the next merge window are supposed to be done a week or
> two before it opens, but as this is a simple change I think I can
> squeeze it in.

It would definitely be nice to see this for 5.20. :)


-- 
真実はいつも一つ！/ Always, there's only one truth!

Re: Using async discard by default with SSDs?

[David Sterba]

On Wed, Jul 27, 2022 at 11:14:01AM -0400, Chris Murphy wrote:
> 
> 
> On Wed, Jul 27, 2022, at 10:56 AM, David Sterba wrote:
> > On Wed, Jul 27, 2022 at 10:50:01AM -0400, Chris Murphy wrote:
> >> What is a likely target kernel version to make discard=async the
> >> default? The merge window for 5.20 closes August 14. Is 5.21 a
> >> practical target?
> >
> > The changes for the next merge window are supposed to be done a week or
> > two before it opens, but as this is a simple change I think I can
> > squeeze it in.
> 
> For 5.20?

Yes, 5.20.

> I'm not aware of any conflict with fstrim. But I wonder if there's a
> preference to coordinate the change with util-linux folks? 

The -o discard and fstrim are independent and can be used at the same
time.

> Currently, util-linux provides fstrim.timer which runs fstrim.service every Monday at 00:00 local time. The command is:
> 
> ExecStart=/usr/sbin/fstrim --listed-in /etc/fstab:/proc/self/mountinfo --verbose --quiet-unsupported
> 
> I'm not sure how they'd go about implementing an exception for Btrfs,
> either entirely or only if a discard mount option is detected. But I
> can ask?

No need for an exception, during mount btrfs track ranges that have been
trimmed and skips them when discard is requested by fstrim, so it's a
no-op.

Re: Using async discard by default with SSDs?

[Chris Murphy]

On Wed, Jul 27, 2022, at 1:47 PM, David Sterba wrote:
> On Wed, Jul 27, 2022 at 11:14:01AM -0400, Chris Murphy wrote:
>> 
>> 
>> On Wed, Jul 27, 2022, at 10:56 AM, David Sterba wrote:
>> > On Wed, Jul 27, 2022 at 10:50:01AM -0400, Chris Murphy wrote:
>
>> > The changes for the next merge window are supposed to be done a week or
>> > two before it opens, but as this is a simple change I think I can
>> > squeeze it in.
>> 
>> For 5.20?
>
> Yes, 5.20.

Well I'll let Josef and Chris arm wrestle over who sends you a patch when they can get ahold of some round tuits. Once it's merged in 5.20, sounds easily backported for Fedora to carry a patch for 5.19.

-- 
Chris Murphy

Re: Using async discard by default with SSDs?

[David Sterba]

On Wed, Jul 27, 2022 at 04:56:40PM +0200, David Sterba wrote:
> On Wed, Jul 27, 2022 at 10:50:01AM -0400, Chris Murphy wrote:
> > On Tue, Jul 26, 2022, at 6:10 PM, Chris Murphy wrote:
> > > On Tue, Jul 26, 2022, at 5:36 PM, David Sterba wrote:
> > >> What is quickly here?
> > >
> > > Seconds, less than a minute, all the blocks pointed to by backup roots 
> > > are empty (zeros).
> > >
> > >  The default timeout is set to 2 minutes and that's
> > >> what I've seen when testing that on a fresh filesystem. 
> > >
> > > Ok I'll retest with 5.19 series. The last time I tested time to drive 
> > > gc backup rootswas circa 5.8.
> > 
> > With 5.19, backup roots remain available for a surprisingly long time,
> > definitely more than 2 minutes. It's not an exhaustive test, just a
> > dozen samples over a half hour, but I never once saw zeros returned.
> > Two out of those dozen, I saw the block already overwritten with a
> > leaf of a newer generation and a completely different owner (e.g. csum
> > tree written at the block address for the backup tree root)  - which
> > would make that backup root useless.
> > 
> > What is a likely target kernel version to make discard=async the
> > default? The merge window for 5.20 closes August 14. Is 5.21 a
> > practical target?
> 
> The changes for the next merge window are supposed to be done a week or
> two before it opens, but as this is a simple change I think I can
> squeeze it in.

This will have to be postponed, Filipe found some problems,
transaction abort can happen due to ENOSPC when the system under load.
This does not happen in most cases but for a feature supposed to be on
by default such thing needs to be fixed first.