[devtools] makechrootpkg: Be recursive when deleting btrfs subvolumes.

Message ID 20170210084608.24587-2-lukeshu@lukeshu.com
State Superseded
Headers show

Commit Message

Luke Shumaker Feb. 10, 2017, 8:46 a.m. UTC
From: Luke Shumaker <lukeshu@sbcglobal.net>

Motivation:

  When installing the necessaryssary dependencies in the chroot, the
  ALPM hooks run; and if 'systemd' is a dependency, then one of the
  hooks is to run systemd-tmpfiles.  There are several tmpfiles.d(5)
  commands that instruct it to create btrfs subvolumes if on btrfs
  (the `v`, `q`, and `Q` commands).

  This causes a problem when we go to delete the chroot.  The command
  `btrfs subvolume delete` won't recursively delete subvolumes; if a
  child subvolume was created, it will fail with the fairly unhelpful
  error message "directory not empty".

Solution:

  Because the subvolume that gets mounted isn't necessarily the
  toplevel subvolume, and `btrfs subvolume list` gives us paths
  relative to the toplevel; we need to figure out how our path relates
  to the toplevel.  Figure out the mountpoint (which turns out to be
  slightly tricky; see below), and call `btrfs subvolume list -a` on
  it to get the list of subvolumes that are visible to us (and quite
  possibly some that aren't; the logic for determining which ones it
  shows is... absurd).  This gives us a list of subvolumes with
  numeric IDs, and paths relative to the toplevel (actually it gives
  us more than that, and we use a hopefully-correct `sed` expression
  to trim it down; the format certainly isn't human-friendly, but it's
  not machine-friendly either.)  So then we look at that list of pairs
  and find the one that matches the ID of the subvolume we're trying
  to delete (which is easy to get with `btrfs subvolume show`); once
  we've found the path of our subvolume, we can use that to filter and
  trim the complete list of paths.  From there the remainder of the
  solution is obvious.

  Now, back to "figure out the mountpoint"; the normal `stat -c %m`
  doesn't work.  It gives the mounted path of the subvolume closest to
  the path we give it, not the actual mountpoint.  Now, it turns out
  that `df` can figure out the correct mountpoint (though I haven't
  investigated how it knows when stat doesn't; but I suspect it parses
  `/proc/mounts`).  So we are reduced to parsing `df`'s output.
---
 makechrootpkg.in | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 2 deletions(-)

Comments

Emil Velikov via arch-projects Feb. 10, 2017, 9:16 a.m. UTC | #1
On Fri, Feb 10, 2017 at 9:46 AM <lukeshu@lukeshu.com> wrote:

> From: Luke Shumaker <lukeshu@sbcglobal.net>
>
> Motivation:
>
>   When installing the necessaryssary dependencies in the chroot, the
>   ALPM hooks run; and if 'systemd' is a dependency, then one of the
>   hooks is to run systemd-tmpfiles.  There are several tmpfiles.d(5)
>   commands that instruct it to create btrfs subvolumes if on btrfs
>   (the `v`, `q`, and `Q` commands).
>
>   This causes a problem when we go to delete the chroot.  The command
>   `btrfs subvolume delete` won't recursively delete subvolumes; if a
>   child subvolume was created, it will fail with the fairly unhelpful
>   error message "directory not empty".
>
> Solution:
>
>   Because the subvolume that gets mounted isn't necessarily the
>   toplevel subvolume, and `btrfs subvolume list` gives us paths
>   relative to the toplevel; we need to figure out how our path relates
>   to the toplevel.  Figure out the mountpoint (which turns out to be
>   slightly tricky; see below), and call `btrfs subvolume list -a` on
>   it to get the list of subvolumes that are visible to us (and quite
>   possibly some that aren't; the logic for determining which ones it
>   shows is... absurd).  This gives us a list of subvolumes with
>   numeric IDs, and paths relative to the toplevel (actually it gives
>   us more than that, and we use a hopefully-correct `sed` expression
>   to trim it down; the format certainly isn't human-friendly, but it's
>   not machine-friendly either.)  So then we look at that list of pairs
>   and find the one that matches the ID of the subvolume we're trying
>   to delete (which is easy to get with `btrfs subvolume show`); once
>   we've found the path of our subvolume, we can use that to filter and
>   trim the complete list of paths.  From there the remainder of the
>   solution is obvious.
>
>   Now, back to "figure out the mountpoint"; the normal `stat -c %m`
>   doesn't work.  It gives the mounted path of the subvolume closest to
>   the path we give it, not the actual mountpoint.  Now, it turns out
>   that `df` can figure out the correct mountpoint (though I haven't
>   investigated how it knows when stat doesn't; but I suspect it parses
>   `/proc/mounts`).  So we are reduced to parsing `df`'s output.
>

