Are the btrfs mount options inconsistent?

Are the btrfs mount options inconsistent?

[David Howells]

Hi Chris,

I'm trying to convert btrfs to use the new mount API stuff and I'm finding it
hard to work out the relationships between some of the arguments, specifically
datacow, datasum and compress*.

What I see is that enabling datasum implies enabling datacow and that
disabling datacow implies disabling datasum - which seems reasonable.

However, selecting compression implies enabling datacow and datasum, and
disabling datacow, as one might expect, disables compression - but disabling
datasum does not.  Is that correct?

David

Re: Are the btrfs mount options inconsistent?

[David Sterba]

On Thu, Aug 16, 2018 at 12:01:25PM +0100, David Howells wrote:
> I'm trying to convert btrfs to use the new mount API stuff and I'm finding it
> hard to work out the relationships between some of the arguments, specifically
> datacow, datasum and compress*.
> 
> What I see is that enabling datasum implies enabling datacow and that
> disabling datacow implies disabling datasum - which seems reasonable.
> 
> However, selecting compression implies enabling datacow and datasum, and
> disabling datacow, as one might expect, disables compression - but disabling
> datasum does not.  Is that correct?

No it's not. Compression needs the checksums so nodatasum should disable
compression, which is missing as you found out.

This invalid combination also causes some problems during device
replace, but this was fixed in 4.18, so the missing part are the mount
options.

Re: Are the btrfs mount options inconsistent?

[David Howells]

David Sterba <dsterba@suse.cz> wrote:

> No it's not. Compression needs the checksums so nodatasum should disable
> compression, which is missing as you found out.

Thanks.

Btw, do fs_info->mount_opt end up inscribed on disk as is?  I don't see
anywhere this obviously happens.

Can I get rid of BTRFS_MOUNT_NOSSD as it would appear to be superfluous with
BTRFS_MOUNT_SSD?

David

Re: Are the btrfs mount options inconsistent?

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 917 bytes --]



On 2018/8/20 下午8:24, David Howells wrote:
> David Sterba <dsterba@suse.cz> wrote:
> 
>> No it's not. Compression needs the checksums so nodatasum should disable
>> compression, which is missing as you found out.
> 
> Thanks.
> 
> Btw, do fs_info->mount_opt end up inscribed on disk as is?  I don't see
> anywhere this obviously happens.

mount_opt should only reflects runtime mount options, and currently
there is no way to store mount option on-disk.

> 
> Can I get rid of BTRFS_MOUNT_NOSSD as it would appear to be superfluous with
> BTRFS_MOUNT_SSD?

As you already found.

But to be more clear, NOSSD shouldn't be a special case.
In fact currently NOSSD only affects whether we will output the message
"enabling ssd optimization", no real effect if I didn't miss anything.

So the the states should be only 3: nossd, ssd, ssd + ssd_spread.

Thanks,
Qu


> 
> David
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Are the btrfs mount options inconsistent?

[David Howells]

Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:

> But to be more clear, NOSSD shouldn't be a special case.
> In fact currently NOSSD only affects whether we will output the message
> "enabling ssd optimization", no real effect if I didn't miss anything.

That's not quite true.  In:

	if (!btrfs_test_opt(fs_info, NOSSD) &&
	    !fs_info->fs_devices->rotating) {
		btrfs_set_and_info(fs_info, SSD, "enabling ssd optimizations");
	}

the call to btrfs_set_and_info() will turn on SSD.

What this seems to me is that, normally, SSD will be turned on automatically
unless at least one of the devices is a rotating medium - but this appears to
be explicitly suppressed by the NOSSD option.

David

Re: Are the btrfs mount options inconsistent?

[Austin S. Hemmelgarn]

On 2018-08-21 09:43, David Howells wrote:
> Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> 
>> But to be more clear, NOSSD shouldn't be a special case.
>> In fact currently NOSSD only affects whether we will output the message
>> "enabling ssd optimization", no real effect if I didn't miss anything.
> 
> That's not quite true.  In:
> 
> 	if (!btrfs_test_opt(fs_info, NOSSD) &&
> 	    !fs_info->fs_devices->rotating) {
> 		btrfs_set_and_info(fs_info, SSD, "enabling ssd optimizations");
> 	}
> 
> the call to btrfs_set_and_info() will turn on SSD.
> 
> What this seems to me is that, normally, SSD will be turned on automatically
> unless at least one of the devices is a rotating medium - but this appears to
> be explicitly suppressed by the NOSSD option.
That's my understanding too (though I may be wrong, I'm not an expert on C).

