[PATCH 1/3] Btrfs-progs: add support to set subvolume/snapshot readonly

[PATCH 1/3] Btrfs-progs: add support to set subvolume/snapshot readonly

[Liu Bo]

Setting subvolume/snapshot readonly has been missing for a long time.

With this patch, we can set a subvolume/snapshot readonly via:

o    btrfs subvolume set-ro <path>

Signed-off-by: Liu Bo <liubo2009@cn.fujitsu.com>
---
 cmds-subvolume.c |   40 ++++++++++++++++++++++++++++++++++++++++
 ioctl.h          |    7 +++++++
 2 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 950fa8f..5ee31cb 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -512,6 +512,44 @@ static int cmd_find_new(int argc, char **argv)
 	return 0;
 }
 
+static const char * const cmd_subvol_set_ro_usage[] = {
+	"btrfs subvolume set-ro <path>",
+	"Set the subvolume ro",
+	NULL
+};
+
+static int cmd_subvol_set_ro(int argc, char **argv)
+{
+	int	ret=0, fd, e;
+	char	*path;
+	struct	btrfs_ioctl_get_set_flags_args args;
+
+	if (check_argc_exact(argc, 2))
+		usage(cmd_subvol_set_ro_usage);
+
+	path = argv[1];
+
+	memset(&args, 0, sizeof(args));
+
+	fd = open_file_or_dir(path);
+	if (fd < 0) {
+		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
+		return 12;
+	}
+
+	args.flags |= BTRFS_SUBVOL_RDONLY;
+	args.objectid = 0;
+	ret = ioctl(fd, BTRFS_IOC_SUBVOL_SETFLAGS, &args);
+	e = errno;
+	close(fd);
+	if( ret < 0 ){
+		fprintf(stderr, "ERROR: unable to set a subvolume RO- %s\n",
+			strerror(e));
+		return 30;
+	}
+	return 0;
+}
+
 const struct cmd_group subvolume_cmd_group = {
 	subvolume_cmd_group_usage, NULL, {
 		{ "create", cmd_subvol_create, cmd_subvol_create_usage, NULL, 0 },
@@ -522,6 +560,8 @@ const struct cmd_group subvolume_cmd_group = {
 			cmd_subvol_get_default_usage, NULL, 0 },
 		{ "set-default", cmd_subvol_set_default,
 			cmd_subvol_set_default_usage, NULL, 0 },
+		{ "set-ro", cmd_subvol_set_ro,
+			cmd_subvol_set_ro_usage, NULL, 0 },
 		{ "find-new", cmd_find_new, cmd_find_new_usage, NULL, 0 },
 		{ 0, 0, 0, 0, 0 }
 	}
diff --git a/ioctl.h b/ioctl.h
index f2e5d8d..9c066eb 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -42,6 +42,11 @@ struct btrfs_ioctl_vol_args_v2 {
 	char name[BTRFS_SUBVOL_NAME_MAX + 1];
 };
 
+struct btrfs_ioctl_get_set_flags_args {
+	__u64 objectid;
+	__u64 flags;
+};
+
 #define BTRFS_FSID_SIZE 16
 #define BTRFS_UUID_SIZE 16
 
@@ -312,6 +317,8 @@ struct btrfs_ioctl_logical_ino_args {
 				    struct btrfs_ioctl_space_args)
 #define BTRFS_IOC_SNAP_CREATE_V2 _IOW(BTRFS_IOCTL_MAGIC, 23, \
 				   struct btrfs_ioctl_vol_args_v2)
+#define BTRFS_IOC_SUBVOL_GETFLAGS _IOW(BTRFS_IOCTL_MAGIC, 25, __u64)
+#define BTRFS_IOC_SUBVOL_SETFLAGS _IOW(BTRFS_IOCTL_MAGIC, 26, __u64)
 #define BTRFS_IOC_SCRUB _IOWR(BTRFS_IOCTL_MAGIC, 27, \
 				struct btrfs_ioctl_scrub_args)
 #define BTRFS_IOC_SCRUB_CANCEL _IO(BTRFS_IOCTL_MAGIC, 28)
-- 
1.6.5.2

[PATCH 2/3] Btrfs-progs: make get default report correctly

[Liu Bo]

We have both set-default and get-default, but actually our get-default
does not show which one is the default one.

This patch plus the kernel side patch fix this.

Signed-off-by: Liu Bo <liubo2009@cn.fujitsu.com>
---
 Makefile         |    6 +++---
 btrfs-list.c     |   44 +++++++++++++++++++++++++++++++++++++++++---
 cmds-subvolume.c |   16 +++-------------
 ioctl.h          |    1 +
 4 files changed, 48 insertions(+), 19 deletions(-)

diff --git a/Makefile b/Makefile
index 79818e6..5d10026 100644
--- a/Makefile
+++ b/Makefile
@@ -4,7 +4,7 @@ CFLAGS = -g -O0
 objects = ctree.o disk-io.o radix-tree.o extent-tree.o print-tree.o \
 	  root-tree.o dir-item.o file-item.o inode-item.o \
 	  inode-map.o crc32c.o rbtree.o extent-cache.o extent_io.o \
-	  volumes.o utils.o btrfs-list.o btrfslabel.o repair.o
+	  volumes.o utils.o btrfs-list.o btrfslabel.o repair.o common.o
 cmds_objects = cmds-subvolume.o cmds-filesystem.o cmds-device.o cmds-scrub.o \
 	       cmds-inspect.o cmds-balance.o
 
@@ -39,8 +39,8 @@ all: version $(progs) manpages
 version:
 	bash version.sh
 
-btrfs: $(objects) btrfs.o help.o common.o $(cmds_objects)
-	$(CC) $(CFLAGS) -o btrfs btrfs.o help.o common.o $(cmds_objects) \
+btrfs: $(objects) btrfs.o help.o $(cmds_objects)
+	$(CC) $(CFLAGS) -o btrfs btrfs.o help.o $(cmds_objects) \
 		$(objects) $(LDFLAGS) $(LIBS) -lpthread
 
 calc-size: $(objects) calc-size.o
diff --git a/btrfs-list.c b/btrfs-list.c
index 5f4a9be..f1baa52 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -34,6 +34,7 @@
 #include "ctree.h"
 #include "transaction.h"
 #include "utils.h"
+#include "commands.h"
 
 /* we store all the roots we find in an rbtree so that we can
  * search for them later.
@@ -668,7 +669,42 @@ static int __list_subvol_fill_paths(int fd, struct root_lookup *root_lookup)
 	return 0;
 }
 
-int list_subvols(int fd, int print_parent)
+int subvol_get_set_flags(int fd, int set, u64 flags, u64 root_id)
+{
+	int ret = 0, e;
+	struct btrfs_ioctl_get_set_flags_args args;
+
+	memset(&args, 0, sizeof(args));
+
+	if (set && flags)
+		args.flags |= flags;
+	args.objectid = root_id;
+
+	if (set)
+		ret = ioctl(fd, BTRFS_IOC_SUBVOL_SETFLAGS, &args);
+	else
+		ret = ioctl(fd, BTRFS_IOC_SUBVOL_GETFLAGS, &args);
+	e = errno;
+	if( ret < 0 ){
+		fprintf(stderr, "ERROR: unable to %s a subvolume flags- %s\n",
+			(set) ? "set" : "get", strerror(e));
+		return 30;
+	} else if (!set) {
+		u64 flags_to_get = args.flags;
+		if (flags_to_get)
+			printf(" (");
+		if (flags_to_get & BTRFS_SUBVOL_RDONLY)
+			printf("Readonly,");
+		if (flags_to_get & BTRFS_SUBVOL_DEFAULT)
+			printf("Default,");
+		if (flags_to_get)
+			printf(")");
+		printf("\n");
+	}
+	return 0;
+}
+
+int list_subvols(int fd, int print_parent, int get_default)
 {
 	struct root_lookup root_lookup;
 	struct rb_node *n;
@@ -704,15 +740,17 @@ int list_subvols(int fd, int print_parent)
 		resolve_root(&root_lookup, entry, &root_id, &parent_id,
 				&level, &path);
 		if (print_parent) {
-			printf("ID %llu parent %llu top level %llu path %s\n",
+			printf("ID %llu parent %llu top level %llu path %s",
 				(unsigned long long)root_id,
 				(unsigned long long)parent_id,
 				(unsigned long long)level, path);
 		} else {
-			printf("ID %llu top level %llu path %s\n",
+			printf("ID %llu top level %llu path %s",
 				(unsigned long long)root_id,
 				(unsigned long long)level, path);
 		}
+		if (subvol_get_set_flags(fd, 0, 0, root_id))
+			printf("\n");
 		free(path);
 		n = rb_prev(n);
 	}
diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 5ee31cb..8783e67 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -32,6 +32,7 @@
 /* btrfs-list.c */
 int list_subvols(int fd, int print_parent, int get_default);
 int find_updated_files(int fd, u64 root_id, u64 oldest_gen);
