Merge lp:~csurbhi/ubuntu/natty/os-prober/os-prober.fix764893 into lp:ubuntu/natty/os-prober

[Surbhi Palande]

Hi,

This patch adds support to mount the "@" subvolume to access a btrfs formatted rootfs. It checks if "@" is present and if so, then unmounts the partition before trying to mount the subvolume. Releases previous to Natty and other OS distros may not have the "/" on the "@" subvolume.

Tested lightly on kvm with Natty-btrfs rootfs.

Please do consider this as a *SRU* for Natty.

Thanks,
Surbhi.

[Colin Watson (cjwatson)]

> mounted=1
> + debug "mounted as $type filesystem"

The indentation is in general wrong here - make sure to check whether
the code you're patching is using tabs or spaces for indentation, and
match it.

> + if [ $type="btrfs" ] && [ -x "$tmpmnt/@" ]; then

The first test is wrong shell syntax. This must be [ "$type" = btrfs ]
instead.

Furthermore, this won't work when we're using grub-mount, which we
generally will be nowadays; and remounting with 'mount -o ro,subvol=@ -t
btrfs' will bring us back to the bad old days before grub-mount, when we
had to choose between either corrupting hibernated filesystems or
failing to mount journalled filesystems that weren't cleanly unmounted.

I suggest instead that we check whether $type is either btrfs or fuse
(using 'case') and whether $tmpmnt/@/lib exists (this should be
reasonably safe since btrfs is only used on Linux); in that case,
bind-mount $tmpmnt/@ on $tmpmnt and remember to umount it twice at the
end. (Alternatively, we could arrange for everything to read from
$tmpmnt/@ without a bind-mount, but I think that would be rather more
code churn.)

> + warn "failed to mount" \
> + "@ ($partition)"

Try to make the warnings a bit more descriptive; most people will have
no idea what our subvolume scheme involves. They should still be
one-line warnings or thereabouts, not essays, but for example
consistently referring to "btrfs subvolume @ on $partition" would be an
improvement.

[Surbhi Palande (csurbhi)]

Hi Colin,

Thanks a lot for the insightful review. Did not know about the fs-corruption issue before. The reading of the $tmpmnt/@ indeed is a lot more code than mount-bind (or remount) and I decided against it just for the same reason before.

I have uploaded the changes after testing them on kvm. Please do let me know if they look good now.

Regards,
Surbhi.

[lp:~csurbhi/ubuntu/natty/os-prober/os-prober.fix764893]

18. By Surbhi Palande on 2011-04-21

mount btrfs subvolume @ when present to access a btrfs formatted rootfs.
(LP: #764893)

[Colin Watson (cjwatson)]

This looks fine now - thanks, and sorry for the delay! I'm merging this (although rebased against lp:os-prober), and adjusting for some minor style nits.

review: Approve

Unmerged revisions

18. By Surbhi Palande on 2011-04-21

mount btrfs subvolume @ when present to access a btrfs formatted rootfs.
(LP: #764893)

=== modified file 'debian/changelog'
--- debian/changelog	2011-04-10 00:36:21 +0000
+++ debian/changelog	2011-04-21 15:53:23 +0000
@@ -1,3 +1,10 @@
+os-prober (1.44ubuntu2) natty; urgency=low
+
+  * mount btrfs subvolume @ when present to access a btrfs formatted rootfs.
+    (LP: #764893)
+
+ -- Surbhi Palande <[e-mail]>  Thu, 21 Apr 2011 17:44:10 +0300
+
 os-prober (1.44ubuntu1) natty; urgency=low
 
   * Cherry-pick from trunk:
 
=== modified file 'linux-boot-probes/common/50mounted-tests'
--- linux-boot-probes/common/50mounted-tests	2011-04-10 00:36:21 +0000
+++ linux-boot-probes/common/50mounted-tests	2011-04-21 15:53:23 +0000
@@ -43,6 +43,26 @@
 	for type in $types; do
 		if mount -o ro -t "$type" "$partition" "$tmpmnt" 2>/dev/null; then
 			mounted=1
+			case "$type" in
+			  btrfs|fuse)
+				if [ -x "$tmpmnt/@/lib" ]; then
+					if ! mount --bind "$tmpmnt/@" \
+						"$tmpmnt"; 
+					then
+						warn "failed to bind mount" \
+						"the subvolume:@ on $partition"
+						if ! umount $tmpmnt
+						then
+							warn "failed to" \
+							"unmount $tmpmnt"
+						fi
+						mounted=
+					fi
+				fi
+				;;
+			  *)
+				;;
+			esac	
 			break
 		fi
 	done
@@ -62,6 +82,23 @@
 				if ! umount "$tmpmnt"; then
 					warn "failed to umount $tmpmnt"
 				fi
+				case $type in
+				  btrfs|fuse)
+					if [ -x "$tmpmnt/@/lib" ]
+					then
+						# umount to account
+						# for the mount-bind
+						if ! umount $tmpmnt
+						then
+							warn "failed to "\
+							"umount $tmpmnt"
+						fi
+					fi
+					;;
+				  *)
+					;;
+				esac
+						
 				rmdir "$tmpmnt" || true
 				exit 0
 			fi
 
=== modified file 'os-probes/common/50mounted-tests'
--- os-probes/common/50mounted-tests	2011-04-10 00:36:21 +0000
+++ os-probes/common/50mounted-tests	2011-04-21 15:53:23 +0000
@@ -57,6 +57,26 @@
 		if mount -o ro -t "$type" "$partition" "$tmpmnt" 2>/dev/null; then
 			debug "mounted as $type filesystem"
 			mounted=1
+			case "$type" in
+			  btrfs|fuse)
+				if [ -x "$tmpmnt/@/lib" ]; then
+					if ! mount --bind "$tmpmnt/@" \
+						"$tmpmnt" 
+					then
+						warn "failed to bind mount" \
+						"the subvolume:@ on $partition"
+						if ! umount $tmpmnt
+						then
+							warn "failed to" \
+							"unmount $tmpmnt"
+						fi
+						mounted=
+					fi
+				fi
+				;;
+			  *)
+				;;
+			esac	
 			break
 		fi
 	done
@@ -71,6 +91,22 @@
 				if ! umount "$tmpmnt"; then
 					warn "failed to umount $tmpmnt"
 				fi
+				case "$type" in
+				  btrfs|fuse)
+					if [ -x "$tmpmnt/@/lib" ]
+					then
+					# umount to account
+					# for the mount-bind
+						if ! umount $tmpmnt
+						then
+							warn "failed to "\
+							"umount $tmpmnt"
+						fi
+					fi
+					;;
+				  *)
+					;;
+				esac
 				rmdir "$tmpmnt" || true
 				exit 0
 			fi