Are the btrfs mount options inconsistent?

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

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.

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

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

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

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

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

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).

There is a real effect.

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

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

What a shame...

Thanks,
Qu

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

David

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

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

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

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.

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.

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.

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

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 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.

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.

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.

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.

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