+int subvol_get_set_flags(int fd, int set, u64 flags, u64 root_id);
 
 static const char * const subvolume_cmd_group_usage[] = {
 	"btrfs subvolume <command> <args>",
@@ -520,33 +521,22 @@ static const char * const cmd_subvol_set_ro_usage[] = {
 
 static int cmd_subvol_set_ro(int argc, char **argv)
 {
-	int	ret=0, fd, e;
+	int	ret=0, fd;
 	char	*path;
-	struct	btrfs_ioctl_get_set_flags_args args;
 
 	if (check_argc_exact(argc, 2))
 		usage(cmd_subvol_set_ro_usage);
 
 	path = argv[1];
 
-	memset(&args, 0, sizeof(args));
-
 	fd = open_file_or_dir(path);
 	if (fd < 0) {
 		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
 		return 12;
 	}
 
-	args.flags |= BTRFS_SUBVOL_RDONLY;
-	args.objectid = 0;
-	ret = ioctl(fd, BTRFS_IOC_SUBVOL_SETFLAGS, &args);
-	e = errno;
+	ret = subvol_get_set_flags(fd, 1, BTRFS_SUBVOL_RDONLY, 0);
 	close(fd);
-	if( ret < 0 ){
-		fprintf(stderr, "ERROR: unable to set a subvolume RO- %s\n",
-			strerror(e));
-		return 30;
-	}
 	return 0;
 }
 
diff --git a/ioctl.h b/ioctl.h
index 9c066eb..936d075 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -32,6 +32,7 @@ struct btrfs_ioctl_vol_args {
 };
 
 #define BTRFS_SUBVOL_RDONLY		(1ULL << 1)
+#define BTRFS_SUBVOL_DEFAULT		(1ULL << 2)
 #define BTRFS_SUBVOL_NAME_MAX 4039
 
 struct btrfs_ioctl_vol_args_v2 {
-- 
1.6.5.2

[PATCH 3/3] Btrfs-progs: add 's' option for 'btrfs subvolume list'

[Liu Bo]

We want 'btrfs subvolume list' to act as 'ls', which means that
we can not only list out all the subvolumes we have, but also list
each single one.

So this patch add 's' option to show a single one:

$ ./btrfs sub list /mnt/btrfs/
ID 256 top level 5 path subvol (Readonly,)
ID 257 top level 5 path snapshot
ID 258 top level 5 path subvol2
ID 259 top level 5 path subvol2/subvol3

$ ./btrfs sub list -s /mnt/btrfs/subvol
ID 256 top level 5 path subvol (Readonly,)

Signed-off-by: Liu Bo <liubo2009@cn.fujitsu.com>
---
 btrfs-list.c     |   41 ++++++++++++++++++++++++++++++++++++++++-
 cmds-subvolume.c |   15 ++++++++++-----
 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/btrfs-list.c b/btrfs-list.c
index f1baa52..3e79239 100644
--- a/btrfs-list.c
+++ b/btrfs-list.c
@@ -312,6 +312,30 @@ static int lookup_ino_path(int fd, struct root_info *ri)
 	return 0;
 }
 
+/*
+ * helper function for getting the root which the file is belonged to.
+ */
+static int lookup_ino_rootid(int fd, u64 *rootid)
+{
+	struct btrfs_ioctl_ino_lookup_args args;
+	int ret, e;
+
+	memset(&args, 0, sizeof(args));
+	args.treeid = 0;
+	args.objectid = BTRFS_FIRST_FREE_OBJECTID;
+
+	ret = ioctl(fd, BTRFS_IOC_INO_LOOKUP, &args);
+	e = errno;
+	if (ret) {
+		fprintf(stderr, "ERROR: Failed to lookup root id - %s\n",
+			strerror(e));
+		return ret;
+	}
+
+	*rootid = args.treeid;
+	return 0;
+}
+
 /* finding the generation for a given path is a two step process.
  * First we use the inode loookup routine to find out the root id
  *
@@ -704,11 +728,12 @@ int subvol_get_set_flags(int fd, int set, u64 flags, u64 root_id)
 	return 0;
 }
 
-int list_subvols(int fd, int print_parent, int get_default)
+int list_subvols(int fd, int print_parent, int print_self, int get_default)
 {
 	struct root_lookup root_lookup;
 	struct rb_node *n;
 	int ret;
+	u64 subvolid = 0;
 
 	ret = __list_subvol_search(fd, &root_lookup);
 	if (ret) {
@@ -725,6 +750,9 @@ int list_subvols(int fd, int print_parent, int get_default)
 	if (ret < 0)
 		return ret;
 
+	if (print_self)
+		lookup_ino_rootid(fd, &subvolid);
+
 	/* now that we have all the subvol-relative paths filled in,
 	 * we have to string the subvols together so that we can get
 	 * a path all the way back to the FS root
@@ -739,6 +767,14 @@ int list_subvols(int fd, int print_parent, int get_default)
 		entry = rb_entry(n, struct root_info, rb_node);
 		resolve_root(&root_lookup, entry, &root_id, &parent_id,
 				&level, &path);
+
+		/* print this subvolume only */
+		if (print_self && subvolid != root_id) {
+			free(path);
+			n = rb_prev(n);
+			continue;
+		}
+
 		if (print_parent) {
 			printf("ID %llu parent %llu top level %llu path %s",
 				(unsigned long long)root_id,
@@ -753,6 +789,9 @@ int list_subvols(int fd, int print_parent, int get_default)
 			printf("\n");
 		free(path);
 		n = rb_prev(n);
+
+		if (print_self)
+			break;
 	}
 
 	return ret;
diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 8783e67..f779b78 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -30,7 +30,7 @@
 #include "commands.h"
 
 /* btrfs-list.c */
-int list_subvols(int fd, int print_parent, int get_default);
+int list_subvols(int fd, int print_parent, int print_self, int get_default);
 int find_updated_files(int fd, u64 root_id, u64 oldest_gen);
 int subvol_get_set_flags(int fd, int set, u64 flags, u64 root_id);
 
@@ -218,10 +218,11 @@ static int cmd_subvol_delete(int argc, char **argv)
 }
 
 static const char * const cmd_subvol_list_usage[] = {
-	"btrfs subvolume list [-p] <path>",
+	"btrfs subvolume list [-ps] <path>",
 	"List subvolumes (and snapshots)",
 	"",
 	"-p     print parent ID",
+	"-s     print this subvolume only",
 	NULL
 };
 
@@ -230,11 +231,12 @@ static int cmd_subvol_list(int argc, char **argv)
 	int fd;
 	int ret;
 	int print_parent = 0;
+	int print_self = 0;
 	char *subvol;
 
 	optind = 1;
 	while(1) {
-		int c = getopt(argc, argv, "p");
+		int c = getopt(argc, argv, "ps");
 		if (c < 0)
 			break;
 
@@ -242,6 +244,9 @@ static int cmd_subvol_list(int argc, char **argv)
 		case 'p':
 			print_parent = 1;
 			break;
+		case 's':
+			print_self = 1;
+			break;
 		default:
 			usage(cmd_subvol_list_usage);
 		}
@@ -267,7 +272,7 @@ static int cmd_subvol_list(int argc, char **argv)
 		fprintf(stderr, "ERROR: can't access '%s'\n", subvol);
 		return 12;
 	}
-	ret = list_subvols(fd, print_parent, 0);
+	ret = list_subvols(fd, print_parent, print_self, 0);
 	if (ret)
 		return 19;
 	return 0;
@@ -426,7 +431,7 @@ static int cmd_subvol_get_default(int argc, char **argv)
 		fprintf(stderr, "ERROR: can't access '%s'\n", subvol);
 		return 12;
 	}
