Tinker with slop space accounting with dedup #12271

rincebrain · 2021-06-23T16:58:28Z

Motivation and Context

The surprising outcome in #12255.

Description

mahrens suggested not including ddt_get_dedup_dspace() in spa_get_slop_space()'s consideration, so rather than reproduce the calculation of spa_get_dspace(), or edit spa_get_dspace() (which had adverse effects on overall accounting when I tried), I just subtract spa->spa_dedup_dspace, which is what ddt_get_dedup_dspace() populates.

How Has This Been Tested?

Ran it through ZTS on my Ubuntu 20.04 x64 testbed without a peep; in addition, on my test pool, writing out a bunch of 100G deduplicatable files, the space used seems to no longer grow unexpectedly as more deduplicated data is written.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

ahrens · 2021-06-23T18:35:19Z

This seems reasonable to me. We probably will want to add a comment explaining what/why we're subtracting out.

rincebrain · 2021-06-23T23:35:20Z

There. I hope I didn't mischaracterize what's going on.

rincebrain · 2021-06-25T22:37:04Z

Okay, after someone commented on it in the bug, noticing that df would occasionally report a stupidly large amount of free space, I implemented a slightly more invasive fix I'll be pushing shortly,

But while testing that, I discovered a behavior that surprised me, and is present in stock code (unless someone modified it since ba91311) - if you delete a 100GB file on a 120GB pool which is entirely deduplicated blocks, the pool will, for a couple seconds, go from reporting 16GB free to 116 GB free, and then back again. When I did this on vanilla code (I expanded the pool so I could do some other things, because it reports 0B free), it looked like this (keep in mind, the pool is doing nothing else this whole time, other than a sleep 15; rm -f [file]; in another window):

Filesystem      Size  Used Avail Use% Mounted on
dedup           306G  128K  306G   1% /dedup
dedup/copyme    2.9T  2.6T  306G  90% /dedup/copyme
Filesystem      Size  Used Avail Use% Mounted on
dedup           406G  128K  406G   1% /dedup
dedup/copyme    2.9T  2.5T  406G  86% /dedup/copyme
Filesystem      Size  Used Avail Use% Mounted on
dedup           406G  128K  406G   1% /dedup
dedup/copyme    2.9T  2.5T  406G  86% /dedup/copyme
Filesystem      Size  Used Avail Use% Mounted on
dedup           406G  128K  406G   1% /dedup
dedup/copyme    2.9T  2.5T  406G  86% /dedup/copyme
Filesystem      Size  Used Avail Use% Mounted on
dedup           406G  128K  406G   1% /dedup
dedup/copyme    2.9T  2.5T  406G  86% /dedup/copyme
Filesystem      Size  Used Avail Use% Mounted on
dedup           309G  128K  309G   1% /dedup
dedup/copyme    2.8T  2.5T  309G  89% /dedup/copyme

So should I worry about that, or if I want to do that, can it be a separate PR, since it's obviously been like this a while?

rincebrain · 2021-06-26T00:01:21Z

module/zfs/ddt.c