The chroot management is getting so complex I wonder if we're not better
off killing the btrfs support (it doesn't buy us much over rsync since the
base (root) chroot tends to be small) or using someone's else container
framework, maybe rkt.
<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Feb 10, 2017 at 9:46 AM &lt;<a href="mailto:lukeshu@lukeshu.com">lukeshu@lukeshu.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Luke Shumaker &lt;<a href="mailto:lukeshu@sbcglobal.net" class="gmail_msg" target="_blank">lukeshu@sbcglobal.net</a>&gt;<br class="gmail_msg">
<br class="gmail_msg">
Motivation:<br class="gmail_msg">
<br class="gmail_msg">
  When installing the necessaryssary dependencies in the chroot, the<br class="gmail_msg">
  ALPM hooks run; and if &#39;systemd&#39; is a dependency, then one of the<br class="gmail_msg">
  hooks is to run systemd-tmpfiles.  There are several tmpfiles.d(5)<br class="gmail_msg">
  commands that instruct it to create btrfs subvolumes if on btrfs<br class="gmail_msg">
  (the `v`, `q`, and `Q` commands).<br class="gmail_msg">
<br class="gmail_msg">
  This causes a problem when we go to delete the chroot.  The command<br class="gmail_msg">
  `btrfs subvolume delete` won&#39;t recursively delete subvolumes; if a<br class="gmail_msg">
  child subvolume was created, it will fail with the fairly unhelpful<br class="gmail_msg">
  error message &quot;directory not empty&quot;.<br class="gmail_msg">
<br class="gmail_msg">
Solution:<br class="gmail_msg">
<br class="gmail_msg">
  Because the subvolume that gets mounted isn&#39;t necessarily the<br class="gmail_msg">
  toplevel subvolume, and `btrfs subvolume list` gives us paths<br class="gmail_msg">
  relative to the toplevel; we need to figure out how our path relates<br class="gmail_msg">
  to the toplevel.  Figure out the mountpoint (which turns out to be<br class="gmail_msg">
  slightly tricky; see below), and call `btrfs subvolume list -a` on<br class="gmail_msg">
  it to get the list of subvolumes that are visible to us (and quite<br class="gmail_msg">
  possibly some that aren&#39;t; the logic for determining which ones it<br class="gmail_msg">
  shows is... absurd).  This gives us a list of subvolumes with<br class="gmail_msg">
  numeric IDs, and paths relative to the toplevel (actually it gives<br class="gmail_msg">
  us more than that, and we use a hopefully-correct `sed` expression<br class="gmail_msg">
  to trim it down; the format certainly isn&#39;t human-friendly, but it&#39;s<br class="gmail_msg">
  not machine-friendly either.)  So then we look at that list of pairs<br class="gmail_msg">
  and find the one that matches the ID of the subvolume we&#39;re trying<br class="gmail_msg">
  to delete (which is easy to get with `btrfs subvolume show`); once<br class="gmail_msg">
  we&#39;ve found the path of our subvolume, we can use that to filter and<br class="gmail_msg">
  trim the complete list of paths.  From there the remainder of the<br class="gmail_msg">
  solution is obvious.<br class="gmail_msg">
<br class="gmail_msg">
  Now, back to &quot;figure out the mountpoint&quot;; the normal `stat -c %m`<br class="gmail_msg">
  doesn&#39;t work.  It gives the mounted path of the subvolume closest to<br class="gmail_msg">
  the path we give it, not the actual mountpoint.  Now, it turns out<br class="gmail_msg">
  that `df` can figure out the correct mountpoint (though I haven&#39;t<br class="gmail_msg">
  investigated how it knows when stat doesn&#39;t; but I suspect it parses<br class="gmail_msg">
  `/proc/mounts`).  So we are reduced to parsing `df`&#39;s output.<br class="gmail_msg"></blockquote><div><br>The chroot management is getting so complex I wonder if we&#39;re not better off killing the btrfs support (it doesn&#39;t buy us much over rsync since the base (root) chroot tends to be small) or using someone&#39;s else container framework, maybe rkt.<br></div></div></div>
