[PATCH v2] btrfs-progs: add warning for mixed profiles filesystem

[PATCH v2] btrfs-progs: add warning for mixed profiles filesystem

[Goffredo Baroncelli]

Hi all,

the aim of this patch set is to issue a warning when a mixed profiles
filesystem is detected. This happens when the filesystems contains
(i.e.) raid1c3 and single chunk for data.

BTRFS has the capability to support a filesystem with mixed profiles.
However this could lead to an unexpected behavior when a new chunk is
allocated (i.e. the chunk profile is not what is wanted). Moreover
if the user is not aware of this, he could assume a redundancy which
doesn't exist (for example because there is some 'single' chunk when
it is expected the filesystem to be full raid1).
A possible cause of a mixed profiles filesystem is an interrupted
balance operation or a not fully balance due to very specific filter.

The check is added to the following btrfs commands:
- btrfs balance pause
- btrfs balance cancel
- btrfs filesystem usage
- btrfs device add

Suggestion about which commands should (not) have this check are 
welcome.

v1 
- first issue
v2 
- add some needed missing pieces about raid1c[34]
- add the check to more btrfs commands

Example of output:

$ sudo ./btrfs fi us /tmp/t/
WARNING: cannot read detailed chunk info, per-device usage will not be shown, run as root
Overall:
    Device size:		  30.00GiB
    Device allocated:		   7.78GiB
    Device unallocated:		  22.22GiB
    Device missing:		     0.00B
    Used:			   1.94GiB
    Free (estimated):		  11.89GiB	(min: 9.77GiB)
    Data ratio:			      2.33
    Metadata ratio:		      1.50
    Global reserve:		   3.25MiB	(used: 0.00B)

Data,single: Size:1.00GiB, Used:973.87MiB (95.10%)

Data,RAID1C3: Size:2.00GiB, Used:178.98MiB (8.74%)

Metadata,single: Size:256.00MiB, Used:94.69MiB (36.99%)

Metadata,RAID1: Size:256.00MiB, Used:188.45MiB (73.61%)

System,single: Size:32.00MiB, Used:16.00KiB (0.05%)

WARNING: ------------------------------------------------------
WARNING: Detection of multiple profiles for a block group type:
WARNING:
WARNING: * DATA ->          [raid1c3, single]
WARNING: * METADATA ->      [raid1, single]
WARNING:
WARNING: Please consider using 'btrfs balance ...' commands set
WARNING: to solve this issue.
WARNING: ------------------------------------------------------


In this case there are two kind of chunks for data (raid1c3 and single)
and metadata (raid1, single).

Patch #1 and #2 are preparatory ones.
Patch #3 contains the code for the check.
Patch #4 adds the check to the command 'btrfs fi us'

Comments are welcome

BR
G.Baroncelli

-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

[PATCH 1/4] Complete the implementation of RAID1C[34].

[Goffredo Baroncelli]

From: Goffredo Baroncelli <kreijack@inwind.it>

- Complete the function btrfs_err_str adding some missing cases.
- Sync the enum btrfs_err_code (in ibbtrfsutil/btrfs.h) with the
rest of the codes (user space and kernel space).
- Add missing fields to btrfs_raid_array[] for raid1c[34]

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 ioctl.h              | 4 ++++
 libbtrfsutil/btrfs.h | 4 +++-
 volumes.c            | 6 ++++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/ioctl.h b/ioctl.h
