[PATCH 0/5] btrfs: Readonly snapshots

[PATCH 0/5] btrfs: Readonly snapshots

[Li Zefan]

(Cc: Sage Weil <sage@newdream.net> for changes in async snapshots)

This patchset adds readonly-snapshots support. You can create a
readonly snapshot, and you can also set a snapshot 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.

You can also try it out by pulling (based on the master branch of
Chris' tree):
	git://repo.or.cz/linux-btrfs-devel.git readonly-snapshots

Note: I 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       |  147 +++++++++++++++++++++++++++++++++++++++++-------
 fs/btrfs/ioctl.h       |   16 ++++-
 fs/btrfs/transaction.c |    8 +++
 fs/btrfs/transaction.h |    1 +
 fs/btrfs/xattr.c       |   18 ++++++
 8 files changed, 196 insertions(+), 41 deletions(-)

[PATCH 1/5] btrfs: Make async snapshot ioctl more generic

[Li Zefan]

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

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

Note: this changes the async snapshot ioctl ABI, which was merged
in .37 merge window, so we have to make this change into .37.

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

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 463d91b..d3f1a60 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -935,23 +935,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_SNAPSHOT_CREATE_ASYNC) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		name = vol_args_v2->name;
+		fd = vol_args_v2->fd;
+		vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
+		if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
+			async = true;
 	} else {
 		vol_args = memdup_user(arg, sizeof(*vol_args));
 		if (IS_ERR(vol_args))
@@ -966,13 +974,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;
 }
@@ -2235,7 +2243,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..bc70584 100644
--- a/fs/btrfs/ioctl.h
+++ b/fs/btrfs/ioctl.h
@@ -30,10 +30,14 @@ 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_SNAPSHOT_CREATE_ASYNC	(1ULL << 0)
+
+#define BTRFS_SNAPSHOT_NAME_MAX 4039
+struct btrfs_ioctl_vol_args_v2 {
 	__s64 fd;
 	__u64 transid;
+	__u64 flags;
+	__u64 unused[4];
 	char name[BTRFS_SNAPSHOT_NAME_MAX + 1];
 };
 
@@ -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

[PATCH 2/5] btrfs: Fix memory leak in a failure path

[Li Zefan]

In btrfs_ioctl_snap_create(), vol_args_v2 is not freed if
copy_to_user() returns failure.

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

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index d3f1a60..ba437ad 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -976,7 +976,7 @@ static noinline int btrfs_ioctl_snap_create(struct file *file,
 		if (copy_to_user(arg +
 				offsetof(struct btrfs_ioctl_vol_args_v2,
 				transid), &transid, sizeof(transid)))
-			return -EFAULT;
+			ret = -EFAULT;
 	}
 out:
 	kfree(vol_args);
-- 
1.6.3

[PATCH 3/5] btrfs: Add helper __setup_root_post()

[Li Zefan]

This eliminates duplicate code in find_and_setup_root() and
btrfs_read_fs_root_no_radix().

(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 b40dfe4..fb650e0 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -959,14 +959,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,
@@ -977,12 +988,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;
 }
 
@@ -1083,8 +1089,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);
@@ -1121,12 +1125,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

[PATCH 4/5] btrfs: Add readonly snapshots support

[Li Zefan]

Usage:

Set BTRFS_SNAPSHOT_CREATE_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       |   33 +++++++++++++++++++++++++--------
 fs/btrfs/ioctl.h       |    1 +
 fs/btrfs/transaction.c |    8 ++++++++
 fs/btrfs/transaction.h |    1 +
 fs/btrfs/xattr.c       |   18 ++++++++++++++++++
 8 files changed, 69 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 8db9234..4b263ed 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 fb650e0..5b88712 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -963,6 +963,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));
@@ -970,6 +971,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 5132c9a..08c3075 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;
@@ -7028,6 +7032,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 ba437ad..7f9c571 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;
 