Dave Reisner Feb. 10, 2017, 2:57 p.m. UTC | #2
On Fri, Feb 10, 2017 at 03:46:08AM -0500, lukeshu@lukeshu.com wrote:
> From: Luke Shumaker <lukeshu@sbcglobal.net>
> 
> Motivation:
> 
>   When installing the necessaryssary dependencies in the chroot, the
>   ALPM hooks run; and if 'systemd' is a dependency, then one of the
>   hooks is to run systemd-tmpfiles.  There are several tmpfiles.d(5)
>   commands that instruct it to create btrfs subvolumes if on btrfs
>   (the `v`, `q`, and `Q` commands).
> 
>   This causes a problem when we go to delete the chroot.  The command
>   `btrfs subvolume delete` won't recursively delete subvolumes; if a
>   child subvolume was created, it will fail with the fairly unhelpful
>   error message "directory not empty".
> 
> Solution:
> 
>   Because the subvolume that gets mounted isn't necessarily the
>   toplevel subvolume, and `btrfs subvolume list` gives us paths
>   relative to the toplevel; we need to figure out how our path relates
>   to the toplevel.  Figure out the mountpoint (which turns out to be
>   slightly tricky; see below), and call `btrfs subvolume list -a` on
>   it to get the list of subvolumes that are visible to us (and quite
>   possibly some that aren't; the logic for determining which ones it
>   shows is... absurd).  This gives us a list of subvolumes with
>   numeric IDs, and paths relative to the toplevel (actually it gives
>   us more than that, and we use a hopefully-correct `sed` expression
>   to trim it down; the format certainly isn't human-friendly, but it's
>   not machine-friendly either.)  So then we look at that list of pairs
>   and find the one that matches the ID of the subvolume we're trying
>   to delete (which is easy to get with `btrfs subvolume show`); once
>   we've found the path of our subvolume, we can use that to filter and
>   trim the complete list of paths.  From there the remainder of the
>   solution is obvious.
> 
>   Now, back to "figure out the mountpoint"; the normal `stat -c %m`
>   doesn't work.  It gives the mounted path of the subvolume closest to
>   the path we give it, not the actual mountpoint.  Now, it turns out
>   that `df` can figure out the correct mountpoint (though I haven't
>   investigated how it knows when stat doesn't; but I suspect it parses
>   `/proc/mounts`).  So we are reduced to parsing `df`'s output.
> ---

If you need to write a commit message this long just to remove what's
currently a single nested subvolume, then I agree with Jan that we need
to rethink our chroot management, or btrfs needs to improve its tooling
(I tend to think the latter is true independent of this patch).

That said, this patch does seem to work as of this writing. I have
concerns that it's a little fragile and might break in the future.

