[PATCH 0/5] Add user-space support for read-only snapshot creation.

[PATCH 0/5] Add user-space support for read-only snapshot creation.

[Andreas Philipp]

There is kernel side support for the creation of read-only snapshots
for some time now, but I did not find any patches for the userspace
btrfs utilites. Thus I created this patchset which is tested and
working with kernel version 2.6.39-rc2.

Andreas Philipp (5):
  Add support for read-only subvolumes.
  Support the new parameters in do_clone(int argc, char** argv).
  Added support for an additional ioctl.
  Test the additional ioctl.
  Updated documentation for btrfs subvolume snapshot.

 btrfs.c        |    6 +++---
 btrfs_cmds.c   |   44 ++++++++++++++++++++++++++++++++++++--------
 ioctl-test.c   |    1 +
 ioctl.h        |   14 ++++++++++++++
 man/btrfs.8.in |   11 ++++++-----
 5 files changed, 60 insertions(+), 16 deletions(-)

-- 
1.7.4.1

[PATCH 1/5] Add support for read-only subvolumes.

[Andreas Philipp]

Use BTRFS_IOC_CREATE_SNAP_V2 instead of BTRFS_IOC_CREATE_SNAP and add
an option for the creation of a readonly snapshot.

Signed-off-by: Andreas Philipp <philipp.andreas@gmail.com>
---
 btrfs_cmds.c |   44 ++++++++++++++++++++++++++++++++++++--------
 1 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/btrfs_cmds.c b/btrfs_cmds.c
index 8031c58..baec675 100644
--- a/btrfs_cmds.c
+++ b/btrfs_cmds.c
@@ -43,7 +43,7 @@
 
 #ifdef __CHECKER__
 #define BLKGETSIZE64 0
-#define BTRFS_IOC_SNAP_CREATE 0
+#define BTRFS_IOC_SNAP_CREATE_V2 0
 #define BTRFS_VOL_NAME_MAX 255
 struct btrfs_ioctl_vol_args { char name[BTRFS_VOL_NAME_MAX]; };
 static inline int ioctl(int fd, int define, void *arg) { return 0; }
@@ -310,13 +310,34 @@ int do_subvol_list(int argc, char **argv)
 int do_clone(int argc, char **argv)
 {
 	char	*subvol, *dst;
-	int	res, fd, fddst, len;
+	int	res, fd, fddst, len, optind = 0, readonly = 0;
 	char	*newname;
 	char	*dstdir;
 
-	subvol = argv[1];
-	dst = argv[2];
-	struct btrfs_ioctl_vol_args	args;
+	while(1) {
+		int c = getopt(argc, argv, "r");
+		if (c < 0)
+			break;
+		switch(c) {
+			case 'r':
+				optind++;
+				readonly = 1;
+				break;
+	                default:
+				fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
+				free(argv);
+				return 1;
+		}
+	}
+	if (argc - optind < 2) {
+		fprintf(stderr, "Invalid arguments for defragment\n");
+		free(argv);
+		return 1;
+	}
+
+	subvol = argv[optind+1];
+	dst = argv[optind+2];
+	struct btrfs_ioctl_vol_args_v2	args;
 
 	res = test_issubvolume(subvol);
 	if(res<0){
@@ -371,12 +392,19 @@ int do_clone(int argc, char **argv)
 		fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir);
 		return 12;
 	}
+	
+	if (readonly) {
+		args.flags |= BTRFS_SUBVOL_RDONLY;
+		printf("Create a readonly snapshot of '%s' in '%s/%s'\n",
+		       subvol, dstdir, newname);
+	}
+	else
+		printf("Create a snapshot of '%s' in '%s/%s'\n",
+		       subvol, dstdir, newname);
 
-	printf("Create a snapshot of '%s' in '%s/%s'\n",
-	       subvol, dstdir, newname);
 	args.fd = fd;
 	strcpy(args.name, newname);
-	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE, &args);
+	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
 
 	close(fd);
 	close(fddst);
-- 
1.7.4.1

[PATCH 2/5] Support the new parameters in do_clone(int argc, char** argv).

[Andreas Philipp]

Now 'btrfs subvolume snapshot' takes not two but only at least two
parameters. Additionally, the help message is updated accordingly.

Signed-off-by: Andreas Philipp <philipp.andreas@gmail.com>
---
 btrfs.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/btrfs.c b/btrfs.c
