btrfs: drop the nossd flag when remounting with -o ssd
diff mbox

Message ID 20170331151904.4091-1-kilobyte@angband.pl
State Superseded
Headers show

Commit Message

Adam Borowski March 31, 2017, 3:19 p.m. UTC
The opposite case was already handled right in the very next switch entry.

Reported-by: Hans van Kranenburg <hans.van.kranenburg@mendix.com>
Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
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(+)

Comments

Hans van Kranenburg March 31, 2017, 4 p.m. UTC | #1
On 03/31/2017 05:19 PM, Adam Borowski wrote:
> The opposite case was already handled right in the very next switch entry.
> 
> Reported-by: Hans van Kranenburg <hans.van.kranenburg@mendix.com>
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>
> ---
> 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(+)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 06bd9b332e18..7342399951ad 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -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,

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.
David Sterba March 31, 2017, 5:10 p.m. UTC | #2
On Fri, Mar 31, 2017 at 06:00:08PM +0200, Hans van Kranenburg wrote:
> On 03/31/2017 05:19 PM, Adam Borowski wrote:
> > The opposite case was already handled right in the very next switch entry.
> > 
> > Reported-by: Hans van Kranenburg <hans.van.kranenburg@mendix.com>
> > Signed-off-by: Adam Borowski <kilobyte@angband.pl>
> > ---
> > 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.

> > 
> >  fs/btrfs/super.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> > index 06bd9b332e18..7342399951ad 100644
> > --- a/fs/btrfs/super.c
> > +++ b/fs/btrfs/super.c
> > @@ -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,
> 
> 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 last one wins.

> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans van Kranenburg April 15, 2017, 7:12 p.m. UTC | #3
On 03/31/2017 07:10 PM, David Sterba wrote:
> On Fri, Mar 31, 2017 at 06:00:08PM +0200, Hans van Kranenburg wrote:
>> On 03/31/2017 05:19 PM, Adam Borowski wrote:
>>> The opposite case was already handled right in the very next switch entry.
>>>
>>> Reported-by: Hans van Kranenburg <hans.van.kranenburg@mendix.com>
>>> Signed-off-by: Adam Borowski <kilobyte@angband.pl>
>>> ---
>>> 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.

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.

> 
>>>
>>>  fs/btrfs/super.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>> index 06bd9b332e18..7342399951ad 100644
>>> --- a/fs/btrfs/super.c
>>> +++ b/fs/btrfs/super.c
>>> @@ -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,
>>
>> 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 last one wins.
> 
>> 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.
>

Patch
diff mbox

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 06bd9b332e18..7342399951ad 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -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,