[PATCH 1/2] btrfs-progs: add ask_user confirmation for btrfstune clear seeding flag

[PATCH 1/2] btrfs-progs: add ask_user confirmation for btrfstune clear seeding flag

[Gui Hecheng]

Clear the seeding flag may cause the original filesystem to be writable,
which is dangerous.
In this case, add user confirmation check when clearing seeding flag.
Also warn the user that the fs is in a dangerous condition when
the seeding flag is cleared if it it forced to.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 btrfstune.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/btrfstune.c b/btrfstune.c
index 3f2f0cd..0e18088 100644
--- a/btrfstune.c
+++ b/btrfstune.c
@@ -56,6 +56,7 @@ static int update_seeding_flag(struct btrfs_root *root, int set_flag)
 			return 1;
 		}
 		super_flags &= ~BTRFS_SUPER_FLAG_SEEDING;
+		fprintf(stderr, "Warning: Seeding flag cleared. This could be dangerous!\n");
 	}
 
 	trans = btrfs_start_transaction(root, 1);
@@ -103,6 +104,7 @@ static void print_usage(void)
 	fprintf(stderr, "\t-S value\tpositive value will enable seeding, zero to disable, negative is not allowed\n");
 	fprintf(stderr, "\t-r \t\tenable extended inode refs\n");
 	fprintf(stderr, "\t-x \t\tenable skinny metadata extent refs\n");
+	fprintf(stderr, "\t-y \t\tsay yes to clear the seeding flag, make sure that you are aware of the danger\n");
 }
 
 int main(int argc, char *argv[])
@@ -113,11 +115,12 @@ int main(int argc, char *argv[])
 	int seeding_flag = 0;
 	u64 seeding_value = 0;
 	int skinny_flag = 0;