If this _isn't_ what's happening, then it needs to be changed so it is, 
that's what the documentation has pretty much always said, and is 
therefore how people expect it to work (also, it needs to work because 
there needs to be an option other than poking around at sysfs attributes 
to disable this on non-rotational media where it's not want4ed).

Re: Are the btrfs mount options inconsistent?

[David Sterba]

On Tue, Aug 21, 2018 at 02:43:35PM +0100, David Howells wrote:
> Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> 
> > But to be more clear, NOSSD shouldn't be a special case.
> > In fact currently NOSSD only affects whether we will output the message
> > "enabling ssd optimization", no real effect if I didn't miss anything.

There is a real effect.

> That's not quite true.  In:
> 
> 	if (!btrfs_test_opt(fs_info, NOSSD) &&
> 	    !fs_info->fs_devices->rotating) {
> 		btrfs_set_and_info(fs_info, SSD, "enabling ssd optimizations");
> 	}
> 
> the call to btrfs_set_and_info() will turn on SSD.
> 
> What this seems to me is that, normally, SSD will be turned on automatically
> unless at least one of the devices is a rotating medium - but this appears to
> be explicitly suppressed by the NOSSD option.

Right. So expected behaviour:

- nothing: auto-detect non-rotating devices, enable SSD mount option in turn
- nossd: disable auto-detection of non-rotating devices
- ssd: enable SSD optimizations uconditionally
- ssd_spread: implies SSD and affects some allocator decisions regarding
              new extent alignments

Re: Are the btrfs mount options inconsistent?

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1367 bytes --]



On 2018/8/21 下午10:24, David Sterba wrote:
> On Tue, Aug 21, 2018 at 02:43:35PM +0100, David Howells wrote:
>> Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>> But to be more clear, NOSSD shouldn't be a special case.
>>> In fact currently NOSSD only affects whether we will output the message
>>> "enabling ssd optimization", no real effect if I didn't miss anything.
> 
> There is a real effect.
> 
>> That's not quite true.  In:
>>
>> 	if (!btrfs_test_opt(fs_info, NOSSD) &&
>> 	    !fs_info->fs_devices->rotating) {
>> 		btrfs_set_and_info(fs_info, SSD, "enabling ssd optimizations");
>> 	}
>>
>> the call to btrfs_set_and_info() will turn on SSD.

Oh, I just missed the btrfs_set part. (even it's myself introduced that
function).

What a shame...

Thanks,
Qu

>>
>> What this seems to me is that, normally, SSD will be turned on automatically
>> unless at least one of the devices is a rotating medium - but this appears to
>> be explicitly suppressed by the NOSSD option.
> 
> Right. So expected behaviour:
> 
> - nothing: auto-detect non-rotating devices, enable SSD mount option in turn
> - nossd: disable auto-detection of non-rotating devices
> - ssd: enable SSD optimizations uconditionally
> - ssd_spread: implies SSD and affects some allocator decisions regarding
>               new extent alignments
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Are the btrfs mount options inconsistent?

[David Howells]

David Sterba <dsterba@suse.cz> wrote:

> - nothing: auto-detect non-rotating devices, enable SSD mount option in turn

And it's right that this should only turn on SSD if *all* the devices are
non-rotating?

David

Re: Are the btrfs mount options inconsistent?

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 556 bytes --]



On 2018/8/21 下午10:35, David Howells wrote:
> David Sterba <dsterba@suse.cz> wrote:
> 
>> - nothing: auto-detect non-rotating devices, enable SSD mount option in turn
> 
> And it's right that this should only turn on SSD if *all* the devices are
> non-rotating?

If I didn't miss anything again, yes.

It's btrfs_open_one_device() who updates fs_devices->rotating, and any
device with rotating disk will set it to 1.
So it needs all device to to non-rotating to meet
!fs_info->fs_devices->rotating.

Thanks,
Qu

> 
> David
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Are the btrfs mount options inconsistent?

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> Can I get rid of BTRFS_MOUNT_NOSSD as it would appear to be superfluous with
> BTRFS_MOUNT_SSD?

Ah...  I guess it's not quite superfluous:

	if (!btrfs_test_opt(fs_info, NOSSD) &&
	    !fs_info->fs_devices->rotating) {
		btrfs_set_and_info(fs_info, SSD, "enabling ssd optimizations");
	}

