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

[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...

[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.

Unmerged revisions

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.

54. By Colin Watson on 2011-01-07

releasing version 5ubuntu1

53. By Colin Watson on 2011-01-07

set Maintainer and Vcs-Bzr for Ubuntu

52. By Colin Watson on 2011-01-07

Allow btrfs /boot on amd64/i386, now that grub2 supports it.

=== modified file 'check.d/no_btrfs_boot'
--- check.d/no_btrfs_boot	2010-01-27 20:14:29 +0000
+++ check.d/no_btrfs_boot	2011-01-28 14:23:38 +0000
@@ -2,6 +2,14 @@
 # grub and other bootloaders that read the filesystem do not support /boot
 # on btrfs. (lilo should work). Detect and warn.
 
+# grub2 works now.
+ARCH="$(archdetect)"
+case $ARCH in
+    amd64/*|*-amd64/*|i386/*|*-i386/*)
+	exit 0
+	;;
+esac
+
 . /lib/partman/lib/base.sh
 
 get_btrfs_root_boot () {
 
=== modified file 'debian/changelog'
--- debian/changelog	2010-12-24 22:24:24 +0000
+++ debian/changelog	2011-01-28 14:23:38 +0000
@@ -1,3 +1,16 @@
+partman-btrfs (5ubuntu2) maverick; 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.
+
+ -- Colin Watson <[e-mail]>  Fri, 07 Jan 2011 15:55:57 +0000
+
 partman-btrfs (5) unstable; urgency=low
 
   [ Updated translations ]
 
=== modified file 'debian/control'
--- debian/control	2011-01-17 21:24:40 +0000
+++ debian/control	2011-01-28 14:23:38 +0000
@@ -1,11 +1,17 @@
 Source: partman-btrfs
 Section: debian-installer
 Priority: standard
-Maintainer: Debian Install System Team <[e-mail]>
+Maintainer: Ubuntu Installer Team <[e-mail]>
+XSBC-Original-Maintainer: Debian Install System Team <[e-mail]>
 Uploaders: Anton Zinoviev <[e-mail]>, Joey Hess <[e-mail]>
 Build-Depends: debhelper (>= 7.0.8), dh-di, po-debconf (>= 0.5.0)
+<<<<<<< TREE
 Vcs-Browser: http://git.debian.org/?p=d-i/partman-btrfs.git
 Vcs-Git: git://git.debian.org/d-i/partman-btrfs.git
+=======
+XS-Debian-Vcs-Svn: svn://svn.debian.org/d-i/trunk/packages/partman/partman-btrfs
+Vcs-Bzr: http://bazaar.launchpad.net/~ubuntu-core-dev/partman-btrfs/ubuntu
+>>>>>>> MERGE-SOURCE
 
 Package: partman-btrfs
 XC-Package-Type: udeb
 
=== modified file 'fstab.d/btrfs'
--- fstab.d/btrfs	2010-06-18 10:21:24 +0000
+++ fstab.d/btrfs	2011-01-28 14:23:38 +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,45 @@
 		[ -f "$id/acting_filesystem" ] || continue
 		method=$(cat $id/method)
 		filesystem=$(cat $id/acting_filesystem)
+		[ -f "$id/mountpoint" ] || continue
+		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
+				options="${options:+$options,subvol=@}"
+				home_path=$path
+				home_mp="$mountpoint"home
+				home_options=${options%,subvol=*}
+				home_options="${home_options:+$home_options,subvol=@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-28 14:23:38 +0000
@@ -11,7 +11,26 @@
 
 case $type in
     btrfs)
-	mount -t btrfs ${options:+-o "$options"} $fs /target$mp || exit 1
+	options=${options%,subvol=*}
+	options=${options:+"-o $options"}
+	mkdir -p /target$mp
+	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=@"}
+		mount -t btrfs $options $fs /target$mp
+		;;
+	    /home)
+		btrfs subvolume create /target$mp/@home
+		umount /target$mp
+		options=${options:+"$options,subvol=@home"}
+		options=${options:="-o subvol=@home"}
+		mount -t btrfs $options $fs /target$mp
+		;;
+	esac
 	echo "umount /target$mp"
 	exit 0
 	;;