* [PATCH 1/4] Complete the implementation of RAID1C[34].
2020-03-31 19:10 [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem Goffredo Baroncelli
@ 2020-03-31 19:10 ` Goffredo Baroncelli
2020-03-31 19:10 ` [PATCH 2/4] btrfs-progs: Add BTRFS_EXTENDED_PROFILE_MASK mask Goffredo Baroncelli
` (4 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Goffredo Baroncelli @ 2020-03-31 19:10 UTC (permalink / raw)
To: linux-btrfs; +Cc: Zygo Blaxell, Qu Wenruo, David Sterba, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 2/4] btrfs-progs: Add BTRFS_EXTENDED_PROFILE_MASK mask.
2020-03-31 19:10 [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem Goffredo Baroncelli
2020-03-31 19:10 ` [PATCH 1/4] Complete the implementation of RAID1C[34] Goffredo Baroncelli
@ 2020-03-31 19:10 ` Goffredo Baroncelli
2020-03-31 19:10 ` [PATCH 3/4] btrfs-progs: Add btrfs_check_for_mixed_profiles_by_* function Goffredo Baroncelli
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Goffredo Baroncelli @ 2020-03-31 19:10 UTC (permalink / raw)
To: linux-btrfs; +Cc: Zygo Blaxell, Qu Wenruo, David Sterba, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 3/4] btrfs-progs: Add btrfs_check_for_mixed_profiles_by_* function
2020-03-31 19:10 [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem Goffredo Baroncelli
2020-03-31 19:10 ` [PATCH 1/4] Complete the implementation of RAID1C[34] Goffredo Baroncelli
2020-03-31 19:10 ` [PATCH 2/4] btrfs-progs: Add BTRFS_EXTENDED_PROFILE_MASK mask Goffredo Baroncelli
@ 2020-03-31 19:10 ` Goffredo Baroncelli
2020-03-31 19:10 ` [PATCH 4/4] btrfs-progs: Add mixed profiles check to some btrfs sub-commands Goffredo Baroncelli
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Goffredo Baroncelli @ 2020-03-31 19:10 UTC (permalink / raw)
To: linux-btrfs; +Cc: Zygo Blaxell, Qu Wenruo, David Sterba, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 4/4] btrfs-progs: Add mixed profiles check to some btrfs sub-commands.
2020-03-31 19:10 [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem Goffredo Baroncelli
` (2 preceding siblings ...)
2020-03-31 19:10 ` [PATCH 3/4] btrfs-progs: Add btrfs_check_for_mixed_profiles_by_* function Goffredo Baroncelli
@ 2020-03-31 19:10 ` Goffredo Baroncelli
2020-03-31 21:46 ` [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem Graham Cobb
2020-04-01 18:25 ` David Sterba
5 siblings, 0 replies; 13+ messages in thread
From: Goffredo Baroncelli @ 2020-03-31 19:10 UTC (permalink / raw)
To: linux-btrfs; +Cc: Zygo Blaxell, Qu Wenruo, David Sterba, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem
2020-03-31 19:10 [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem Goffredo Baroncelli
` (3 preceding siblings ...)
2020-03-31 19:10 ` [PATCH 4/4] btrfs-progs: Add mixed profiles check to some btrfs sub-commands Goffredo Baroncelli
@ 2020-03-31 21:46 ` Graham Cobb
2020-03-31 22:05 ` Zygo Blaxell
2020-04-01 18:25 ` David Sterba
5 siblings, 1 reply; 13+ messages in thread
From: Graham Cobb @ 2020-03-31 21:46 UTC (permalink / raw)
To: Goffredo Baroncelli, linux-btrfs; +Cc: Zygo Blaxell, Qu Wenruo, David Sterba
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.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem
2020-03-31 21:46 ` [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem Graham Cobb
@ 2020-03-31 22:05 ` Zygo Blaxell
2020-04-01 10:58 ` Graham Cobb
2020-04-01 17:15 ` Goffredo Baroncelli
0 siblings, 2 replies; 13+ messages in thread
From: Zygo Blaxell @ 2020-03-31 22:05 UTC (permalink / raw)
To: Graham Cobb; +Cc: Goffredo Baroncelli, linux-btrfs, Qu Wenruo, David Sterba
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem
2020-03-31 22:05 ` Zygo Blaxell
@ 2020-04-01 10:58 ` Graham Cobb
2020-04-01 17:15 ` Goffredo Baroncelli
1 sibling, 0 replies; 13+ messages in thread
From: Graham Cobb @ 2020-04-01 10:58 UTC (permalink / raw)
To: Zygo Blaxell; +Cc: Goffredo Baroncelli, linux-btrfs, Qu Wenruo, David Sterba
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.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem
2020-03-31 22:05 ` Zygo Blaxell
2020-04-01 10:58 ` Graham Cobb
@ 2020-04-01 17:15 ` Goffredo Baroncelli
2020-04-01 18:20 ` David Sterba
1 sibling, 1 reply; 13+ messages in thread
From: Goffredo Baroncelli @ 2020-04-01 17:15 UTC (permalink / raw)
To: Zygo Blaxell, Graham Cobb; +Cc: linux-btrfs, Qu Wenruo, David Sterba
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem
2020-04-01 17:15 ` Goffredo Baroncelli
@ 2020-04-01 18:20 ` David Sterba
2020-04-01 19:05 ` Goffredo Baroncelli
0 siblings, 1 reply; 13+ messages in thread
From: David Sterba @ 2020-04-01 18:20 UTC (permalink / raw)
To: kreijack; +Cc: Zygo Blaxell, Graham Cobb, linux-btrfs, Qu Wenruo, 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem
2020-04-01 18:20 ` David Sterba
@ 2020-04-01 19:05 ` Goffredo Baroncelli
0 siblings, 0 replies; 13+ messages in thread
From: Goffredo Baroncelli @ 2020-04-01 19:05 UTC (permalink / raw)
To: dsterba, Zygo Blaxell, Graham Cobb, linux-btrfs, Qu Wenruo, David Sterba
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem
2020-03-31 19:10 [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem Goffredo Baroncelli
` (4 preceding siblings ...)
2020-03-31 21:46 ` [PATCH v2] btrfs-progs: add warning for mixed profiles filesystem Graham Cobb
@ 2020-04-01 18:25 ` David Sterba
5 siblings, 0 replies; 13+ messages in thread
From: David Sterba @ 2020-04-01 18:25 UTC (permalink / raw)
To: Goffredo Baroncelli; +Cc: linux-btrfs, Zygo Blaxell, Qu Wenruo, 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/4] btrfs-progs: Add btrfs_check_for_mixed_profiles_by_* function
2020-03-25 20:10 [RFC][PATCH] btrfs-progs: add warning for mixed prfofiles filesystem Goffredo Baroncelli
@ 2020-03-25 20:10 ` Goffredo Baroncelli
0 siblings, 0 replies; 13+ messages in thread
From: Goffredo Baroncelli @ 2020-03-25 20:10 UTC (permalink / raw)
To: linux-btrfs; +Cc: Zygo Blaxell, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread