Merge lp:~csurbhi/partman-btrfs/testing-subvol into lp:~ubuntu-core-dev/partman-btrfs/ubuntu

[Surbhi Palande]

Our aim is to be able to do automatic snapshots of home and / separately. Having a separate subvolume for / and another subvolume for /home allows us to take snapshots of these subvolumes independently.

Note that we have two basic scenarios:
1) /home is a separate partition and is mounted on dir home in /
2) /home is not a separate partition and is simply a dir under /

We need to basically achieve the following through the installer:
1) create the subvolume @ for / and @home for /home (just a naming convention discussed)
2) mount @ on / and mount @home on /home
3) prepare fstab such that every time the subvolumes are mounted.

====

Code:
1) fstab.d/btrfs checks if /home is a separate partition or not. If / is btrfs formatted then it adds an entry to mount the subvol=@home on /home even if /home is not a separate partition.
2) In case /home is a separate btrfs partition then it adds an entry to mount the subvolume=@home on /home.
3) Only when both / and /home are not btrfs formatted, then no entry corresponding to @home is added.

mount.d/btrfs gets the mountpoints by reading the fstab.d/btrfs and so it always gets to mount /home in both the cases discussed above.

======

Testing:
1) have tested the code for the following cases:
a) /home:btrfs /:btrfs
b) no separate /home, /:btrfs
c) /home:ext2 /:btrfs
d) /home:btrfs, /:ext2
e) /home:ext2 /:ext2
f) no separate /home, /:ext2

====

Please do let me know what you think of it! Thanks!

[Colin Watson (cjwatson)]

Could you propose this for merging into
lp:~ubuntu-core-dev/partman-btrfs/ubuntu instead, rather than the
upstream lp:partman-btrfs? That way I won't see the changes from
5ubuntu1 in the review diff.

On Fri, Jan 28, 2011 at 02:14:14PM -0000, Surbhi Palande wrote:
> Code:
> 1) fstab.d/btrfs checks if /home is a separate partition or not. If / is btrfs formatted then it adds an entry to mount the subvol=@home on /home even if /home is not a separate partition.
> 2) In case /home is a separate btrfs partition then it adds an entry to mount the subvolume=@home on /home.
> 3) Only when both / and /home are not btrfs formatted, then no entry corresponding to @home is added.

Your general approach looks fine to me.

> if [ "$mountpoint" = / ]; then
> + if [ $home_found = unknown ]; then

In shell, a good rule of thumb is to always put "" around any word
containing $ unless there's a specific reason not to. It doesn't matter
in practice here because the value of home_found is always a single
word, but it would be good style to use "$home_found" instead (and
similar for other $-expansions in your branch). I know this guideline
isn't followed universally in the installer, but I'd like to see at
least new code following it.

(Good reason not to put "" around a word containing $ are when you
explicitly want word splitting to be performed, or in the first argument
to 'case' which is never word-split so doesn't require quoting.)

More generally, I think there may be an ordering problem here. You're
iterating in disk/partition order, so this will go wrong if it happens
that the /home partition is on an earlier disk or partition than /.
This probably won't be the common case but we should still deal with it.

I suggest that you should make this file have two loops, both with
roughly the same form (don't worry about optimising away multiple calls
to 'open_dialog PARTITIONS' etc. - this code is run a few times, but it
isn't particularly time-critical). The first should just check whether
there's a partition with mountpoint=/home. The second should be more or
less what was there before your change, but with the extra changes to
options and home_options. That way, it will work whichever way round
the partitions happen to be.

> - mount -t btrfs ${options:+-o "$options"} $fs /target$mp || exit 1
> + options=${options%,subvol=*}
> + options=${options:+"-o $options"}

I'd find it clearer to put the quotes around the whole $-expansion here
(and similarly below), i.e.:

  options="${options:+-o $options}"

This means you don't have to wonder whether it's quoted that way because
the "" are supposed to end up in the value of options.

> + mkdir -p /target$mp

You can skip this - mount.d scripts are called from
partman-target/finish.d/mount_partitions, which always creates the
directory first.

> + mount -t btrfs $options $fs /target$mp || exit 1
> + case $mp in
> + /)
> + btrfs subvolume create /target$mp/@
> + umount /target$mp
> + options=${options:+"$options,subvol=@"}
> + options=${options:="-o subvol=@"}

Using := here is odd. "${options:=-o subvol=@}" assigns to options, and
then you assign the result of that to options again ...

The repe...

[Surbhi Palande (csurbhi)]

Hi Colin,
Thanks for the review. I have made the following changes:
1) Added "" around the $variables where ever necessary in both the files (mount.d/btrfs and fstab.d/btrfs)
2) removed the unnecessary mkdir -p /target$mp from mount.d/btrfs
3) removed the options=${options:="something"} statements.

I have not changed the logic as it seems that the partitioning order shall be taken care of by the existing code.
===

Tested the code for /home:btrfs and /:btrfs.

Please do let me know if the patch looks better now. Thanks!

[lp:~csurbhi/partman-btrfs/testing-subvol]

55. By Surbhi Palande on 2011-01-31

Adding support for creating and respectively mounting btrfs subvolumes
corresponding to / and /home, in case of a btrfs rootfs.

[Surbhi Palande (csurbhi)]

Following change added:
Need to remove the subvol=@ before creating the subvolume from the mount options. This removal could be when options="defaults,subvol=@" or options="subvol=@". Added a clause to remove the "subvol=@" when no comma is present. (Comments added in the changed code)

[Colin Watson (cjwatson)]

Looks fine now, so I'm merging this. Thanks!

 review approve

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2011-01-07 15:59:48 +0000
+++ debian/changelog	2011-01-31 15:02:11 +0000
@@ -1,3 +1,10 @@
+partman-btrfs (5ubuntu2) natty; urgency=low
+
+  * Adding support for creating and respectively mounting btrfs subvolumes
+    corresponding to / and /home, in case of a btrfs rootfs.
+
+ -- Surbhi Palande <[e-mail]>  Fri, 28 Jan 2011 15:35:18 +0200
+
 partman-btrfs (5ubuntu1) natty; urgency=low
 
   * Allow btrfs /boot on amd64/i386, now that grub2 supports it.

=== modified file 'fstab.d/btrfs'
--- fstab.d/btrfs	2010-06-18 10:21:24 +0000
+++ fstab.d/btrfs	2011-01-31 15:02:11 +0000
@@ -2,6 +2,8 @@
 
 . /lib/partman/lib/base.sh
 
+home_found="unknown"
+
 for dev in $DEVICES/*; do
 	[ -d $dev ] || continue
 	cd $dev
@@ -12,23 +14,44 @@
 		[ -f "$id/acting_filesystem" ] || continue
 		method=$(cat $id/method)
 		filesystem=$(cat $id/acting_filesystem)
+		mountpoint=$(cat $id/mountpoint)
 		case "$filesystem" in
 		    btrfs)
 			[ -f "$id/mountpoint" ] || continue
-			mountpoint=$(cat $id/mountpoint)
 			# due to #249322, #255135, #258117:
 			if [ "$mountpoint" = /tmp ]; then
 				rm -f $id/options/noexec
 			fi
 			options=$(get_mountoptions $dev $id)
 			if [ "$mountpoint" = / ]; then
+				if [ "$home_found" = "unknown" ]; then
+					home_found="false"
+				fi
 				pass=1
+				home_options="${options:+$options,}subvol=@home"
+				options="${options:+$options,}subvol=@"
+				home_path="$path"
+				home_mp="$mountpoint"home
+			elif [ "$mountpoint" = /home ]; then
+				pass=2
+				options="${options:+$options,}subvol=@home"
+				home_found=true
 			else
 				pass=2
 			fi
 			echo "$path" "$mountpoint" btrfs $options 0 $pass
 			;;
+		    *)
+			if [ "$mountpoint" = "/home" ]; then
+				home_found="true"
+			fi
+			;;
 		esac
 	done
 	close_dialog
 done
+
+if [ "$home_found" = "false" ]; then
+	echo "$home_path" "$home_mp" btrfs "$home_options" 0 2
+	home_found="true"
+fi	

=== modified file 'mount.d/btrfs'
--- mount.d/btrfs	2010-01-27 20:14:29 +0000
+++ mount.d/btrfs	2011-01-31 15:02:11 +0000
@@ -11,7 +11,25 @@
 
 case $type in
     btrfs)
+	options="${options%,subvol=*}"
+	#for removing the option subvol,when thats the only option
+	#eg: options=="subvol=@", no comma present
+	options="${options%subvol=*}"
 	mount -t btrfs ${options:+-o "$options"} $fs /target$mp || exit 1
+	case $mp in
+	    /)
+		btrfs subvolume create /target$mp/@
+		umount /target$mp
+		options="${options:+$options,}subvol=@"
+		mount -t btrfs -o $options $fs /target$mp
+		;;
+	    /home)
+		btrfs subvolume create /target$mp/@home
+		umount /target$mp
+		options="${options:+$options,}subvol=@home"
+		mount -t btrfs -o $options $fs /target$mp
+		;;
+	esac
 	echo "umount /target$mp"
 	exit 0
 	;;