@@ -351,7 +354,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 btrfs_pending_snapshot *pending_snapshot;
@@ -368,6 +372,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)) {
@@ -497,7 +502,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;
@@ -529,7 +534,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);
@@ -889,7 +894,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;
@@ -907,7 +913,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);
@@ -926,7 +932,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:
@@ -943,6 +949,7 @@ static noinline int btrfs_ioctl_snap_create(struct file *file,
 	u64 fd;
 	u64 transid = 0;
 	bool async = false;
+	bool readonly = false;
 	int ret;
 
 	if (v2) {
@@ -950,7 +957,8 @@ static noinline int btrfs_ioctl_snap_create(struct file *file,
 		if (IS_ERR(vol_args_v2))
 			return PTR_ERR(vol_args_v2);
 
-		if (vol_args_v2->flags & ~BTRFS_SNAPSHOT_CREATE_ASYNC) {
+		if (vol_args_v2->flags &
+		    ~(BTRFS_SNAPSHOT_CREATE_ASYNC | BTRFS_SNAPSHOT_RDONLY)) {
 			ret = -EINVAL;
 			goto out;
 		}
@@ -960,6 +968,8 @@ static noinline int btrfs_ioctl_snap_create(struct file *file,
 		vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
 		if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
 			async = true;
+		if (vol_args_v2->flags & BTRFS_SNAPSHOT_RDONLY)
+			readonly = true;
 	} else {
 		vol_args = memdup_user(arg, sizeof(*vol_args));
 		if (IS_ERR(vol_args))
@@ -970,7 +980,7 @@ static noinline int btrfs_ioctl_snap_create(struct file *file,
 	}
 
 	ret = btrfs_ioctl_snap_create_transid(file, name, fd,
-					      subvol, &transid);
+					      subvol, &transid, readonly);
 
 	if (!ret && async) {
 		if (copy_to_user(arg +
@@ -1621,6 +1631,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;
@@ -1943,6 +1956,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 bc70584..ff15fb2 100644
--- a/fs/btrfs/ioctl.h
+++ b/fs/btrfs/ioctl.h
@@ -31,6 +31,7 @@ struct btrfs_ioctl_vol_args {
 };
 
 #define BTRFS_SNAPSHOT_CREATE_ASYNC	(1ULL << 0)
+#define BTRFS_SNAPSHOT_RDONLY		(1ULL << 1)
 
 #define BTRFS_SNAPSHOT_NAME_MAX 4039
 struct btrfs_ioctl_vol_args_v2 {
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 1fffbc0..fe85149 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -909,6 +909,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) {
@@ -965,6 +966,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

[PATCH 5/5] btrfs: Add ioctl to set snapshot readonly/writable

[Li Zefan]

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

Usage:

Set BTRFS_SNAPSHOT_RDONLY/WRITABLE of btrfs_ioctl_vol_arg_v2->flags,
and then call ioctl(BTRFS_IOCTL_SNAP_SETFLAGS);

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

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7f9c571..34d8683 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -939,6 +939,24 @@ out:
 	return ret;
 }
 
+static int snap_check_flags(u64 flags, bool create)
+{
+	u64 valid_flags = BTRFS_SNAPSHOT_RDONLY | BTRFS_SNAPSHOT_WRITABLE;
+
+	if (create)
+		valid_flags |= BTRFS_SNAPSHOT_CREATE_ASYNC;
+
+	if (flags & valid_flags)
+		return -EINVAL;
+
+	/* readonly and writable are mutually exclusive */
+	if ((flags & BTRFS_SNAPSHOT_RDONLY) &&
+	    (flags & BTRFS_SNAPSHOT_WRITABLE))
+		return -EINVAL;
+
+	return 0;
+}
+
 static noinline int btrfs_ioctl_snap_create(struct file *file,
 					    void __user *arg, int subvol,
 					    bool v2)
@@ -957,11 +975,9 @@ static noinline int btrfs_ioctl_snap_create(struct file *file,
 		if (IS_ERR(vol_args_v2))
 			return PTR_ERR(vol_args_v2);
 
-		if (vol_args_v2->flags &
-		    ~(BTRFS_SNAPSHOT_CREATE_ASYNC | BTRFS_SNAPSHOT_RDONLY)) {
-			ret = -EINVAL;
+		ret = snap_check_flags(vol_args_v2->flags, true);
+		if (ret)
 			goto out;
-		}
 
 		name = vol_args_v2->name;
 		fd = vol_args_v2->fd;
@@ -995,6 +1011,65 @@ out:
 	return ret;
 }
 
+static noinline int btrfs_ioctl_snap_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_v2;
+	u64 root_flags;
+	u64 flags;
+	int err;
+
+	if (root->fs_info->sb->s_flags & MS_RDONLY)
+		return -EROFS;
+
+	vol_args_v2 = memdup_user(arg, sizeof(*vol_args_v2));
+	if (IS_ERR(vol_args_v2))
+		return PTR_ERR(vol_args_v2);
+	flags = vol_args_v2->flags;
+
+	err = snap_check_flags(flags, false);
+	if (err)
+		goto out;
+
+	down_write(&root->fs_info->subvol_sem);
+
+	/* nothing to do */
+	if ((BTRFS_SNAPSHOT_RDONLY && root->readonly) ||
+	    (BTRFS_SNAPSHOT_WRITABLE && !root->readonly))
+		goto out_unlock;
+
+	root_flags = btrfs_root_flags(&root->root_item);
+	if (flags & BTRFS_SNAPSHOT_RDONLY) {
+		btrfs_set_root_flags(&root->root_item,
+				     root_flags | BTRFS_ROOT_SNAP_RDONLY);
+		root->readonly = true;
+	}
+	if (flags & BTRFS_SNAPSHOT_WRITABLE) {
+		btrfs_set_root_flags(&root->root_item,
+				     root_flags & ~BTRFS_ROOT_SNAP_RDONLY);
+		root->readonly = false;
+	}
+
+	trans = btrfs_start_transaction(root, 1);
+	if (IS_ERR(trans)) {
+		err = PTR_ERR(trans);
+		goto out_unlock;
+	}
+
+	err = 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_v2);
+	return err;
+}
+
 /*
  * helper to check if the subvolume references other subvolumes
  */
@@ -1503,6 +1578,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;
@@ -2266,6 +2344,8 @@ long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_snap_create(file, argp, 1, 0);
 	case BTRFS_IOC_SNAP_DESTROY:
 		return btrfs_ioctl_snap_destroy(file, argp);
+	case BTRFS_IOC_SNAP_SETFLAGS:
+		return btrfs_ioctl_snap_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 ff15fb2..559fd27 100644
--- a/fs/btrfs/ioctl.h
+++ b/fs/btrfs/ioctl.h
@@ -32,6 +32,7 @@ struct btrfs_ioctl_vol_args {
 
 #define BTRFS_SNAPSHOT_CREATE_ASYNC	(1ULL << 0)
 #define BTRFS_SNAPSHOT_RDONLY		(1ULL << 1)
+#define BTRFS_SNAPSHOT_WRITABLE		(1ULL << 2)
 
 #define BTRFS_SNAPSHOT_NAME_MAX 4039
 struct btrfs_ioctl_vol_args_v2 {
@@ -194,4 +195,6 @@ 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_SNAP_SETFLAGS _IOW(BTRFS_IOCTL_MAGIC, 24, \
+				   struct btrfs_ioctl_vol_args_v2)
 #endif
-- 
1.6.3

Re: [PATCH 1/5] btrfs: Make async snapshot ioctl more generic

[Goffredo Baroncelli]

Hi Li,

great work, but I have some suggestions:

On Monday, 29 November, 2010, Li Zefan wrote:
> So we don't have to add new structures as we create more ioctls
> for snapshots.
> 
> Now to create async snapshot, set BTRFS_SNAPSHOT_CREATE_ASYNC bit of
> vol_arg_v2->flags, and then call ioctl(BTRFS_IOCT_SNAP_CREATE_V2).
> 
> Note: this changes the async snapshot ioctl ABI, which was merged
> in .37 merge window, so we have to make this change into .37.
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  fs/btrfs/ioctl.c |   34 +++++++++++++++++++++-------------
>  fs/btrfs/ioctl.h |   12 ++++++++----
>  2 files changed, 29 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 463d91b..d3f1a60 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -935,23 +935,31 @@ out:
>  
>  static noinline int btrfs_ioctl_snap_create(struct file *file,
>  					    void __user *arg, int subvol,
> -					    int async)
> +					    bool v2)

This is a aesthetic suggestion: instead of passing a flag called v2 I suggest 
to add two wrapper functions, which extract the parameters and passes all 
available parameter to the "generic" function. Something like:

static inline btrfs_ioctl_snap_create_v1(struct file *file, 
						void __user *arg, int subvol)
{
                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';

		btrfs_ioctl_snap_create_generic(file, subvol, name, fd, 0, 0);

}

static inline btrfs_ioctl_snap_create_v2(struct file *file, 
						void __user *arg, int subvol,
						)
{
                vol_args_v2 = memdup_user(arg, sizeof(*vol_args_v2));
                if (IS_ERR(vol_args_v2))
                        return PTR_ERR(vol_args_v2);

                ret = snap_check_flags(vol_args_v2->flags, true);
                if (ret)
                        goto out;

                name = vol_args_v2->name;
                fd = vol_args_v2->fd;
                vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
                if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
                        async = true;
                if (vol_args_v2->flags & BTRFS_SNAPSHOT_RDONLY)
                        readonly = true;

		btrfs_ioctl_snap_create_generic(file, subvol, name, fd, async,
			 readonly);

}

and 

  	case BTRFS_IOC_SNAP_CREATE:
  		return btrfs_ioctl_snap_create_v1(file, argp, 0);
	case BTRFS_IOC_SNAP_CREATE_V2:
  		return btrfs_ioctl_snap_create_v2(file, argp, 0);


Frankly speaking, we could get rid of the subvol parameter adding another 
wrapper function like "btrfs_ioctl_subvol_create( )", but this would be 
another story :)

>  {
>  	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_SNAPSHOT_CREATE_ASYNC) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		name = vol_args_v2->name;
> +		fd = vol_args_v2->fd;
> +		vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
> +		if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
> +			async = true;
>  	} else {
>  		vol_args = memdup_user(arg, sizeof(*vol_args));
>  		if (IS_ERR(vol_args))
> @@ -966,13 +974,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;
>  }
> @@ -2235,7 +2243,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..bc70584 100644
> --- a/fs/btrfs/ioctl.h
> +++ b/fs/btrfs/ioctl.h
> @@ -30,10 +30,14 @@ 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_SNAPSHOT_CREATE_ASYNC	(1ULL << 0)
> +
> +#define BTRFS_SNAPSHOT_NAME_MAX 4039
> +struct btrfs_ioctl_vol_args_v2 {
>  	__s64 fd;
>  	__u64 transid;
> +	__u64 flags;
> +	__u64 unused[4];
>  	char name[BTRFS_SNAPSHOT_NAME_MAX + 1];
>  };

Why the unused fields ? What happens if you use a more recent btrfs-tools 
which take advantage of these fields but the kernel is an old one ? At the 
minimum please check the flags so
	(flags^(BTRFS_SNAPSHOT_CREATE_ASYNC|BTRFS_SNAPSHOT_RDONLY)) == 0
and
	unused[0..3] == 0

For future expansion I suggest to use a different ioctl. To me it seems a more 
robust API.

>  
> @@ -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)

I repeat: great work, please take my comments only a suggestion to improve 
instead of an empty criticism.

Regards
G.Baroncelli

>  #endif
> -- 
> 1.6.3
> 
> --
> 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
> 


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

Re: [PATCH 5/5] btrfs: Add ioctl to set snapshot readonly/writable

[Goffredo Baroncelli]

Hi Li,

On Monday, 29 November, 2010, Li Zefan wrote:
> This allows us to set a snapshot readonly or writable on the fly.
> 
> Usage:
> 
> Set BTRFS_SNAPSHOT_RDONLY/WRITABLE of btrfs_ioctl_vol_arg_v2->flags,
> and then call ioctl(BTRFS_IOCTL_SNAP_SETFLAGS);

I really appreciate your work, but I have some doubt about this interface. In 
particolar:
- how get the flags of a subvolume ? I suggest to implement a pair of ioctls:
	- subvolume_setflags  -> get the flags
	- subvolume_getflags  -> set the flags
These ioctls would be more generic (there are a lot of flags which may be 
interested to put in the "root" of a subvolume: think about 
compress/nocompress, (no)datasum...)
- For the reason abowe, I suggest to replace SNAPSHOT with SUBVOLUME
- Finally, with a pair of  get/set_flags functions we can avoid the use of the 
flags BTRFS_SNAPSHOT_WRITABLE.


> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  fs/btrfs/ioctl.c |   88 
+++++++++++++++++++++++++++++++++++++++++++++++++++--
>  fs/btrfs/ioctl.h |    3 ++
>  2 files changed, 87 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 7f9c571..34d8683 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -939,6 +939,24 @@ out:
>  	return ret;
>  }
>  
> +static int snap_check_flags(u64 flags, bool create)
> +{
> +	u64 valid_flags = BTRFS_SNAPSHOT_RDONLY | BTRFS_SNAPSHOT_WRITABLE;
> +
> +	if (create)
> +		valid_flags |= BTRFS_SNAPSHOT_CREATE_ASYNC;
> +
> +	if (flags & valid_flags)
> +		return -EINVAL;
> +
> +	/* readonly and writable are mutually exclusive */
> +	if ((flags & BTRFS_SNAPSHOT_RDONLY) &&
> +	    (flags & BTRFS_SNAPSHOT_WRITABLE))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static noinline int btrfs_ioctl_snap_create(struct file *file,
>  					    void __user *arg, int subvol,
>  					    bool v2)
> @@ -957,11 +975,9 @@ static noinline int btrfs_ioctl_snap_create(struct file 
*file,
>  		if (IS_ERR(vol_args_v2))
>  			return PTR_ERR(vol_args_v2);
>  
> -		if (vol_args_v2->flags &
> -		    ~(BTRFS_SNAPSHOT_CREATE_ASYNC | BTRFS_SNAPSHOT_RDONLY)) {
> -			ret = -EINVAL;
> +		ret = snap_check_flags(vol_args_v2->flags, true);
> +		if (ret)
>  			goto out;
> -		}
>  
>  		name = vol_args_v2->name;
>  		fd = vol_args_v2->fd;
> @@ -995,6 +1011,65 @@ out:
>  	return ret;
>  }
>  
> +static noinline int btrfs_ioctl_snap_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_v2;
> +	u64 root_flags;
> +	u64 flags;
> +	int err;
> +
> +	if (root->fs_info->sb->s_flags & MS_RDONLY)
> +		return -EROFS;
> +
> +	vol_args_v2 = memdup_user(arg, sizeof(*vol_args_v2));
> +	if (IS_ERR(vol_args_v2))
> +		return PTR_ERR(vol_args_v2);
> +	flags = vol_args_v2->flags;
> +
> +	err = snap_check_flags(flags, false);
> +	if (err)
> +		goto out;
> +
> +	down_write(&root->fs_info->subvol_sem);
> +
> +	/* nothing to do */
> +	if ((BTRFS_SNAPSHOT_RDONLY && root->readonly) ||
> +	    (BTRFS_SNAPSHOT_WRITABLE && !root->readonly))
> +		goto out_unlock;
> +
> +	root_flags = btrfs_root_flags(&root->root_item);
> +	if (flags & BTRFS_SNAPSHOT_RDONLY) {
> +		btrfs_set_root_flags(&root->root_item,
> +				     root_flags | BTRFS_ROOT_SNAP_RDONLY);
> +		root->readonly = true;
> +	}
> +	if (flags & BTRFS_SNAPSHOT_WRITABLE) {
> +		btrfs_set_root_flags(&root->root_item,
> +				     root_flags & ~BTRFS_ROOT_SNAP_RDONLY);
> +		root->readonly = false;
> +	}
> +
> +	trans = btrfs_start_transaction(root, 1);
> +	if (IS_ERR(trans)) {
> +		err = PTR_ERR(trans);
> +		goto out_unlock;
> +	}
> +
> +	err = 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_v2);
> +	return err;
> +}
> +
>  /*
>   * helper to check if the subvolume references other subvolumes
>   */
> @@ -1503,6 +1578,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;
> @@ -2266,6 +2344,8 @@ long btrfs_ioctl(struct file *file, unsigned int
>  		return btrfs_ioctl_snap_create(file, argp, 1, 0);
>  	case BTRFS_IOC_SNAP_DESTROY:
>  		return btrfs_ioctl_snap_destroy(file, argp);
> +	case BTRFS_IOC_SNAP_SETFLAGS:
> +		return btrfs_ioctl_snap_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 ff15fb2..559fd27 100644
> --- a/fs/btrfs/ioctl.h
> +++ b/fs/btrfs/ioctl.h
> @@ -32,6 +32,7 @@ struct btrfs_ioctl_vol_args {
>  
>  #define BTRFS_SNAPSHOT_CREATE_ASYNC	(1ULL << 0)
>  #define BTRFS_SNAPSHOT_RDONLY		(1ULL << 1)
> +#define BTRFS_SNAPSHOT_WRITABLE		(1ULL << 2)
>  
>  #define BTRFS_SNAPSHOT_NAME_MAX 4039
>  struct btrfs_ioctl_vol_args_v2 {
> @@ -194,4 +195,6 @@ 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_SNAP_SETFLAGS _IOW(BTRFS_IOCTL_MAGIC, 24, \
> +				   struct btrfs_ioctl_vol_args_v2)
>  #endif
> -- 
> 1.6.3
> 
> --
> 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
> 


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

Re: [PATCH 1/5] btrfs: Make async snapshot ioctl more generic

[Sage Weil]

Hi Li,

On Mon, 29 Nov 2010, Li Zefan wrote:
> So we don't have to add new structures as we create more ioctls
> for snapshots.
> 
> Now to create async snapshot, set BTRFS_SNAPSHOT_CREATE_ASYNC bit of
> vol_arg_v2->flags, and then call ioctl(BTRFS_IOCT_SNAP_CREATE_V2).
> 
> Note: this changes the async snapshot ioctl ABI, which was merged
> in .37 merge window, so we have to make this change into .37.

These changes all look good to me.

Thanks!
sage



> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  fs/btrfs/ioctl.c |   34 +++++++++++++++++++++-------------
>  fs/btrfs/ioctl.h |   12 ++++++++----
>  2 files changed, 29 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 463d91b..d3f1a60 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -935,23 +935,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_SNAPSHOT_CREATE_ASYNC) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		name = vol_args_v2->name;
> +		fd = vol_args_v2->fd;
> +		vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
> +		if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
> +			async = true;
>  	} else {
>  		vol_args = memdup_user(arg, sizeof(*vol_args));
>  		if (IS_ERR(vol_args))
> @@ -966,13 +974,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;
>  }
> @@ -2235,7 +2243,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..bc70584 100644
> --- a/fs/btrfs/ioctl.h
> +++ b/fs/btrfs/ioctl.h
> @@ -30,10 +30,14 @@ 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_SNAPSHOT_CREATE_ASYNC	(1ULL << 0)
> +
> +#define BTRFS_SNAPSHOT_NAME_MAX 4039
> +struct btrfs_ioctl_vol_args_v2 {
>  	__s64 fd;
>  	__u64 transid;
> +	__u64 flags;
> +	__u64 unused[4];
>  	char name[BTRFS_SNAPSHOT_NAME_MAX + 1];
>  };
>  
> @@ -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
> 
> --
> 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/5] btrfs: Make async snapshot ioctl more generic

[Li Zefan]

Goffredo Baroncelli wrote:
> Hi Li,
> 
> great work, but I have some suggestions:
> 
> On Monday, 29 November, 2010, Li Zefan wrote:
>> So we don't have to add new structures as we create more ioctls
>> for snapshots.
>>
>> Now to create async snapshot, set BTRFS_SNAPSHOT_CREATE_ASYNC bit of
>> vol_arg_v2->flags, and then call ioctl(BTRFS_IOCT_SNAP_CREATE_V2).
>>
>> Note: this changes the async snapshot ioctl ABI, which was merged
>> in .37 merge window, so we have to make this change into .37.
>>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>>  fs/btrfs/ioctl.c |   34 +++++++++++++++++++++-------------
>>  fs/btrfs/ioctl.h |   12 ++++++++----
>>  2 files changed, 29 insertions(+), 17 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 463d91b..d3f1a60 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -935,23 +935,31 @@ out:
>>  
>>  static noinline int btrfs_ioctl_snap_create(struct file *file,
>>  					    void __user *arg, int subvol,
>> -					    int async)
>> +					    bool v2)
> 
> This is a aesthetic suggestion: instead of passing a flag called v2 I suggest 
> to add two wrapper functions, which extract the parameters and passes all 
> available parameter to the "generic" function. Something like:
> 