-	ret = list_subvols(fd, 0, 1);
+	ret = list_subvols(fd, 0, 0, 1);
 	if (ret)
 		return 19;
 	return 0;
-- 
1.6.5.2

Re: [PATCH 1/3] Btrfs-progs: add support to set subvolume/snapshot readonly

[Ilya Dryomov]

On Fri, Jun 29, 2012 at 06:00:36PM +0800, Liu Bo wrote:
> Setting subvolume/snapshot readonly has been missing for a long time.
> 
> With this patch, we can set a subvolume/snapshot readonly via:
> 
> o    btrfs subvolume set-ro <path>

Alexander's 'btrfs property' patches do exactly this, but in a much more
generic and extensible way.  'btrfs property' subgroup provides a
uniform interface for getting and setting properties of filesystem
objects in general, not only those of subvolumes and snapshots.  It
provides a much better user interface, and it also allows us to easily
rethink kernel-user interface for generic get/set in future.

Thanks,

		Ilya

Re: [PATCH 2/3] Btrfs-progs: make get default report correctly

[Ilya Dryomov]

On Fri, Jun 29, 2012 at 06:00:37PM +0800, Liu Bo wrote:
> We have both set-default and get-default, but actually our get-default
> does not show which one is the default one.
> 
> This patch plus the kernel side patch fix this.

Are you referring to the fact that get-default in Chris' btrfs-progs
master lists all subvolumes instead of showing the default one?  If so,
I fixed that problem a while ago, patch is in Hugo's integration branch.

Thanks,

		Ilya

Re: [PATCH 2/3] Btrfs-progs: make get default report correctly

[Liu Bo]

On 06/29/2012 06:36 PM, Ilya Dryomov wrote:

> On Fri, Jun 29, 2012 at 06:00:37PM +0800, Liu Bo wrote:
>> We have both set-default and get-default, but actually our get-default
>> does not show which one is the default one.
>>
>> This patch plus the kernel side patch fix this.
> 
> Are you referring to the fact that get-default in Chris' btrfs-progs
> master lists all subvolumes instead of showing the default one?  If so,
> I fixed that problem a while ago, patch is in Hugo's integration branch.
> 


So can you please show me a link about your patch?

thanks,
liubo

> Thanks,
> 
> 		Ilya
> --
> 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
> 

Re: [PATCH 1/3] Btrfs-progs: add support to set subvolume/snapshot readonly

[Liu Bo]

On 06/29/2012 06:21 PM, Ilya Dryomov wrote:

> On Fri, Jun 29, 2012 at 06:00:36PM +0800, Liu Bo wrote:
>> Setting subvolume/snapshot readonly has been missing for a long time.
>>
>> With this patch, we can set a subvolume/snapshot readonly via:
>>
>> o    btrfs subvolume set-ro <path>
> 
> Alexander's 'btrfs property' patches do exactly this, but in a much more
> generic and extensible way.  'btrfs property' subgroup provides a
> uniform interface for getting and setting properties of filesystem
> objects in general, not only those of subvolumes and snapshots.  It
> provides a much better user interface, and it also allows us to easily
> rethink kernel-user interface for generic get/set in future.
> 


Thanks for the explanation!

But I prefer keeping the current categories {subvolume,filesystem,device,...}:

o Compatibility, we cannot remove the old commands until we make sure that no users will
  use them.

o We've three properties {default, readonly, lable}, is it worthy making another new interface?

o Current categories are clear and clean.


thanks,
liubo

> Thanks,
> 
> 		Ilya
> --
> 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
> 

Re: [PATCH 2/3] Btrfs-progs: make get default report correctly

[Ilya Dryomov]

On Mon, Jul 02, 2012 at 09:39:45AM +0800, Liu Bo wrote:
> On 06/29/2012 06:36 PM, Ilya Dryomov wrote:
> 
> > On Fri, Jun 29, 2012 at 06:00:37PM +0800, Liu Bo wrote:
> >> We have both set-default and get-default, but actually our get-default
> >> does not show which one is the default one.
> >>
> >> This patch plus the kernel side patch fix this.
> > 
> > Are you referring to the fact that get-default in Chris' btrfs-progs
> > master lists all subvolumes instead of showing the default one?  If so,
> > I fixed that problem a while ago, patch is in Hugo's integration branch.
> > 
> 
> 
> So can you please show me a link about your patch?

I'm sorry, it turns out Hugo hasn't pushed it yet.  Here is a link:
http://thread.gmane.org/gmane.comp.file-systems.btrfs/16187/

Thanks,

		Ilya