index 46314cf..f70d64b 100644
--- a/btrfs.c
+++ b/btrfs.c
@@ -44,9 +44,9 @@ static struct Command commands[] = {
 	/*
 		avoid short commands different for the case only
 	*/
-	{ do_clone, 2,
-	  "subvolume snapshot", "<source> [<dest>/]<name>\n"
-		"Create a writable snapshot of the subvolume <source> with\n"
+	{ do_clone, -2,
+	  "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
+		"Create a writable/readonly snapshot of the subvolume <source> with\n"
 		"the name <name> in the <dest> directory."
 	},
 	{ do_delete_subvolume, 1,
-- 
1.7.4.1

[PATCH 3/5] Added support for an additional ioctl.

[Andreas Philipp]

Added BTRFS_IOC_SNAP_CREATE_V2 and struct btrfs_ioctl_vol_args_v2 as
defined in fs/btrfs/ioctl.h in the kernel sources.

Signed-off-by: Andreas Philipp <philipp.andreas@gmail.com>
---
 ioctl.h |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/ioctl.h b/ioctl.h
index 776d7a9..358f814 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -30,6 +30,17 @@ struct btrfs_ioctl_vol_args {
 	char name[BTRFS_PATH_NAME_MAX + 1];
 };
 
+#define BTRFS_SUBVOL_RDONLY		(1ULL << 1)
+#define BTRFS_SUBVOL_NAME_MAX 4039
+
+struct btrfs_ioctl_vol_args_v2 {
+	__s64 fd;
+	__u64 transid;
+	__u64 flags;
+	__u64 unused[4];
+	char name[BTRFS_SUBVOL_NAME_MAX + 1];
+};
+
 struct btrfs_ioctl_search_key {
 	/* which root are we searching.  0 is the tree of tree roots */
 	__u64 tree_id;
@@ -132,6 +143,7 @@ struct btrfs_ioctl_space_args {
 	struct btrfs_ioctl_space_info spaces[0];
 };
 
+/* BTRFS_IOC_SNAP_CREATE is no longer used by the btrfs command */
 #define BTRFS_IOC_SNAP_CREATE _IOW(BTRFS_IOCTL_MAGIC, 1, \
 				   struct btrfs_ioctl_vol_args)
 #define BTRFS_IOC_DEFRAG _IOW(BTRFS_IOCTL_MAGIC, 2, \
@@ -169,4 +181,6 @@ struct btrfs_ioctl_space_args {
 #define BTRFS_IOC_DEFAULT_SUBVOL _IOW(BTRFS_IOCTL_MAGIC, 19, u64)
 #define BTRFS_IOC_SPACE_INFO _IOWR(BTRFS_IOCTL_MAGIC, 20, \
 				    struct btrfs_ioctl_space_args)
+#define BTRFS_IOC_SNAP_CREATE_V2 _IOW(BTRFS_IOCTL_MAGIC, 23, \
+				   struct btrfs_ioctl_vol_args_v2)
 #endif
-- 
1.7.4.1

[PATCH 4/5] Test the additional ioctl.

[Andreas Philipp]

Signed-off-by: Andreas Philipp <philipp.andreas@gmail.com>
---
 ioctl-test.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/ioctl-test.c b/ioctl-test.c
index 7cf3bc2..1c27d61 100644
--- a/ioctl-test.c
+++ b/ioctl-test.c
@@ -22,6 +22,7 @@ unsigned long ioctls[] = {
 	BTRFS_IOC_INO_LOOKUP,
 	BTRFS_IOC_DEFAULT_SUBVOL,
 	BTRFS_IOC_SPACE_INFO,
+	BTRFS_IOC_SNAP_CREATE_V2,
 	0 };
 
 int main(int ac, char **av)
-- 
1.7.4.1

[PATCH 5/5] Updated documentation for btrfs subvolume snapshot.

[Andreas Philipp]

Signed-off-by: Andreas Philipp <philipp.andreas@gmail.com>
---
 man/btrfs.8.in |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/man/btrfs.8.in b/man/btrfs.8.in
index 26ef982..b59bc6f 100644
--- a/man/btrfs.8.in
+++ b/man/btrfs.8.in
@@ -5,7 +5,7 @@
 .SH NAME
 btrfs \- control a btrfs filesystem
 .SH SYNOPSIS
-\fBbtrfs\fP \fBsubvolume snapshot\fP\fI <source> [<dest>/]<name>\fP
+\fBbtrfs\fP \fBsubvolume snapshot\fP\fI [-r] <source> [<dest>/]<name>\fP
 .PP
 \fBbtrfs\fP \fBsubvolume delete\fP\fI <subvolume>\fP
 .PP
@@ -70,10 +70,11 @@ command.
 .SH COMMANDS
 .TP
 
-\fBsubvolume snapshot\fR\fI <source> [<dest>/]<name>\fR
-Create a writable snapshot of the subvolume \fI<source>\fR with the name
-\fI<name>\fR in the \fI<dest>\fR directory. If \fI<source>\fR is not a
-subvolume, \fBbtrfs\fR returns an error.
+\fBsubvolume snapshot\fR\fI [-r] <source> [<dest>/]<name>\fR
+Create a writable/readonly snapshot of the subvolume \fI<source>\fR with the
+name \fI<name>\fR in the \fI<dest>\fR directory. If \fI<source>\fR is not a
+subvolume, \fBbtrfs\fR returns an error. If \fI-r\fR is given, the snapshot
+will be readonly.
 .TP
 
 \fBsubvolume delete\fR\fI <subvolume>\fR
-- 
1.7.4.1

[PATCH 1/5] Add support for read-only subvolumes.

[Andreas Philipp]

Use BTRFS_IOC_CREATE_SNAP_V2 instead of BTRFS_IOC_CREATE_SNAP and add
an option for the creation of a readonly snapshot.

Signed-off-by: Andreas Philipp <philipp.andreas@gmail.com>
---
 btrfs_cmds.c |   44 ++++++++++++++++++++++++++++++++++++--------
 1 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/btrfs_cmds.c b/btrfs_cmds.c
index 8031c58..baec675 100644
--- a/btrfs_cmds.c
+++ b/btrfs_cmds.c
@@ -43,7 +43,7 @@
 
 #ifdef __CHECKER__
 #define BLKGETSIZE64 0
-#define BTRFS_IOC_SNAP_CREATE 0
+#define BTRFS_IOC_SNAP_CREATE_V2 0
 #define BTRFS_VOL_NAME_MAX 255
 struct btrfs_ioctl_vol_args { char name[BTRFS_VOL_NAME_MAX]; };
 static inline int ioctl(int fd, int define, void *arg) { return 0; }
@@ -310,13 +310,34 @@ int do_subvol_list(int argc, char **argv)
 int do_clone(int argc, char **argv)
 {
 	char	*subvol, *dst;
-	int	res, fd, fddst, len;
+	int	res, fd, fddst, len, optind = 0, readonly = 0;
 	char	*newname;
 	char	*dstdir;
 
-	subvol = argv[1];
-	dst = argv[2];
-	struct btrfs_ioctl_vol_args	args;
+	while(1) {
+		int c = getopt(argc, argv, "r");
+		if (c < 0)
+			break;
+		switch(c) {
+			case 'r':
+				optind++;
+				readonly = 1;
+				break;
+	                default:
+				fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
+				free(argv);
+				return 1;
+		}
+	}
+	if (argc - optind < 2) {
+		fprintf(stderr, "Invalid arguments for defragment\n");
+		free(argv);
+		return 1;
+	}
+
+	subvol = argv[optind+1];
+	dst = argv[optind+2];
+	struct btrfs_ioctl_vol_args_v2	args;
 
 	res = test_issubvolume(subvol);
 	if(res<0){
@@ -371,12 +392,19 @@ int do_clone(int argc, char **argv)
 		fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir);
 		return 12;
 	}
+	
+	if (readonly) {
+		args.flags |= BTRFS_SUBVOL_RDONLY;
+		printf("Create a readonly snapshot of '%s' in '%s/%s'\n",
+		       subvol, dstdir, newname);
+	}
+	else
+		printf("Create a snapshot of '%s' in '%s/%s'\n",
+		       subvol, dstdir, newname);
 
-	printf("Create a snapshot of '%s' in '%s/%s'\n",
-	       subvol, dstdir, newname);
 	args.fd = fd;
 	strcpy(args.name, newname);
-	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE, &args);
+	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
 
 	close(fd);
 	close(fddst);
-- 
1.7.4.1

Re: [PATCH 1/5] Add support for read-only subvolumes.

[Goffredo Baroncelli]

Hi Andreas,

On 04/25/2011 03:47 PM, Andreas Philipp wrote:
> Use BTRFS_IOC_CREATE_SNAP_V2 instead of BTRFS_IOC_CREATE_SNAP and add
> an option for the creation of a readonly snapshot.
> 
> Signed-off-by: Andreas Philipp <philipp.andreas@gmail.com>
> ---
>  btrfs_cmds.c |   44 ++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/btrfs_cmds.c b/btrfs_cmds.c
> index 8031c58..baec675 100644
> --- a/btrfs_cmds.c
> +++ b/btrfs_cmds.c
> @@ -43,7 +43,7 @@
>  
>  #ifdef __CHECKER__

It is not related to your parch.. but does anyone know why we re-define
some constants if __CHECKER__ is defined ?

>  #define BLKGETSIZE64 0
> -#define BTRFS_IOC_SNAP_CREATE 0
> +#define BTRFS_IOC_SNAP_CREATE_V2 0
>  #define BTRFS_VOL_NAME_MAX 255
>  struct btrfs_ioctl_vol_args { char name[BTRFS_VOL_NAME_MAX]; };
>  static inline int ioctl(int fd, int define, void *arg) { return 0; }
> @@ -310,13 +310,34 @@ int do_subvol_list(int argc, char **argv)
>  int do_clone(int argc, char **argv)
>  {
>  	char	*subvol, *dst;
> -	int	res, fd, fddst, len;
> +	int	res, fd, fddst, len, optind = 0, readonly = 0;
>  	char	*newname;
>  	char	*dstdir;
>  
> -	subvol = argv[1];
> -	dst = argv[2];
> -	struct btrfs_ioctl_vol_args	args;
> +	while(1) {
> +		int c = getopt(argc, argv, "r");
> +		if (c < 0)
> +			break;
> +		switch(c) {
> +			case 'r':
> +				optind++;
> +				readonly = 1;
> +				break;
> +	                default:
> +				fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
> +				free(argv);
> +				return 1;
> +		}
> +	}
> +	if (argc - optind < 2) {
> +		fprintf(stderr, "Invalid arguments for defragment\n");
                                                       ^^^^^^^^^^^^

May be that you need to replace "defragment" with "subvolume snapshot" ?

> +		free(argv);
> +		return 1;
> +	}
> +
> +	subvol = argv[optind+1];
> +	dst = argv[optind+2];
> +	struct btrfs_ioctl_vol_args_v2	args;

Does the "standard C" allow to define a variable in the middle in a
function instead of in the begin ?
Anyway, even not required, I suggest to fill "args" by zero.

+	memset(&args, 0, sizeog(args));

>  
>  	res = test_issubvolume(subvol);
>  	if(res<0){
> @@ -371,12 +392,19 @@ int do_clone(int argc, char **argv)
>  		fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir);
>  		return 12;
>  	}
> +	
> +	if (readonly) {
> +		args.flags |= BTRFS_SUBVOL_RDONLY;
> +		printf("Create a readonly snapshot of '%s' in '%s/%s'\n",
> +		       subvol, dstdir, newname);
> +	}
> +	else
> +		printf("Create a snapshot of '%s' in '%s/%s'\n",
> +		       subvol, dstdir, newname);
>  
It is not required, but I suggest to use also in the "else" the brackets
( {} ).

> -	printf("Create a snapshot of '%s' in '%s/%s'\n",
> -	       subvol, dstdir, newname);
>  	args.fd = fd;
>  	strcpy(args.name, newname);
> -	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE, &args);
> +	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
>  
>  	close(fd);
>  	close(fddst);

