All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/5] Btrfs: Readonly snapshots
@ 2010-12-09  9:09 Li Zefan
  2010-12-09  9:10 ` [PATCH v2 1/5] Btrfs: Make async snapshot ioctl more generic Li Zefan
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Li Zefan @ 2010-12-09  9:09 UTC (permalink / raw)
  To: linux-btrfs

This patchset adds readonly-snapshots support. You can create a
readonly snapshot, and you can also set a snapshot (subvolume also)
readonly/writable on the fly.

A few readonly checks are added in setattr, permission, remove_xattr
and set_xattr callbacks, as well as in some ioctls.

The patchset is also available in:
	git://repo.or.cz/linux-btrfs-devel.git readonly-snapshots

Note: I've changed the async snapshot ABI. So if the patchset is acceptable,
the first patch has to be merged into .37 to avoid ABI breakage.

---
 fs/btrfs/ctree.h       |    3 +
 fs/btrfs/disk-io.c     |   36 +++++----
 fs/btrfs/inode.c       |    8 ++
 fs/btrfs/ioctl.c       |  193 +++++++++++++++++++++++++++++++++++++++---------
 fs/btrfs/ioctl.h       |   19 ++++-
 fs/btrfs/transaction.c |    8 ++
 fs/btrfs/transaction.h |    1 +
 fs/btrfs/xattr.c       |   18 +++++
 8 files changed, 230 insertions(+), 56 deletions(-)

^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH v2 1/5] Btrfs: Make async snapshot ioctl more generic
  2010-12-09  9:09 [PATCH v2 0/5] Btrfs: Readonly snapshots Li Zefan
@ 2010-12-09  9:10 ` Li Zefan
  2010-12-09  9:10 ` [PATCH v2 2/5] Btrfs: Refactor btrfs_ioctl_snap_create() Li Zefan
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Li Zefan @ 2010-12-09  9:10 UTC (permalink / raw)
  To: linux-btrfs

So we don't have to add new structures as we create more ioctls
for snapshots/subvolumes.

Now to create async snapshot, set BTRFS_SUBVOL_CREATE_SNAP_ASYNC bit
of vol_arg_v2->flags, and then call ioctl(BTRFS_IOCT_SNAP_CREATE_V2).

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c |   34 +++++++++++++++++++++-------------
 fs/btrfs/ioctl.h |   14 +++++++++-----
 2 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index f1c9bb4..dc953bc 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -947,23 +947,31 @@ out:
 
 static noinline int btrfs_ioctl_snap_create(struct file *file,
 					    void __user *arg, int subvol,
-					    int async)
+					    bool v2)
 {
 	struct btrfs_ioctl_vol_args *vol_args = NULL;
-	struct btrfs_ioctl_async_vol_args *async_vol_args = NULL;
+	struct btrfs_ioctl_vol_args_v2 *vol_args_v2 = NULL;
 	char *name;
 	u64 fd;
 	u64 transid = 0;
+	bool async = false;
 	int ret;
 
-	if (async) {
-		async_vol_args = memdup_user(arg, sizeof(*async_vol_args));
-		if (IS_ERR(async_vol_args))
-			return PTR_ERR(async_vol_args);
+	if (v2) {
+		vol_args_v2 = memdup_user(arg, sizeof(*vol_args_v2));
+		if (IS_ERR(vol_args_v2))
+			return PTR_ERR(vol_args_v2);
 
-		name = async_vol_args->name;
-		fd = async_vol_args->fd;
-		async_vol_args->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
+		if (vol_args_v2->flags & ~BTRFS_SUBVOL_CREATE_SNAP_ASYNC) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		name = vol_args_v2->name;
+		fd = vol_args_v2->fd;
+		vol_args_v2->name[BTRFS_SUBVOL_NAME_MAX] = '\0';
+		if (vol_args_v2->flags & BTRFS_SUBVOL_CREATE_SNAP_ASYNC)
+			async = true;
 	} else {
 		vol_args = memdup_user(arg, sizeof(*vol_args));
 		if (IS_ERR(vol_args))
@@ -978,13 +986,13 @@ static noinline int btrfs_ioctl_snap_create(struct file *file,
 
 	if (!ret && async) {
 		if (copy_to_user(arg +
-				offsetof(struct btrfs_ioctl_async_vol_args,
+				offsetof(struct btrfs_ioctl_vol_args_v2,
 				transid), &transid, sizeof(transid)))
 			return -EFAULT;
 	}
-
+out:
 	kfree(vol_args);
-	kfree(async_vol_args);
+	kfree(vol_args_v2);
 
 	return ret;
 }
@@ -2246,7 +2254,7 @@ long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_getversion(file, argp);
 	case BTRFS_IOC_SNAP_CREATE:
 		return btrfs_ioctl_snap_create(file, argp, 0, 0);
-	case BTRFS_IOC_SNAP_CREATE_ASYNC:
+	case BTRFS_IOC_SNAP_CREATE_V2:
 		return btrfs_ioctl_snap_create(file, argp, 0, 1);
 	case BTRFS_IOC_SUBVOL_CREATE:
 		return btrfs_ioctl_snap_create(file, argp, 1, 0);
diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
index 17c99eb..9affc49 100644
--- a/fs/btrfs/ioctl.h
+++ b/fs/btrfs/ioctl.h
@@ -30,11 +30,15 @@ struct btrfs_ioctl_vol_args {
 	char name[BTRFS_PATH_NAME_MAX + 1];
 };
 
-#define BTRFS_SNAPSHOT_NAME_MAX 4079
-struct btrfs_ioctl_async_vol_args {
+#define BTRFS_SUBVOL_CREATE_SNAP_ASYNC	(1ULL << 0)
+
+#define BTRFS_SUBVOL_NAME_MAX 4039
+struct btrfs_ioctl_vol_args_v2 {
 	__s64 fd;
 	__u64 transid;
-	char name[BTRFS_SNAPSHOT_NAME_MAX + 1];
+	__u64 flags;
+	__u64 unused[4];
+	char name[BTRFS_SUBVOL_NAME_MAX + 1];
 };
 
 #define BTRFS_INO_LOOKUP_PATH_MAX 4080
@@ -187,6 +191,6 @@ struct btrfs_ioctl_space_args {
 				    struct btrfs_ioctl_space_args)
 #define BTRFS_IOC_START_SYNC _IOR(BTRFS_IOCTL_MAGIC, 24, __u64)
 #define BTRFS_IOC_WAIT_SYNC  _IOW(BTRFS_IOCTL_MAGIC, 22, __u64)
-#define BTRFS_IOC_SNAP_CREATE_ASYNC _IOW(BTRFS_IOCTL_MAGIC, 23, \
-				   struct btrfs_ioctl_async_vol_args)
+#define BTRFS_IOC_SNAP_CREATE_V2 _IOW(BTRFS_IOCTL_MAGIC, 23, \
+				   struct btrfs_ioctl_vol_args_v2)
 #endif
-- 
1.6.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH v2 2/5] Btrfs: Refactor btrfs_ioctl_snap_create()
  2010-12-09  9:09 [PATCH v2 0/5] Btrfs: Readonly snapshots Li Zefan
  2010-12-09  9:10 ` [PATCH v2 1/5] Btrfs: Make async snapshot ioctl more generic Li Zefan