Re: [PATCH 1/3] Btrfs-progs: add support to set subvolume/snapshot readonly

[Ilya Dryomov]

On Mon, Jul 02, 2012 at 10:07:42AM +0800, Liu Bo wrote:
> On 06/29/2012 06:21 PM, Ilya Dryomov wrote:
> 
> > On Fri, Jun 29, 2012 at 06:00:36PM +0800, Liu Bo wrote:
> >> Setting subvolume/snapshot readonly has been missing for a long time.
> >>
> >> With this patch, we can set a subvolume/snapshot readonly via:
> >>
> >> o    btrfs subvolume set-ro <path>
> > 
> > Alexander's 'btrfs property' patches do exactly this, but in a much more
> > generic and extensible way.  'btrfs property' subgroup provides a
> > uniform interface for getting and setting properties of filesystem
> > objects in general, not only those of subvolumes and snapshots.  It
> > provides a much better user interface, and it also allows us to easily
> > rethink kernel-user interface for generic get/set in future.
> > 
> 
> 
> Thanks for the explanation!
> 
> But I prefer keeping the current categories {subvolume,filesystem,device,...}:
> 
> o Compatibility, we cannot remove the old commands until we make sure that no users will
>   use them.

We are not going to remove old commands any time soon.  However, adding
new ones that clearly fall into get/set category, is not a good idea.
Especially when there is a generic interface on its way.

> 
> o We've three properties {default, readonly, lable}, is it worthy making another new interface?

It's not just about subvolumes.  There will be a lot more properties on
the table as filesystem matures, for example device speeds, subvolume
profiles, quotas.

> 
> o Current categories are clear and clean.

Once again, it's not just about subvolumes.  Current categories are
indeed clear, but adding two commands for each non-trivial property that
comes up in future does not seem practical to me.

Thanks,

		Ilya

Re: [PATCH 2/3] Btrfs-progs: make get default report correctly

[Liu Bo]

On 07/02/2012 05:45 PM, Ilya Dryomov wrote:

> On Mon, Jul 02, 2012 at 09:39:45AM +0800, Liu Bo wrote:
>> On 06/29/2012 06:36 PM, Ilya Dryomov wrote:
>>
>>> On Fri, Jun 29, 2012 at 06:00:37PM +0800, Liu Bo wrote:
>>>> We have both set-default and get-default, but actually our get-default
>>>> does not show which one is the default one.
>>>>
>>>> This patch plus the kernel side patch fix this.
>>> Are you referring to the fact that get-default in Chris' btrfs-progs
>>> master lists all subvolumes instead of showing the default one?  If so,
>>> I fixed that problem a while ago, patch is in Hugo's integration branch.
>>>
>>
>> So can you please show me a link about your patch?
> 
> I'm sorry, it turns out Hugo hasn't pushed it yet.  Here is a link:
> http://thread.gmane.org/gmane.comp.file-systems.btrfs/16187/
> 


Fairly good patches.

thanks,
liubo

> Thanks,
> 
> 		Ilya
> --
> 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
> 

Re: [PATCH 1/3] Btrfs-progs: add support to set subvolume/snapshot readonly

[Liu Bo]

On 07/02/2012 06:00 PM, Ilya Dryomov wrote:

> On Mon, Jul 02, 2012 at 10:07:42AM +0800, Liu Bo wrote:
>> On 06/29/2012 06:21 PM, Ilya Dryomov wrote:
>>
>>> On Fri, Jun 29, 2012 at 06:00:36PM +0800, Liu Bo wrote:
>>>> Setting subvolume/snapshot readonly has been missing for a long time.
>>>>
>>>> With this patch, we can set a subvolume/snapshot readonly via:
>>>>
>>>> o    btrfs subvolume set-ro <path>
>>> Alexander's 'btrfs property' patches do exactly this, but in a much more
>>> generic and extensible way.  'btrfs property' subgroup provides a
>>> uniform interface for getting and setting properties of filesystem
>>> objects in general, not only those of subvolumes and snapshots.  It
>>> provides a much better user interface, and it also allows us to easily
>>> rethink kernel-user interface for generic get/set in future.
>>>
>>
>> Thanks for the explanation!
>>
>> But I prefer keeping the current categories {subvolume,filesystem,device,...}:
>>
>> o Compatibility, we cannot remove the old commands until we make sure that no users will
>>   use them.
> 
> We are not going to remove old commands any time soon.  However, adding
> new ones that clearly fall into get/set category, is not a good idea.
> Especially when there is a generic interface on its way.
> 
>> o We've three properties {default, readonly, lable}, is it worthy making another new interface?
> 
> It's not just about subvolumes.  There will be a lot more properties on
> the table as filesystem matures, for example device speeds, subvolume
> profiles, quotas.
> 
>> o Current categories are clear and clean.
> 
> Once again, it's not just about subvolumes.  Current categories are
> indeed clear, but adding two commands for each non-trivial property that
> comes up in future does not seem practical to me.
> 



I see, that's reasonable. :)

Besides set/get-ro and get-default, I also want to have 'btrfs subvolume list' work as 'ls', that is,
it can list not only all of items, but also a single item.

And I have made a patch for that (it refers to [PATCH 3/3] Btrfs-progs: add 's' option for 'btrfs subvolume list').

What's your opinion about it?

thanks,
liubo

> Thanks,
> 
> 		Ilya
> --
> 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
> 