It appears to be more of a four-state thing: definitely no, maybe no, yes and
yes+spread.

David

Do btrfs compression option changes need to be atomic?

[David Howells]

Should changes to the compression options on a btrfs mount be atomic, the
problem being that they're split across three variables?

Further to that, how much of an issue is it if the configuration is split out
into its own struct that is accessed from struct btrfs_fs_info using RCU?

David

Re: Do btrfs compression option changes need to be atomic?

[Chris Mason]

On 21 Aug 2018, at 9:46, David Howells wrote:

> Should changes to the compression options on a btrfs mount be atomic, 
> the
> problem being that they're split across three variables?
>
> Further to that, how much of an issue is it if the configuration is 
> split out
> into its own struct that is accessed from struct btrfs_fs_info using 
> RCU?
>

Hi David,

They shouldn't need to be atomic, we're making compression decisions on 
a per-extent basis and sometimes bailing on the compression if it 
doesn't make things smaller.

-chris

Re: Do btrfs compression option changes need to be atomic?

[David Sterba]

On Tue, Aug 21, 2018 at 02:46:30PM +0100, David Howells wrote:
> Should changes to the compression options on a btrfs mount be atomic, the
> problem being that they're split across three variables?

Do you mean the compression type (btrfs_fs_info::compress_type) and the
related bits in btrfs_fs_info::mount_opt ? There are more compression
types but not used in the mount context.  I assume you're interested
only in the mount-time settings, otherwise the defrag and per-inode
compression has higher priority over the global settings.

When an extent is going to be compressed, the current value of
compression type is read and then passed around.

> Further to that, how much of an issue is it if the configuration is split out
> into its own struct that is accessed from struct btrfs_fs_info using RCU?

Depends on how intrusive it's going to be, the mount opions are tested
at many places. The RCU overhead and "locking" is lightweight enough so
it should not be a problem in principle, but without seeing the code I
can't tell.

Re: Do btrfs compression option changes need to be atomic?

[David Howells]

David Sterba <dsterba@suse.cz> wrote:

> Do you mean the compression type (btrfs_fs_info::compress_type) and the
> related bits in btrfs_fs_info::mount_opt ? There are more compression
> types but not used in the mount context.  I assume you're interested
> only in the mount-time settings, otherwise the defrag and per-inode
> compression has higher priority over the global settings.

Yes.  However, it would appear that remount can race with using the fs_info
variables, so the settings can be changing whilst you're going through the
compress_file_range() function.

I'm not sure it'll hurt exactly, but you can also find, say, DATACOW being
disabled whilst you're considering compressing.

> > Further to that, how much of an issue is it if the configuration is split
> > out into its own struct that is accessed from struct btrfs_fs_info using
> > RCU?
> 
> Depends on how intrusive it's going to be, the mount opions are tested
> at many places. The RCU overhead and "locking" is lightweight enough so
> it should not be a problem in principle, but without seeing the code I
> can't tell.

Actually, a better way of doing this might be to put a subset of the settings
into a single variable, say:

	struct btrfs_fs_info {
		...
		unsigned int	data_storage_opt;
	#define BTRFS_DATA_NONE			0x0000
	#define BTRFS_DATA_COW			0x0001
	#define BTRFS_DATA_SUM			0x0002
	#define BTRFS_DATA_COMPRESS		0x0003
	#define BTRFS_DATA_FORCE_COMPRESS	0x0004
	#define BTRFS_DATA__STORAGE_OPT		0x000f
	#define BTRFS_DATA_COMPRESS_ZLIB	0x0000
	#define BTRFS_DATA_COMPRESS_LZO		0x0010
	#define BTRFS_DATA_COMPRESS_ZSTD	0x0020
	#define BTRFS_DATA__COMPRESS_TYPE	0x00f0
	#define BTRFS_DATA__COMPRESS_LEVEL	0x0f00
		...
	};

That way it might be possible for the datacow, datasum, compress and
compress-force options to be handled atomically.

David

Re: Do btrfs compression option changes need to be atomic?

[David Sterba]

On Tue, Aug 21, 2018 at 03:34:06PM +0100, David Howells wrote:
> David Sterba <dsterba@suse.cz> wrote:
> 
> > Do you mean the compression type (btrfs_fs_info::compress_type) and the
> > related bits in btrfs_fs_info::mount_opt ? There are more compression
> > types but not used in the mount context.  I assume you're interested
> > only in the mount-time settings, otherwise the defrag and per-inode
> > compression has higher priority over the global settings.
> 
> Yes.  However, it would appear that remount can race with using the fs_info
> variables, so the settings can be changing whilst you're going through the
> compress_file_range() function.

