Btrfs devel - Oct 2009 - [PATCH] Prevent btrfsck to run on mounted filesystems

As recently discussed on the list, btrfsck should only be run on unmounted
filesystems. This patch adds a short check for the mount status at the beginning
of btrfsck. If the FS is mounted, the program aborts showing an error message.

Signed-off-by: Andi Drebes <lists-receive@programmierforen.de>
---
diff --git a/btrfsck.c b/btrfsck.c
index 73f1836..87ae776 100644
--- a/btrfsck.c
+++ b/btrfsck.c
@@ -28,6 +28,7 @@
 #include "transaction.h"
 #include "list.h"
 #include "version.h"
+#include "utils.h"
 
 static u64 bytes_used = 0;
 static u64 total_csum_bytes = 0;
@@ -2819,6 +2820,16 @@ int main(int ac, char **av)
 	if (ac < 2)
 		print_usage();
 
+	ret = check_mounted(av[1]);
+	if (ret < 0) {
+		fprintf(stderr, "error checking %s mount status\n", av[1]);
+		return 1;
+	}
+	if (ret == 1) {
+		fprintf(stderr, "%s is mounted. btrfsck can only be run on an unmounted
filesystem.\n", av[1]);
+		return 1;
+	}
+
 	radix_tree_init();
 	cache_tree_init(&root_cache);
 	root = open_ctree(av[1], 0, 0);
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Oct 29, 2009 at 09:52:15PM +0100, Andi Drebes
wrote:
> As recently discussed on the list, btrfsck should only be run on unmounted
filesystems. This patch adds a short check for the mount status at the beginning
of btrfsck. If the FS is mounted, the program aborts showing an error message.
Just open the nodes with O_EXCL and you''ll get all the checking for
free.  Also make sure that for a pure, read-only checks instead of a
repair to allow running on at least a read-only mounted filesystem.

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Oct 30, 2009 at 01:02:44AM -0400, Christoph Hellwig
wrote:
> On Thu, Oct 29, 2009 at 09:52:15PM +0100, Andi Drebes wrote:
> > As recently discussed on the list, btrfsck should only be run on
unmounted filesystems. This patch adds a short check for the mount status at the
beginning of btrfsck. If the FS is mounted, the program aborts showing an error
message.
> 
> Just open the nodes with O_EXCL and you''ll get all the checking
for
> free.  Also make sure that for a pure, read-only checks instead of a
> repair to allow running on at least a read-only mounted filesystem.
> 
Thanks for working on this patch Andi.

In this case O_EXCL is going to be more accurate just because the
mounted check doesn''t cover every disk in the FS.  For now btrfsck
doesn''t really give consistent results even readonly on a mounted
filesystem.  We should prevent it with a message just to prevent
confusion.

-chris

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> > Just open the nodes with O_EXCL and you''ll get all the
checking for
> > free.  Also make sure that for a pure, read-only checks instead of a
> > repair to allow running on at least a read-only mounted filesystem.
> 
> Thanks for working on this patch Andi.
> 
> In this case O_EXCL is going to be more accurate just because the
> mounted check doesn''t cover every disk in the FS.  For now btrfsck
> doesn''t really give consistent results even readonly on a mounted
> filesystem.  We should prevent it with a message just to prevent
> confusion.
Thanks for the reply. I''ll do this as soon as I understand the code
that is affected by those changes (I''m still in the learning phase and
stepping through the code).

	Cheers,
		Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi!
> > In this case O_EXCL is going to be more accurate just because the
> > mounted check doesn''t cover every disk in the FS.  For now
btrfsck
> > doesn''t really give consistent results even readonly on a
mounted
> > filesystem.  We should prevent it with a message just to prevent
> > confusion.
> Thanks for the reply. I''ll do this as soon as I understand the
code
> that is affected by those changes (I''m still in the learning phase
> and stepping through the code).   
OK, I''ve stepped through the code so far and I think I now have an idea
of what should be changed. However, concerning the solution, I was confusing two
things.

Christoph Hellwig earlier wrote:
> Just open the nodes with O_EXCL and you''ll get all the checking
for
> free.
I thought that open() with O_EXCL would fail if the file is already opened RW
somewhere else. But the man page for open() says something else:

"O_EXCL: Ensure that this call creates the file: if this flag  is 
specified  in  conjunction with O_CREAT, and pathname already exists, then
open() will fail."

And especially:

"The behavior of O_EXCL is  undefined if O_CREAT is not specified."

After having read this, I don''t see how this can help to prevent
btrfsck to open a device that is mounted. What am I getting wrong? Currently, I
would do the same check as in the original patch, but in btrfs_open_devices()
instead of main().

	Cheers,
		Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html