Sure, as long as it won't result in code duplication and can
improve readability.

> static inline btrfs_ioctl_snap_create_v1(struct file *file, 
> 						void __user *arg, int subvol)
> {
>                 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';
> 
> 		btrfs_ioctl_snap_create_generic(file, subvol, name, fd, 0, 0);
> 
> }
> 
> static inline btrfs_ioctl_snap_create_v2(struct file *file, 
> 						void __user *arg, int subvol,
> 						)
> {
>                 vol_args_v2 = memdup_user(arg, sizeof(*vol_args_v2));
>                 if (IS_ERR(vol_args_v2))
>                         return PTR_ERR(vol_args_v2);
> 
>                 ret = snap_check_flags(vol_args_v2->flags, true);
>                 if (ret)
>                         goto out;
> 
>                 name = vol_args_v2->name;
>                 fd = vol_args_v2->fd;
>                 vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
>                 if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
>                         async = true;
>                 if (vol_args_v2->flags & BTRFS_SNAPSHOT_RDONLY)
>                         readonly = true;
> 
> 		btrfs_ioctl_snap_create_generic(file, subvol, name, fd, async,
> 			 readonly);
> 
> }
> 
> and 
> 
>   	case BTRFS_IOC_SNAP_CREATE:
>   		return btrfs_ioctl_snap_create_v1(file, argp, 0);
> 	case BTRFS_IOC_SNAP_CREATE_V2:
>   		return btrfs_ioctl_snap_create_v2(file, argp, 0);
> 
> 
> Frankly speaking, we could get rid of the subvol parameter adding another 
> wrapper function like "btrfs_ioctl_subvol_create( )", but this would be 
> another story :)
> 