Re: [PATCH 3/3] Btrfs-progs: add 's' option for 'btrfs subvolume list'

[David Sterba]

On Fri, Jun 29, 2012 at 06:00:38PM +0800, Liu Bo wrote:
> We want 'btrfs subvolume list' to act as 'ls', which means that
> we can not only list out all the subvolumes we have, but also list
> each single one.
> 
> So this patch add 's' option to show a single one:
> 
> $ ./btrfs sub list /mnt/btrfs/
> ID 256 top level 5 path subvol (Readonly,)
> ID 257 top level 5 path snapshot
> ID 258 top level 5 path subvol2
> ID 259 top level 5 path subvol2/subvol3
> 
> $ ./btrfs sub list -s /mnt/btrfs/subvol
> ID 256 top level 5 path subvol (Readonly,)

suggestions and comments:

1) show the subvolume flags only if an option is given, similar to -p
   (to show parent subvol),

2) move the flags before the subvolume path -- it is of a
   variable length and it's a bit easier (but not reliable) to parse it
   from scripts

sidenote: 'find /mnt -inum 256 -print0' will list all subvolumes in a
way that's resitent to funny characters in the path, but is slow as it
has to traverse the filesystem (and 'find' does not support a true
breadth-first-search, needs to be iterated with -mindepth/maxdepth).

the 'subvol list' command could mimic the -print0 and print the
subvolume paths terminated by '\0'.

3) drop the , if there's only one subvol property

4) the '-s' option on the mountpoint does not show anything, though I
   would expect that, eg when the mountpoint is a subvolume

--

one comment to code below

david

> 
> Signed-off-by: Liu Bo <liubo2009@cn.fujitsu.com>
> ---
>  btrfs-list.c     |   41 ++++++++++++++++++++++++++++++++++++++++-
>  cmds-subvolume.c |   15 ++++++++++-----
>  2 files changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/btrfs-list.c b/btrfs-list.c
> index f1baa52..3e79239 100644
> --- a/btrfs-list.c
> +++ b/btrfs-list.c
> @@ -312,6 +312,30 @@ static int lookup_ino_path(int fd, struct root_info *ri)
>  	return 0;
>  }
>  
> +/*
> + * helper function for getting the root which the file is belonged to.
> + */
> +static int lookup_ino_rootid(int fd, u64 *rootid)
> +{
> +	struct btrfs_ioctl_ino_lookup_args args;
> +	int ret, e;
> +
> +	memset(&args, 0, sizeof(args));
> +	args.treeid = 0;
> +	args.objectid = BTRFS_FIRST_FREE_OBJECTID;
> +
> +	ret = ioctl(fd, BTRFS_IOC_INO_LOOKUP, &args);
> +	e = errno;
> +	if (ret) {
> +		fprintf(stderr, "ERROR: Failed to lookup root id - %s\n",
> +			strerror(e));
> +		return ret;
> +	}
> +
> +	*rootid = args.treeid;
> +	return 0;
> +}
> +
>  /* finding the generation for a given path is a two step process.
>   * First we use the inode loookup routine to find out the root id
>   *
> @@ -704,11 +728,12 @@ int subvol_get_set_flags(int fd, int set, u64 flags, u64 root_id)
>  	return 0;
>  }
>  
> -int list_subvols(int fd, int print_parent, int get_default)
> +int list_subvols(int fd, int print_parent, int print_self, int get_default)
>  {
>  	struct root_lookup root_lookup;
>  	struct rb_node *n;
>  	int ret;
> +	u64 subvolid = 0;
>  
>  	ret = __list_subvol_search(fd, &root_lookup);
>  	if (ret) {
> @@ -725,6 +750,9 @@ int list_subvols(int fd, int print_parent, int get_default)
>  	if (ret < 0)
>  		return ret;
>  
> +	if (print_self)
> +		lookup_ino_rootid(fd, &subvolid);

you should probably check the return value

> +
>  	/* now that we have all the subvol-relative paths filled in,
>  	 * we have to string the subvols together so that we can get
>  	 * a path all the way back to the FS root
> @@ -739,6 +767,14 @@ int list_subvols(int fd, int print_parent, int get_default)
>  		entry = rb_entry(n, struct root_info, rb_node);
>  		resolve_root(&root_lookup, entry, &root_id, &parent_id,
>  				&level, &path);
> +
> +		/* print this subvolume only */
> +		if (print_self && subvolid != root_id) {
> +			free(path);
> +			n = rb_prev(n);
> +			continue;
> +		}
> +
>  		if (print_parent) {
>  			printf("ID %llu parent %llu top level %llu path %s",
>  				(unsigned long long)root_id,
> @@ -753,6 +789,9 @@ int list_subvols(int fd, int print_parent, int get_default)
>  			printf("\n");
>  		free(path);
>  		n = rb_prev(n);
> +
> +		if (print_self)
> +			break;
>  	}
>  
>  	return ret;
> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
> index 8783e67..f779b78 100644
> --- a/cmds-subvolume.c
> +++ b/cmds-subvolume.c
> @@ -30,7 +30,7 @@
>  #include "commands.h"
>  
>  /* btrfs-list.c */
> -int list_subvols(int fd, int print_parent, int get_default);
> +int list_subvols(int fd, int print_parent, int print_self, int get_default);
>  int find_updated_files(int fd, u64 root_id, u64 oldest_gen);
>  int subvol_get_set_flags(int fd, int set, u64 flags, u64 root_id);
>  
> @@ -218,10 +218,11 @@ static int cmd_subvol_delete(int argc, char **argv)
>  }
>  
>  static const char * const cmd_subvol_list_usage[] = {
> -	"btrfs subvolume list [-p] <path>",
> +	"btrfs subvolume list [-ps] <path>",
>  	"List subvolumes (and snapshots)",
>  	"",
>  	"-p     print parent ID",
> +	"-s     print this subvolume only",
>  	NULL
>  };
>  
> @@ -230,11 +231,12 @@ static int cmd_subvol_list(int argc, char **argv)
>  	int fd;
>  	int ret;
>  	int print_parent = 0;
> +	int print_self = 0;
>  	char *subvol;
>  
>  	optind = 1;
>  	while(1) {
> -		int c = getopt(argc, argv, "p");
> +		int c = getopt(argc, argv, "ps");
>  		if (c < 0)
>  			break;
>  
> @@ -242,6 +244,9 @@ static int cmd_subvol_list(int argc, char **argv)
>  		case 'p':
>  			print_parent = 1;
>  			break;
> +		case 's':
> +			print_self = 1;
> +			break;
>  		default:
>  			usage(cmd_subvol_list_usage);
>  		}
> @@ -267,7 +272,7 @@ static int cmd_subvol_list(int argc, char **argv)
>  		fprintf(stderr, "ERROR: can't access '%s'\n", subvol);
>  		return 12;
>  	}
> -	ret = list_subvols(fd, print_parent, 0);
> +	ret = list_subvols(fd, print_parent, print_self, 0);
>  	if (ret)
>  		return 19;
>  	return 0;
> @@ -426,7 +431,7 @@ static int cmd_subvol_get_default(int argc, char **argv)
>  		fprintf(stderr, "ERROR: can't access '%s'\n", subvol);
>  		return 12;
>  	}
> -	ret = list_subvols(fd, 0, 1);
> +	ret = list_subvols(fd, 0, 0, 1);
>  	if (ret)
>  		return 19;
>  	return 0;
> -- 
> 1.6.5.2
> 
> --
> 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