index 4e7efd94..ade6dcb9 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -798,6 +798,10 @@ static inline char *btrfs_err_str(enum btrfs_err_code err_code)
 	switch (err_code) {
 		case BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET:
 			return "unable to go below two devices on raid1";
+		case BTRFS_ERROR_DEV_RAID1C3_MIN_NOT_MET:
+			return "unable to go below three devices on raid1c3";
+		case BTRFS_ERROR_DEV_RAID1C4_MIN_NOT_MET:
+			return "unable to go below four devices on raid1c4";
 		case BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET:
 			return "unable to go below four devices on raid10";
 		case BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET:
diff --git a/libbtrfsutil/btrfs.h b/libbtrfsutil/btrfs.h
index 4bc12754..60d51ff6 100644
--- a/libbtrfsutil/btrfs.h
+++ b/libbtrfsutil/btrfs.h
@@ -830,7 +830,9 @@ enum btrfs_err_code {
 	BTRFS_ERROR_DEV_TGT_REPLACE,
 	BTRFS_ERROR_DEV_MISSING_NOT_FOUND,
 	BTRFS_ERROR_DEV_ONLY_WRITABLE,
-	BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS
+	BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS,
+	BTRFS_ERROR_DEV_RAID1C3_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_RAID1C4_MIN_NOT_MET,
 };
 
 #define BTRFS_IOC_SNAP_CREATE _IOW(BTRFS_IOCTL_MAGIC, 1, \
diff --git a/volumes.c b/volumes.c
index b46bf598..7f84fbba 100644
--- a/volumes.c
+++ b/volumes.c
@@ -65,6 +65,9 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 2,
 		.devs_increment	= 3,
 		.ncopies	= 3,
+		.raid_name	= "raid1c3",
+		.bg_flag	= BTRFS_BLOCK_GROUP_RAID1C3,
+		.mindev_error	= BTRFS_ERROR_DEV_RAID1C3_MIN_NOT_MET,
 	},
 	[BTRFS_RAID_RAID1C4] = {
 		.sub_stripes	= 1,
@@ -74,6 +77,9 @@ const struct btrfs_raid_attr btrfs_raid_array[BTRFS_NR_RAID_TYPES] = {
 		.tolerated_failures = 3,
 		.devs_increment	= 4,
 		.ncopies	= 4,
+		.raid_name	= "raid1c4",
+		.bg_flag	= BTRFS_BLOCK_GROUP_RAID1C4,
+		.mindev_error	= BTRFS_ERROR_DEV_RAID1C4_MIN_NOT_MET,
 	},
 	[BTRFS_RAID_DUP] = {
 		.sub_stripes	= 1,
-- 
2.26.0

[PATCH 2/4] btrfs-progs: Add BTRFS_EXTENDED_PROFILE_MASK mask.

[Goffredo Baroncelli]

From: Goffredo Baroncelli <kreijack@inwind.it>

Add BTRFS_EXTENDED_PROFILE_MASK to consider also the
BTRFS_AVAIL_ALLOC_BIT_SINGLE bit.

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 ctree.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ctree.h b/ctree.h
index 691481bc..9d69fa94 100644
--- a/ctree.h
+++ b/ctree.h
@@ -1005,6 +1005,9 @@ enum btrfs_raid_types {
 /* used in struct btrfs_balance_args fields */
 #define BTRFS_AVAIL_ALLOC_BIT_SINGLE	(1ULL << 48)
 
+#define BTRFS_EXTENDED_PROFILE_MASK	(BTRFS_BLOCK_GROUP_PROFILE_MASK | \
+					 BTRFS_AVAIL_ALLOC_BIT_SINGLE)
+
 /*
  * GLOBAL_RSV does not exist as a on-disk block group type and is used
  * internally for exporting info about global block reserve from space infos
-- 
2.26.0

[PATCH 3/4] btrfs-progs: Add btrfs_check_for_mixed_profiles_by_* function

[Goffredo Baroncelli]

From: Goffredo Baroncelli <kreijack@inwind.it>

Show a warning if a mixed profiles filesystem
is detected.

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 common/utils.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++++
 common/utils.h |   3 ++
 2 files changed, 129 insertions(+)

diff --git a/common/utils.c b/common/utils.c
index a7761683..e3f18709 100644
--- a/common/utils.c
+++ b/common/utils.c
@@ -1710,3 +1710,129 @@ void print_all_devices(struct list_head *devices)
 		print_device_info(dev, "\t");
 	printf("\n");
 }
+
+static int bit_count(u64 x)
+{
+	int ret = 0;
+
+	while (x) {
+		if (x & 1)
+			ret++;
+		x >>= 1;
+	}
+	return ret;
+}
+
+static void print_profiles(FILE *out, u64 profiles)
+{
+	int i;
+	int first = true;
+
+	for (i = 0 ; i < BTRFS_NR_RAID_TYPES ; i++) {
+		if (!(btrfs_raid_array[i].bg_flag & profiles))
+			continue;
+
+		if (!first)
+			fprintf(out, ", ");
+		fprintf(out, "%s", btrfs_raid_array[i].raid_name);
+		first = false;
+	}
+	if (profiles & BTRFS_AVAIL_ALLOC_BIT_SINGLE) {
+		if (!first)
+			fprintf(out, ", ");
+		fprintf(out, "%s",
+			btrfs_raid_array[BTRFS_RAID_SINGLE].raid_name);
+	}
+}
+
+int btrfs_check_for_mixed_profiles_by_path(char *path)
+{
+	int fd;
+	int ret;
+	DIR *dirstream;
+
+	fd = btrfs_open_dir(path, &dirstream, 0);
+	if (fd < 0)
+		return -1;
+	closedir(dirstream);
+
+	ret = btrfs_check_for_mixed_profiles_by_fd(fd);
+	close(fd);
+
+	return ret;
+}
+
+int btrfs_check_for_mixed_profiles_by_fd(int fd)
+{
+	int ret;
+	int i;
+	struct btrfs_ioctl_space_args *sargs;
+	u64 data_profiles = 0;
+	u64 metadata_profiles = 0;
+	u64 system_profiles = 0;
+	u64 mixed_profiles = 0;
+	static const u64 mixed_profile_fl = BTRFS_BLOCK_GROUP_METADATA |
+		BTRFS_BLOCK_GROUP_DATA;
+
+	ret = get_df(fd, &sargs);
+	if (ret < 0)
+		return -1;
+
+	for (i = 0 ; i < sargs->total_spaces ; i++) {
+		u64 flags = sargs->spaces[i].flags;
+
+		if (!(flags & BTRFS_BLOCK_GROUP_PROFILE_MASK))
+			flags |= BTRFS_AVAIL_ALLOC_BIT_SINGLE;
+
+		if ((flags & mixed_profile_fl) == mixed_profile_fl)
+			mixed_profiles |= flags;
+		else if (flags & BTRFS_BLOCK_GROUP_DATA)
+			data_profiles |= flags;
+		else if (flags & BTRFS_BLOCK_GROUP_METADATA)
+			metadata_profiles |= flags;
+		else if (flags & BTRFS_BLOCK_GROUP_SYSTEM)
+			system_profiles |= flags;
+	}
+	free(sargs);
+
+	data_profiles &= BTRFS_EXTENDED_PROFILE_MASK;
+	system_profiles &= BTRFS_EXTENDED_PROFILE_MASK;
+	mixed_profiles &= BTRFS_EXTENDED_PROFILE_MASK;
+	metadata_profiles &= BTRFS_EXTENDED_PROFILE_MASK;
+
+	if ((bit_count(data_profiles) <= 1) &&
+	    (bit_count(metadata_profiles) <= 1) &&
+	    (bit_count(system_profiles) <= 1) &&
+	    (bit_count(mixed_profiles) <= 1))
+		return 0;
+
+	fprintf(stderr, "WARNING: ------------------------------------------------------\n");
+	fprintf(stderr, "WARNING: Detection of multiple profiles for a block group type:\n");
+	fprintf(stderr, "WARNING:\n");
+	if (bit_count(data_profiles) > 1) {
+		fprintf(stderr, "WARNING: * DATA ->          [");
+		print_profiles(stderr, data_profiles);
+		fprintf(stderr, "]\n");
+	}
+	if (bit_count(metadata_profiles) > 1) {
+		fprintf(stderr, "WARNING: * METADATA ->      [");
+		print_profiles(stderr, metadata_profiles);
+		fprintf(stderr, "]\n");
+	}
+	if (bit_count(mixed_profiles) > 1) {
+		fprintf(stderr, "WARNING: * DATA+METADATA -> [");
+		print_profiles(stderr, mixed_profiles);
+		fprintf(stderr, "]\n");
+	}
+	if (bit_count(system_profiles) > 1) {
+		fprintf(stderr, "WARNING: * SYSTEM ->        [");
+		print_profiles(stderr, system_profiles);
+		fprintf(stderr, "]\n");
+	}
+	fprintf(stderr, "WARNING:\n");
+	fprintf(stderr, "WARNING: Please consider using 'btrfs balance ...' commands set\n");
+	fprintf(stderr, "WARNING: to solve this issue.\n");
+	fprintf(stderr, "WARNING: ------------------------------------------------------\n");
+
+	return 1;
+}
diff --git a/common/utils.h b/common/utils.h
index 5c1afda9..662c9e38 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -137,4 +137,7 @@ u64 rand_u64(void);
 unsigned int rand_range(unsigned int upper);
 void init_rand_seed(u64 seed);
 
+int btrfs_check_for_mixed_profiles_by_path(char *path);
+int btrfs_check_for_mixed_profiles_by_fd(int fd);
+
 #endif
-- 
2.26.0

[PATCH 4/4] btrfs-progs: Add mixed profiles check to some btrfs sub-commands.

[Goffredo Baroncelli]

From: Goffredo Baroncelli <kreijack@inwind.it>

Add a check in some btrfs subcommands to detect if a filesystem
has mixed profiles for data/metadata/system. In this case
a warning is showed.

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 cmds/balance.c          | 2 ++
 cmds/device.c           | 3 +++
 cmds/filesystem-usage.c | 1 +
 cmds/filesystem.c       | 1 +
 4 files changed, 7 insertions(+)

diff --git a/cmds/balance.c b/cmds/balance.c
index 5392a604..20d0ebc1 100644
--- a/cmds/balance.c
+++ b/cmds/balance.c
@@ -716,6 +716,7 @@ static int cmd_balance_pause(const struct cmd_struct *cmd,
 			ret = 1;
 	}
 
+	btrfs_check_for_mixed_profiles_by_fd(fd);
 	close_file_or_dir(fd, dirstream);
 	return ret;
 }
@@ -756,6 +757,7 @@ static int cmd_balance_cancel(const struct cmd_struct *cmd,
 			ret = 1;
 	}
 
+	btrfs_check_for_mixed_profiles_by_fd(fd);
 	close_file_or_dir(fd, dirstream);
 	return ret;
 }
diff --git a/cmds/device.c b/cmds/device.c
index 24158308..d83f92a7 100644
--- a/cmds/device.c
+++ b/cmds/device.c
@@ -143,6 +143,7 @@ static int cmd_device_add(const struct cmd_struct *cmd,
 	}
 
 error_out:
+	btrfs_check_for_mixed_profiles_by_fd(fdmnt);
 	close_file_or_dir(fdmnt, dirstream);
 	return !!ret;
 }
@@ -225,6 +226,7 @@ static int _cmd_device_remove(const struct cmd_struct *cmd,
 		}
 	}
 
+	btrfs_check_for_mixed_profiles_by_fd(fdmnt);
 	close_file_or_dir(fdmnt, dirstream);
 	return !!ret;
 }
@@ -659,6 +661,7 @@ static int cmd_device_usage(const struct cmd_struct *cmd, int argc, char **argv)
 		}
 
 		ret = _cmd_device_usage(fd, argv[i], unit_mode);
+		btrfs_check_for_mixed_profiles_by_fd(fd);
 		close_file_or_dir(fd, dirstream);
 
 		if (ret)
diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
index aa7065d5..ce07d80f 100644
--- a/cmds/filesystem-usage.c
+++ b/cmds/filesystem-usage.c
@@ -1043,6 +1043,7 @@ static int cmd_filesystem_usage(const struct cmd_struct *cmd,
 		ret = print_filesystem_usage_by_chunk(fd, chunkinfo, chunkcount,
 				devinfo, devcount, argv[i], unit_mode, tabular);
 cleanup:
+		btrfs_check_for_mixed_profiles_by_fd(fd);
 		close_file_or_dir(fd, dirstream);
 		free(chunkinfo);
 		free(devinfo);
diff --git a/cmds/filesystem.c b/cmds/filesystem.c
index 4f22089a..c4bb13dd 100644
--- a/cmds/filesystem.c
+++ b/cmds/filesystem.c
@@ -111,6 +111,7 @@ static int cmd_filesystem_df(const struct cmd_struct *cmd,
 		error("get_df failed: %m");
 	}
 
+	btrfs_check_for_mixed_profiles_by_fd(fd);
 	close_file_or_dir(fd, dirstream);
 	return !!ret;
 }
-- 
2.26.0

Re: [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem

[Graham Cobb]

On 31/03/2020 20:10, Goffredo Baroncelli wrote:
> WARNING: ------------------------------------------------------
> WARNING: Detection of multiple profiles for a block group type:
> WARNING:
> WARNING: * DATA ->          [raid1c3, single]
> WARNING: * METADATA ->      [raid1, single]
> WARNING:
> WARNING: Please consider using 'btrfs balance ...' commands set
> WARNING: to solve this issue.
> WARNING: ------------------------------------------------------

The check is a good a idea but I think the warning is too strong. I
would prefer that the word "Warning" is reserved for cases and
operations that may actually damage data (such as reformating a
filesystem). [Note: in a previous job, my employer decided that the word
Warning was ONLY to be used if there was a risk of harm to a human - for
example, electrical safety]

Also, btrfs fi usage is something that I routinely run continuously in a
window (using watch) when a remove/replace/balance operation is in
progress to monitor at a glance what is happening - I don't want to
waste all that space on the screen. To say nothing of the annoyance of
having it shouting at me for weeks on end while **I AM TRYING TO FIX THE
DAMN PROBLEM!**.

I would suggest a more compact layout and factual tone. Something like:

Caution: This filesystem has multiple profiles for a block group type
so new block groups will have unpredictable profiles.
 * DATA ->          [raid1c3, single]
 * METADATA ->      [raid1, single]
Use of 'btrfs balance' is recommended as soon as possible to move all
blocks to a single profile for each of data and metadata.

Re: [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem

[Zygo Blaxell]

On Tue, Mar 31, 2020 at 10:46:17PM +0100, Graham Cobb wrote:
> On 31/03/2020 20:10, Goffredo Baroncelli wrote:
> > WARNING: ------------------------------------------------------
> > WARNING: Detection of multiple profiles for a block group type:
> > WARNING:
> > WARNING: * DATA ->          [raid1c3, single]
> > WARNING: * METADATA ->      [raid1, single]
> > WARNING:
> > WARNING: Please consider using 'btrfs balance ...' commands set
> > WARNING: to solve this issue.
> > WARNING: ------------------------------------------------------
> 
> The check is a good a idea but I think the warning is too strong. I
> would prefer that the word "Warning" is reserved for cases and
> operations that may actually damage data (such as reformating a
> filesystem). [Note: in a previous job, my employer decided that the word
> Warning was ONLY to be used if there was a risk of harm to a human - for
> example, electrical safety]
> 
> Also, btrfs fi usage is something that I routinely run continuously in a
> window (using watch) when a remove/replace/balance operation is in

I was going to say please put all the new output lines at the bottom,
so that 'watch' windows can be minimally sized without having to write
something like

	watch 'btrfs fi usage /foo | sed -e "g/WARNING:/d"'

People with short terminal windows running btrfs fi usage directly from
the command line would probably complain about extra lines at the bottom...

Another good idea here would be a --quiet switch, or
'--no-profile-warning'.

> progress to monitor at a glance what is happening - I don't want to
> waste all that space on the screen. To say nothing of the annoyance of
> having it shouting at me for weeks on end while **I AM TRYING TO FIX THE
> DAMN PROBLEM!**.
> 
> I would suggest a more compact layout and factual tone. Something like:
> 
> Caution: This filesystem has multiple profiles for a block group type
> so new block groups will have unpredictable profiles.
>  * DATA ->          [raid1c3, single]
>  * METADATA ->      [raid1, single]
> Use of 'btrfs balance' is recommended as soon as possible to move all
> blocks to a single profile for each of data and metadata.

How about a one-liner:

	NOTE: Multiple profiles detected.  See 'man btrfs-filesystem'.

with a section in the btrfs-filesystem man page giving a detailed
description of the problem and examples of possible remedies.

Re: [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem

[Graham Cobb]

On 31/03/2020 23:05, Zygo Blaxell wrote:
> On Tue, Mar 31, 2020 at 10:46:17PM +0100, Graham Cobb wrote:
>> On 31/03/2020 20:10, Goffredo Baroncelli wrote:
>>> WARNING: ------------------------------------------------------
>>> WARNING: Detection of multiple profiles for a block group type:
>>> WARNING:
>>> WARNING: * DATA ->          [raid1c3, single]
>>> WARNING: * METADATA ->      [raid1, single]
>>> WARNING:
>>> WARNING: Please consider using 'btrfs balance ...' commands set
>>> WARNING: to solve this issue.
>>> WARNING: ------------------------------------------------------
>>
...
>> I would suggest a more compact layout and factual tone. Something like:
>>
>> Caution: This filesystem has multiple profiles for a block group type
>> so new block groups will have unpredictable profiles.
>>  * DATA ->          [raid1c3, single]
>>  * METADATA ->      [raid1, single]
>> Use of 'btrfs balance' is recommended as soon as possible to move all
>> blocks to a single profile for each of data and metadata.
> 
> How about a one-liner:
> 
> 	NOTE: Multiple profiles detected.  See 'man btrfs-filesystem'.
> 
> with a section in the btrfs-filesystem man page giving a detailed
> description of the problem and examples of possible remedies.
> 

Even better. I have just realised that the information in the "* DATA ->
         [raid1c3, single]" part of the message is, of course, available
in the rest of the output so doesn't need to be included.

I would suggest including a very brief description of the problem, in
order to prompt people to actually take action. How about:

Caution: Multiple profiles detected which will cause unpredictable
profiles for new data.  See 'man btrfs-filesystem' for recommendations.

Re: [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem

[Goffredo Baroncelli]

On 4/1/20 12:05 AM, Zygo Blaxell wrote:
> On Tue, Mar 31, 2020 at 10:46:17PM +0100, Graham Cobb wrote:
>> On 31/03/2020 20:10, Goffredo Baroncelli wrote:
>>> WARNING: ------------------------------------------------------
>>> WARNING: Detection of multiple profiles for a block group type:
>>> WARNING:
>>> WARNING: * DATA ->          [raid1c3, single]
>>> WARNING: * METADATA ->      [raid1, single]
>>> WARNING:
>>> WARNING: Please consider using 'btrfs balance ...' commands set
>>> WARNING: to solve this issue.
>>> WARNING: ------------------------------------------------------
>>
>> The check is a good a idea but I think the warning is too strong. I
>> would prefer that the word "Warning" is reserved for cases and
>> operations that may actually damage data (such as reformating a
>> filesystem). [Note: in a previous job, my employer decided that the word
>> Warning was ONLY to be used if there was a risk of harm to a human - for
>> example, electrical safety]

In some fields, like medical devices, terms "warning", "caution", "notice"
are strictly regulated; and your employer is right: warning is
required only when an human harm could happen.

In btrfs however, the rules are a bit relaxed; so we have only ERROR (fatal)
and Warning (which is more like caution).

However I think that an unexpected profile is to be considered a severe fault.
I.e. you could have a RAID5 instead of a RAID1, where the former is more
fragile than the latter.
Moreover I suspect that a lot of problems reported on mailing list born from
a mixed profile filesystem...

>>
>> Also, btrfs fi usage is something that I routinely run continuously in a
>> window (using watch) when a remove/replace/balance operation is in
> 
> I was going to say please put all the new output lines at the bottom,

The added lines are the last ones. Do you mean top ?

> so that 'watch' windows can be minimally sized without having to write
> something like
> 
> 	watch 'btrfs fi usage /foo | sed -e "g/WARNING:/d"'
> 
> People with short terminal windows running btrfs fi usage directly from
> the command line would probably complain about extra lines at the bottom...
> 
> Another good idea here would be a --quiet switch, or
> '--no-profile-warning'.

I think that having a global switch like '--no-profile-warning' is a good thing.

> 
>> progress to monitor at a glance what is happening - I don't want to
>> waste all that space on the screen. To say nothing of the annoyance of
>> having it shouting at me for weeks on end while **I AM TRYING TO FIX THE
>> DAMN PROBLEM!**.
>>
>> I would suggest a more compact layout and factual tone. Something like:
>>
>> Caution: This filesystem has multiple profiles for a block group type
>> so new block groups will have unpredictable profiles.
>>   * DATA ->          [raid1c3, single]
>>   * METADATA ->      [raid1, single]
>> Use of 'btrfs balance' is recommended as soon as possible to move all
>> blocks to a single profile for each of data and metadata.
> 
> How about a one-liner:
> 
> 	NOTE: Multiple profiles detected.  See 'man btrfs-filesystem'.


WARNING: Multiple profiles detected.  See 'man btrfs-filesystem'.
WARNING: data -> [raid1c3, single], metadata -> [raid1, single]

I think that the one above could be a good compromise.

I am thinking also to combine '--no-profile-warning' and '--verbose' to have
three different warning level (--no-profile-warning -> no warning at all, nothing
-> small warning, --verbose -> "giant" warning).
However the Anand's patches set for a global --verbose is still pending.
Also these global switches can be sets in an environment variables (like BTRFS_GLOBAL_OPTIONS).

> 
> with a section in the btrfs-filesystem man page giving a detailed
> description of the problem and examples of possible remedies.
> 

I will try, however my English is terrific....

BR
G.Baroncelli
-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem

[David Sterba]

On Wed, Apr 01, 2020 at 07:15:19PM +0200, Goffredo Baroncelli wrote:
> On 4/1/20 12:05 AM, Zygo Blaxell wrote:
> > On Tue, Mar 31, 2020 at 10:46:17PM +0100, Graham Cobb wrote:
> >> On 31/03/2020 20:10, Goffredo Baroncelli wrote:
> >>> WARNING: ------------------------------------------------------
> >>> WARNING: Detection of multiple profiles for a block group type:
> >>> WARNING:
> >>> WARNING: * DATA ->          [raid1c3, single]
> >>> WARNING: * METADATA ->      [raid1, single]
> >>> WARNING:
> >>> WARNING: Please consider using 'btrfs balance ...' commands set
> >>> WARNING: to solve this issue.
> >>> WARNING: ------------------------------------------------------
> >>
> >> The check is a good a idea but I think the warning is too strong. I
> >> would prefer that the word "Warning" is reserved for cases and
> >> operations that may actually damage data (such as reformating a
> >> filesystem). [Note: in a previous job, my employer decided that the word
> >> Warning was ONLY to be used if there was a risk of harm to a human - for
> >> example, electrical safety]
> 
> In some fields, like medical devices, terms "warning", "caution", "notice"
> are strictly regulated; and your employer is right: warning is
> required only when an human harm could happen.
> 
> In btrfs however, the rules are a bit relaxed; so we have only ERROR (fatal)
> and Warning (which is more like caution).
> 
> However I think that an unexpected profile is to be considered a severe fault.

I agree with the 'unexpected' part and as I read the other feedback, but
the mixed profiles do not emerge out of nowhere. It requires a balance
operation and that it changes the filesystem layout I consider expected.

> I.e. you could have a RAID5 instead of a RAID1, where the former is more
> fragile than the latter.
> Moreover I suspect that a lot of problems reported on mailing list born from
> a mixed profile filesystem...

Do you have an example? Converting between some priofiles is tricky, eg.
when the striped ones need enough space on all devices, unlike mirrored
that need that on 2/3/4 devices. But that's not something I feel is
related to lack of the warning.

> >> Also, btrfs fi usage is something that I routinely run continuously in a
> >> window (using watch) when a remove/replace/balance operation is in
> > 
> > I was going to say please put all the new output lines at the bottom,
> 
> The added lines are the last ones. Do you mean top ?
> 
> > so that 'watch' windows can be minimally sized without having to write
> > something like
> > 
> > 	watch 'btrfs fi usage /foo | sed -e "g/WARNING:/d"'
> > 
> > People with short terminal windows running btrfs fi usage directly from
> > the command line would probably complain about extra lines at the bottom...
> > 
> > Another good idea here would be a --quiet switch, or
> > '--no-profile-warning'.
> 
> I think that having a global switch like '--no-profile-warning' is a good thing.
> 
> > 
> >> progress to monitor at a glance what is happening - I don't want to
> >> waste all that space on the screen. To say nothing of the annoyance of
> >> having it shouting at me for weeks on end while **I AM TRYING TO FIX THE
> >> DAMN PROBLEM!**.
> >>
> >> I would suggest a more compact layout and factual tone. Something like:
> >>
> >> Caution: This filesystem has multiple profiles for a block group type
> >> so new block groups will have unpredictable profiles.
> >>   * DATA ->          [raid1c3, single]
> >>   * METADATA ->      [raid1, single]
> >> Use of 'btrfs balance' is recommended as soon as possible to move all
> >> blocks to a single profile for each of data and metadata.
> > 
> > How about a one-liner:
> > 
> > 	NOTE: Multiple profiles detected.  See 'man btrfs-filesystem'.
> 
> 
> WARNING: Multiple profiles detected.  See 'man btrfs-filesystem'.
> WARNING: data -> [raid1c3, single], metadata -> [raid1, single]
> 
> I think that the one above could be a good compromise.

I agree, though I'd prefer only the reference to manual page with enough
examples and commandds to see what it means and what to do next.

The output style of the tools I generally try to maintain is to provide
building blocks or combining information from various sources together,
not necessarily a tool that comes with warning everywhere a first time
user can be surprised.

> I am thinking also to combine '--no-profile-warning' and '--verbose' to have
> three different warning level (--no-profile-warning -> no warning at all, nothing
> -> small warning, --verbose -> "giant" warning).

That sounds like an overkill to me, the option is too long and I think
slightly experienced users would have to use it all the time. If we
could find commands where the warning makes most sense, like right
before or after balance, then it's fine to add it to the output. But for
'fi df' or 'fi us' it's IMHO inappropriate.

This is all usability tuning, so I'm open to discussion or
disagreements, we'll find some solution.

Re: [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem

[David Sterba]

On Tue, Mar 31, 2020 at 09:10:41PM +0200, Goffredo Baroncelli wrote:
> Patch #1 and #2 are preparatory ones.

I'll add the first two patches to devel as they're independent and once
we agree to which commands to add the warning the rest will follow.
Thanks.

Re: [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem

[Goffredo Baroncelli]

Hi David,

On 4/1/20 8:20 PM, David Sterba wrote:
> On Wed, Apr 01, 2020 at 07:15:19PM +0200, Goffredo Baroncelli wrote:
>> On 4/1/20 12:05 AM, Zygo Blaxell wrote:
>>> On Tue, Mar 31, 2020 at 10:46:17PM +0100, Graham Cobb wrote:
>>>> On 31/03/2020 20:10, Goffredo Baroncelli wrote:
>>>>> WARNING: ------------------------------------------------------
>>>>> WARNING: Detection of multiple profiles for a block group type:
>>>>> WARNING:
>>>>> WARNING: * DATA ->          [raid1c3, single]
>>>>> WARNING: * METADATA ->      [raid1, single]
>>>>> WARNING:
>>>>> WARNING: Please consider using 'btrfs balance ...' commands set
>>>>> WARNING: to solve this issue.
>>>>> WARNING: ------------------------------------------------------
>>>>
>>>> The check is a good a idea but I think the warning is too strong. I
>>>> would prefer that the word "Warning" is reserved for cases and
>>>> operations that may actually damage data (such as reformating a
>>>> filesystem). [Note: in a previous job, my employer decided that the word
>>>> Warning was ONLY to be used if there was a risk of harm to a human - for
>>>> example, electrical safety]
>>
>> In some fields, like medical devices, terms "warning", "caution", "notice"
>> are strictly regulated; and your employer is right: warning is
>> required only when an human harm could happen.
>>
>> In btrfs however, the rules are a bit relaxed; so we have only ERROR (fatal)
>> and Warning (which is more like caution).
>>
>> However I think that an unexpected profile is to be considered a severe fault.
> 
> I agree with the 'unexpected' part and as I read the other feedback, but
> the mixed profiles do not emerge out of nowhere. It requires a balance
> operation and that it changes the filesystem layout I consider expected.

If the balance+conversion operation ends without problem AND if it
converts all the filesystem, a "mixed" profile doesn't appear.
However for the other case like:
- an interrupt of balance process for an external reasons (which likely
get the attention of the user, until he forgets to complete the process)
- a balance made with some filter (like -convert=xxx,usage=yyy)

The results is a mixed profile. To avoid that the user has to do a lot
of attention.
Moreover if you watch the output of a "fi us" (or "dev us") you cannot
understand which profile is representative of the filesystem. (see below)


> 
>> I.e. you could have a RAID5 instead of a RAID1, where the former is more
>> fragile than the latter.
>> Moreover I suspect that a lot of problems reported on mailing list born from
>> a mixed profile filesystem...
> 
> Do you have an example? Converting between some priofiles is tricky, eg.
> when the striped ones need enough space on all devices, unlike mirrored
> that need that on 2/3/4 devices. But that's not something I feel is
> related to lack of the warning.

Read this more as sensation than a true fact.
However what seemed strange to me was the fact that if you have a raid6 profile,
and convert it to a raid1 without completing the process. Then the next chunk will
be a raid6. Other case: you have a raid1 profile, then convert it to a raid6 but not
complete the process. Then the next chunk will be a raid6.
raid6 wins always... [*] This is not what an user expects; I expect that
the latest balance profile wins...

[*] See the thread "Question: how understand the raid profile of a btrfs filesystem".

> 
>>>> Also, btrfs fi usage is something that I routinely run continuously in a
>>>> window (using watch) when a remove/replace/balance operation is in
>>>
>>> I was going to say please put all the new output lines at the bottom,
>>
>> The added lines are the last ones. Do you mean top ?
>>
>>> so that 'watch' windows can be minimally sized without having to write
>>> something like
>>>
>>> 	watch 'btrfs fi usage /foo | sed -e "g/WARNING:/d"'
>>>
>>> People with short terminal windows running btrfs fi usage directly from
>>> the command line would probably complain about extra lines at the bottom...
>>>
>>> Another good idea here would be a --quiet switch, or
>>> '--no-profile-warning'.
>>
>> I think that having a global switch like '--no-profile-warning' is a good thing.
>>
>>>
>>>> progress to monitor at a glance what is happening - I don't want to
>>>> waste all that space on the screen. To say nothing of the annoyance of
>>>> having it shouting at me for weeks on end while **I AM TRYING TO FIX THE
>>>> DAMN PROBLEM!**.
>>>>
>>>> I would suggest a more compact layout and factual tone. Something like:
>>>>
>>>> Caution: This filesystem has multiple profiles for a block group type
>>>> so new block groups will have unpredictable profiles.
>>>>    * DATA ->          [raid1c3, single]
>>>>    * METADATA ->      [raid1, single]
>>>> Use of 'btrfs balance' is recommended as soon as possible to move all
>>>> blocks to a single profile for each of data and metadata.
>>>
>>> How about a one-liner:
>>>
>>> 	NOTE: Multiple profiles detected.  See 'man btrfs-filesystem'.
>>
>>
>> WARNING: Multiple profiles detected.  See 'man btrfs-filesystem'.
>> WARNING: data -> [raid1c3, single], metadata -> [raid1, single]
>>
>> I think that the one above could be a good compromise.
> 
> I agree, though I'd prefer only the reference to manual page with enough
> examples and commandds to see what it means and what to do next.
> 
> The output style of the tools I generally try to maintain is to provide
> building blocks or combining information from various sources together,
> not necessarily a tool that comes with warning everywhere a first time
> user can be surprised.
> 
>> I am thinking also to combine '--no-profile-warning' and '--verbose' to have
>> three different warning level (--no-profile-warning -> no warning at all, nothing
>> -> small warning, --verbose -> "giant" warning).
> 
> That sounds like an overkill to me, the option is too long and I think
> slightly experienced users would have to use it all the time. 

My feeling is that in the 99% of the case the user doesn't see the warning. So
he doesn't need to use --no-profile-warning.
For the other cases something strange is happening. And it is better a warning
than forget a "mixed profile". I thought the switch more for a script than
an "human" user.

May be that we should ensure that the warning doesn't appear when
a balance is in progress. But only when a balance is paused/finished/stopped.
Unfortunately this seems to require root privileges.

> If we
> could find commands where the warning makes most sense, like right
> before or after balance, then it's fine to add it to the output. But for
> 'fi df' or 'fi us' it's IMHO inappropriate.

My feeling is that a mixed profile is the result of not appropriate
use of "btrfs bala". So in a standard condition the warning should not
appear.
In a non standard condition (like a balance to gain some space) the user
should have all the support to avoid any error.
This is the reason of the "WARNING".

> 
> This is all usability tuning, so I'm open to discussion or
> disagreements, we'll find some solution.

Adding or removing a check is simple; so for me it is ok to starts with
the agreed commands and then extend to other if we want.

I will work at the man page. I think that on this point we all agree.


BR
G.Baronelli

-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

[PATCH 3/4] btrfs-progs: Add btrfs_check_for_mixed_profiles_by_* function

[Goffredo Baroncelli]

From: Goffredo Baroncelli <kreijack@inwind.it>

Show a warning if a mixed profiles filesystem
is detected.

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 common/utils.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++++
 common/utils.h |   3 ++
 2 files changed, 129 insertions(+)

diff --git a/common/utils.c b/common/utils.c
index 4ce36836..e7cd66eb 100644
--- a/common/utils.c
+++ b/common/utils.c
@@ -1710,3 +1710,129 @@ void print_all_devices(struct list_head *devices)
 		print_device_info(dev, "\t");
 	printf("\n");
 }
+
+static int bit_count(u64 x)
+{
+	int ret = 0;
+
+	while (x) {
+		if (x & 1)
+			ret++;
+		x >>= 1;
+	}
+	return ret;
+}
+
+static void print_profiles(FILE *out, u64 profiles)
+{
+	int i;
+	int first = true;
+
+	for (i = 0 ; i < BTRFS_NR_RAID_TYPES ; i++) {
+		if (!(btrfs_raid_array[i].bg_flag & profiles))
+			continue;
+
+		if (!first)
+			fprintf(out, ", ");
+		fprintf(out, "%s", btrfs_raid_array[i].raid_name);
+		first = false;
+	}
+	if (profiles & BTRFS_AVAIL_ALLOC_BIT_SINGLE) {
+		if (!first)
+			fprintf(out, ", ");
+		fprintf(out, "%s",
+			btrfs_raid_array[BTRFS_RAID_SINGLE].raid_name);
+	}
+}
+
+int btrfs_check_for_mixed_profiles_by_path(char *path)
+{
+	int fd;
+	int ret;
+	DIR *dirstream;
+
+	fd = btrfs_open_dir(path, &dirstream, 0);
+	if (fd < 0)
+		return -1;
+	closedir(dirstream);
+
+	ret = btrfs_check_for_mixed_profiles_by_fd(fd);
+	close(fd);
+
+	return ret;
+}
+
+int btrfs_check_for_mixed_profiles_by_fd(int fd)
+{
+	int ret;
+	int i;
+	struct btrfs_ioctl_space_args *sargs;
+	u64 data_profiles = 0;
+	u64 metadata_profiles = 0;
+	u64 system_profiles = 0;
+	u64 mixed_profiles = 0;
+	static const u64 mixed_profile_fl = BTRFS_BLOCK_GROUP_METADATA |
+		BTRFS_BLOCK_GROUP_DATA;
+
+	ret = get_df(fd, &sargs);
+	if (ret < 0)
+		return -1;
+
+	for (i = 0 ; i < sargs->total_spaces ; i++) {
+		u64 flags = sargs->spaces[i].flags;
+
+		if (!(flags & BTRFS_BLOCK_GROUP_PROFILE_MASK))
+			flags |= BTRFS_AVAIL_ALLOC_BIT_SINGLE;
+
+		if ((flags & mixed_profile_fl) == mixed_profile_fl)
+			mixed_profiles |= flags;
+		else if (flags & BTRFS_BLOCK_GROUP_DATA)
+			data_profiles |= flags;
+		else if (flags & BTRFS_BLOCK_GROUP_METADATA)
+			metadata_profiles |= flags;
+		else if (flags & BTRFS_BLOCK_GROUP_SYSTEM)
+			system_profiles |= flags;
+	}
+	free(sargs);
+
+	data_profiles &= BTRFS_EXTENDED_PROFILE_MASK;
+	system_profiles &= BTRFS_EXTENDED_PROFILE_MASK;
+	mixed_profiles &= BTRFS_EXTENDED_PROFILE_MASK;
+	metadata_profiles &= BTRFS_EXTENDED_PROFILE_MASK;
+
+	if ((bit_count(data_profiles) <= 1) &&
+	    (bit_count(metadata_profiles) <= 1) &&
+	    (bit_count(system_profiles) <= 1) &&
+	    (bit_count(mixed_profiles) <= 1))
+		return 0;
+
+	fprintf(stderr, "WARNING: ------------------------------------------------------\n");
+	fprintf(stderr, "WARNING: Detection of multiple profiles for a block group type:\n");
+	fprintf(stderr, "WARNING:\n");
+	if (bit_count(data_profiles) > 1) {
+		fprintf(stderr, "WARNING: * DATA ->          [");
+		print_profiles(stderr, data_profiles);
+		fprintf(stderr, "]\n");
+	}
+	if (bit_count(metadata_profiles) > 1) {
+		fprintf(stderr, "WARNING: * METADATA ->      [");
+		print_profiles(stderr, metadata_profiles);
+		fprintf(stderr, "]\n");
+	}
+	if (bit_count(mixed_profiles) > 1) {
+		fprintf(stderr, "WARNING: * DATA+METADATA -> [");
+		print_profiles(stderr, mixed_profiles);
+		fprintf(stderr, "]\n");
+	}
+	if (bit_count(system_profiles) > 1) {
+		fprintf(stderr, "WARNING: * SYSTEM ->        [");
+		print_profiles(stderr, system_profiles);
+		fprintf(stderr, "]\n");
+	}
+	fprintf(stderr, "WARNING:\n");
+	fprintf(stderr, "WARNING: Please consider using 'btrfs balance ...' commands set\n");
+	fprintf(stderr, "WARNING: to solve this issue.\n");
+	fprintf(stderr, "WARNING: ------------------------------------------------------\n");
+
+	return 1;
+}
diff --git a/common/utils.h b/common/utils.h
index 5c1afda9..662c9e38 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -137,4 +137,7 @@ u64 rand_u64(void);
 unsigned int rand_range(unsigned int upper);
 void init_rand_seed(u64 seed);
 
+int btrfs_check_for_mixed_profiles_by_path(char *path);
+int btrfs_check_for_mixed_profiles_by_fd(int fd);
+
 #endif
-- 
2.26.0.rc2