I thought about that too.

>>  {
>>  	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_SNAPSHOT_CREATE_ASYNC) {
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		name = vol_args_v2->name;
>> +		fd = vol_args_v2->fd;
>> +		vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
>> +		if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
>> +			async = true;
>>  	} else {
>>  		vol_args = memdup_user(arg, sizeof(*vol_args));
>>  		if (IS_ERR(vol_args))
>> @@ -966,13 +974,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;
>>  }
>> @@ -2235,7 +2243,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..bc70584 100644
>> --- a/fs/btrfs/ioctl.h
>> +++ b/fs/btrfs/ioctl.h
>> @@ -30,10 +30,14 @@ 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_SNAPSHOT_CREATE_ASYNC	(1ULL << 0)
>> +
>> +#define BTRFS_SNAPSHOT_NAME_MAX 4039
>> +struct btrfs_ioctl_vol_args_v2 {
>>  	__s64 fd;
>>  	__u64 transid;
>> +	__u64 flags;
>> +	__u64 unused[4];
>>  	char name[BTRFS_SNAPSHOT_NAME_MAX + 1];
>>  };
> 
> Why the unused fields ? What happens if you use a more recent btrfs-tools 
> which take advantage of these fields but the kernel is an old one ? At the 

It's common that we reserve some place in an ABI for future expansion.
If later those unused bytes are used, it should make sure it won't
break old kernels.