+	int force_clear = 0;
 	int ret;
 
 	optind = 1;
 	while(1) {
-		int c = getopt(argc, argv, "S:rx");
+		int c = getopt(argc, argv, "S:rxy");
 		if (c < 0)
 			break;
 		switch(c) {
@@ -131,6 +134,9 @@ int main(int argc, char *argv[])
 		case 'x':
 			skinny_flag = 1;
 			break;
+		case 'y':
+			force_clear = 1;
+			break;
 		default:
 			print_usage();
 			return 1;
@@ -151,6 +157,13 @@ int main(int argc, char *argv[])
 		return 1;
 	}
 
+	if (!seeding_flag && force_clear) {
+		fprintf(stderr,
+			"ERROR: -y option only work with -S option.\n");
+		print_usage();
+		return 1;
+	}
+
 	ret = check_mounted(device);
 	if (ret < 0) {
 		fprintf(stderr, "Could not check mount status: %s\n",
@@ -169,6 +182,15 @@ int main(int argc, char *argv[])
 	}
 
 	if (seeding_flag) {
+		if (!seeding_value && !force_clear) {
+			fprintf(stderr, "Warning: This is dangerous, clearing the seeding flag will make the original filesystem writable.\n");
+			ret = ask_user("We are going to clear the seeding flag, are you sure?");
+			if (!ret) {
+				fprintf(stderr, "Clear seeding flag canceled\n");
+				return 1;
+			}
+		}
+
 		ret = update_seeding_flag(root, seeding_value);
 		if (!ret)
 			success++;
-- 
1.8.1.4

[PATCH] btrfs-progs: add mount status check for btrfs-image

[Gui Hecheng]

The btrfs-image tool should not be run on a mounted filesystem.
The undergoing fs operations may change what you have imaged a while ago,
this makes the image meanmingless.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 btrfs-image.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/btrfs-image.c b/btrfs-image.c
index 985aa26..9719d1c 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -2570,10 +2570,20 @@ int main(int argc, char *argv[])
 			num_threads = 1;
 	}
 
-	if (create)
+	if (create) {
+		ret = check_mounted(source);
+		if (ret < 0) {
+			fprintf(stderr, "Could not check mount status: %s\n",
+			strerror(-ret));
+			exit(1);
+		} else if (ret) {
+			fprintf(stderr, "The device is busy\n");
+			exit(1);
+		}
+
 		ret = create_metadump(source, out, num_threads,
 				      compress_level, sanitize, walk_trees);
-	else
+	} else
 		ret = restore_metadump(source, out, old_restore, 1,
 				       multi_devices);
 	if (ret) {
-- 
1.8.1.4

Re: [PATCH] btrfs-progs: add mount status check for btrfs-image

[David Sterba]

On Thu, Jul 03, 2014 at 10:06:34AM +0800, Gui Hecheng wrote:
> The btrfs-image tool should not be run on a mounted filesystem.

Should not, but for some values of "sometimes" it makes sense, eg.
capturing image of an otherwise quiescent filesystem, a read-only mount
or after a crash.

This utility is used for debugging so I'd prefer to let the user do as
he likes, though printing the warning about the mount status is a good
improvement.

> The undergoing fs operations may change what you have imaged a while ago,
> this makes the image meanmingless.

I'm not familiar with the image format, but maybe we can set a bit in
the header when the filesystem was not captured cleanly.

Re: [PATCH] btrfs-progs: add mount status check for btrfs-image

[Gui Hecheng]

On Thu, 2014-07-03 at 18:58 +0200, David Sterba wrote:
> On Thu, Jul 03, 2014 at 10:06:34AM +0800, Gui Hecheng wrote:
> > The btrfs-image tool should not be run on a mounted filesystem.
> 
> Should not, but for some values of "sometimes" it makes sense, eg.
> capturing image of an otherwise quiescent filesystem, a read-only mount
> or after a crash.

Ah, read-only mount is really a case.

> This utility is used for debugging so I'd prefer to let the user do as
> he likes, though printing the warning about the mount status is a good
> improvement.

I agree, then I'll keep the check_mounted and just give a prompt and let
it continue.

> > The undergoing fs operations may change what you have imaged a while ago,
> > this makes the image meanmingless.
> 
> I'm not familiar with the image format, but maybe we can set a bit in
> the header when the filesystem was not captured cleanly.

Hmm...This is a point, I think I'll give it a try in another patch.

[PATCH v2] btrfs-progs: add mount status check for btrfs-image

[Gui Hecheng]

When btrfs-image run on a mounted filesystem,
the undergoing fs operations may change what you have imaged a while ago.
In this case, give a warning to remind the user that he may not
get a consistent image he wants.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
changelog:
	v1->v2: just give a warning if device mounted and let it continue
---
 btrfs-image.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/btrfs-image.c b/btrfs-image.c
index 985aa26..e1d5482 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -2570,10 +2570,18 @@ int main(int argc, char *argv[])
 			num_threads = 1;
 	}
 
-	if (create)
+	if (create) {
+		ret = check_mounted(source);
+		if (ret < 0) {
+			fprintf(stderr, "Could not check mount status: %s\n",
+				strerror(-ret));
+			exit(1);
+		} else if (ret)
+			fprintf(stderr, "WARNING: The device is mounted. Make sure you didn't do modifications.\n");
+
 		ret = create_metadump(source, out, num_threads,
 				      compress_level, sanitize, walk_trees);
-	else
+	} else
 		ret = restore_metadump(source, out, old_restore, 1,
 				       multi_devices);
 	if (ret) {
-- 
1.8.1.4

[PATCH] btrfs-progs: prevent select invalid dev super after dev replace

[Gui Hecheng]

After dev replace, we should not select the superblock of the
replaced dev. Otherwise, all the superblokcs will be overwritten
by this invalid superblock.

To prevent this case, let btrfs-select-super check the first
superblock on the selected dev. If the magic doesn't match,
then the dev is a replaced dev and error message will show up.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 btrfs-select-super.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/btrfs-select-super.c b/btrfs-select-super.c
index d7cd187..cba3414 100644
--- a/btrfs-select-super.c
+++ b/btrfs-select-super.c
@@ -45,6 +45,9 @@ int main(int ac, char **av)
 	int ret;
 	u64 num = 0;
 	u64 bytenr = 0;
+	int fd;
+	u8 buf[BTRFS_SUPER_INFO_SIZE];
+	struct btrfs_super_block *sb = (struct btrfs_super_block *)buf;
 
 	while(1) {
 		int c;
@@ -86,6 +89,26 @@ int main(int ac, char **av)
 		return -EBUSY;
 	}
 
+	/*
+	 * After dev replace, the first super block of replaced dev will be cleared,
+	 * don't select that dev.
+	 */
+	fd = open(av[optind], O_RDONLY, 0666);
+	if (fd < 0)
+		return -errno;
+
+	ret = pread64(fd, buf, sizeof(buf), BTRFS_SUPER_INFO_OFFSET);
+	if (ret < sizeof(buf)) {
+		close(fd);
+		return -errno;
+	}
+
+	if (sb->magic != cpu_to_le64(BTRFS_MAGIC)) {
+		fprintf(stderr, "Cannot select an invalid super block.\n");
+		close(fd);
+		return 1;
+	}
+
 	root = open_ctree(av[optind], bytenr, 1);
 
 	if (!root) {
-- 
1.8.1.4

Re: [PATCH] btrfs-progs: prevent select invalid dev super after dev replace

[David Sterba]

On Thu, Jul 03, 2014 at 10:06:35AM +0800, Gui Hecheng wrote:
> After dev replace, we should not select the superblock of the
> replaced dev. Otherwise, all the superblokcs will be overwritten
> by this invalid superblock.
> 
> To prevent this case, let btrfs-select-super check the first
> superblock on the selected dev. If the magic doesn't match,
> then the dev is a replaced dev and error message will show up.

Is this patch needed if Qu's superblock checksum patches are applied?
Thanks.

Re: [PATCH] btrfs-progs: prevent select invalid dev super after dev replace

[Gui Hecheng]

On Thu, 2014-07-03 at 20:10 +0200, David Sterba wrote:
> On Thu, Jul 03, 2014 at 10:06:35AM +0800, Gui Hecheng wrote:
> > After dev replace, we should not select the superblock of the
> > replaced dev. Otherwise, all the superblokcs will be overwritten
> > by this invalid superblock.
> > 
> > To prevent this case, let btrfs-select-super check the first
> > superblock on the selected dev. If the magic doesn't match,
> > then the dev is a replaced dev and error message will show up.
> 
> Is this patch needed if Qu's superblock checksum patches are applied?
> Thanks.
Hmm...I think it is not neccessary then, please *ignore* this one.
Thanks, David.

[PATCH 2/2] btrfs-progs: update manpage with new option -y for btrfstune

[Gui Hecheng]

The new -y option says yes to clear the seeding flag,
this is to facilitate scripting.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 Documentation/btrfstune.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/btrfstune.txt b/Documentation/btrfstune.txt
index bf44bdf..2406b0b 100644
--- a/Documentation/btrfstune.txt
+++ b/Documentation/btrfstune.txt
@@ -24,7 +24,10 @@ Enable seeding forces a fs readonly so that you can use it to build other filesy
 Enable extended inode refs.
 -x::
 Enable skinny metadata extent refs.
-
+-y::
+Say yes to clear the seeding flag.
+Clear the seeding flag may cause the original filesystem to be writable which is dangerous.
+So you need to confirm that you really want to clear it.
 
 EXIT STATUS
 -----------
-- 
1.8.1.4

Re: [PATCH 1/2] btrfs-progs: add ask_user confirmation for btrfstune clear seeding flag

[David Sterba]

On Thu, Jul 03, 2014 at 10:06:33AM +0800, Gui Hecheng wrote:
> Clear the seeding flag may cause the original filesystem to be writable,
> which is dangerous.

Can you please describe the dangerous scenario a bit more? This would
also go to the documentation so it's not only to satisfy my curiosity.

Dropping the seeding flag could be dangerous if the filesystem starts in
seeding mode, a new device is added, some writes are done, then
filesystem is unmounted.

Now it's a 2 device filesystem, where the orignal holds some data and
without the seeding flag it would accept new writes. Still ok for me,
though this is probably the time where some user assumptions may break.

> In this case, add user confirmation check when clearing seeding flag.
> Also warn the user that the fs is in a dangerous condition when
> the seeding flag is cleared if it it forced to.

The -y option is tied only to the seeding option, but it should IMO be
more general and called --force.

> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> ---
>  btrfstune.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/btrfstune.c b/btrfstune.c
> index 3f2f0cd..0e18088 100644
> --- a/btrfstune.c
> +++ b/btrfstune.c
> @@ -103,6 +104,7 @@ static void print_usage(void)
>  	fprintf(stderr, "\t-S value\tpositive value will enable seeding, zero to disable, negative is not allowed\n");
>  	fprintf(stderr, "\t-r \t\tenable extended inode refs\n");
>  	fprintf(stderr, "\t-x \t\tenable skinny metadata extent refs\n");
> +	fprintf(stderr, "\t-y \t\tsay yes to clear the seeding flag, make sure that you are aware of the danger\n");

The help text could say someting like

		"--force\tallow dangerous changes\n"

btrfstune only allows setting the bit for extref and skinny-metadata,
unsetting would be dangerous as well.

Re: [PATCH 1/2] btrfs-progs: add ask_user confirmation for btrfstune clear seeding flag

[Gui Hecheng]

On Thu, 2014-07-03 at 18:51 +0200, David Sterba wrote:
> On Thu, Jul 03, 2014 at 10:06:33AM +0800, Gui Hecheng wrote:
> > Clear the seeding flag may cause the original filesystem to be writable,
> > which is dangerous.
> 
> Can you please describe the dangerous scenario a bit more? This would
> also go to the documentation so it's not only to satisfy my curiosity.

Yes, I'll include a certain scenario in the changelog of a v2 patch.

> Dropping the seeding flag could be dangerous if the filesystem starts in
> seeding mode, a new device is added, some writes are done, then
> filesystem is unmounted.
> 
> Now it's a 2 device filesystem, where the orignal holds some data and
> without the seeding flag it would accept new writes. Still ok for me,
> though this is probably the time where some user assumptions may break.
> 
> > In this case, add user confirmation check when clearing seeding flag.
> > Also warn the user that the fs is in a dangerous condition when
> > the seeding flag is cleared if it it forced to.
> 
> The -y option is tied only to the seeding option, but it should IMO be
> more general and called --force.

I agree.

> > Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> > ---
> >  btrfstune.c | 24 +++++++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> > 
> > diff --git a/btrfstune.c b/btrfstune.c
> > index 3f2f0cd..0e18088 100644
> > --- a/btrfstune.c
> > +++ b/btrfstune.c
> > @@ -103,6 +104,7 @@ static void print_usage(void)
> >  	fprintf(stderr, "\t-S value\tpositive value will enable seeding, zero to disable, negative is not allowed\n");
> >  	fprintf(stderr, "\t-r \t\tenable extended inode refs\n");
> >  	fprintf(stderr, "\t-x \t\tenable skinny metadata extent refs\n");
> > +	fprintf(stderr, "\t-y \t\tsay yes to clear the seeding flag, make sure that you are aware of the danger\n");
> 
> The help text could say someting like
> 
> 		"--force\tallow dangerous changes\n"
> 
> btrfstune only allows setting the bit for extref and skinny-metadata,
> unsetting would be dangerous as well.
On my part, I don't find any scenarioes for these two, could you please
remind me more?

Re: [PATCH 1/2] btrfs-progs: add ask_user confirmation for btrfstune clear seeding flag

[David Sterba]

On Fri, Jul 04, 2014 at 09:59:13AM +0800, Gui Hecheng wrote:
> > > --- a/btrfstune.c
> > > +++ b/btrfstune.c
> > > @@ -103,6 +104,7 @@ static void print_usage(void)
> > >  	fprintf(stderr, "\t-S value\tpositive value will enable seeding, zero to disable, negative is not allowed\n");
> > >  	fprintf(stderr, "\t-r \t\tenable extended inode refs\n");
> > >  	fprintf(stderr, "\t-x \t\tenable skinny metadata extent refs\n");
> > > +	fprintf(stderr, "\t-y \t\tsay yes to clear the seeding flag, make sure that you are aware of the danger\n");
> > 
> > The help text could say someting like
> > 
> > 		"--force\tallow dangerous changes\n"
> > 
> > btrfstune only allows setting the bit for extref and skinny-metadata,
> > unsetting would be dangerous as well.
> On my part, I don't find any scenarioes for these two, could you please
> remind me more?

For example the extended refs add a new key type representing the
hardlinks that do not fit into the leaf. Dropping the bit from incompat
bits will hide that. Mounting the filesystem on kernels < 3.7 will not
work as expected then.

The extref key is not necessarily used even if the bit is set, only if
the hardlinks do not fit, so dropping the bit will not automatically
cause a disaster.

[PATCH v2] btrfs-progs: add ask_user confirmation for btrfstune clear seeding flag

[Gui Hecheng]

If we do the following:
	# mkfs.btrfs -f <dev>
	# mount <dev> <mnt>
	# dd if=/dev/urandom of=<mnt>/data bs=1M count=100
	# umount <dev>
	# btrfstune -S 1 <dev> 		<--- make seeding device
	# mount <dev> <mnt>
	# btrfs dev add -f <dev2> <mnt>
	# umount <dev>
	# btrfstune -S 0 <dev>		<--- clear seeding flag
	# mount <dev2> <mnt>		<=== new device not mountable

When mounting the new device, btrfs will check whether
the seeding flag is set when try to open seeding device.
If the user clears the seeding flag of the seeding device,
the new device will not be mountable. Even set the seeding
flag back will not recovery this problem, because the generation
has been changed. So clear the seeding flag has the chance to
damage the derived new fs.

So I add user confirmation check when clearing seeding flag.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
changelog
	v1->v2: add certain scenario description
---
 btrfstune.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/btrfstune.c b/btrfstune.c
index 3f2f0cd..0895204 100644
--- a/btrfstune.c
+++ b/btrfstune.c
@@ -56,6 +56,7 @@ static int update_seeding_flag(struct btrfs_root *root, int set_flag)
 			return 1;
 		}
 		super_flags &= ~BTRFS_SUPER_FLAG_SEEDING;
+		fprintf(stderr, "Warning: Seeding flag cleared.\n");
 	}
 
 	trans = btrfs_start_transaction(root, 1);
@@ -103,6 +104,7 @@ static void print_usage(void)
 	fprintf(stderr, "\t-S value\tpositive value will enable seeding, zero to disable, negative is not allowed\n");
 	fprintf(stderr, "\t-r \t\tenable extended inode refs\n");
 	fprintf(stderr, "\t-x \t\tenable skinny metadata extent refs\n");
+	fprintf(stderr, "\t-f \t\tforce to clear flags, make sure that you are aware of the dangers\n");
 }
 
 int main(int argc, char *argv[])
@@ -113,11 +115,12 @@ int main(int argc, char *argv[])
 	int seeding_flag = 0;
 	u64 seeding_value = 0;
 	int skinny_flag = 0;
+	int force = 0;
 	int ret;
 
 	optind = 1;
 	while(1) {
-		int c = getopt(argc, argv, "S:rx");
+		int c = getopt(argc, argv, "S:rxf");
 		if (c < 0)
 			break;
 		switch(c) {
@@ -131,6 +134,9 @@ int main(int argc, char *argv[])
 		case 'x':
 			skinny_flag = 1;
 			break;
+		case 'f':
+			force = 1;
+			break;
 		default:
 			print_usage();
 			return 1;
@@ -169,6 +175,15 @@ int main(int argc, char *argv[])
 	}
 
 	if (seeding_flag) {
+		if (!seeding_value && !force) {
+			fprintf(stderr, "Warning: This is dangerous, clearing the seeding flag may cause the derived device not to be mountable!\n");
+			ret = ask_user("We are going to clear the seeding flag, are you sure?");
+			if (!ret) {
+				fprintf(stderr, "Clear seeding flag canceled\n");
+				return 1;
+			}
+		}
+
 		ret = update_seeding_flag(root, seeding_value);
 		if (!ret)
 			success++;
-- 
1.8.1.4

[PATCH v2] btrfs-progs: update manpage with new option -f for btrfstune

[Gui Hecheng]

The new option -f will force to do dangerous changes.
e.g. clear the seeding flag.
---
changelog
	v1->v2: use -f instead of -y
---
 Documentation/btrfstune.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/btrfstune.txt b/Documentation/btrfstune.txt
index bf44bdf..85a7165 100644
--- a/Documentation/btrfstune.txt
+++ b/Documentation/btrfstune.txt
@@ -24,7 +24,8 @@ Enable seeding forces a fs readonly so that you can use it to build other filesy
 Enable extended inode refs.
 -x::
 Enable skinny metadata extent refs.
-
+-f::
+Allow dangerous changes, e.g. clear the seeding flag
 
 EXIT STATUS
 -----------
-- 
1.8.1.4

Re: [PATCH v2] btrfs-progs: update manpage with new option -f for btrfstune

[David Sterba]

On Mon, Jul 07, 2014 at 09:54:53AM +0800, Gui Hecheng wrote:
> The new option -f will force to do dangerous changes.
> e.g. clear the seeding flag.

missing signed-off-by

> --- a/Documentation/btrfstune.txt
> +++ b/Documentation/btrfstune.txt
> @@ -24,7 +24,8 @@ Enable seeding forces a fs readonly so that you can use it to build other filesy
>  Enable extended inode refs.
>  -x::
>  Enable skinny metadata extent refs.
> -
> +-f::
> +Allow dangerous changes, e.g. clear the seeding flag

Please enhance this, we've discussed it under previous patch iterations.
Thanks.