>  makechrootpkg.in | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/makechrootpkg.in b/makechrootpkg.in
> index 5c4b530..01e9e96 100644
> --- a/makechrootpkg.in
> +++ b/makechrootpkg.in
> @@ -80,6 +80,61 @@ load_vars() {
>  	return 0
>  }
>  
> +# Usage: btrfs_subvolume_id $SUBVOLUME
> +btrfs_subvolume_id() (
> +	set -o pipefail
> +	LC_ALL=C btrfs subvolume show "$1" | sed -n 's/^\tSubvolume ID:\s*//p'

This looks like you're parsing human readable output to get the subvol
ID. Is this really the only way to get this information?

> +)
> +
> +# Usage: btrfs_subvolume_list_all $FILEPATH
> +#
> +# Given $FILEPATH somewhere on a mounted btrfs filesystem, print the
> +# ID and full path of every subvolume on the filesystem, one per line
> +# in the format "$ID $PATH".
> +btrfs_subvolume_list_all() (
> +	set -o pipefail
> +	local mountpoint
> +	mountpoint="$(df --output=target "$1" | sed 1d)" || return $?

"return $?" is a long winded way of just "return". The status is
implied.

> +	LC_ALL=C btrfs subvolume list -a "$mountpoint" | sed -r 's|^ID ([0-9]+) .* path (<FS_TREE>/)?(\S*).*|\1 \3|'

You might want to validate that you actually get an integer back here,
rather than just assuming the calls succeed. You cannot rely on sed to
provide a useful exit status.

> +)
> +
> +# Usage: btrfs_subvolume_list $SUBVOLUME
> +#
> +# Assuming that $SUBVOLUME is a btrfs subvolume, list all child
> +# subvolumes; from most deeply nested to most shallowly nested.
> +#
> +# This is intended to be a sane version of `btrfs subvolume list`.
> +btrfs_subvolume_list() {
> +	local subvolume=$1
> +
> +	local id all path subpath
> +	id="$(btrfs_subvolume_id "$subvolume")" || return $?
> +	all="$(btrfs_subvolume_list_all "$subvolume")" || return $?
> +	path="$(sed -n "s/^$id //p" <<<"$all")"

Rather than injecting unvalidated data into a sed program, I'd suggest
using awk:

path=$(awk -v id="$1" '$1 == id { sub($1 FS, ""); print }' <<<"$all")

> +	while read -r id subpath; do
> +		if [[ "$subpath" = "$path"/* ]]; then
> +			printf '%s\n' "${subpath#"${path}/"}"
> +		fi
> +	done <<<"$all" | LC_ALL=C sort --reverse
> +}
> +
> +# Usage: btrfs_subvolume_delete $SUBVOLUME
> +#
> +# Assuming that $SUBVOLUME is a btrfs subvolume, delete it and all
> +# subvolumes below it.
> +#
> +# This is intended to be a recursive version of
> +# `btrfs subvolume delete`.
> +btrfs_subvolume_delete() {
> +	local dir="$1"
> +	local subvolumes subvolume
> +	subvolumes=($(btrfs_subvolume_list "$dir")) || return $?

This is a broken way to read lines into an array, because you're not
reading lines at all -- you're reading words.

> +	for subvolume in "${subvolumes[@]}"; do
> +		btrfs subvolume delete "$dir/$subvolume" || return $?
> +	done
> +	btrfs subvolume delete "$dir"
> +}
> +
>  create_chroot() {
>  	# Lock the chroot we want to use. We'll keep this lock until we exit.
>  	lock 9 "$copydir.lock" "Locking chroot copy [%s]" "$copy"
> @@ -92,7 +147,7 @@ create_chroot() {
>  		stat_busy "Creating clean working copy [%s]" "$copy"
>  		if [[ "$chroottype" == btrfs ]] && ! mountpoint -q "$copydir"; then
>  			if [[ -d $copydir ]]; then
> -				btrfs subvolume delete "$copydir" >/dev/null ||
> +				btrfs_subvolume_delete "$copydir" >/dev/null ||
>  					die "Unable to delete subvolume %s" "$copydir"
>  			fi
>  			btrfs subvolume snapshot "$chrootdir/root" "$copydir" >/dev/null ||
> @@ -114,7 +169,7 @@ create_chroot() {
>  clean_temporary() {
>  	stat_busy "Removing temporary copy [%s]" "$copy"
>  	if [[ "$chroottype" == btrfs ]] && ! mountpoint -q "$copydir"; then
> -		btrfs subvolume delete "$copydir" >/dev/null ||
> +		btrfs_subvolume_delete "$copydir" >/dev/null ||
>  			die "Unable to delete subvolume %s" "$copydir"
>  	else
>  		# avoid change of filesystem in case of an umount failure
> -- 
> 2.11.1
Luke Shumaker Feb. 10, 2017, 8:30 p.m. UTC | #3
On Fri, 10 Feb 2017 09:57:32 -0500,
Dave Reisner wrote:
> > +	LC_ALL=C btrfs subvolume show "$1" | sed -n 's/^\tSubvolume ID:\s*//p'
> 
> This looks like you're parsing human readable output to get the subvol
> ID. Is this really the only way to get this information?

As far as I can tell, yes.

> > +)
> > +
> > +# Usage: btrfs_subvolume_list_all $FILEPATH
> > +#
> > +# Given $FILEPATH somewhere on a mounted btrfs filesystem, print the
> > +# ID and full path of every subvolume on the filesystem, one per line
> > +# in the format "$ID $PATH".
> > +btrfs_subvolume_list_all() (
> > +	set -o pipefail
> > +	local mountpoint
> > +	mountpoint="$(df --output=target "$1" | sed 1d)" || return $?
> 
> "return $?" is a long winded way of just "return". The status is
> implied.

Huh.  TIL.  Thanks.

> > +	LC_ALL=C btrfs subvolume list -a "$mountpoint" | sed -r 's|^ID ([0-9]+) .* path (<FS_TREE>/)?(\S*).*|\1 \3|'
> 
> You might want to validate that you actually get an integer back here,
> rather than just assuming the calls succeed. You cannot rely on sed to
> provide a useful exit status.

I suppose that would be a good idea.  The `set -o pipefail` a couple
of lines up will catch the cases where the failure is from `btrfs`;
the check would be for if btrfs's output format changes.

I really should have called it out in a comment and the commit message
that that regex is the place where I most expect btrfs to break us in
the future.  The output format is a space-separated sequence of "key
value key value..."; the problem is that both keys and values can
contain space, and there's no escaping or indication of when this
happens.  The reason I tacked `^` in the expression is that there's
already a "parent ID" key.  The continued success of the regex assumes
that there will never be another key or value that contains " path",
and that there is no space in the path value.

So yeah, I probably should add a check.

> > +	id="$(btrfs_subvolume_id "$subvolume")" || return $?
> > +	all="$(btrfs_subvolume_list_all "$subvolume")" || return $?
> > +	path="$(sed -n "s/^$id //p" <<<"$all")"
> 
> Rather than injecting unvalidated data into a sed program, I'd suggest
> using awk:
> 
> path=$(awk -v id="$1" '$1 == id { sub($1 FS, ""); print }' <<<"$all")

Sure.  Sed was simpler, and I figured that if `btrfs` starts
misbehaving, the whole thing is hosed anyway.

> > +	subvolumes=($(btrfs_subvolume_list "$dir")) || return $?
> 
> This is a broken way to read lines into an array, because you're not
> reading lines at all -- you're reading words.
> 
> > +	for subvolume in "${subvolumes[@]}"; do
> > +		btrfs subvolume delete "$dir/$subvolume" || return $?
> > +	done

Yeah, but the lines are guaranteed to be words because of the `(\S*)`
in the regexp in `btrfs_subvolume_list_all`.

But I should have written it as

	subvolumes="$(btrfs_subvolume_list "$dir")" || return
	for read -r subvolume; do
		btrfs subvolume delete "$dir/$subvolume" || return
	done <<<"$subvolumes"
Bartłomiej Piotrowski Feb. 16, 2017, 7:54 a.m. UTC | #4
On 2017-02-10 10:16, Jan Alexander Steffens via arch-projects wrote:
> The chroot management is getting so complex I wonder if we're not better
> off killing the btrfs support (it doesn't buy us much over rsync since
> the base (root) chroot tends to be small) or using someone's else
> container framework, maybe rkt.

I don't think rkt would simplify a lot, it's quite complicated on its own.

Call me a simp, but can't we just maintain a list of subvolumes to
delete? For now it's just one item.

Bartłomiej
Emil Velikov via arch-projects Feb. 16, 2017, 11:10 a.m. UTC | #5
Le 16/02/2017 à 08:54, Bartłomiej Piotrowski a écrit :

> On 2017-02-10 10:16, Jan Alexander Steffens via arch-projects wrote:
>> The chroot management is getting so complex I wonder if we're not better
>> off killing the btrfs support (it doesn't buy us much over rsync since
>> the base (root) chroot tends to be small) or using someone's else
>> container framework, maybe rkt.
> I don't think rkt would simplify a lot, it's quite complicated on its own.
>
> Call me a simp, but can't we just maintain a list of subvolumes to
> delete? For now it's just one item.
>
> Bartłomiej

I think this is the most reasonable thing to do right now, and keep it
that way as long as the item list stays sufficiently short, and if it
ever start to grow, reconsider things.

That won’t be more ugly than aliasing extra-x86_64-build to `sudo btrfs
subvolume delete
/var/lib/archbuild/extra-x86_64/archange/var/lib/machines &&
extra-x86_64-build`…

Bruno
Luke Shumaker Feb. 17, 2017, 8:18 p.m. UTC | #6
On Thu, 16 Feb 2017 06:10:08 -0500,
Bruno Pagani via arch-projects wrote:
> > Call me a simp, but can't we just maintain a list of subvolumes to
> > delete? For now it's just one item.
> 
> I think this is the most reasonable thing to do right now, and keep it
> that way as long as the item list stays sufficiently short, and if it
> ever start to grow, reconsider things.

A simple solution that had not occurred to me:

What about just having it attempt to `btrfs subvolume delete` all
sudirectries of the chroot; and expect most of them to fail; only
actually caring about the deletion of the root of the chroot?

Something like

	btrfs_subvolume_delete() {
		local dir="$1"
		find "$dir" -mindepth 1 -type d -print0|sort -z --reverse|xargs -r0 btrfs subvolume delete &>/dev/null
		btrfs subvolume delete "$dir"
	}

Patch

diff --git a/makechrootpkg.in b/makechrootpkg.in
index 5c4b530..01e9e96 100644
--- a/makechrootpkg.in
+++ b/makechrootpkg.in
@@ -80,6 +80,61 @@  load_vars() {
 	return 0
 }
 
+# Usage: btrfs_subvolume_id $SUBVOLUME
+btrfs_subvolume_id() (
+	set -o pipefail
+	LC_ALL=C btrfs subvolume show "$1" | sed -n 's/^\tSubvolume ID:\s*//p'
+)
+
+# Usage: btrfs_subvolume_list_all $FILEPATH
+#
+# Given $FILEPATH somewhere on a mounted btrfs filesystem, print the
+# ID and full path of every subvolume on the filesystem, one per line
+# in the format "$ID $PATH".
+btrfs_subvolume_list_all() (
+	set -o pipefail
+	local mountpoint
+	mountpoint="$(df --output=target "$1" | sed 1d)" || return $?
+	LC_ALL=C btrfs subvolume list -a "$mountpoint" | sed -r 's|^ID ([0-9]+) .* path (<FS_TREE>/)?(\S*).*|\1 \3|'
+)
+
+# Usage: btrfs_subvolume_list $SUBVOLUME
+#
+# Assuming that $SUBVOLUME is a btrfs subvolume, list all child
+# subvolumes; from most deeply nested to most shallowly nested.
+#
+# This is intended to be a sane version of `btrfs subvolume list`.
+btrfs_subvolume_list() {
+	local subvolume=$1
+
+	local id all path subpath
+	id="$(btrfs_subvolume_id "$subvolume")" || return $?
+	all="$(btrfs_subvolume_list_all "$subvolume")" || return $?
+	path="$(sed -n "s/^$id //p" <<<"$all")"
+	while read -r id subpath; do
+		if [[ "$subpath" = "$path"/* ]]; then
+			printf '%s\n' "${subpath#"${path}/"}"
+		fi
+	done <<<"$all" | LC_ALL=C sort --reverse
+}
+
+# Usage: btrfs_subvolume_delete $SUBVOLUME
+#
+# Assuming that $SUBVOLUME is a btrfs subvolume, delete it and all
+# subvolumes below it.
+#
+# This is intended to be a recursive version of
+# `btrfs subvolume delete`.
+btrfs_subvolume_delete() {
+	local dir="$1"
+	local subvolumes subvolume
+	subvolumes=($(btrfs_subvolume_list "$dir")) || return $?
+	for subvolume in "${subvolumes[@]}"; do
+		btrfs subvolume delete "$dir/$subvolume" || return $?
+	done
+	btrfs subvolume delete "$dir"
+}
+
 create_chroot() {
 	# Lock the chroot we want to use. We'll keep this lock until we exit.
 	lock 9 "$copydir.lock" "Locking chroot copy [%s]" "$copy"
@@ -92,7 +147,7 @@  create_chroot() {
 		stat_busy "Creating clean working copy [%s]" "$copy"
 		if [[ "$chroottype" == btrfs ]] && ! mountpoint -q "$copydir"; then
 			if [[ -d $copydir ]]; then
-				btrfs subvolume delete "$copydir" >/dev/null ||
+				btrfs_subvolume_delete "$copydir" >/dev/null ||
 					die "Unable to delete subvolume %s" "$copydir"
 			fi
 			btrfs subvolume snapshot "$chrootdir/root" "$copydir" >/dev/null ||
@@ -114,7 +169,7 @@  create_chroot() {
 clean_temporary() {
 	stat_busy "Removing temporary copy [%s]" "$copy"
 	if [[ "$chroottype" == btrfs ]] && ! mountpoint -q "$copydir"; then
-		btrfs subvolume delete "$copydir" >/dev/null ||
+		btrfs_subvolume_delete "$copydir" >/dev/null ||
 			die "Unable to delete subvolume %s" "$copydir"
 	else
 		# avoid change of filesystem in case of an umount failure