Re: [PATCH 3/3] Btrfs-progs: add 's' option for 'btrfs subvolume list'

[Liu Bo]

On 07/04/2012 11:41 PM, David Sterba wrote:

> On Fri, Jun 29, 2012 at 06:00:38PM +0800, Liu Bo wrote:
>> We want 'btrfs subvolume list' to act as 'ls', which means that
>> we can not only list out all the subvolumes we have, but also list
>> each single one.
>>
>> So this patch add 's' option to show a single one:
>>
>> $ ./btrfs sub list /mnt/btrfs/
>> ID 256 top level 5 path subvol (Readonly,)
>> ID 257 top level 5 path snapshot
>> ID 258 top level 5 path subvol2
>> ID 259 top level 5 path subvol2/subvol3
>>
>> $ ./btrfs sub list -s /mnt/btrfs/subvol
>> ID 256 top level 5 path subvol (Readonly,)
> 
> suggestions and comments:
> 
> 1) show the subvolume flags only if an option is given, similar to -p
>    (to show parent subvol),
> 
> 2) move the flags before the subvolume path -- it is of a
>    variable length and it's a bit easier (but not reliable) to parse it
>    from scripts
> 
> sidenote: 'find /mnt -inum 256 -print0' will list all subvolumes in a
> way that's resitent to funny characters in the path, but is slow as it
> has to traverse the filesystem (and 'find' does not support a true
> breadth-first-search, needs to be iterated with -mindepth/maxdepth).
> 
> the 'subvol list' command could mimic the -print0 and print the
> subvolume paths terminated by '\0'.
> 
> 3) drop the , if there's only one subvol property
> 
> 4) the '-s' option on the mountpoint does not show anything, though I
>    would expect that, eg when the mountpoint is a subvolume
> 
> --
> 
> one comment to code below
> 


Hi David,

Thanks a lot for reviewing this!  Really nice advice.

This patch is not compatible with Alex's 'btrfs property' patchset, and after some
discussion with Alex and Ilya, I've agreed to rebase this patch onto their patchset,
but I'll definitely apply all of your comments. :)

thanks,
liubo