Regards
G.Baroncelli

Re: [PATCH 1/5] Add support for read-only subvolumes.

[Chris Mason]

Excerpts from Goffredo Baroncelli's message of 2011-04-25 17:34:46 -0400:
> Hi Andreas,
> 
> On 04/25/2011 03:47 PM, Andreas Philipp wrote:
> > Use BTRFS_IOC_CREATE_SNAP_V2 instead of BTRFS_IOC_CREATE_SNAP and add
> > an option for the creation of a readonly snapshot.
> > 
> > Signed-off-by: Andreas Philipp <philipp.andreas@gmail.com>
> > ---
> >  btrfs_cmds.c |   44 ++++++++++++++++++++++++++++++++++++--------
> >  1 files changed, 36 insertions(+), 8 deletions(-)
> > 
> > diff --git a/btrfs_cmds.c b/btrfs_cmds.c
> > index 8031c58..baec675 100644
> > --- a/btrfs_cmds.c
> > +++ b/btrfs_cmds.c
> > @@ -43,7 +43,7 @@
> >  
> >  #ifdef __CHECKER__
> 
> It is not related to your parch.. but does anyone know why we re-define
> some constants if __CHECKER__ is defined ?

This is old support for the sparse program, which finds a good number of
bugs.

> 
> >  #define BLKGETSIZE64 0
> > -#define BTRFS_IOC_SNAP_CREATE 0
> > +#define BTRFS_IOC_SNAP_CREATE_V2 0
> >  #define BTRFS_VOL_NAME_MAX 255
> >  struct btrfs_ioctl_vol_args { char name[BTRFS_VOL_NAME_MAX]; };
> >  static inline int ioctl(int fd, int define, void *arg) { return 0; }
> > @@ -310,13 +310,34 @@ int do_subvol_list(int argc, char **argv)
> >  int do_clone(int argc, char **argv)
> >  {
> >      char    *subvol, *dst;
> > -    int    res, fd, fddst, len;
> > +    int    res, fd, fddst, len, optind = 0, readonly = 0;
> >      char    *newname;
> >      char    *dstdir;
> >  
> > -    subvol = argv[1];
> > -    dst = argv[2];
> > -    struct btrfs_ioctl_vol_args    args;
> > +    while(1) {
> > +        int c = getopt(argc, argv, "r");
> > +        if (c < 0)
> > +            break;
> > +        switch(c) {
> > +            case 'r':
> > +                optind++;
> > +                readonly = 1;
> > +                break;
> > +                    default:
> > +                fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
> > +                free(argv);
> > +                return 1;
> > +        }
> > +    }
> > +    if (argc - optind < 2) {
> > +        fprintf(stderr, "Invalid arguments for defragment\n");
>                                                        ^^^^^^^^^^^^
> 
> May be that you need to replace "defragment" with "subvolume snapshot" ?
> 
> > +        free(argv);
> > +        return 1;
> > +    }
> > +
> > +    subvol = argv[optind+1];
> > +    dst = argv[optind+2];
> > +    struct btrfs_ioctl_vol_args_v2    args;
> 
> Does the "standard C" allow to define a variable in the middle in a
> function instead of in the begin ?
> Anyway, even not required, I suggest to fill "args" by zero.

I'd prefer all the variables are at the stop of a scope.

> 
> +    memset(&args, 0, sizeog(args));
> 
> >  
> >      res = test_issubvolume(subvol);
> >      if(res<0){
> > @@ -371,12 +392,19 @@ int do_clone(int argc, char **argv)
> >          fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir);
> >          return 12;
> >      }
> > +    
> > +    if (readonly) {
> > +        args.flags |= BTRFS_SUBVOL_RDONLY;
> > +        printf("Create a readonly snapshot of '%s' in '%s/%s'\n",
> > +               subvol, dstdir, newname);
> > +    }
> > +    else
> > +        printf("Create a snapshot of '%s' in '%s/%s'\n",
> > +               subvol, dstdir, newname);
> >  
> It is not required, but I suggest to use also in the "else" the brackets
> ( {} ).

