Cosmetics bug: remounting ssd does not clear nossd

PATCH Superseded Hans van Kranenburg 3 participants 13 messages Mar 30, 2017 – Apr 15, 2017 Btrfs file system

lore

Share & tools

lore

Hans van Kranenburg

author

2017-03-30 23:01 UTC #0 lore raw

If I have a filesystem that shows this...

rw,relatime,ssd,space_cache=v2,subvolid=5,subvol=/

...and then I do this...

-# mount -o remount,nossd /mnt/btrfs/

...then it shows...

rw,relatime,nossd,space_cache=v2,subvolid=5,subvol=/

...but when I do this...

-# mount -o remount,ssd /mnt/btrfs/

...it ends up with a rotational identity crisis:

rw,relatime,nossd,ssd,space_cache=v2,subvolid=5,subvol=/

            ^^^^^ ^^^

The only thing the nossd option does when mounting first time is preventing that ssd gets enabled automatically. When doing nossd with remount, it clears out the ssd flag, but telling it to ssd again does not clear the nossd flag.

The actual kernel code which changes behaviour based on these options only ever checks for SSD, never for NOSSD.

So, the situation does not lead to any wrong behaviour right now, but it's confusing, and it could be a source of bugs in the future, when anything would test for NOSSD explicitely and make a decision based on the result.

Moo,

Signature · 1 line

Hans van Kranenburg

Adam Borowski

2017-03-31 15:18 UTC #1 lore raw

PATCH 1/1 btrfs: drop the nossd flag when remounting with -o ssd

The opposite case was already handled right in the very next switch entry.

Reported-by Hans van Kranenburg <[email protected]>

Signed-off-by Adam Borowski <[email protected]>

---
Not sure if setting NOSSD should also disable SSD_SPREAD, there's currently no way to disable that option once set.

 fs/btrfs/super.c | 2 ++
 1 file changed, 2 insertions(+)

fs/btrfs/super.c +2 -0

index 06bd9b332e18..7342399951ad 100644
@@ -549,11 +549,13 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 		case Opt_ssd:
 			btrfs_set_and_info(info, SSD,
 					   "use ssd allocation scheme");
+			btrfs_clear_opt(info->mount_opt, NOSSD);
 			break;
 		case Opt_ssd_spread:
 			btrfs_set_and_info(info, SSD_SPREAD,
 					   "use spread ssd allocation scheme");
 			btrfs_set_opt(info->mount_opt, SSD);