compress_file_range reads the compress_type at the beginning, so remount
does not affect that and the function will continue using that for the
extent. This is racy in the sense that there's some compression going on
while the mount options do not say so.

I think this is not a big deal, there's sync_filesystem call at the
beginning of remount, so most of the data use the previous settings.
The window where new IO starts but remount has not updated the mount
options is short, so yes this is racy though I'm not sure if there's a
good way to actually measure that. The result is that eg. some extents
got compressed but should not have been.

The remount would have to flush IO and freeze all new, then change
options and thaw, but this starts to sound too heavyweight.

> I'm not sure it'll hurt exactly, but you can also find, say, DATACOW being
> disabled whilst you're considering compressing.

I think the constraint don't need to be strictly atomic, with some
intermediate states that are valid but neither the old nor what the new
options say. The race window spans a few instructions so as long as the
combination is valid or double checked later, I'd consider it ok.

The ordering of the bits set/cleared can prevent unwanted combinations,
like: when the COMPRESS bit is set, there's no NODATACOW anymore:

- clear NODATACOW
- set compress_type
- set COMPRESS

I haven't checked with the sources if this is so.

> > > Further to that, how much of an issue is it if the configuration is split
> > > out into its own struct that is accessed from struct btrfs_fs_info using
> > > RCU?
> > 
> > Depends on how intrusive it's going to be, the mount opions are tested
> > at many places. The RCU overhead and "locking" is lightweight enough so
> > it should not be a problem in principle, but without seeing the code I
> > can't tell.
> 
> Actually, a better way of doing this might be to put a subset of the settings
> into a single variable, say:
> 
> 	struct btrfs_fs_info {
> 		...
> 		unsigned int	data_storage_opt;
> 	#define BTRFS_DATA_NONE			0x0000
> 	#define BTRFS_DATA_COW			0x0001
> 	#define BTRFS_DATA_SUM			0x0002
> 	#define BTRFS_DATA_COMPRESS		0x0003
> 	#define BTRFS_DATA_FORCE_COMPRESS	0x0004
> 	#define BTRFS_DATA__STORAGE_OPT		0x000f
> 	#define BTRFS_DATA_COMPRESS_ZLIB	0x0000
> 	#define BTRFS_DATA_COMPRESS_LZO		0x0010
> 	#define BTRFS_DATA_COMPRESS_ZSTD	0x0020
> 	#define BTRFS_DATA__COMPRESS_TYPE	0x00f0
> 	#define BTRFS_DATA__COMPRESS_LEVEL	0x0f00
> 		...
> 	};
> 
> That way it might be possible for the datacow, datasum, compress and
> compress-force options to be handled atomically.

So the changes would be possible in one atomic (bitwise) call? Instead
of:

  clear_bit(opts, NODATACOW)
  set_bit(opts, COMPRESS)

do:

  set_bits_with_mask(opts, COMPRESS, BTRFS_DATA__STORAGE_OPT)

?

Side note: up to now we're all used to the NO* naming and semantics of
the feature bits, so it might take some time to do the mental switch.

Re: Do btrfs compression option changes need to be atomic?

[David Howells]

David Sterba <dsterba@suse.cz> wrote:

> The ordering of the bits set/cleared can prevent unwanted combinations,
> like: when the COMPRESS bit is set, there's no NODATACOW anymore:
> 
> - clear NODATACOW
> - set compress_type
> - set COMPRESS
> 
> I haven't checked with the sources if this is so.

The current code doesn't use memory barriers or atomic ops, so you must assume
that those memory operations can be reordered/merged by the compiler or the
cpu.

> So the changes would be possible in one atomic (bitwise) call? Instead
> of:
> 
>   clear_bit(opts, NODATACOW)
>   set_bit(opts, COMPRESS)
> 
> do:
> 
>   set_bits_with_mask(opts, COMPRESS, BTRFS_DATA__STORAGE_OPT)

After the mount API changes, the value for the variable will be composed in a
separate context and then applied with a single assignment after printing any
messages about changes in active options.

> Side note: up to now we're all used to the NO* naming and semantics of
> the feature bits, so it might take some time to do the mental switch.

Yeah - I've actually switched them all to positive naming in what I have so
far as it simplifies the code and makes it easier to think about (for me at
least).

David