Yes please.

> 
> > -    printf("Create a snapshot of '%s' in '%s/%s'\n",
> > -           subvol, dstdir, newname);
> >      args.fd = fd;
> >      strcpy(args.name, newname);
> > -    res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE, &args);
> > +    res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
> >  
> >      close(fd);
> >      close(fddst);
> 

-chris

Re: [PATCH 1/5] Add support for read-only subvolumes.

[Li Zefan]

Andreas Philipp wrote:
> Use BTRFS_IOC_CREATE_SNAP_V2 instead of BTRFS_IOC_CREATE_SNAP and add
> an option for the creation of a readonly snapshot.
> 
> Signed-off-by: Andreas Philipp <philipp.andreas@gmail.com>
> ---
>  btrfs_cmds.c |   44 ++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/btrfs_cmds.c b/btrfs_cmds.c
> index 8031c58..baec675 100644
> --- a/btrfs_cmds.c
> +++ b/btrfs_cmds.c
> @@ -43,7 +43,7 @@
>  
>  #ifdef __CHECKER__
>  #define BLKGETSIZE64 0
> -#define BTRFS_IOC_SNAP_CREATE 0
> +#define BTRFS_IOC_SNAP_CREATE_V2 0
>  #define BTRFS_VOL_NAME_MAX 255
>  struct btrfs_ioctl_vol_args { char name[BTRFS_VOL_NAME_MAX]; };
>  static inline int ioctl(int fd, int define, void *arg) { return 0; }
> @@ -310,13 +310,34 @@ int do_subvol_list(int argc, char **argv)
>  int do_clone(int argc, char **argv)
>  {
>  	char	*subvol, *dst;
> -	int	res, fd, fddst, len;
> +	int	res, fd, fddst, len, optind = 0, readonly = 0;
>  	char	*newname;
>  	char	*dstdir;
>  
> -	subvol = argv[1];
> -	dst = argv[2];
> -	struct btrfs_ioctl_vol_args	args;
> +	while(1) {
> +		int c = getopt(argc, argv, "r");

need a blank line after variable declarations.

> +		if (c < 0)
> +			break;
> +		switch(c) {

switch (c)

> +			case 'r':

switch (c) {
case:
	...
...

> +				optind++;
> +				readonly = 1;
> +				break;
> +	                default:
> +				fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
> +				free(argv);
> +				return 1;
> +		}
> +	}
> +	if (argc - optind < 2) {
> +		fprintf(stderr, "Invalid arguments for defragment\n");
> +		free(argv);
> +		return 1;
> +	}
> +
> +	subvol = argv[optind+1];
> +	dst = argv[optind+2];
> +	struct btrfs_ioctl_vol_args_v2	args;

memset(&args, 0, sizeof(args));

>  
>  	res = test_issubvolume(subvol);
>  	if(res<0){
> @@ -371,12 +392,19 @@ int do_clone(int argc, char **argv)
>  		fprintf(stderr, "ERROR: can't access to '%s'\n", dstdir);
>  		return 12;
>  	}
> +	
> +	if (readonly) {
> +		args.flags |= BTRFS_SUBVOL_RDONLY;
> +		printf("Create a readonly snapshot of '%s' in '%s/%s'\n",
> +		       subvol, dstdir, newname);
> +	}
> +	else

} else 

> +		printf("Create a snapshot of '%s' in '%s/%s'\n",
> +		       subvol, dstdir, newname);
>  
> -	printf("Create a snapshot of '%s' in '%s/%s'\n",
> -	       subvol, dstdir, newname);
>  	args.fd = fd;
>  	strcpy(args.name, newname);
> -	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE, &args);
> +	res = ioctl(fddst, BTRFS_IOC_SNAP_CREATE_V2, &args);
>  
>  	close(fd);
>  	close(fddst);