> minimum please check the flags so
> 	(flags^(BTRFS_SNAPSHOT_CREATE_ASYNC|BTRFS_SNAPSHOT_RDONLY)) == 0

This is checked.

> and
> 	unused[0..3] == 0

No, we don't need to check this. I don't think other ioctls that have
reserved bytes check this.

> 
> For future expansion I suggest to use a different ioctl. To me it seems a more 
> robust API.

I'd like to avoid new ioctls if possible. If we had had some reserved
bytes in struct btrfs_ioctl_vol_args, we wouldn't need to create
a v2 ioctl.

> 
>>  
>> @@ -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)
> 
> I repeat: great work, please take my comments only a suggestion to improve 
> instead of an empty criticism.
> 

Thanks!

> Regards
> G.Baroncelli
> 
>>  #endif
>> -- 
>> 1.6.3
>>

Re: [PATCH 5/5] btrfs: Add ioctl to set snapshot readonly/writable

[Li Zefan]

Goffredo Baroncelli wrote:
> Hi Li,
> 
> On Monday, 29 November, 2010, Li Zefan wrote:
>> This allows us to set a snapshot readonly or writable on the fly.
>>
>> Usage:
>>
>> Set BTRFS_SNAPSHOT_RDONLY/WRITABLE of btrfs_ioctl_vol_arg_v2->flags,
>> and then call ioctl(BTRFS_IOCTL_SNAP_SETFLAGS);
> 
> I really appreciate your work, but I have some doubt about this interface. In 
> particolar:

It's the interface that I would like to be discussed. Thanks!

> - how get the flags of a subvolume ? I suggest to implement a pair of ioctls:
> 	- subvolume_setflags  -> get the flags
> 	- subvolume_getflags  -> set the flags
> These ioctls would be more generic (there are a lot of flags which may be 
> interested to put in the "root" of a subvolume: think about 
> compress/nocompress, (no)datasum...)
> - For the reason abowe, I suggest to replace SNAPSHOT with SUBVOLUME
> - Finally, with a pair of  get/set_flags functions we can avoid the use of the 
> flags BTRFS_SNAPSHOT_WRITABLE.
> 

There are some reasons that I created this interface:

- set/getflags should set/get root flags which reflect in struct
btrfs_root_item->flags.

- btrfs_root_item->flags was not used at all before this patch, so
(no)compress and (no)datasum is not reflect in ->flags.

- _CREATE_ASYNC flag is to create snapshot asynchronously, so it's not
a flag of tree root.

- It seems to me there's no user requirement for getflags ioctl to
return _RDONLY/_WRITABLE flags of a tree root?

- By suggesting BTRFS_SUBVOL_RDONLY, does it impliy not only snapshot
but also a subvolume can be made readonly?

> 
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>>  fs/btrfs/ioctl.c |   88 
> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  fs/btrfs/ioctl.h |    3 ++
>>  2 files changed, 87 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 7f9c571..34d8683 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -939,6 +939,24 @@ out:
>>  	return ret;
>>  }
>>  
>> +static int snap_check_flags(u64 flags, bool create)
>> +{
>> +	u64 valid_flags = BTRFS_SNAPSHOT_RDONLY | BTRFS_SNAPSHOT_WRITABLE;
>> +
>> +	if (create)
>> +		valid_flags |= BTRFS_SNAPSHOT_CREATE_ASYNC;
>> +
>> +	if (flags & valid_flags)
>> +		return -EINVAL;
>> +
>> +	/* readonly and writable are mutually exclusive */
>> +	if ((flags & BTRFS_SNAPSHOT_RDONLY) &&
>> +	    (flags & BTRFS_SNAPSHOT_WRITABLE))
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>>  static noinline int btrfs_ioctl_snap_create(struct file *file,
>>  					    void __user *arg, int subvol,
>>  					    bool v2)
>> @@ -957,11 +975,9 @@ static noinline int btrfs_ioctl_snap_create(struct file 
> *file,
>>  		if (IS_ERR(vol_args_v2))
>>  			return PTR_ERR(vol_args_v2);
>>  
>> -		if (vol_args_v2->flags &
>> -		    ~(BTRFS_SNAPSHOT_CREATE_ASYNC | BTRFS_SNAPSHOT_RDONLY)) {
>> -			ret = -EINVAL;
>> +		ret = snap_check_flags(vol_args_v2->flags, true);
>> +		if (ret)
>>  			goto out;
>> -		}
>>  
>>  		name = vol_args_v2->name;
>>  		fd = vol_args_v2->fd;
>> @@ -995,6 +1011,65 @@ out:
>>  	return ret;
>>  }
>>  
>> +static noinline int btrfs_ioctl_snap_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_v2;
>> +	u64 root_flags;
>> +	u64 flags;
>> +	int err;
>> +
>> +	if (root->fs_info->sb->s_flags & MS_RDONLY)
>> +		return -EROFS;
>> +
>> +	vol_args_v2 = memdup_user(arg, sizeof(*vol_args_v2));
>> +	if (IS_ERR(vol_args_v2))
>> +		return PTR_ERR(vol_args_v2);
>> +	flags = vol_args_v2->flags;
>> +
>> +	err = snap_check_flags(flags, false);
>> +	if (err)
>> +		goto out;
>> +
>> +	down_write(&root->fs_info->subvol_sem);
>> +
>> +	/* nothing to do */
>> +	if ((BTRFS_SNAPSHOT_RDONLY && root->readonly) ||
>> +	    (BTRFS_SNAPSHOT_WRITABLE && !root->readonly))
>> +		goto out_unlock;
>> +
>> +	root_flags = btrfs_root_flags(&root->root_item);
>> +	if (flags & BTRFS_SNAPSHOT_RDONLY) {
>> +		btrfs_set_root_flags(&root->root_item,
>> +				     root_flags | BTRFS_ROOT_SNAP_RDONLY);
>> +		root->readonly = true;
>> +	}
>> +	if (flags & BTRFS_SNAPSHOT_WRITABLE) {
>> +		btrfs_set_root_flags(&root->root_item,
>> +				     root_flags & ~BTRFS_ROOT_SNAP_RDONLY);
>> +		root->readonly = false;
>> +	}
>> +
>> +	trans = btrfs_start_transaction(root, 1);
>> +	if (IS_ERR(trans)) {
>> +		err = PTR_ERR(trans);
>> +		goto out_unlock;
>> +	}
>> +
>> +	err = 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_v2);
>> +	return err;
>> +}
>> +
>>  /*
>>   * helper to check if the subvolume references other subvolumes
>>   */
>> @@ -1503,6 +1578,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;
>> @@ -2266,6 +2344,8 @@ long btrfs_ioctl(struct file *file, unsigned int
>>  		return btrfs_ioctl_snap_create(file, argp, 1, 0);
>>  	case BTRFS_IOC_SNAP_DESTROY:
>>  		return btrfs_ioctl_snap_destroy(file, argp);
>> +	case BTRFS_IOC_SNAP_SETFLAGS:
>> +		return btrfs_ioctl_snap_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 ff15fb2..559fd27 100644
>> --- a/fs/btrfs/ioctl.h
>> +++ b/fs/btrfs/ioctl.h
>> @@ -32,6 +32,7 @@ struct btrfs_ioctl_vol_args {
>>  
>>  #define BTRFS_SNAPSHOT_CREATE_ASYNC	(1ULL << 0)
>>  #define BTRFS_SNAPSHOT_RDONLY		(1ULL << 1)
>> +#define BTRFS_SNAPSHOT_WRITABLE		(1ULL << 2)
>>  
>>  #define BTRFS_SNAPSHOT_NAME_MAX 4039
>>  struct btrfs_ioctl_vol_args_v2 {
>> @@ -194,4 +195,6 @@ 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_SNAP_SETFLAGS _IOW(BTRFS_IOCTL_MAGIC, 24, \
>> +				   struct btrfs_ioctl_vol_args_v2)
>>  #endif
>> -- 
>> 1.6.3
>>
>> --
>> 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/5] btrfs: Make async snapshot ioctl more generic

[Sage Weil]

Hi Chris,

Is this ioctl change destined for 2.6.37?  If it (or something similar) 
looks good it should probably be merged (independent of the rest of the 
series) this time around before we're stuck with the current version.

Thanks-
sage


On Mon, 29 Nov 2010, Li Zefan wrote:

> So we don't have to add new structures as we create more ioctls
> for snapshots.
> 
> Now to create async snapshot, set BTRFS_SNAPSHOT_CREATE_ASYNC bit of
> vol_arg_v2->flags, and then call ioctl(BTRFS_IOCT_SNAP_CREATE_V2).
> 
> Note: this changes the async snapshot ioctl ABI, which was merged
> in .37 merge window, so we have to make this change into .37.
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  fs/btrfs/ioctl.c |   34 +++++++++++++++++++++-------------
>  fs/btrfs/ioctl.h |   12 ++++++++----
>  2 files changed, 29 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 463d91b..d3f1a60 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -935,23 +935,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_SNAPSHOT_CREATE_ASYNC) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		name = vol_args_v2->name;
> +		fd = vol_args_v2->fd;
> +		vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
> +		if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
> +			async = true;
>  	} else {
>  		vol_args = memdup_user(arg, sizeof(*vol_args));
>  		if (IS_ERR(vol_args))
> @@ -966,13 +974,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;
>  }
> @@ -2235,7 +2243,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..bc70584 100644
> --- a/fs/btrfs/ioctl.h
> +++ b/fs/btrfs/ioctl.h
> @@ -30,10 +30,14 @@ 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_SNAPSHOT_CREATE_ASYNC	(1ULL << 0)
> +
> +#define BTRFS_SNAPSHOT_NAME_MAX 4039
> +struct btrfs_ioctl_vol_args_v2 {
>  	__s64 fd;
>  	__u64 transid;
> +	__u64 flags;
> +	__u64 unused[4];
>  	char name[BTRFS_SNAPSHOT_NAME_MAX + 1];
>  };
>  
> @@ -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
> 
> --
> 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/5] btrfs: Make async snapshot ioctl more generic