+			btrfs_clear_opt(info->mount_opt, NOSSD);
 			break;
 		case Opt_nossd:
 			btrfs_set_and_info(info, NOSSD,

Signature · 1 line

2.11.0

Hans van Kranenburg

author

2017-03-31 16:00 UTC #2 lore raw

Re: [PATCH] btrfs: drop the nossd flag when remounting with -o ssd

Quoted 19 lines · Adam Borowski

The opposite case was already handled right in the very next switch entry.
Reported-by Hans van Kranenburg <[email protected]>
Signed-off-by Adam Borowski <[email protected]>
---
Not sure if setting NOSSD should also disable SSD_SPREAD, there's currently no way to disable that option once set.
 fs/btrfs/super.c | 2 ++
 1 file changed, 2 insertions(+)
fs/btrfs/super.c +0 -0
index 06bd9b332e18..7342399951ad 100644
@@ -549,11 +549,13 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 		case Opt_ssd:
 			btrfs_set_and_info(info, SSD,
 					   "use ssd allocation scheme");

fs/btrfs/super.c int btrfs_parse_options(struct btrfs_fs_info *info, char *options, · line 561

Quoted 10 lines

fs/btrfs/super.c +2 -0

+			btrfs_clear_opt(info->mount_opt, NOSSD);
 			break;
 		case Opt_ssd_spread:
 			btrfs_set_and_info(info, SSD_SPREAD,
 					   "use spread ssd allocation scheme");
 			btrfs_set_opt(info->mount_opt, SSD);
+			btrfs_clear_opt(info->mount_opt, NOSSD);
 			break;
 		case Opt_nossd:
 			btrfs_set_and_info(info, NOSSD,

How did you test this?

This was also my first thought, but here's a weird thing:

-# mount -o nossd /dev/sdx /mnt/btrfs/

BTRFS info (device sdx): not using ssd allocation scheme

-# mount -o remount,ssd /mnt/btrfs/

BTRFS info (device sdx): use ssd allocation scheme

-# mount -o remount,nossd /mnt/btrfs/

BTRFS info (device sdx): use ssd allocation scheme

That means that the case Opt_nossd: is never reached when doing this?

And... what should be the result of doing:
-# mount -o remount,nossd,ssd /mnt/btrfs/

I guess it should be that the last one in the sequence wins?

The fact that nossd,ssd,ssd_spread are different options complicates the whole thing, compared to e.g. autodefrag, noautodefrag.

Signature · 1 line

Hans van Kranenburg

David Sterba

2017-03-31 17:11 UTC #3 lore raw

Re: [PATCH] btrfs: drop the nossd flag when remounting with -o ssd

Quoted 8 lines · Hans van Kranenburg

Quoted 7 lines · Adam Borowski
The opposite case was already handled right in the very next switch entry.
Reported-by Hans van Kranenburg <[email protected]>
Signed-off-by Adam Borowski <[email protected]>
---
Not sure if setting NOSSD should also disable SSD_SPREAD, there's currently no way to disable that option once set.

Missing inverse of ssd_spread is probably unintentional, as we once added all complementary no* options, this one was forgotten.

And yes, nossd should turn off ssd and ssd_spread, as ssd_spread without ssd does not nothing anyway.

Quoted 37 lines

Quoted 12 lines
 fs/btrfs/super.c | 2 ++
 1 file changed, 2 insertions(+)
fs/btrfs/super.c +0 -0
index 06bd9b332e18..7342399951ad 100644
@@ -549,11 +549,13 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 		case Opt_ssd:
 			btrfs_set_and_info(info, SSD,
 					   "use ssd allocation scheme");
fs/btrfs/super.c int btrfs_parse_options(struct btrfs_fs_info *info, char *options, · line 561
Quoted 10 lines
fs/btrfs/super.c +2 -0
+			btrfs_clear_opt(info->mount_opt, NOSSD);
 			break;
 		case Opt_ssd_spread:
 			btrfs_set_and_info(info, SSD_SPREAD,
 					   "use spread ssd allocation scheme");
 			btrfs_set_opt(info->mount_opt, SSD);
+			btrfs_clear_opt(info->mount_opt, NOSSD);
 			break;
 		case Opt_nossd:
 			btrfs_set_and_info(info, NOSSD,
How did you test this?
This was also my first thought, but here's a weird thing:
-# mount -o nossd /dev/sdx /mnt/btrfs/
BTRFS info (device sdx): not using ssd allocation scheme
-# mount -o remount,ssd /mnt/btrfs/
BTRFS info (device sdx): use ssd allocation scheme
-# mount -o remount,nossd /mnt/btrfs/

Quoted 8 lines

BTRFS info (device sdx): use ssd allocation scheme
That means that the case Opt_nossd: is never reached when doing this?
And... what should be the result of doing:
-# mount -o remount,nossd,ssd /mnt/btrfs/
I guess it should be that the last one in the sequence wins?

The last one wins.

Quoted 2 lines

The fact that nossd,ssd,ssd_spread are different options complicates the whole thing, compared to e.g. autodefrag, noautodefrag.

I think the the ssd flags reflect the autodetection of ssd, unlike autodefrag and others.

The ssd options says "enable the ssd mode", but it could be also auto-detected if the non-rotational device is detected.

nossd says, "do not do the autodetection, even if it's a non-rot device, also disable all ssd modes".

The manual page is not entirely clear about that, I'll update it.

So Adam's patch needs to be updated so NOSSD also disables SSD_SPREAD. Adding the 'nossd_spread' would be good to have, even if it might be just a marginal usecase.

Adam Borowski

2017-03-31 20:08 UTC #4 lore raw

PATCH v2 1/1 btrfs: drop the nossd flag when remounting with -o ssd

And when turning on nossd, drop ssd_spread.

Reported-by Hans van Kranenburg <[email protected]>

Signed-off-by Adam Borowski <[email protected]>

---

Quoted 10 lines · David Sterba

Quoted 3 lines · Hans van Kranenburg
Quoted 2 lines · Adam Borowski
Not sure if setting NOSSD should also disable SSD_SPREAD, there's currently no way to disable that option once set.
Missing inverse of ssd_spread is probably unintentional, as we once added all complementary no* options, this one was forgotten.
And yes, nossd should turn off ssd and ssd_spread, as ssd_spread without ssd does not nothing anyway.

Added that.

Quoted 10 lines

Quoted 10 lines
How did you test this?
This was also my first thought, but here's a weird thing:
-# mount -o nossd /dev/sdx /mnt/btrfs/
BTRFS info (device sdx): not using ssd allocation scheme
-# mount -o remount,ssd /mnt/btrfs/

Quoted 7 lines

Quoted 7 lines
BTRFS info (device sdx): use ssd allocation scheme
-# mount -o remount,nossd /mnt/btrfs/
BTRFS info (device sdx): use ssd allocation scheme
That means that the case Opt_nossd: is never reached when doing this?

Seems to work for me:

[/tmp]# mount -onoatime foo /mnt/vol1

[  619.436745] BTRFS: device fsid 954fd6c3-b3ce-4355-b79a-60ece7a6a4e0 devid 1 transid 5 /dev/loop0
[  619.438625] BTRFS info (device loop0): disk space caching is enabled
[  619.438627] BTRFS info (device loop0): has skinny extents
[  619.438629] BTRFS info (device loop0): flagging fs with big metadata feature
[  619.441989] BTRFS info (device loop0): creating UUID tree
[/tmp]# mount -oremount,ssd /mnt/vol1
[  629.755584] BTRFS info (device loop0): use ssd allocation scheme
[  629.755589] BTRFS info (device loop0): disk space caching is enabled
[/tmp]# mount -oremount,nossd /mnt/vol1
[  633.675867] BTRFS info (device loop0): not using ssd allocation scheme
[  633.675872] BTRFS info (device loop0): disk space caching is enabled

Quoted 5 lines

Quoted 2 lines
The fact that nossd,ssd,ssd_spread are different options complicates the whole thing, compared to e.g. autodefrag, noautodefrag.
I think the the ssd flags reflect the autodetection of ssd, unlike autodefrag and others.

The autodetection works for /dev/sd* and /dev/mmcblk*, but not for most other devices.

Two examples:
nbd to a piece of rotating rust says:

[45697.575192] BTRFS info (device nbd0): detected SSD devices, enabling SSD mode

loop on tmpfs (and in case it spills, all swap is on ssd):
claims it's rotational

Quoted 5 lines

The ssd options says "enable the ssd mode", but it could be also auto-detected if the non-rotational device is detected.
nossd says, "do not do the autodetection, even if it's a non-rot device, also disable all ssd modes".

These two options are nice whenever the autodetection goes wrong.

Quoted 1 line

So Adam's patch needs to be updated so NOSSD also disables SSD_SPREAD.

M'kay, updated this patch.

Quoted 2 lines

Adding the 'nossd_spread' would be good to have, even if it might be just a marginal usecase.

Not sure if there's much point. In any case, that's a separate patch. Should I add one while we're here?

Meow!

 fs/btrfs/super.c | 3 +++
 1 file changed, 3 insertions(+)

fs/btrfs/super.c +3 -0

index 06bd9b332e18..ac1ca22d0c34 100644
@@ -549,16 +549,19 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 		case Opt_ssd:
 			btrfs_set_and_info(info, SSD,
 					   "use ssd allocation scheme");
+			btrfs_clear_opt(info->mount_opt, NOSSD);
 			break;
 		case Opt_ssd_spread:
 			btrfs_set_and_info(info, SSD_SPREAD,
 					   "use spread ssd allocation scheme");
 			btrfs_set_opt(info->mount_opt, SSD);
+			btrfs_clear_opt(info->mount_opt, NOSSD);
 			break;
 		case Opt_nossd:
 			btrfs_set_and_info(info, NOSSD,
 					     "not using ssd allocation scheme");
 			btrfs_clear_opt(info->mount_opt, SSD);
+			btrfs_clear_opt(info->mount_opt, SSD_SPREAD);
 			break;
 		case Opt_barrier:
 			btrfs_clear_and_info(info, NOBARRIER,

Signature · 1 line

2.11.0

Hans van Kranenburg

author

2017-03-31 20:25 UTC #5 lore raw

Re: [PATCH v2] btrfs: drop the nossd flag when remounting with -o ssd

Quoted 40 lines · Adam Borowski

And when turning on nossd, drop ssd_spread.
Reported-by Hans van Kranenburg <[email protected]>
Signed-off-by Adam Borowski <[email protected]>
---
Quoted 10 lines · David Sterba
Quoted 3 lines · Hans van Kranenburg
Quoted 2 lines · Adam Borowski
Not sure if setting NOSSD should also disable SSD_SPREAD, there's currently no way to disable that option once set.
Missing inverse of ssd_spread is probably unintentional, as we once added all complementary no* options, this one was forgotten.
And yes, nossd should turn off ssd and ssd_spread, as ssd_spread without ssd does not nothing anyway.
Added that.
Quoted 10 lines
Quoted 10 lines
How did you test this?
This was also my first thought, but here's a weird thing:
-# mount -o nossd /dev/sdx /mnt/btrfs/
BTRFS info (device sdx): not using ssd allocation scheme
-# mount -o remount,ssd /mnt/btrfs/
Quoted 7 lines
Quoted 7 lines
BTRFS info (device sdx): use ssd allocation scheme
-# mount -o remount,nossd /mnt/btrfs/
BTRFS info (device sdx): use ssd allocation scheme
That means that the case Opt_nossd: is never reached when doing this?
Seems to work for me:
[/tmp]# mount -onoatime foo /mnt/vol1

Quoted 11 lines

[  619.436745] BTRFS: device fsid 954fd6c3-b3ce-4355-b79a-60ece7a6a4e0 devid 1 transid 5 /dev/loop0
[  619.438625] BTRFS info (device loop0): disk space caching is enabled
[  619.438627] BTRFS info (device loop0): has skinny extents
[  619.438629] BTRFS info (device loop0): flagging fs with big metadata feature
[  619.441989] BTRFS info (device loop0): creating UUID tree
[/tmp]# mount -oremount,ssd /mnt/vol1
[  629.755584] BTRFS info (device loop0): use ssd allocation scheme
[  629.755589] BTRFS info (device loop0): disk space caching is enabled
[/tmp]# mount -oremount,nossd /mnt/vol1
[  633.675867] BTRFS info (device loop0): not using ssd allocation scheme
[  633.675872] BTRFS info (device loop0): disk space caching is enabled

Yes, but we're not doing the same thing here.

You have a file via a loop mount. If I do that, I get the same output as you show, the right messages when I remount ssd and nossd.

My test was lvm based on an ssd. When I mount that, I get the "detected SSD devices, enabling SSD mode", and everytime I remount, being it ssd or nossd, it *always* says "use ssd allocation scheme".

So, this needs some more research I guess. It doesn't feel right.

Quoted 15 lines

Quoted 5 lines
Quoted 2 lines
The fact that nossd,ssd,ssd_spread are different options complicates the whole thing, compared to e.g. autodefrag, noautodefrag.
I think the the ssd flags reflect the autodetection of ssd, unlike autodefrag and others.
The autodetection works for /dev/sd* and /dev/mmcblk*, but not for most other devices.
Two examples:
nbd to a piece of rotating rust says:
[45697.575192] BTRFS info (device nbd0): detected SSD devices, enabling SSD mode
loop on tmpfs (and in case it spills, all swap is on ssd):
claims it's rotational

Quoted 9 lines

Quoted 5 lines
The ssd options says "enable the ssd mode", but it could be also auto-detected if the non-rotational device is detected.
nossd says, "do not do the autodetection, even if it's a non-rot device, also disable all ssd modes".
These two options are nice whenever the autodetection goes wrong.
Quoted 1 line
So Adam's patch needs to be updated so NOSSD also disables SSD_SPREAD.

Ack.

Quoted 4 lines

M'kay, updated this patch.
Quoted 2 lines
Adding the 'nossd_spread' would be good to have, even if it might be just a marginal usecase.

Please no, don't make it more complex if not needed.

Quoted 2 lines

Not sure if there's much point. In any case, that's a separate patch. Should I add one while we're here?

Since the whole ssd thing is a bit of a joke actually, I'd rather see it replaces with an option to choose an extent allocator algorithm.

The amount of if statements using this SSD things in btrfs in the kernel can be counted on one hand, and what they actually do is quite questionable (food for another mail thread).

Quoted 31 lines

Meow!

 fs/btrfs/super.c | 3 +++
 1 file changed, 3 insertions(+)

fs/btrfs/super.c +3 -0

index 06bd9b332e18..ac1ca22d0c34 100644
@@ -549,16 +549,19 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 		case Opt_ssd:
 			btrfs_set_and_info(info, SSD,
 					   "use ssd allocation scheme");
+			btrfs_clear_opt(info->mount_opt, NOSSD);
 			break;
 		case Opt_ssd_spread:
 			btrfs_set_and_info(info, SSD_SPREAD,
 					   "use spread ssd allocation scheme");
 			btrfs_set_opt(info->mount_opt, SSD);
+			btrfs_clear_opt(info->mount_opt, NOSSD);
 			break;
 		case Opt_nossd:
 			btrfs_set_and_info(info, NOSSD,
 					     "not using ssd allocation scheme");
 			btrfs_clear_opt(info->mount_opt, SSD);
+			btrfs_clear_opt(info->mount_opt, SSD_SPREAD);
 			break;
 		case Opt_barrier:
 			btrfs_clear_and_info(info, NOBARRIER,

Signature · 1 line

Hans van Kranenburg

Adam Borowski

2017-03-31 20:43 UTC #6 lore raw

Re: [PATCH v2] btrfs: drop the nossd flag when remounting with -o ssd

Quoted 35 lines · Hans van Kranenburg

Quoted 21 lines
Quoted 10 lines
Quoted 10 lines
How did you test this?
This was also my first thought, but here's a weird thing:
-# mount -o nossd /dev/sdx /mnt/btrfs/
BTRFS info (device sdx): not using ssd allocation scheme
-# mount -o remount,ssd /mnt/btrfs/
Quoted 7 lines
Quoted 7 lines
BTRFS info (device sdx): use ssd allocation scheme
-# mount -o remount,nossd /mnt/btrfs/
BTRFS info (device sdx): use ssd allocation scheme
That means that the case Opt_nossd: is never reached when doing this?
Seems to work for me:
[/tmp]# mount -onoatime foo /mnt/vol1
Quoted 11 lines
[  619.436745] BTRFS: device fsid 954fd6c3-b3ce-4355-b79a-60ece7a6a4e0 devid 1 transid 5 /dev/loop0
[  619.438625] BTRFS info (device loop0): disk space caching is enabled
[  619.438627] BTRFS info (device loop0): has skinny extents
[  619.438629] BTRFS info (device loop0): flagging fs with big metadata feature
[  619.441989] BTRFS info (device loop0): creating UUID tree
[/tmp]# mount -oremount,ssd /mnt/vol1
[  629.755584] BTRFS info (device loop0): use ssd allocation scheme
[  629.755589] BTRFS info (device loop0): disk space caching is enabled
[/tmp]# mount -oremount,nossd /mnt/vol1
[  633.675867] BTRFS info (device loop0): not using ssd allocation scheme
[  633.675872] BTRFS info (device loop0): disk space caching is enabled
Yes, but we're not doing the same thing here.

Quoted 8 lines

You have a file via a loop mount. If I do that, I get the same output as you show, the right messages when I remount ssd and nossd.
My test was lvm based on an ssd. When I mount that, I get the "detected SSD devices, enabling SSD mode", and everytime I remount, being it ssd or nossd, it *always* says "use ssd allocation scheme".
So, this needs some more research I guess. It doesn't feel right.

I can't reproduce:

[~]# cat /proc/swaps
Filename Type Size Used Priority
/dev/sda2 partition 8822780 0 -1
[~]# swapoff /dev/sda2
[~]# mkfs.btrfs -f /dev/sda2
...

[ 2459.856819] BTRFS info (device sda2): detected SSD devices, enabling SSD mode
[ 2459.857699] BTRFS info (device sda2): creating UUID tree
[ 2477.234868] BTRFS info (device sda2): not using ssd allocation scheme
[ 2477.234873] BTRFS info (device sda2): disk space caching is enabled
[ 2482.306649] BTRFS info (device sda2): use ssd allocation scheme
[ 2482.306654] BTRFS info (device sda2): disk space caching is enabled
[ 2483.618578] BTRFS info (device sda2): not using ssd allocation scheme
[ 2483.618583] BTRFS info (device sda2): disk space caching is enabled

Same partition on lvm:

[ 2813.259749] BTRFS info (device dm-0): detected SSD devices, enabling SSD mode
[ 2813.260586] BTRFS info (device dm-0): creating UUID tree
[ 2827.131076] BTRFS info (device dm-0): not using ssd allocation scheme
[ 2827.131081] BTRFS info (device dm-0): disk space caching is enabled
[ 2828.618841] BTRFS info (device dm-0): use ssd allocation scheme
[ 2828.618845] BTRFS info (device dm-0): disk space caching is enabled
[ 2829.546796] BTRFS info (device dm-0): not using ssd allocation scheme
[ 2829.546801] BTRFS info (device dm-0): disk space caching is enabled
[ 2833.770787] BTRFS info (device dm-0): use ssd allocation scheme
[ 2833.770792] BTRFS info (device dm-0): disk space caching is enabled

Seems to flip back and forth correctly for me.

Are you sure you have this patch applied?

Quoted 8 lines

Quoted 2 lines
Quoted 2 lines
Adding the 'nossd_spread' would be good to have, even if it might be just a marginal usecase.
Please no, don't make it more complex if not needed.
Quoted 2 lines
Not sure if there's much point. In any case, that's a separate patch. Should I add one while we're here?

Quoted 6 lines

Since the whole ssd thing is a bit of a joke actually, I'd rather see it replaces with an option to choose an extent allocator algorithm.
The amount of if statements using this SSD things in btrfs in the kernel can be counted on one hand, and what they actually do is quite questionable (food for another mail thread).

Ok, let's fix only existing options for now then.

Signature · 4 lines

⢀⣴⠾⠻⢶⣦⠀ Meow!
⣾⠁⢠⠒⠀⣿⡁
⢿⡄⠘⠷⠚⠋⠀ Collisions shmolisions, let's see them find a collision or second
⠈⠳⣄⠀⠀⠀⠀ preimage for double rot13!

Hans van Kranenburg

author

2017-03-31 20:50 UTC #7 lore raw

Re: [PATCH v2] btrfs: drop the nossd flag when remounting with -o ssd

Quoted 42 lines · Adam Borowski

Quoted 11 lines · Hans van Kranenburg
Yes, but we're not doing the same thing here.
You have a file via a loop mount. If I do that, I get the same output as you show, the right messages when I remount ssd and nossd.
My test was lvm based on an ssd. When I mount that, I get the "detected SSD devices, enabling SSD mode", and everytime I remount, being it ssd or nossd, it *always* says "use ssd allocation scheme".
So, this needs some more research I guess. It doesn't feel right.
I can't reproduce:
[~]# cat /proc/swaps
Filename Type Size Used Priority
/dev/sda2 partition 8822780 0 -1
[~]# swapoff /dev/sda2
[~]# mkfs.btrfs -f /dev/sda2
...
[ 2459.856819] BTRFS info (device sda2): detected SSD devices, enabling SSD mode
[ 2459.857699] BTRFS info (device sda2): creating UUID tree
[ 2477.234868] BTRFS info (device sda2): not using ssd allocation scheme
[ 2477.234873] BTRFS info (device sda2): disk space caching is enabled
[ 2482.306649] BTRFS info (device sda2): use ssd allocation scheme
[ 2482.306654] BTRFS info (device sda2): disk space caching is enabled
[ 2483.618578] BTRFS info (device sda2): not using ssd allocation scheme
[ 2483.618583] BTRFS info (device sda2): disk space caching is enabled
Same partition on lvm:
[ 2813.259749] BTRFS info (device dm-0): detected SSD devices, enabling SSD mode
[ 2813.260586] BTRFS info (device dm-0): creating UUID tree
[ 2827.131076] BTRFS info (device dm-0): not using ssd allocation scheme
[ 2827.131081] BTRFS info (device dm-0): disk space caching is enabled
[ 2828.618841] BTRFS info (device dm-0): use ssd allocation scheme
[ 2828.618845] BTRFS info (device dm-0): disk space caching is enabled
[ 2829.546796] BTRFS info (device dm-0): not using ssd allocation scheme
[ 2829.546801] BTRFS info (device dm-0): disk space caching is enabled
[ 2833.770787] BTRFS info (device dm-0): use ssd allocation scheme
[ 2833.770792] BTRFS info (device dm-0): disk space caching is enabled

Quoted 3 lines

Seems to flip back and forth correctly for me.
Are you sure you have this patch applied?

Oh ok, that's with the patch. The output I show is without the patch.

If it does my output without the patch instead and the right output with it applied, then the puzzle pieces are in the right place again.

Signature · 1 line

Hans van Kranenburg

David Sterba

2017-04-03 12:25 UTC #8 lore raw

Re: [PATCH v2] btrfs: drop the nossd flag when remounting with -o ssd

In reply to Hans van Kranenburg

Quoted 4 lines · Hans van Kranenburg

Quoted 2 lines
Quoted 2 lines
Adding the 'nossd_spread' would be good to have, even if it might be just a marginal usecase.
Please no, don't make it more complex if not needed.

The only use is when ssd,ssd_spread are on and then I'd just want to disable ssd_spread, without disabling ssd at the same time.

1. mount -o ssd,ssd_spread
2. mount -o remount,nossd_spread

compared to

1. mount -o ssd,ssd_spread
2. mount -o remount,nossd
3. mount -o remount,ssd

I'd vote for adding nossd_spread, as the 'no-' options are common and otherwise disabling ssd_spread would be another usage exception.

Quoted 5 lines

Quoted 2 lines
Not sure if there's much point. In any case, that's a separate patch. Should I add one while we're here?
Since the whole ssd thing is a bit of a joke actually, I'd rather see it replaces with an option to choose an extent allocator algorithm.

Yeah, SSD is not the shiny new tech anymore, so we'd need something more future proof.

Quoted 3 lines

The amount of if statements using this SSD things in btrfs in the kernel can be counted on one hand, and what they actually do is quite questionable (food for another mail thread).

That's right, do you have suggestions for futher SSD optimizations? Other than better block alignment, faster flushes and no seek penalty, I don't see much else.

Hans van Kranenburg

author

2017-04-03 22:43 UTC #9 lore raw

Re: [PATCH v2] btrfs: drop the nossd flag when remounting with -o ssd

Quoted 12 lines · David Sterba

Quoted 4 lines · Hans van Kranenburg
Quoted 2 lines
Quoted 2 lines
Adding the 'nossd_spread' would be good to have, even if it might be just a marginal usecase.
Please no, don't make it more complex if not needed.
The only use is when ssd,ssd_spread are on and then I'd just want to disable ssd_spread, without disabling ssd at the same time.
1. mount -o ssd,ssd_spread
2. mount -o remount,nossd_spread

Quoted 8 lines

compared to
1. mount -o ssd,ssd_spread
2. mount -o remount,nossd
3. mount -o remount,ssd
I'd vote for adding nossd_spread, as the 'no-' options are common and otherwise disabling ssd_spread would be another usage exception.

Yes, 'nossd_spread' would intuitively be the thing to try to get rid of 'ssd_spread' on a mounted fs. But, nossd_spread is not a feature, just like noautodefrag isn't. nossd *is* a feature, but also a remount option... :o)

The mount manpage displays the values as a choice between 3 exclusive
options: ssd|nossd|ssd_spread

They're like an increasing level of magic that is being applied:
nossd < ssd < ssd_spread

So, that documentation with the | makes me think: I have to choose either one. But that's not how it behaves, since some of them can appear

But don't listen to me, I don't know what the best thing is.

Quoted 9 lines

Quoted 5 lines
Quoted 2 lines
Not sure if there's much point. In any case, that's a separate patch. Should I add one while we're here?
Since the whole ssd thing is a bit of a joke actually, I'd rather see it replaces with an option to choose an extent allocator algorithm.
Yeah, SSD is not the shiny new tech anymore, so we'd need something more future proof.

Quoted 7 lines

Quoted 3 lines
The amount of if statements using this SSD things in btrfs in the kernel can be counted on one hand, and what they actually do is quite questionable (food for another mail thread).
That's right, do you have suggestions for futher SSD optimizations? Other than better block alignment, faster flushes and no seek penalty, I don't see much else.

Yes, I'd like to start a discussion about that, but not buried in this thread, and it's not about SSDs, but about a larger filesystem than the average desktop computer and trade-offs between free space fragmentation (going ENOSPC when 30 of your 40TiB is in use...) and metadata write amplification (smaller writes leading to more cow, and, "let's track extent tree storage inside the extent tree", which causes huge avalanches of writing and writing and writing metadata with nossd).

And currently it's the ssd options that are influencing this a bit. But, it doesn't make sense to punish people with a slow rotating drive with things like having to write 40GiB of metadata when you feed 1 GiB of data to balance...

But, more about that later, otherwise this is going to look too much like a rant.

Signature · 1 line

Hans van Kranenburg

David Sterba

2017-04-10 17:35 UTC #10 lore raw

Re: [PATCH v2] btrfs: drop the nossd flag when remounting with -o ssd

Quoted 27 lines · Hans van Kranenburg

Quoted 12 lines · David Sterba
Quoted 4 lines · Hans van Kranenburg
Quoted 2 lines
Quoted 2 lines
Adding the 'nossd_spread' would be good to have, even if it might be just a marginal usecase.
Please no, don't make it more complex if not needed.
The only use is when ssd,ssd_spread are on and then I'd just want to disable ssd_spread, without disabling ssd at the same time.
1. mount -o ssd,ssd_spread
2. mount -o remount,nossd_spread
Quoted 8 lines
compared to
1. mount -o ssd,ssd_spread
2. mount -o remount,nossd
3. mount -o remount,ssd
I'd vote for adding nossd_spread, as the 'no-' options are common and otherwise disabling ssd_spread would be another usage exception.
Yes, 'nossd_spread' would intuitively be the thing to try to get rid of 'ssd_spread' on a mounted fs. But, nossd_spread is not a feature, just like noautodefrag isn't. nossd *is* a feature, but also a remount option... :o)

Quoted 8 lines

The mount manpage displays the values as a choice between 3 exclusive
options: ssd|nossd|ssd_spread
They're like an increasing level of magic that is being applied:
nossd < ssd < ssd_spread
So, that documentation with the | makes me think: I have to choose either one. But that's not how it behaves, since some of them can appear

The documentation groups options by functionality, the "|" makes it indeed confusing as exclusive options in this case. The rendering is not consistent, I have "|" and also ",". I hope that the text should explain more than could fit into the brief option list and I'd like to avoid complicating the syntax.

Quoted 1 line

But don't listen to me, I don't know what the best thing is.

Neither do I, so we talk about various aspects and update code or documentation if we can make it more clear. I appreciate your feedback.

David Sterba

2017-04-03 12:26 UTC #11 lore raw

Re: [PATCH v2] btrfs: drop the nossd flag when remounting with -o ssd

In reply to Adam Borowski

Quoted 4 lines · Adam Borowski

And when turning on nossd, drop ssd_spread.
Reported-by Hans van Kranenburg <[email protected]>
Signed-off-by Adam Borowski <[email protected]>

I've folded the two patches and queued for 4.11. Thanks.

Hans van Kranenburg

author

2017-04-15 19:12 UTC #12 lore raw

Re: [PATCH] btrfs: drop the nossd flag when remounting with -o ssd

In reply to David Sterba

Quoted 10 lines · David Sterba

Quoted 8 lines · Hans van Kranenburg
Quoted 7 lines · Adam Borowski
The opposite case was already handled right in the very next switch entry.
Reported-by Hans van Kranenburg <[email protected]>
Signed-off-by Adam Borowski <[email protected]>
---
Not sure if setting NOSSD should also disable SSD_SPREAD, there's currently no way to disable that option once set.

Quoted 5 lines

Missing inverse of ssd_spread is probably unintentional, as we once added all complementary no* options, this one was forgotten.
And yes, nossd should turn off ssd and ssd_spread, as ssd_spread without ssd does not nothing anyway.

ssd_spread is tested exactly one time, in free-space-cache.c in btrfs_find_space_cluster

As far as I can quickly find out, that code is reached regardless of which other options are set.

So, it *does* something anyway if ssd is not set. And I think it prevents all writes from being split into multiple extents.

So, nossd,ssd_spread (no ssd) is right now something that you can end up with (when the patch here is not applied) when playing with -o remount.

Quoted 67 lines

Quoted 38 lines
Quoted 13 lines
 fs/btrfs/super.c | 2 ++
 1 file changed, 2 insertions(+)
fs/btrfs/super.c +0 -0
index 06bd9b332e18..7342399951ad 100644
@@ -549,11 +549,13 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 		case Opt_ssd:
 			btrfs_set_and_info(info, SSD,
 					   "use ssd allocation scheme");
fs/btrfs/super.c int btrfs_parse_options(struct btrfs_fs_info *info, char *options, · line 561
Quoted 10 lines
fs/btrfs/super.c +2 -0
+			btrfs_clear_opt(info->mount_opt, NOSSD);
 			break;
 		case Opt_ssd_spread:
 			btrfs_set_and_info(info, SSD_SPREAD,
 					   "use spread ssd allocation scheme");
 			btrfs_set_opt(info->mount_opt, SSD);
+			btrfs_clear_opt(info->mount_opt, NOSSD);
 			break;
 		case Opt_nossd:
 			btrfs_set_and_info(info, NOSSD,
How did you test this?
This was also my first thought, but here's a weird thing:
-# mount -o nossd /dev/sdx /mnt/btrfs/
BTRFS info (device sdx): not using ssd allocation scheme
-# mount -o remount,ssd /mnt/btrfs/
BTRFS info (device sdx): use ssd allocation scheme
-# mount -o remount,nossd /mnt/btrfs/
Quoted 8 lines
BTRFS info (device sdx): use ssd allocation scheme
That means that the case Opt_nossd: is never reached when doing this?
And... what should be the result of doing:
-# mount -o remount,nossd,ssd /mnt/btrfs/
I guess it should be that the last one in the sequence wins?
The last one wins.
Quoted 2 lines
The fact that nossd,ssd,ssd_spread are different options complicates the whole thing, compared to e.g. autodefrag, noautodefrag.
I think the the ssd flags reflect the autodetection of ssd, unlike autodefrag and others.
The ssd options says "enable the ssd mode", but it could be also auto-detected if the non-rotational device is detected.
nossd says, "do not do the autodetection, even if it's a non-rot device, also disable all ssd modes".
The manual page is not entirely clear about that, I'll update it.
So Adam's patch needs to be updated so NOSSD also disables SSD_SPREAD. Adding the 'nossd_spread' would be good to have, even if it might be just a marginal usecase.

Signature · 1 line

Hans van Kranenburg