Re: [PATCH v0 17/18] btrfs: add qgroup ioctls

[Andi Kleen]

Arne Jansen  writes:
> +
> + if (copy_to_user(arg, sa, sizeof(*sa)))
> + ret = -EFAULT;
> +
> + if (trans) {
> + err = btrfs_commit_transaction(trans, root);
> + if (err && !ret)
> + ret = err;
> + }

It would seem safer to put the copy to user outside the transaction.
A cto can in principle cause new writes (e.g. if it causes COW), so 
you may end up with nested transactions. Even if that works somehow
(not sure) it seems to be a thing better avoided.

> +
> + sa = memdup_user(arg, sizeof(*sa));
> + if (IS_ERR(sa))
> + return PTR_ERR(sa);
> +
> + trans = btrfs_join_transaction(root);
> + if (IS_ERR(trans)) {
> + ret = PTR_ERR(trans);
> + goto out;
> + }

This code seems to be duplicated a lot. Can it be consolidated?

-Andi
-- 
a...@linux.intel.com -- Speaking for myself only
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v0 17/18] btrfs: add qgroup ioctls

[Arne Jansen]

Ioctls to control the qgroup feature like adding and
removing qgroups and assigning qgroups.

Signed-off-by: Arne Jansen 
---
 fs/btrfs/ioctl.c |  185 ++
 fs/btrfs/ioctl.h |   27 
 2 files changed, 212 insertions(+), 0 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index fade500..f46bc35 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2834,6 +2834,183 @@ static long btrfs_ioctl_scrub_progress(struct 
btrfs_root *root,
return ret;
 }
 
+static long btrfs_ioctl_quota_ctl(struct btrfs_root *root, void __user *arg)
+{
+   struct btrfs_ioctl_quota_ctl_args *sa;
+   struct btrfs_trans_handle *trans = NULL;
+   int ret;
+   int err;
+
+   if (!capable(CAP_SYS_ADMIN))
+   return -EPERM;
+
+   if (root->fs_info->sb->s_flags & MS_RDONLY)
+   return -EROFS;
+
+   sa = memdup_user(arg, sizeof(*sa));
+   if (IS_ERR(sa))
+   return PTR_ERR(sa);
+
+   if (sa->cmd != BTRFS_QUOTA_CTL_RESCAN) {
+   trans = btrfs_start_transaction(root, 2);
+   if (IS_ERR(trans)) {
+   ret = PTR_ERR(trans);
+   goto out;
+   }
+   }
+
+   switch (sa->cmd) {
+   case BTRFS_QUOTA_CTL_ENABLE:
+   ret = btrfs_quota_enable(trans, root->fs_info);
+   break;
+   case BTRFS_QUOTA_CTL_DISABLE:
+   ret = btrfs_quota_disable(trans, root->fs_info);
+   break;
+   case BTRFS_QUOTA_CTL_RESCAN:
+   ret = btrfs_quota_rescan(root->fs_info);
+   break;
+   default:
+   ret = -EINVAL;
+   break;
+   }
+
+   if (copy_to_user(arg, sa, sizeof(*sa)))
+   ret = -EFAULT;
+
+   if (trans) {
+   err = btrfs_commit_transaction(trans, root);
+   if (err && !ret)
+   ret = err;
+   }
+
+out:
+   kfree(sa);
+   return ret;
+}
+
+static long btrfs_ioctl_qgroup_assign(struct btrfs_root *root, void __user 
*arg)
+{
+   struct btrfs_ioctl_qgroup_assign_args *sa;
+   struct btrfs_trans_handle *trans;
+   int ret;
+   int err;
+
+   if (!capable(CAP_SYS_ADMIN))
+   return -EPERM;
+
+   if (root->fs_info->sb->s_flags & MS_RDONLY)
+   return -EROFS;
+
+   sa = memdup_user(arg, sizeof(*sa));
+   if (IS_ERR(sa))
+   return PTR_ERR(sa);
+
+   trans = btrfs_join_transaction(root);
+   if (IS_ERR(trans)) {
+   ret = PTR_ERR(trans);
+   goto out;
+   }
+
+   /* FIXME: check if the IDs really exist */
+   if (sa->assign) {
+   ret = btrfs_add_qgroup_relation(trans, root->fs_info,
+   sa->src, sa->dst);
+   } else {
+   ret = btrfs_del_qgroup_relation(trans, root->fs_info,
+   sa->src, sa->dst);
+   }
+
+   err = btrfs_end_transaction(trans, root);
+   if (err && !ret)
+   ret = err;
+
+out:
+   kfree(sa);
+   return ret;
+}
+
+static long btrfs_ioctl_qgroup_create(struct btrfs_root *root, void __user 
*arg)
+{
+   struct btrfs_ioctl_qgroup_create_args *sa;
+   struct btrfs_trans_handle *trans;
+   int ret;
+   int err;
+
+   if (!capable(CAP_SYS_ADMIN))
+   return -EPERM;
+
+   if (root->fs_info->sb->s_flags & MS_RDONLY)
+   return -EROFS;
+
+   sa = memdup_user(arg, sizeof(*sa));
+   if (IS_ERR(sa))
+   return PTR_ERR(sa);
+
+   trans = btrfs_join_transaction(root);
+   if (IS_ERR(trans)) {
+   ret = PTR_ERR(trans);
+   goto out;
+   }
+
+   /* FIXME: check if the IDs really exist */
+   if (sa->create) {
+   ret = btrfs_create_qgroup(trans, root->fs_info, sa->qgroupid,
+ NULL);
+   } else {
+   ret = btrfs_remove_qgroup(trans, root->fs_info, sa->qgroupid);
+   }
+
+   err = btrfs_end_transaction(trans, root);
+   if (err && !ret)
+   ret = err;
+
+out:
+   kfree(sa);
+   return ret;
+}
+
+static long btrfs_ioctl_qgroup_limit(struct btrfs_root *root, void __user *arg)
+{
+   struct btrfs_ioctl_qgroup_limit_args *sa;
+   struct btrfs_trans_handle *trans;
+   int ret;
+   int err;
+   u64 qgroupid;
+
+   if (!capable(CAP_SYS_ADMIN))
+   return -EPERM;
+
+   if (root->fs_info->sb->s_flags & MS_RDONLY)
+   return -EROFS;
+
+   sa = memdup_user(arg, sizeof(*sa));
+   if (IS_ERR(sa))
+   return PTR_ERR(sa);
+
+   trans = btrfs_join_transaction(root);
+   if (IS_ERR(trans)) {
+   ret = PTR_ERR(trans);
+   goto out;
+   }
+
+   qgroupid = sa->qgroupid;
+   if (!qgroupid) {
+