[Li Zefan]

03:04, Sage Weil wrote:
> Hi Chris,
> 
> Is this ioctl change destined for 2.6.37?  If it (or something similar) 
> looks good it should probably be merged (independent of the rest of the 
> series) this time around before we're stuck with the current version.
> 

I'm going to post a revised patchset today or tomorrow. Thanks!

btw, is there a typo?

#define BTRFS_IOC_SPACE_INFO _IOWR(BTRFS_IOCTL_MAGIC, 20, \
                                    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)

It seems we should have given 21 to START_SYNC ioctl.

> Thanks-
> sage
> 
> 
> On Mon, 29 Nov 2010, Li Zefan wrote:
> 
>> So we don't have to add new structures as we create more ioctls
>> for snapshots.
>>
>> Now to create async snapshot, set BTRFS_SNAPSHOT_CREATE_ASYNC bit of
>> vol_arg_v2->flags, and then call ioctl(BTRFS_IOCT_SNAP_CREATE_V2).
>>
>> Note: this changes the async snapshot ioctl ABI, which was merged
>> in .37 merge window, so we have to make this change into .37.
>>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>>  fs/btrfs/ioctl.c |   34 +++++++++++++++++++++-------------
>>  fs/btrfs/ioctl.h |   12 ++++++++----
>>  2 files changed, 29 insertions(+), 17 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 463d91b..d3f1a60 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -935,23 +935,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_SNAPSHOT_CREATE_ASYNC) {
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		name = vol_args_v2->name;
>> +		fd = vol_args_v2->fd;
>> +		vol_args_v2->name[BTRFS_SNAPSHOT_NAME_MAX] = '\0';
>> +		if (vol_args_v2->flags & BTRFS_SNAPSHOT_CREATE_ASYNC)
>> +			async = true;
>>  	} else {
>>  		vol_args = memdup_user(arg, sizeof(*vol_args));
>>  		if (IS_ERR(vol_args))
>> @@ -966,13 +974,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;
>>  }
>> @@ -2235,7 +2243,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..bc70584 100644
>> --- a/fs/btrfs/ioctl.h
>> +++ b/fs/btrfs/ioctl.h
>> @@ -30,10 +30,14 @@ 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_SNAPSHOT_CREATE_ASYNC	(1ULL << 0)
>> +
>> +#define BTRFS_SNAPSHOT_NAME_MAX 4039
>> +struct btrfs_ioctl_vol_args_v2 {
>>  	__s64 fd;
>>  	__u64 transid;
>> +	__u64 flags;
>> +	__u64 unused[4];
>>  	char name[BTRFS_SNAPSHOT_NAME_MAX + 1];
>>  };
>>  
>> @@ -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
>>
>> --

Re: [PATCH 1/5] btrfs: Make async snapshot ioctl more generic

[Goffredo Baroncelli]

On Monday, 29 November, 2010, Goffredo Baroncelli wrote:
> Why the unused fields ? What happens if you use a more recent btrfs-tools 
> which take advantage of these fields but the kernel is an old one ? At the 
> minimum please check the flags so
>         (flags^(BTRFS_SNAPSHOT_CREATE_ASYNC|BTRFS_SNAPSHOT_RDONLY)) == 0
> and
>         unused[0..3] == 0


Sorry, the check should be

(flags & (~(BTRFS_SNAPSHOT_CREATE_ASYNC|BTRFS_SNAPSHOT_RDONLY))) == 0
> 
> For future expansion I suggest to use a different ioctl. To me it seems a 
more 
> robust API.
> 


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