> david
> 
>> Signed-off-by: Liu Bo <liubo2009@cn.fujitsu.com>
>> ---
>>  btrfs-list.c     |   41 ++++++++++++++++++++++++++++++++++++++++-
>>  cmds-subvolume.c |   15 ++++++++++-----
>>  2 files changed, 50 insertions(+), 6 deletions(-)
>>
>> diff --git a/btrfs-list.c b/btrfs-list.c
>> index f1baa52..3e79239 100644
>> --- a/btrfs-list.c
>> +++ b/btrfs-list.c
>> @@ -312,6 +312,30 @@ static int lookup_ino_path(int fd, struct root_info *ri)
>>  	return 0;
>>  }
>>  
>> +/*
>> + * helper function for getting the root which the file is belonged to.
>> + */
>> +static int lookup_ino_rootid(int fd, u64 *rootid)
>> +{
>> +	struct btrfs_ioctl_ino_lookup_args args;
>> +	int ret, e;
>> +
>> +	memset(&args, 0, sizeof(args));
>> +	args.treeid = 0;
>> +	args.objectid = BTRFS_FIRST_FREE_OBJECTID;
>> +
>> +	ret = ioctl(fd, BTRFS_IOC_INO_LOOKUP, &args);
>> +	e = errno;
>> +	if (ret) {
>> +		fprintf(stderr, "ERROR: Failed to lookup root id - %s\n",
>> +			strerror(e));
>> +		return ret;
>> +	}
>> +
>> +	*rootid = args.treeid;
>> +	return 0;
>> +}
>> +
>>  /* finding the generation for a given path is a two step process.
>>   * First we use the inode loookup routine to find out the root id
>>   *
>> @@ -704,11 +728,12 @@ int subvol_get_set_flags(int fd, int set, u64 flags, u64 root_id)
>>  	return 0;
>>  }
>>  
>> -int list_subvols(int fd, int print_parent, int get_default)
>> +int list_subvols(int fd, int print_parent, int print_self, int get_default)
>>  {
>>  	struct root_lookup root_lookup;
>>  	struct rb_node *n;
>>  	int ret;
>> +	u64 subvolid = 0;
>>  
>>  	ret = __list_subvol_search(fd, &root_lookup);
>>  	if (ret) {
>> @@ -725,6 +750,9 @@ int list_subvols(int fd, int print_parent, int get_default)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> +	if (print_self)
>> +		lookup_ino_rootid(fd, &subvolid);
> 
> you should probably check the return value
> 
>> +
>>  	/* now that we have all the subvol-relative paths filled in,
>>  	 * we have to string the subvols together so that we can get
>>  	 * a path all the way back to the FS root
>> @@ -739,6 +767,14 @@ int list_subvols(int fd, int print_parent, int get_default)
>>  		entry = rb_entry(n, struct root_info, rb_node);
>>  		resolve_root(&root_lookup, entry, &root_id, &parent_id,
>>  				&level, &path);
>> +
>> +		/* print this subvolume only */
>> +		if (print_self && subvolid != root_id) {
>> +			free(path);
>> +			n = rb_prev(n);
>> +			continue;
>> +		}
>> +
>>  		if (print_parent) {
>>  			printf("ID %llu parent %llu top level %llu path %s",
>>  				(unsigned long long)root_id,
>> @@ -753,6 +789,9 @@ int list_subvols(int fd, int print_parent, int get_default)
>>  			printf("\n");
>>  		free(path);
>>  		n = rb_prev(n);
>> +
>> +		if (print_self)
>> +			break;
>>  	}
>>  
>>  	return ret;
>> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
>> index 8783e67..f779b78 100644
>> --- a/cmds-subvolume.c
>> +++ b/cmds-subvolume.c
>> @@ -30,7 +30,7 @@
>>  #include "commands.h"
>>  
>>  /* btrfs-list.c */
>> -int list_subvols(int fd, int print_parent, int get_default);
>> +int list_subvols(int fd, int print_parent, int print_self, int get_default);
>>  int find_updated_files(int fd, u64 root_id, u64 oldest_gen);
>>  int subvol_get_set_flags(int fd, int set, u64 flags, u64 root_id);
>>  
>> @@ -218,10 +218,11 @@ static int cmd_subvol_delete(int argc, char **argv)
>>  }
>>  
>>  static const char * const cmd_subvol_list_usage[] = {
>> -	"btrfs subvolume list [-p] <path>",
>> +	"btrfs subvolume list [-ps] <path>",
>>  	"List subvolumes (and snapshots)",
>>  	"",
>>  	"-p     print parent ID",
>> +	"-s     print this subvolume only",
>>  	NULL
>>  };
>>  
>> @@ -230,11 +231,12 @@ static int cmd_subvol_list(int argc, char **argv)
>>  	int fd;
>>  	int ret;
>>  	int print_parent = 0;
>> +	int print_self = 0;
>>  	char *subvol;
>>  
>>  	optind = 1;
>>  	while(1) {
>> -		int c = getopt(argc, argv, "p");
>> +		int c = getopt(argc, argv, "ps");
>>  		if (c < 0)
>>  			break;
>>  
>> @@ -242,6 +244,9 @@ static int cmd_subvol_list(int argc, char **argv)
>>  		case 'p':
>>  			print_parent = 1;
>>  			break;
>> +		case 's':
>> +			print_self = 1;
>> +			break;
>>  		default:
>>  			usage(cmd_subvol_list_usage);
>>  		}
>> @@ -267,7 +272,7 @@ static int cmd_subvol_list(int argc, char **argv)
>>  		fprintf(stderr, "ERROR: can't access '%s'\n", subvol);
>>  		return 12;
>>  	}
>> -	ret = list_subvols(fd, print_parent, 0);
>> +	ret = list_subvols(fd, print_parent, print_self, 0);
>>  	if (ret)
>>  		return 19;
>>  	return 0;
>> @@ -426,7 +431,7 @@ static int cmd_subvol_get_default(int argc, char **argv)
>>  		fprintf(stderr, "ERROR: can't access '%s'\n", subvol);
>>  		return 12;
>>  	}
>> -	ret = list_subvols(fd, 0, 1);
>> +	ret = list_subvols(fd, 0, 0, 1);
>>  	if (ret)
>>  		return 19;
>>  	return 0;
>> -- 
>> 1.6.5.2
>>
>> --
>> 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
> --
> 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
>