@ 2010-12-09  9:10 ` Li Zefan
  2010-12-09  9:10 ` [PATCH v2 3/5] Btrfs: Add helper __setup_root_post() Li Zefan
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Li Zefan @ 2010-12-09  9:10 UTC (permalink / raw)
  To: linux-btrfs

Split it into two functions for two different ioctls, since they
share no common code.

Also fix a memory leak in a failure path.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c |   71 +++++++++++++++++++++++++++--------------------------
 1 files changed, 36 insertions(+), 35 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index dc953bc..5ecd532 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -946,54 +946,55 @@ out:
 }
 
 static noinline int btrfs_ioctl_snap_create(struct file *file,
-					    void __user *arg, int subvol,
-					    bool v2)
+					    void __user *arg, int subvol)
 {
-	struct btrfs_ioctl_vol_args *vol_args = NULL;
-	struct btrfs_ioctl_vol_args_v2 *vol_args_v2 = NULL;
-	char *name;
-	u64 fd;
+	struct btrfs_ioctl_vol_args *vol_args;
+	int ret;
+
+	vol_args = memdup_user(arg, sizeof(*vol_args));
+	if (IS_ERR(vol_args))
+		return PTR_ERR(vol_args);
+	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
+
+	ret = btrfs_ioctl_snap_create_transid(file, vol_args->name,
+					      vol_args->fd, subvol, NULL);
+
+	kfree(vol_args);
+	return ret;
+}
+
+static noinline int btrfs_ioctl_snap_create_v2(struct file *file,
+					       void __user *arg, int subvol)
+{
+	struct btrfs_ioctl_vol_args_v2 *vol_args;
 	u64 transid = 0;
 	bool async = false;
 	int ret;
 
-	if (v2) {
-		vol_args_v2 = memdup_user(arg, sizeof(*vol_args_v2));
-		if (IS_ERR(vol_args_v2))
-			return PTR_ERR(vol_args_v2);
-
-		if (vol_args_v2->flags & ~BTRFS_SUBVOL_CREATE_SNAP_ASYNC) {
-			ret = -EINVAL;
-			goto out;
-		}
+	vol_args = memdup_user(arg, sizeof(*vol_args));
+	if (IS_ERR(vol_args))
+		return PTR_ERR(vol_args);
 
-		name = vol_args_v2->name;
-		fd = vol_args_v2->fd;
-		vol_args_v2->name[BTRFS_SUBVOL_NAME_MAX] = '\0';
-		if (vol_args_v2->flags & BTRFS_SUBVOL_CREATE_SNAP_ASYNC)
-			async = true;
-	} else {
-		vol_args = memdup_user(arg, sizeof(*vol_args));
-		if (IS_ERR(vol_args))
-			return PTR_ERR(vol_args);
-		name = vol_args->name;
-		fd = vol_args->fd;
-		vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
+	if (vol_args->flags & ~BTRFS_SUBVOL_CREATE_SNAP_ASYNC) {
+		ret = -EINVAL;
+		goto out;
 	}
 
-	ret = btrfs_ioctl_snap_create_transid(file, name, fd,
-					      subvol, &transid);
+	vol_args->name[BTRFS_SUBVOL_NAME_MAX] = '\0';
+	if (vol_args->flags & BTRFS_SUBVOL_CREATE_SNAP_ASYNC)
+		async = true;
+
+	ret = btrfs_ioctl_snap_create_transid(file, vol_args->name,
+					      vol_args->fd, subvol, &transid);
 
 	if (!ret && async) {
 		if (copy_to_user(arg +
 				offsetof(struct btrfs_ioctl_vol_args_v2,
 				transid), &transid, sizeof(transid)))
-			return -EFAULT;
+			ret = -EFAULT;
 	}
 out:
 	kfree(vol_args);
-	kfree(vol_args_v2);
-
 	return ret;
 }
 
@@ -2253,11 +2254,11 @@ long btrfs_ioctl(struct file *file, unsigned int
 	case FS_IOC_GETVERSION:
 		return btrfs_ioctl_getversion(file, argp);
 	case BTRFS_IOC_SNAP_CREATE:
-		return btrfs_ioctl_snap_create(file, argp, 0, 0);
+		return btrfs_ioctl_snap_create(file, argp, 0);
 	case BTRFS_IOC_SNAP_CREATE_V2:
-		return btrfs_ioctl_snap_create(file, argp, 0, 1);
+		return btrfs_ioctl_snap_create_v2(file, argp, 0);
 	case BTRFS_IOC_SUBVOL_CREATE:
-		return btrfs_ioctl_snap_create(file, argp, 1, 0);
+		return btrfs_ioctl_snap_create(file, argp, 1);
 	case BTRFS_IOC_SNAP_DESTROY:
 		return btrfs_ioctl_snap_destroy(file, argp);
 	case BTRFS_IOC_DEFAULT_SUBVOL:
-- 
1.6.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH v2 3/5] Btrfs: Add helper __setup_root_post()
  2010-12-09  9:09 [PATCH v2 0/5] Btrfs: Readonly snapshots Li Zefan
  2010-12-09  9:10 ` [PATCH v2 1/5] Btrfs: Make async snapshot ioctl more generic Li Zefan
  2010-12-09  9:10 ` [PATCH v2 2/5] Btrfs: Refactor btrfs_ioctl_snap_create() Li Zefan
@ 2010-12-09  9:10 ` Li Zefan
  2010-12-09  9:11 ` [PATCH v2 4/5] Btrfs: Add readonly snapshots support Li Zefan
  2010-12-09  9:11 ` [PATCH v2 5/5] Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctl Li Zefan
  4 siblings, 0 replies; 11+ messages in thread
From: Li Zefan @ 2010-12-09  9:10 UTC (permalink / raw)
  To: linux-btrfs

Eliminate duplicate code in btrfs_read_fs_root_no_radix() and
find_and_setup_root().

Also prepare for the next patch.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 fs/btrfs/disk-io.c |   31 +++++++++++++++----------------
 1 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 33b6d45..ca20c6d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -987,14 +987,25 @@ static int __setup_root(u32 nodesize, u32 leafsize, u32 sectorsize,
 	return 0;
 }
 
+static void __setup_root_post(struct btrfs_root *root)
+{
+	u32 blocksize;
+	u64 generation;
+
+	generation = btrfs_root_generation(&root->root_item);
+	blocksize = btrfs_level_size(root, btrfs_root_level(&root->root_item));
+	root->node = read_tree_block(root, btrfs_root_bytenr(&root->root_item),
+				     blocksize, generation);
+	BUG_ON(!root->node);
+	root->commit_root = btrfs_root_node(root);
+}
+
 static int find_and_setup_root(struct btrfs_root *tree_root,
 			       struct btrfs_fs_info *fs_info,
 			       u64 objectid,
 			       struct btrfs_root *root)
 {
 	int ret;
-	u32 blocksize;
-	u64 generation;
 
 	__setup_root(tree_root->nodesize, tree_root->leafsize,
 		     tree_root->sectorsize, tree_root->stripesize,
@@ -1005,12 +1016,7 @@ static int find_and_setup_root(struct btrfs_root *tree_root,
 		return -ENOENT;
 	BUG_ON(ret);
 
-	generation = btrfs_root_generation(&root->root_item);
-	blocksize = btrfs_level_size(root, btrfs_root_level(&root->root_item));
-	root->node = read_tree_block(root, btrfs_root_bytenr(&root->root_item),
-				     blocksize, generation);
-	BUG_ON(!root->node);
-	root->commit_root = btrfs_root_node(root);
+	__setup_root_post(root);
 	return 0;
 }
 
@@ -1111,8 +1117,6 @@ struct btrfs_root *btrfs_read_fs_root_no_radix(struct btrfs_root *tree_root,
 	struct btrfs_fs_info *fs_info = tree_root->fs_info;
 	struct btrfs_path *path;
 	struct extent_buffer *l;
-	u64 generation;
-	u32 blocksize;
 	int ret = 0;
 
 	root = kzalloc(sizeof(*root), GFP_NOFS);
@@ -1149,12 +1153,7 @@ struct btrfs_root *btrfs_read_fs_root_no_radix(struct btrfs_root *tree_root,
 		return ERR_PTR(ret);
 	}
 
-	generation = btrfs_root_generation(&root->root_item);
-	blocksize = btrfs_level_size(root, btrfs_root_level(&root->root_item));
-	root->node = read_tree_block(root, btrfs_root_bytenr(&root->root_item),
-				     blocksize, generation);
-	root->commit_root = btrfs_root_node(root);
-	BUG_ON(!root->node);
+	__setup_root_post(root);
 out:
 	if (location->objectid != BTRFS_TREE_LOG_OBJECTID)
 		root->ref_cows = 1;
-- 
1.6.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH v2 4/5] Btrfs: Add readonly snapshots support
  2010-12-09  9:09 [PATCH v2 0/5] Btrfs: Readonly snapshots Li Zefan
                   ` (2 preceding siblings ...)
  2010-12-09  9:10 ` [PATCH v2 3/5] Btrfs: Add helper __setup_root_post() Li Zefan
@ 2010-12-09  9:11 ` Li Zefan
  2010-12-09 19:34   ` Goffredo Baroncelli
  2010-12-09  9:11 ` [PATCH v2 5/5] Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctl Li Zefan
  4 siblings, 1 reply; 11+ messages in thread
From: Li Zefan @ 2010-12-09  9:11 UTC (permalink / raw)
  To: linux-btrfs

Usage:

Set BTRFS_SUBVOL_RDONLY of btrfs_ioctl_vol_arg_v2->flags, and call
ioctl(BTRFS_I0CTL_SNAP_CREATE_V2).

Implementation:

- In disk set readonly bit of btrfs_root_item->flags, and in memory
set btrfs_root->readonly.

- Add readonly checks in btrfs_permission (inode_permission),
btrfs_setattr, btrfs_set/remove_xattr and some ioctls.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 fs/btrfs/ctree.h       |    3 +++
 fs/btrfs/disk-io.c     |    5 +++++
 fs/btrfs/inode.c       |    8 ++++++++
 fs/btrfs/ioctl.c       |   40 +++++++++++++++++++++++++++++++---------
 fs/btrfs/ioctl.h       |    1 +
 fs/btrfs/transaction.c |    8 ++++++++
 fs/btrfs/transaction.h |    1 +
 fs/btrfs/xattr.c       |   18 ++++++++++++++++++
 8 files changed, 75 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index af52f6d..ad37c78 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -597,6 +597,8 @@ struct btrfs_dir_item {
 	u8 type;
 } __attribute__ ((__packed__));
 
+#define BTRFS_ROOT_SNAP_RDONLY	(1ULL << 0)
+
 struct btrfs_root_item {
 	struct btrfs_inode_item inode;
 	__le64 generation;
@@ -1116,6 +1118,7 @@ struct btrfs_root {
 	int defrag_running;
 	char *name;
 	int in_sysfs;
+	bool readonly;
 
 	/* the dirty list is only used by non-reference counted roots */
 	struct list_head dirty_list;
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index ca20c6d..74748df 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -991,6 +991,7 @@ static void __setup_root_post(struct btrfs_root *root)
 {
 	u32 blocksize;
 	u64 generation;
+	u64 flags;
 
 	generation = btrfs_root_generation(&root->root_item);
 	blocksize = btrfs_level_size(root, btrfs_root_level(&root->root_item));
@@ -998,6 +999,10 @@ static void __setup_root_post(struct btrfs_root *root)
 				     blocksize, generation);
 	BUG_ON(!root->node);
 	root->commit_root = btrfs_root_node(root);
+
+	flags = btrfs_root_flags(&root->root_item);
+	if (flags & BTRFS_ROOT_SNAP_RDONLY)
+		root->readonly = true;
 }
 
 static int find_and_setup_root(struct btrfs_root *tree_root,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0f34cae..e99676d 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3671,8 +3671,12 @@ static int btrfs_setattr_size(struct inode *inode, struct iattr *attr)
 static int btrfs_setattr(struct dentry *dentry, struct iattr *attr)
 {
 	struct inode *inode = dentry->d_inode;
+	struct btrfs_root *root = BTRFS_I(inode)->root;
 	int err;
 
+	if (root->readonly)
+		return -EROFS;
+
 	err = inode_change_ok(inode, attr);
 	if (err)
 		return err;
@@ -7207,6 +7211,10 @@ static int btrfs_set_page_dirty(struct page *page)
 
 static int btrfs_permission(struct inode *inode, int mask)
 {
+	struct btrfs_root *root = BTRFS_I(inode)->root;
+
+	if (root->readonly && (mask & MAY_WRITE))
+		return -EROFS;
 	if ((BTRFS_I(inode)->flags & BTRFS_INODE_READONLY) && (mask & MAY_WRITE))
 		return -EACCES;
 	return generic_permission(inode, mask, btrfs_check_acl);
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 5ecd532..db2b231 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -147,6 +147,9 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	unsigned int flags, oldflags;
 	int ret;
 
+	if (root->readonly)
+		return -EROFS;
+
 	if (copy_from_user(&flags, arg, sizeof(flags)))
 		return -EFAULT;
 
@@ -360,7 +363,8 @@ fail:
 }
 
 static int create_snapshot(struct btrfs_root *root, struct dentry *dentry,
-			   char *name, int namelen, u64 *async_transid)
+			   char *name, int namelen, u64 *async_transid,
+			   bool readonly)
 {
 	struct inode *inode;
 	struct dentry *parent;
@@ -378,6 +382,7 @@ static int create_snapshot(struct btrfs_root *root, struct dentry *dentry,
 	btrfs_init_block_rsv(&pending_snapshot->block_rsv);
 	pending_snapshot->dentry = dentry;
 	pending_snapshot->root = root;
+	pending_snapshot->readonly = readonly;
 
 	trans = btrfs_start_transaction(root->fs_info->extent_root, 5);
 	if (IS_ERR(trans)) {
@@ -509,7 +514,7 @@ static inline int btrfs_may_create(struct inode *dir, struct dentry *child)
 static noinline int btrfs_mksubvol(struct path *parent,
 				   char *name, int namelen,
 				   struct btrfs_root *snap_src,
-				   u64 *async_transid)
+				   u64 *async_transid, bool readonly)
 {
 	struct inode *dir  = parent->dentry->d_inode;
 	struct dentry *dentry;
@@ -541,7 +546,7 @@ static noinline int btrfs_mksubvol(struct path *parent,
 
 	if (snap_src) {
 		error = create_snapshot(snap_src, dentry,
-					name, namelen, async_transid);
+					name, namelen, async_transid, readonly);
 	} else {
 		error = create_subvol(BTRFS_I(dir)->root, dentry,
 				      name, namelen, async_transid);
@@ -901,7 +906,8 @@ static noinline int btrfs_ioctl_snap_create_transid(struct file *file,
 						    char *name,
 						    unsigned long fd,
 						    int subvol,
-						    u64 *transid)
+						    u64 *transid,
+						    bool readonly)
 {
 	struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root;
 	struct file *src_file;
@@ -919,7 +925,7 @@ static noinline int btrfs_ioctl_snap_create_transid(struct file *file,
 
 	if (subvol) {
 		ret = btrfs_mksubvol(&file->f_path, name, namelen,
-				     NULL, transid);
+				     NULL, transid, readonly);
 	} else {
 		struct inode *src_inode;
 		src_file = fget(fd);
@@ -938,7 +944,7 @@ static noinline int btrfs_ioctl_snap_create_transid(struct file *file,
 		}
 		ret = btrfs_mksubvol(&file->f_path, name, namelen,
 				     BTRFS_I(src_inode)->root,
-				     transid);
+				     transid, readonly);
 		fput(src_file);
 	}
 out:
@@ -957,7 +963,8 @@ static noinline int btrfs_ioctl_snap_create(struct file *file,
 	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
 
 	ret = btrfs_ioctl_snap_create_transid(file, vol_args->name,
-					      vol_args->fd, subvol, NULL);
+					      vol_args->fd, subvol,
+					      NULL, false);
 
 	kfree(vol_args);
 	return ret;
@@ -969,13 +976,15 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file,
 	struct btrfs_ioctl_vol_args_v2 *vol_args;
 	u64 transid = 0;
 	bool async = false;
+	bool readonly = false;
 	int ret;
 
 	vol_args = memdup_user(arg, sizeof(*vol_args));
 	if (IS_ERR(vol_args))
 		return PTR_ERR(vol_args);
 
-	if (vol_args->flags & ~BTRFS_SUBVOL_CREATE_SNAP_ASYNC) {
+	if (vol_args->flags &
+	    ~(BTRFS_SUBVOL_CREATE_SNAP_ASYNC | BTRFS_SUBVOL_RDONLY)) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -983,9 +992,12 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file,
 	vol_args->name[BTRFS_SUBVOL_NAME_MAX] = '\0';
 	if (vol_args->flags & BTRFS_SUBVOL_CREATE_SNAP_ASYNC)
 		async = true;
+	if (vol_args->flags & BTRFS_SUBVOL_RDONLY)
+		readonly = true;
 
 	ret = btrfs_ioctl_snap_create_transid(file, vol_args->name,
-					      vol_args->fd, subvol, &transid);
+					      vol_args->fd, subvol,
+					      &transid, readonly);
 
 	if (!ret && async) {
 		if (copy_to_user(arg +
@@ -1506,6 +1518,9 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp)
 	struct btrfs_ioctl_defrag_range_args *range;
 	int ret;
 
+	if (root->readonly)
+		return -EROFS;
+
 	ret = mnt_want_write(file->f_path.mnt);
 	if (ret)
 		return ret;
@@ -1634,6 +1649,9 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,
 	if (!(file->f_mode & FMODE_WRITE) || (file->f_flags & O_APPEND))
 		return -EINVAL;
 
+	if (root->readonly)
+		return -EROFS;
+
 	ret = mnt_want_write(file->f_path.mnt);
 	if (ret)
 		return ret;
@@ -1955,6 +1973,10 @@ static long btrfs_ioctl_trans_start(struct file *file)
 	if (file->private_data)
 		goto out;
 
+	ret = -EROFS;
+	if (root->readonly)
+		goto out;
+
 	ret = mnt_want_write(file->f_path.mnt);
 	if (ret)
 		goto out;
diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
index 9affc49..192fa99 100644
--- a/fs/btrfs/ioctl.h
+++ b/fs/btrfs/ioctl.h
@@ -31,6 +31,7 @@ struct btrfs_ioctl_vol_args {
 };
 
 #define BTRFS_SUBVOL_CREATE_SNAP_ASYNC	(1ULL << 0)
+#define BTRFS_SUBVOL_RDONLY		(1ULL << 1)
 
 #define BTRFS_SUBVOL_NAME_MAX 4039
 struct btrfs_ioctl_vol_args_v2 {
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index f50e931..30ee54c 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -910,6 +910,7 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
 	u64 to_reserve = 0;
 	u64 index = 0;
 	u64 objectid;
+	u64 root_flags;
 
 	new_root_item = kmalloc(sizeof(*new_root_item), GFP_NOFS);
 	if (!new_root_item) {
@@ -967,6 +968,13 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
 	btrfs_set_root_last_snapshot(&root->root_item, trans->transid);
 	memcpy(new_root_item, &root->root_item, sizeof(*new_root_item));
 
+	root_flags = btrfs_root_flags(new_root_item);
+	if (pending->readonly)
+		root_flags |= BTRFS_ROOT_SNAP_RDONLY;
+	else
+		root_flags &= ~BTRFS_ROOT_SNAP_RDONLY;
+	btrfs_set_root_flags(new_root_item, root_flags);
+
 	old = btrfs_lock_root_node(root);
 	btrfs_cow_block(trans, root, old, NULL, 0, &old);
 	btrfs_set_lock_blocking(old);
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index f104b57..229a594 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -62,6 +62,7 @@ struct btrfs_pending_snapshot {
 	struct btrfs_block_rsv block_rsv;
 	/* extra metadata reseration for relocation */
 	int error;
+	bool readonly;
 	struct list_head list;
 };
 
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 698fdd2..858ad4a 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -316,6 +316,15 @@ ssize_t btrfs_getxattr(struct dentry *dentry, const char *name,
 int btrfs_setxattr(struct dentry *dentry, const char *name, const void *value,
 		   size_t size, int flags)
 {
+	struct btrfs_root *root = BTRFS_I(dentry->d_inode)->root;
+
+	/*
+	 * The permission on security.* and system.* is not checked
+	 * in permission().
+	 */
+	if (root->readonly)
+		return -EROFS;
+
 	/*
 	 * If this is a request for a synthetic attribute in the system.*
 	 * namespace use the generic infrastructure to resolve a handler
@@ -336,6 +345,15 @@ int btrfs_setxattr(struct dentry *dentry, const char *name, const void *value,
 
 int btrfs_removexattr(struct dentry *dentry, const char *name)
 {
+	struct btrfs_root *root = BTRFS_I(dentry->d_inode)->root;
+
+	/*
+	 * The permission on security.* and system.* is not checked
+	 * in permission().
+	 */
+	if (root->readonly)
+		return -EROFS;
+
 	/*
 	 * If this is a request for a synthetic attribute in the system.*
 	 * namespace use the generic infrastructure to resolve a handler
-- 
1.6.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH v2 5/5] Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctl
  2010-12-09  9:09 [PATCH v2 0/5] Btrfs: Readonly snapshots Li Zefan
                   ` (3 preceding siblings ...)
  2010-12-09  9:11 ` [PATCH v2 4/5] Btrfs: Add readonly snapshots support Li Zefan
@ 2010-12-09  9:11 ` Li Zefan
  2010-12-09 20:09   ` Goffredo Baroncelli
  4 siblings, 1 reply; 11+ messages in thread
From: Li Zefan @ 2010-12-09  9:11 UTC (permalink / raw)
  To: linux-btrfs

This allows us to set a snapshot or a subvolume readonly or writable
on the fly.

Usage:

Set BTRFS_SUBVOL_RDONLY of btrfs_ioctl_vol_arg_v2->flags, and then
call ioctl(BTRFS_IOCTL_SUBVOL_SETFLAGS);

Changelog for v2:
- Add _GETFLAGS ioctl.
- Check if the passed fd is the root of a subvolume.
- Change the name from _SNAP_SETFLAGS to _SUBVOL_SETFLAGS.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c |   92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/ioctl.h |    4 ++
 2 files changed, 96 insertions(+), 0 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index db2b231..29304c7 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1010,6 +1010,94 @@ out:
 	return ret;
 }
 
+static noinline int btrfs_ioctl_subvol_getflags(struct file *file,
+						void __user *arg)
+{
+	struct inode *inode = fdentry(file)->d_inode;
+	struct btrfs_root *root = BTRFS_I(inode)->root;
+	struct btrfs_ioctl_vol_args_v2 *vol_args;
+	int ret = 0;
+	u64 flags = 0;
+
+	if (inode->i_ino != BTRFS_FIRST_FREE_OBJECTID)
+		return -EINVAL;
+
+	vol_args = memdup_user(arg, sizeof(*vol_args));
+	if (IS_ERR(vol_args))
+		return PTR_ERR(vol_args);
+
+	down_read(&root->fs_info->subvol_sem);
+	if (root->readonly)
+		flags |= BTRFS_SUBVOL_RDONLY;
+	up_read(&root->fs_info->subvol_sem);
+
+	if (copy_to_user(arg + offsetof(struct btrfs_ioctl_vol_args_v2, flags),
+			 &flags, sizeof(flags)))
+		ret = -EFAULT;
+
+	kfree(vol_args);
+	return ret;
+}
+
+static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
+					      void __user *arg)
+{
+	struct inode *inode = fdentry(file)->d_inode;
+	struct btrfs_root *root = BTRFS_I(inode)->root;
+	struct btrfs_trans_handle *trans;
+	struct btrfs_ioctl_vol_args_v2 *vol_args;
+	u64 root_flags;
+	u64 flags;
+	int ret = 0;
+
+	if (root->fs_info->sb->s_flags & MS_RDONLY)
+		return -EROFS;
+
+	if (inode->i_ino != BTRFS_FIRST_FREE_OBJECTID)
+		return -EINVAL;
+
+	vol_args = memdup_user(arg, sizeof(*vol_args));
+	if (IS_ERR(vol_args))
+		return PTR_ERR(vol_args);
+	flags = vol_args->flags;
+
+	if (flags & ~BTRFS_SUBVOL_RDONLY) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	down_write(&root->fs_info->subvol_sem);
+
+	/* nothing to do */
+	if (!!(flags & BTRFS_SUBVOL_RDONLY) == root->readonly)
+		goto out_unlock;
+
+	root_flags = btrfs_root_flags(&root->root_item);
+	if (flags & BTRFS_SUBVOL_RDONLY)
+		btrfs_set_root_flags(&root->root_item,
+				     root_flags | BTRFS_ROOT_SNAP_RDONLY);
+	else
+		btrfs_set_root_flags(&root->root_item,
+				     root_flags & ~BTRFS_ROOT_SNAP_RDONLY);
+	root->readonly = !root->readonly;
+
+	trans = btrfs_start_transaction(root, 1);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto out_unlock;
+	}
+
+	ret = btrfs_update_root(trans, root,
+				&root->root_key, &root->root_item);
+
+	btrfs_commit_transaction(trans, root);
+out_unlock:
+	up_write(&root->fs_info->subvol_sem);
+out:
+	kfree(vol_args);
+	return ret;
+}
+
 /*
  * helper to check if the subvolume references other subvolumes
  */
@@ -2283,6 +2371,10 @@ long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_snap_create(file, argp, 1);
 	case BTRFS_IOC_SNAP_DESTROY:
 		return btrfs_ioctl_snap_destroy(file, argp);
+	case BTRFS_IOC_SUBVOL_GETFLAGS:
+		return btrfs_ioctl_subvol_getflags(file, argp);
+	case BTRFS_IOC_SUBVOL_SETFLAGS:
+		return btrfs_ioctl_subvol_setflags(file, argp);
 	case BTRFS_IOC_DEFAULT_SUBVOL:
 		return btrfs_ioctl_default_subvol(file, argp);
 	case BTRFS_IOC_DEFRAG:
diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
index 192fa99..19f7259 100644
--- a/fs/btrfs/ioctl.h
+++ b/fs/btrfs/ioctl.h
@@ -194,4 +194,8 @@ struct btrfs_ioctl_space_args {
 #define BTRFS_IOC_WAIT_SYNC  _IOW(BTRFS_IOCTL_MAGIC, 22, __u64)
 #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, \
+				   struct btrfs_ioctl_vol_args_v2)
+#define BTRFS_IOC_SUBVOL_SETFLAGS _IOW(BTRFS_IOCTL_MAGIC, 26, \
+				   struct btrfs_ioctl_vol_args_v2)
 #endif
-- 
1.6.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/5] Btrfs: Add readonly snapshots support
  2010-12-09  9:11 ` [PATCH v2 4/5] Btrfs: Add readonly snapshots support Li Zefan
@ 2010-12-09 19:34   ` Goffredo Baroncelli
  2010-12-10  6:43     ` Li Zefan
  0 siblings, 1 reply; 11+ messages in thread
From: Goffredo Baroncelli @ 2010-12-09 19:34 UTC (permalink / raw)
  To: Li Zefan; +Cc: linux-btrfs

Hi Li,

On Thursday, 09 December, 2010, Li Zefan wrote:
> Usage:
> 
> Set BTRFS_SUBVOL_RDONLY of btrfs_ioctl_vol_arg_v2->flags, and call
> ioctl(BTRFS_I0CTL_SNAP_CREATE_V2).
> 
> Implementation:
> 
> - In disk set readonly bit of btrfs_root_item->flags, and in memory
> set btrfs_root->readonly.
> 
> - Add readonly checks in btrfs_permission (inode_permission),
> btrfs_setattr, btrfs_set/remove_xattr and some ioctls.
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  fs/btrfs/ctree.h       |    3 +++
>  fs/btrfs/disk-io.c     |    5 +++++
>  fs/btrfs/inode.c       |    8 ++++++++
>  fs/btrfs/ioctl.c       |   40 +++++++++++++++++++++++++++++++---------
>  fs/btrfs/ioctl.h       |    1 +
>  fs/btrfs/transaction.c |    8 ++++++++
>  fs/btrfs/transaction.h |    1 +
>  fs/btrfs/xattr.c       |   18 ++++++++++++++++++
>  8 files changed, 75 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index af52f6d..ad37c78 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -597,6 +597,8 @@ struct btrfs_dir_item {
>  	u8 type;
>  } __attribute__ ((__packed__));
>  
> +#define BTRFS_ROOT_SNAP_RDONLY	(1ULL << 0)
> +
>  struct btrfs_root_item {
>  	struct btrfs_inode_item inode;
>  	__le64 generation;
> @@ -1116,6 +1118,7 @@ struct btrfs_root {
>  	int defrag_running;
>  	char *name;
>  	int in_sysfs;
> +	bool readonly;

Does make sense to store the same information in two places ?
If we have access to root->readonly, we have also access to "root-
>root_item.flags". Because we need the latter, we can get rid of the former.


We can replace a test like

	if(root->readonly) 

with

	if(root->root_item.flags & BTRFS_ROOT_SNAP_RDONLY)

Or better we can create a two macros like:

#define btrfs_root_readonly(x) ((x)->root_item.flags & BTRFS_ROOT_SNAP_RDONLY)
#define btrfs_root_set_readonly(x, ro) 					    \
	do{ 	(x)->root_item.flags = 					    \
			((x)->root_item.flags & ~BTRFS_ROOT_SNAP_RDONLY) |  \
			(ro ? BTRFS_ROOT_SNAP_RDONLY : 0 ); 		    \
	}while(0)


Sorry for to be too late for this kind of suggestion. But I think that this 
optimization may help to avoid misalignment between the two variables (see my 
reply in the next patch).
[...]
-- 
gpg key@ keyserver.linux.it: Goffredo Baroncelli (ghigo) <kreijack@inwind.it>
Key fingerprint = 4769 7E51 5293 D36C 814E  C054 BF04 F161 3DC5 0512

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v2 5/5] Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctl
  2010-12-09  9:11 ` [PATCH v2 5/5] Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctl Li Zefan
@ 2010-12-09 20:09   ` Goffredo Baroncelli
  2010-12-10  7:12     ` Li Zefan
  0 siblings, 1 reply; 11+ messages in thread
From: Goffredo Baroncelli @ 2010-12-09 20:09 UTC (permalink / raw)
  To: Li Zefan; +Cc: linux-btrfs

Hi Li,

On Thursday, 09 December, 2010, Li Zefan wrote:
> This allows us to set a snapshot or a subvolume readonly or writable
> on the fly.
> 
> Usage:
> 
> Set BTRFS_SUBVOL_RDONLY of btrfs_ioctl_vol_arg_v2->flags, and then
> call ioctl(BTRFS_IOCTL_SUBVOL_SETFLAGS);
> 
> Changelog for v2:
> - Add _GETFLAGS ioctl.
> - Check if the passed fd is the root of a subvolume.
> - Change the name from _SNAP_SETFLAGS to _SUBVOL_SETFLAGS.
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  fs/btrfs/ioctl.c |   92 
++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/btrfs/ioctl.h |    4 ++
>  2 files changed, 96 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index db2b231..29304c7 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1010,6 +1010,94 @@ out:
[...]
> +	struct btrfs_ioctl_vol_args_v2 *vol_args;
> +	int ret = 0;
> +	u64 flags = 0;
> +
> +	if (inode->i_ino != BTRFS_FIRST_FREE_OBJECTID)
> +		return -EINVAL;
> +
> +	vol_args = memdup_user(arg, sizeof(*vol_args));

Would be better to avoid a dynamic allocation for a so small struct ? 
And as more general comment: what is the reason to pass a so complex struct 
only for setting/getting the flags ?
Would be it better to pass directly a u64 variable.

[..]
> +
> +static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
> +					      void __user *arg)
> +{
> +	struct inode *inode = fdentry(file)->d_inode;
> +	struct btrfs_root *root = BTRFS_I(inode)->root;
> +	struct btrfs_trans_handle *trans;
> +	struct btrfs_ioctl_vol_args_v2 *vol_args;
> +	u64 root_flags;
> +	u64 flags;
> +	int ret = 0;
> +
> +	if (root->fs_info->sb->s_flags & MS_RDONLY)
> +		return -EROFS;
> +
> +	if (inode->i_ino != BTRFS_FIRST_FREE_OBJECTID)
> +		return -EINVAL;
> +
> +	vol_args = memdup_user(arg, sizeof(*vol_args));

Same as before.

> +	if (IS_ERR(vol_args))
> +		return PTR_ERR(vol_args);
> +	flags = vol_args->flags;
> +
> +	if (flags & ~BTRFS_SUBVOL_RDONLY) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	down_write(&root->fs_info->subvol_sem);
> +
> +	/* nothing to do */
> +	if (!!(flags & BTRFS_SUBVOL_RDONLY) == root->readonly)
> +		goto out_unlock;

This is only an aesthetic comment: I prefer a simpler style like

	if ((flags & BTRFS_SUBVOL_RDONLY) && root->readonly)
		goto out_unlock;

But I know that every body has its style :-)

> +
> +	root_flags = btrfs_root_flags(&root->root_item);
> +	if (flags & BTRFS_SUBVOL_RDONLY)
> +		btrfs_set_root_flags(&root->root_item,
> +				     root_flags | BTRFS_ROOT_SNAP_RDONLY);
> +	else
> +		btrfs_set_root_flags(&root->root_item,
> +				     root_flags & ~BTRFS_ROOT_SNAP_RDONLY);
> +	root->readonly = !root->readonly;

I double checked this line. But if I read the code correctly I think that the 
line above is wrong: the field "root->readonly" is flipped regardless the 
value of the flags passed by the user.

Moreover I have another question: why internally the flags is 
BTRFS_ROOT_SNAP_RDONLY, instead in user space the flag is BTRFS_SUBVOL_RDONLY 
? 

I suggest to 
- rename BTRFS_SUBVOL_RDONLY in BTRFS_SUBVOL_CREATE_SNAP_RDONLY (like 
BTRFS_SUBVOL_CREATE_SNAP_ASYNC)
- rename BTRFS_ROOT_SNAP_RDONLY in BTRFS_ROOT_FLAGS_RDONLY and use both in 
userspace and in kernel space this flag. I suggested to remove SNAP because 
the flag make sense also for a "standard" subvolume.


Gegards 
G.Baroncelli


-- 
gpg key@ keyserver.linux.it: Goffredo Baroncelli (ghigo) <kreijack@inwind.it>
Key fingerprint = 4769 7E51 5293 D36C 814E  C054 BF04 F161 3DC5 0512

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/5] Btrfs: Add readonly snapshots support
  2010-12-09 19:34   ` Goffredo Baroncelli
@ 2010-12-10  6:43     ` Li Zefan
  0 siblings, 0 replies; 11+ messages in thread
From: Li Zefan @ 2010-12-10  6:43 UTC (permalink / raw)
  To: kreijack; +Cc: linux-btrfs

>> +#define BTRFS_ROOT_SNAP_RDONLY	(1ULL << 0)
>> +
>>  struct btrfs_root_item {
>>  	struct btrfs_inode_item inode;
>>  	__le64 generation;
>> @@ -1116,6 +1118,7 @@ struct btrfs_root {
>>  	int defrag_running;
>>  	char *name;
>>  	int in_sysfs;
>> +	bool readonly;
> 
> Does make sense to store the same information in two places ?
> If we have access to root->readonly, we have also access to "root-
>> root_item.flags". Because we need the latter, we can get rid of the former.
> 
> 
> We can replace a test like
> 
> 	if(root->readonly) 
> 
> with
> 
> 	if(root->root_item.flags & BTRFS_ROOT_SNAP_RDONLY)
> 
> Or better we can create a two macros like:
> 
> #define btrfs_root_readonly(x) ((x)->root_item.flags & BTRFS_ROOT_SNAP_RDONLY)
> #define btrfs_root_set_readonly(x, ro) 					    \
> 	do{ 	(x)->root_item.flags = 					    \
> 			((x)->root_item.flags & ~BTRFS_ROOT_SNAP_RDONLY) |  \
> 			(ro ? BTRFS_ROOT_SNAP_RDONLY : 0 ); 		    \
> 	}while(0)
> 
> 

Makes sense.

(except that inline functions are preferable)

> Sorry for to be too late for this kind of suggestion. But I think that this 
> optimization may help to avoid misalignment between the two variables (see my 
> reply in the next patch).
> [...]

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v2 5/5] Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctl
  2010-12-09 20:09   ` Goffredo Baroncelli
@ 2010-12-10  7:12     ` Li Zefan
  2010-12-10  7:49       ` Ian! D. Allen
  0 siblings, 1 reply; 11+ messages in thread
From: Li Zefan @ 2010-12-10  7:12 UTC (permalink / raw)
  To: kreijack; +Cc: linux-btrfs

Goffredo Baroncelli wrote:
> Hi Li,
> 
> On Thursday, 09 December, 2010, Li Zefan wrote:
>> This allows us to set a snapshot or a subvolume readonly or writable
>> on the fly.
>>
>> Usage:
>>
>> Set BTRFS_SUBVOL_RDONLY of btrfs_ioctl_vol_arg_v2->flags, and then
>> call ioctl(BTRFS_IOCTL_SUBVOL_SETFLAGS);
>>
>> Changelog for v2:
>> - Add _GETFLAGS ioctl.
>> - Check if the passed fd is the root of a subvolume.
>> - Change the name from _SNAP_SETFLAGS to _SUBVOL_SETFLAGS.
>>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>>  fs/btrfs/ioctl.c |   92 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  fs/btrfs/ioctl.h |    4 ++
>>  2 files changed, 96 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index db2b231..29304c7 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1010,6 +1010,94 @@ out:
> [...]
>> +	struct btrfs_ioctl_vol_args_v2 *vol_args;
>> +	int ret = 0;
>> +	u64 flags = 0;
>> +
>> +	if (inode->i_ino != BTRFS_FIRST_FREE_OBJECTID)
>> +		return -EINVAL;
>> +
>> +	vol_args = memdup_user(arg, sizeof(*vol_args));
> 
> Would be better to avoid a dynamic allocation for a so small struct ? 
> And as more general comment: what is the reason to pass a so complex struct 
> only for setting/getting the flags ?
> Would be it better to pass directly a u64 variable.
> 

I was considering using the same structure for all subvolume ioctls,
but here seems it's overkill..

> [..]
>> +
>> +static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>> +					      void __user *arg)
>> +{
>> +	struct inode *inode = fdentry(file)->d_inode;
>> +	struct btrfs_root *root = BTRFS_I(inode)->root;
>> +	struct btrfs_trans_handle *trans;
>> +	struct btrfs_ioctl_vol_args_v2 *vol_args;
>> +	u64 root_flags;
>> +	u64 flags;
>> +	int ret = 0;
>> +
>> +	if (root->fs_info->sb->s_flags & MS_RDONLY)
>> +		return -EROFS;
>> +
>> +	if (inode->i_ino != BTRFS_FIRST_FREE_OBJECTID)
>> +		return -EINVAL;
>> +
>> +	vol_args = memdup_user(arg, sizeof(*vol_args));
> 
> Same as before.
> 
>> +	if (IS_ERR(vol_args))
>> +		return PTR_ERR(vol_args);
>> +	flags = vol_args->flags;
>> +
>> +	if (flags & ~BTRFS_SUBVOL_RDONLY) {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	down_write(&root->fs_info->subvol_sem);
>> +
>> +	/* nothing to do */
>> +	if (!!(flags & BTRFS_SUBVOL_RDONLY) == root->readonly)
>> +		goto out_unlock;
> 
> This is only an aesthetic comment: I prefer a simpler style like
> 
> 	if ((flags & BTRFS_SUBVOL_RDONLY) && root->readonly)
> 		goto out_unlock;
> 

They are not equivalent. The former checks if the flags and the
root both have readonly bit set or cleared.

> But I know that every body has its style :-)
> 
>> +
>> +	root_flags = btrfs_root_flags(&root->root_item);
>> +	if (flags & BTRFS_SUBVOL_RDONLY)
>> +		btrfs_set_root_flags(&root->root_item,
>> +				     root_flags | BTRFS_ROOT_SNAP_RDONLY);
>> +	else
>> +		btrfs_set_root_flags(&root->root_item,
>> +				     root_flags & ~BTRFS_ROOT_SNAP_RDONLY);
>> +	root->readonly = !root->readonly;
> 
> I double checked this line. But if I read the code correctly I think that the 
> line above is wrong: the field "root->readonly" is flipped regardless the 
> value of the flags passed by the user.
> 

Yep, that's because if we don't need to flip it, we've already exited early.

Note, there's only one flag.

> Moreover I have another question: why internally the flags is 
> BTRFS_ROOT_SNAP_RDONLY, instead in user space the flag is BTRFS_SUBVOL_RDONLY 
> ? 
> 

That's my carelessness..

> I suggest to 
> - rename BTRFS_SUBVOL_RDONLY in BTRFS_SUBVOL_CREATE_SNAP_RDONLY (like 
> BTRFS_SUBVOL_CREATE_SNAP_ASYNC)
> - rename BTRFS_ROOT_SNAP_RDONLY in BTRFS_ROOT_FLAGS_RDONLY and use both in 
> userspace and in kernel space this flag. I suggested to remove SNAP because 
> the flag make sense also for a "standard" subvolume.
> 

I'd prefer not to mix flags for root_item flags and vol ioctl flags.

And CREATE_RDONLY and CREATE_ASYNC can also be valid for subvolumes too.
Support for async subvolume creation is already there, just lack of an
user interface. So I've changed _CREATE_SNAP_ASYNC to _CREATE_ASYNC
in the patch I sent just now.

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v2 5/5] Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctl
  2010-12-10  7:12     ` Li Zefan
@ 2010-12-10  7:49       ` Ian! D. Allen
  0 siblings, 0 replies; 11+ messages in thread
From: Ian! D. Allen @ 2010-12-10  7:49 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Li Zefan

On Fri, Dec 10, 2010 at 03:12:41PM +0800, Li Zefan wrote:
> >> +	/* nothing to do */
> >> +	if (!!(flags & BTRFS_SUBVOL_RDONLY) == root->readonly)
> >> +		goto out_unlock;
> > 
> > This is only an aesthetic comment: I prefer a simpler style like
> > 
> > 	if ((flags & BTRFS_SUBVOL_RDONLY) && root->readonly)
> > 		goto out_unlock;
> 
> They are not equivalent. The former checks if the flags and the
> root both have readonly bit set or cleared.

This is an excellent reason to add what you just replied (above) as a
comment into the code ahead of that line, so others don't make the same
mistake in interpreting what it does:

        /* nothing to do */
        /* check if flags and root both have readonly bit set or cleared */
        if (!!(flags & BTRFS_SUBVOL_RDONLY) == root->readonly)
                goto out_unlock;

"Debugging is twice as hard as writing the code in the first place.
 Therefore, if you write the code as cleverly as possible, you are,
 by definition, not smart enough to debug it." - Brian W. Kernighan

-- 
| Ian! D. Allen  -  idallen@idallen.ca  -  Ottawa, Ontario, Canada
| Home Page: http://idallen.com/   Contact Improv: http://contactimprov.ca/
| College professor (Free/Libre GNU+Linux) at: http://teaching.idallen.com/
| Defend digital freedom:  http://eff.org/  and have fun:  http://fools.ca/

^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-12-10  7:49 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-09  9:09 [PATCH v2 0/5] Btrfs: Readonly snapshots Li Zefan
2010-12-09  9:10 ` [PATCH v2 1/5] Btrfs: Make async snapshot ioctl more generic Li Zefan
2010-12-09  9:10 ` [PATCH v2 2/5] Btrfs: Refactor btrfs_ioctl_snap_create() Li Zefan
2010-12-09  9:10 ` [PATCH v2 3/5] Btrfs: Add helper __setup_root_post() Li Zefan
2010-12-09  9:11 ` [PATCH v2 4/5] Btrfs: Add readonly snapshots support Li Zefan
2010-12-09 19:34   ` Goffredo Baroncelli
2010-12-10  6:43     ` Li Zefan
2010-12-09  9:11 ` [PATCH v2 5/5] Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctl Li Zefan
2010-12-09 20:09   ` Goffredo Baroncelli
2010-12-10  7:12     ` Li Zefan
2010-12-10  7:49       ` Ian! D. Allen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.