-			    class++) {
-				ddt_histogram_add(ddh,
-				    &ddt->ddt_histogram_cache[type][class]);
+		if (ddt) {


To answer why this is at all relevant - if this is called early in pool import (which the spa_update_dspace() call I added will do), this condition is false, which leads to "fun" a few functions deeper when it actually dereferences.

rincebrain · 2021-06-26T00:03:07Z

module/zfs/ddt.c

+				    class < DDT_CLASSES; class++) {
+					ddt_histogram_add(ddh,
+					    &ddt->ddt_histogram_cache
+					    [type][class]);


I absolutely loathe this reformatting, but I loathe it less than any alternate formats I could come up with.

Suggestions welcome.

One suggestion, change the initial for loop to:

for (enum ddt_type type = 0; type < DDT_TYPES && ddt, type++) {

I debated it a bit, but I think ultimately I do prefer that.

Done.

mmaybee · 2021-06-28T23:47:54Z

module/zfs/spa_misc.c

+	 * Sometimes, this is unset when we get here.
+	 * If that's the case, the adjustment below won't do
+	 * anything reasonable, so we regenerate it.


How about changing this to: Make sure spa_dedup_dspace has been set

mmaybee · 2021-06-28T23:48:01Z

module/zfs/ddt.c

+				    class < DDT_CLASSES; class++) {
+					ddt_histogram_add(ddh,
+					    &ddt->ddt_histogram_cache
+					    [type][class]);


One suggestion, change the initial for loop to:

for (enum ddt_type type = 0; type < DDT_TYPES && ddt, type++) {

Let's not include the deduplicated space usage in the slop space reservation, it leads to surprising outcomes. Signed-off-by: Rich Ercolani <[email protected]>

Sometimes, we get into spa_get_slop_space() with spa_dedup_dspace=~0ULL, AKA "unset", while spa_dspace is correctly set. So let's call the code to update it before we use it if we hit that case. Signed-off-by: Rich Ercolani <[email protected]>

* Tinker with slop space accounting with dedup Do not include the deduplicated space usage in the slop space reservation, it leads to surprising outcomes. * Update spa_dedup_dspace sometimes Sometimes, we get into spa_get_slop_space() with spa_dedup_dspace=~0ULL, AKA "unset", while spa_dspace is correctly set. So call the code to update it before we use it if we hit that case. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Signed-off-by: Rich Ercolani <[email protected]> Closes openzfs#12271

* Tinker with slop space accounting with dedup Do not include the deduplicated space usage in the slop space reservation, it leads to surprising outcomes. * Update spa_dedup_dspace sometimes Sometimes, we get into spa_get_slop_space() with spa_dedup_dspace=~0ULL, AKA "unset", while spa_dspace is correctly set. So call the code to update it before we use it if we hit that case. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Signed-off-by: Rich Ercolani <[email protected]> Closes #12271

* Tinker with slop space accounting with dedup Do not include the deduplicated space usage in the slop space reservation, it leads to surprising outcomes. * Update spa_dedup_dspace sometimes Sometimes, we get into spa_get_slop_space() with spa_dedup_dspace=~0ULL, AKA "unset", while spa_dspace is correctly set. So call the code to update it before we use it if we hit that case. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Signed-off-by: Rich Ercolani <[email protected]> Closes openzfs#12271

* Tinker with slop space accounting with dedup Do not include the deduplicated space usage in the slop space reservation, it leads to surprising outcomes. * Update spa_dedup_dspace sometimes Sometimes, we get into spa_get_slop_space() with spa_dedup_dspace=~0ULL, AKA "unset", while spa_dspace is correctly set. So call the code to update it before we use it if we hit that case. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Signed-off-by: Rich Ercolani <[email protected]> Closes #12271

* Tinker with slop space accounting with dedup Do not include the deduplicated space usage in the slop space reservation, it leads to surprising outcomes. * Update spa_dedup_dspace sometimes Sometimes, we get into spa_get_slop_space() with spa_dedup_dspace=~0ULL, AKA "unset", while spa_dspace is correctly set. So call the code to update it before we use it if we hit that case. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Mark Maybee <[email protected]> Signed-off-by: Rich Ercolani <[email protected]> Closes openzfs#12271

behlendorf requested a review from ahrens June 23, 2021 17:24

behlendorf added the Status: Code Review Needed Ready for review and testing label Jun 23, 2021

rincebrain force-pushed the dedup_slop branch from d7872e9 to 4fe0d06 Compare June 23, 2021 23:33

rincebrain commented Jun 26, 2021

View reviewed changes

ahrens assigned mmaybee Jun 28, 2021

ahrens approved these changes Jun 28, 2021

View reviewed changes

mmaybee approved these changes Jun 28, 2021

View reviewed changes

Tinker with slop space accounting with dedup

66a882e

Let's not include the deduplicated space usage in the slop space reservation, it leads to surprising outcomes. Signed-off-by: Rich Ercolani <[email protected]>

rincebrain force-pushed the dedup_slop branch from d4f0d83 to a308570 Compare June 29, 2021 02:24

Update spa_dedup_dspace sometimes

a33be9f

Sometimes, we get into spa_get_slop_space() with spa_dedup_dspace=~0ULL, AKA "unset", while spa_dspace is correctly set. So let's call the code to update it before we use it if we hit that case. Signed-off-by: Rich Ercolani <[email protected]>

rincebrain force-pushed the dedup_slop branch from a308570 to a33be9f Compare June 29, 2021 02:24

mmaybee added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 29, 2021

mmaybee merged commit 1325434 into openzfs:master Jul 13, 2021

behlendorf mentioned this pull request Jul 22, 2021

Panic in spa_update_space #12380

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tinker with slop space accounting with dedup #12271

Tinker with slop space accounting with dedup #12271

rincebrain commented Jun 23, 2021

ahrens commented Jun 23, 2021

rincebrain commented Jun 23, 2021

rincebrain commented Jun 25, 2021

rincebrain Jun 26, 2021

rincebrain Jun 26, 2021

mmaybee Jun 28, 2021

rincebrain Jun 29, 2021

mmaybee Jun 28, 2021

rincebrain Jun 29, 2021

mmaybee Jun 28, 2021

Tinker with slop space accounting with dedup #12271

Tinker with slop space accounting with dedup #12271

Conversation

rincebrain commented Jun 23, 2021

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

ahrens commented Jun 23, 2021

rincebrain commented Jun 23, 2021

rincebrain commented Jun 25, 2021

rincebrain Jun 26, 2021

Choose a reason for hiding this comment

rincebrain Jun 26, 2021

Choose a reason for hiding this comment

mmaybee Jun 28, 2021

Choose a reason for hiding this comment

rincebrain Jun 29, 2021

Choose a reason for hiding this comment

mmaybee Jun 28, 2021

Choose a reason for hiding this comment

rincebrain Jun 29, 2021

Choose a reason for hiding this comment

mmaybee Jun 28, 2021

Choose a reason for hiding this comment