From: Goffredo Baroncelli <kreijack@libero.it>
To: Li Zefan <lizf@cn.fujitsu.com>
Cc: linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 5/5] btrfs: Add ioctl to set snapshot readonly/writable
Date: Mon, 29 Nov 2010 19:52:13 +0100 [thread overview]
Message-ID: <201011291952.13358.kreijack@libero.it> (raw)
In-Reply-To: <4CF35E6B.2030307@cn.fujitsu.com>
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
next prev parent reply other threads:[~2010-11-29 18:52 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-11-29 8:02 [PATCH 0/5] btrfs: Readonly snapshots Li Zefan
2010-11-29 8:02 ` [PATCH 1/5] btrfs: Make async snapshot ioctl more generic Li Zefan
2010-11-29 18:52 ` Goffredo Baroncelli
2010-11-30 1:13 ` Li Zefan
2010-11-29 19:28 ` Sage Weil
2010-12-07 19:04 ` Sage Weil
2010-12-08 1:09 ` Li Zefan
2010-11-29 8:03 ` [PATCH 2/5] btrfs: Fix memory leak in a failure path Li Zefan
2010-11-29 8:03 ` [PATCH 3/5] btrfs: Add helper __setup_root_post() Li Zefan
2010-11-29 8:03 ` [PATCH 4/5] btrfs: Add readonly snapshots support Li Zefan
2010-11-29 8:03 ` [PATCH 5/5] btrfs: Add ioctl to set snapshot readonly/writable Li Zefan
2010-11-29 18:52 ` Goffredo Baroncelli [this message]
2010-11-30 7:03 ` Li Zefan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201011291952.13358.kreijack@libero.it \
--to=kreijack@libero.it \
--cc=linux-btrfs@vger.kernel.org \
--cc=lizf@cn.fujitsu.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.