I recomend running checkpatch.pl for those patches.

Re: [PATCH 1/5] Add support for read-only subvolumes.

[Li Zefan]

> +	while(1) {
> +		int c = getopt(argc, argv, "r");
> +		if (c < 0)
> +			break;
> +		switch(c) {
> +			case 'r':
> +				optind++;
> +				readonly = 1;
> +				break;
> +	                default:
> +				fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
> +				free(argv);
> +				return 1;
> +		}
> +	}
> +	if (argc - optind < 2) {
> +		fprintf(stderr, "Invalid arguments for defragment\n");
> +		free(argv);
> +		return 1;
> +	}
> +
> +	subvol = argv[optind+1];
> +	dst = argv[optind+2];
> +	struct btrfs_ioctl_vol_args_v2	args;
>  

I don't think this can compile.. This structure is added in the 3rd patch.

Re: [PATCH 1/5] Add support for read-only subvolumes.

[Li Zefan]

>> +	subvol = argv[optind+1];
>> +	dst = argv[optind+2];
>> +	struct btrfs_ioctl_vol_args_v2	args;
> 
> Does the "standard C" allow to define a variable in the middle in a
> function instead of in the begin ?
> Anyway, even not required, I suggest to fill "args" by zero.
> 
> +	memset(&args, 0, sizeog(args));
> 

It's necessary, otherwise args.unused[4] and args.transid will have
arbitrary value.