Merge ~rafaeldtinoco/ubuntu/+source/bcache-tools:lp1861941-bionic into ubuntu/+source/bcache-tools:ubuntu/bionic-devel

[Rafael David Tinoco]

SUMMARY:

BUG: https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1861941
TESTCASE: https://paste.ubuntu.com/p/37KGy2Smnp/
PPA: https://launchpad.net/~rafaeldtinoco/+archive/ubuntu/lp1861941

BCACHE-TOOLS:

GROOVY: https://tinyurl.com/yxonp5hz (merged)
FOCAL: https://tinyurl.com/y3tbd3un (committed + verified)
BIONIC: THIS ONE

SYSTEMD-UDEV:

GROOVY: https://tinyurl.com/y4w22s57 (merged patch rewording, will wait next upload)
FOCAL: https://tinyurl.com/y2uqbyll (merged patch, will ask for an upload - SRU)
BIONIC: BEING WORKED

KERNEL:

Decided that won't be changed as the udev bcache changes won't cause any harm. Will ask for the patch to be dropped from groovy's kernel since there isn't any need to keep it.

[Rafael David Tinoco (rafaeldtinoco)]

Subscribed @paelzer directly as he was the one to review merges for Groovy and Focal. Bionic time has arrived =).

[Rafael David Tinoco (rafaeldtinoco)]

I've got to be honest here.. bionic is affected ** in theory ** .. but I could *not* reproduce the same issue in Bionic using 18.04 kernel or the HWE one.

(k)rafaeldtinoco@bcachebionic:~/scripts$ dpkg-query -f '${Package}###${Version}\n' -W 'bcache-tools'
bcache-tools###1.0.8-2ubuntu0.1~ppa1

I could, though, make sure that the reproducer works and there are no regressions:

(k)rafaeldtinoco@bcachebionic:~/scripts$ sudo ./test-bcache.sh
/mnt is a mountpoint
/dev/bcache0: 2 bytes were erased at offset 0x00000438 (ext4): 53 ef
writing 1 to /sys/class/block/bcache0/bcache/stop
writing 1 to /sys/fs/bcache/d959d8c4-d6ba-4798-b145-f3f2b8137d88/stop
/dev/vdc: 16 bytes were erased at offset 0x00001018 (bcache): c6 85 73 f6 4e 1a 45 ca 82 65 f5 7f 48 ba 6d 81

...

mounting bcache0 to /mnt
total 0
drwxr-xr-x 2 root root 60 Aug 5 21:30 .
drwxr-xr-x 3 root root 60 Aug 5 21:30 ..
lrwxrwxrwx 1 root root 13 Aug 5 21:30 70f53747-cc4b-427d-8300-f6ae3ff5c41e -> ../../bcache0
Everything OK

[Rafael David Tinoco (rafaeldtinoco)]

So, after this change getting accepted for Groovy and Focal, I really would like not to rely in kernel patches for bcache CACHED uuid events. This method (being SRUed here) is the most correct and effective way, and proved to be racy-less when other devices trigger udevs).

Please share your thoughts.

[Rafael David Tinoco (rafaeldtinoco)]

I have updated the case with SRU notes and added a specific note for [Test Case] saying that the issue is not reproducible in Bionic but it would be better for all supported versions to have the same "bcached" environment feed: the wrapper reading superblock of the bcache devices.

[Christian Ehrhardt  (paelzer)]

LGTM - changes are as discussed and match.
I have tried to force it as well but couldn't - I thought my setup is bad but I've read it was the same for you.

Never the less for an issue like this (racy by nature) I too agree that I'd prefer to have the fix in Bionic despite not being able to perfectly reproduce it there.

What I wondered - can we show that the new code works correctly on Bionic by modifying the script that is placed to do something else - then show the issues (this confirms it is executed as expected). Then restore the correct content of the script and trigger things again - show the resulting by-uuid links.

+1 - doing the modified test I suggested is up to you

review: Approve

[Rafael David Tinoco (rafaeldtinoco)]

I tried some changes to mock the issue... but they made the thing so artificial that I think its better just to leave as it is. I'm confident that wrapper script is a better approach for symlink creation - than the actual - with or without the bug.

Thanks for reviewing this for me @paelzer.

LOGs:

$ debdiff ./*.dsc 2>&1 | grep -v gpg| diffstat
 changelog | 7 ++
 control | 5 +-
 patches/0003-Add-bcache-export-cached-helper.patch | 104 ++++++++++++++++++++++++++++++++++++++++++
 patches/series | 1
 4 files changed, 115 insertions(+), 2 deletions(-)

(c)rafaeldtinoco@bionic:~/.../sources/ubuntu/bcache-tools$ git ubuntu tag --upload

(c)rafaeldtinoco@bionic:~/.../sources/ubuntu/bcache-tools$ git describe --tags
upload/1.0.8-2ubuntu0.1

(c)rafaeldtinoco@bionic:~/.../sources/ubuntu/bcache-tools$ git push pkg upload/1.0.8-2ubuntu0.1
Counting objects: 16, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (16/16), done.
Writing objects: 100% (16/16), 3.51 KiB | 1.17 MiB/s, done.
Total 16 (delta 11), reused 0 (delta 0)
remote: Checking connectivity: 16, done.
To ssh://git.launchpad.net/ubuntu/+source/bcache-tools
 * [new tag] upload/1.0.8-2ubuntu0.1 -> upload/1.0.8-2ubuntu0.1

(c)rafaeldtinoco@bionic:~/work/sources/ubuntu$ dput ubuntu bcache-tools_1.0.8-2ubuntu0.1_source.changes
Checking signature on .changes
gpg: /home/rafaeldtinoco/work/sources/ubuntu/bcache-tools_1.0.8-2ubuntu0.1_source.changes: Valid signature from A93E0E0AD83C0D0F
Checking signature on .dsc
gpg: /home/rafaeldtinoco/work/sources/ubuntu/bcache-tools_1.0.8-2ubuntu0.1.dsc: Valid signature from A93E0E0AD83C0D0F
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading bcache-tools_1.0.8-2ubuntu0.1.dsc: done.
  Uploading bcache-tools_1.0.8-2ubuntu0.1.debian.tar.xz: done.
  Uploading bcache-tools_1.0.8-2ubuntu0.1_source.buildinfo: done.
  Uploading bcache-tools_1.0.8-2ubuntu0.1_source.changes: done.
Successfully uploaded packages.

[Ryan Harper (raharper)]

> saying that the issue is not reproducible in Bionic

ISTR that Bionic rootfs with GA kernel or even -updates kernel
was not easily reproduced. However, I did observe issues with -hwe
and edge kernels over bionic rootfs.

The difficulty in producing on bionic/ga originally lead me to look
at the kernel for a potential regression; we've been running without
this change on bionic for quite sometime without issues. With focal
and bionic running focal kernel (-hwe) that's where it will show up.

FWIW, I'm +1 on getting the user-space tools in bionic since they provide
the rules and can handle a newer kernel which the issue may be more easily
reproduced.

diff --git a/debian/changelog b/debian/changelog
index 77cb821..cf755d7 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+bcache-tools (1.0.8-2ubuntu0.18.04.1) bionic; urgency=medium
+
+  [ Ryan Harper ]
+  * Add helper script to read bcache devs superblock (LP: #1861941)
+
+ -- Rafael David Tinoco <[e-mail]>  Wed, 05 Aug 2020 17:44:05 -0300
+
 bcache-tools (1.0.8-2build1) artful; urgency=medium
 
   * No-change rebuild to pick up -fPIE compiler default in static
diff --git a/debian/control b/debian/control
index 4a138f6..4f60931 100644
--- a/debian/control
+++ b/debian/control
@@ -1,5 +1,6 @@
 Source: bcache-tools
-Maintainer: David Mohr <[e-mail]>
+Maintainer: Ubuntu Developers <[e-mail]>
+XSBC-Original-Maintainer: David Mohr <[e-mail]>
 Uploaders: Robie Basak <[e-mail]>
 Section: utils
 Priority: optional
@@ -11,7 +12,7 @@ Homepage: http://bcache.evilpiepirate.org/
 
 Package: bcache-tools
 Architecture: linux-any
-Depends: ${shlibs:Depends}, ${misc:Depends}
+Depends: ${shlibs:Depends}, ${misc:Depends}, gawk
 Recommends: initramfs-tools | linux-initramfs-tool
 Description: bcache userspace tools
  Bcache allows the use of SSDs to cache other block devices.
diff --git a/debian/patches/0003-Add-bcache-export-cached-helper.patch b/debian/patches/0003-Add-bcache-export-cached-helper.patch
new file mode 100644
index 0000000..f24424b
--- /dev/null
+++ b/debian/patches/0003-Add-bcache-export-cached-helper.patch
@@ -0,0 +1,104 @@
+Description: Add bcache-export-cached helper to export CACHED_UUID and
+ CACHED_LABEL always
+
+Linux kernel bcache driver does not always emit a uevent[1] for when a backing
+device is bound to a bcacheN device.  When this happens, the udev rule for
+creating /dev/bcache/by-uuid or /dev/bcache/by-label symlinks does not fire and
+removes any persistent symlink to a specific backing device since the bcache
+minor numbers (bcache0, 1, 2) are not guaranteed across reboots.
+
+This script reads the superblock of the bcache device slaves, ensuring the
+slave is a backing device via sb.version check, extracts the dev.uuid and
+dev.label values and exports them to udev for triggering the symlink rules in
+the existing rules file.
+
+1. https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1729145
+
+Author: Ryan Harper <[e-mail]>
+Bug: https://bugs.debian.org/890446
+Bug-Ubuntu: https://launchpad.net/bugs/1861941
+Forwarded: https://github.com/koverstreet/bcache-tools/pull/6/
+Reviewed-By: Rafael David Tinoco <[e-mail]>
+Last-Update: 2020-07-22
+
+--- a/69-bcache.rules
++++ b/69-bcache.rules
+@@ -23,10 +23,9 @@
+ LABEL="bcache_backing_end"
+ 
+ # Cached devices: symlink
+-DRIVER=="bcache", ENV{CACHED_UUID}=="?*", \
+-        SYMLINK+="bcache/by-uuid/$env{CACHED_UUID}"
+-DRIVER=="bcache", ENV{CACHED_LABEL}=="?*", \
+-        SYMLINK+="bcache/by-label/$env{CACHED_LABEL}"
++IMPORT{program}="bcache-export-cached $tempnode"
++ENV{CACHED_UUID}=="?*", SYMLINK+="bcache/by-uuid/$env{CACHED_UUID}"
++ENV{CACHED_LABEL}=="?*", SYMLINK+="bcache/by-label/$env{CACHED_LABEL}"
+ 
+ LABEL="bcache_end"
+ 
+--- a/Makefile
++++ b/Makefile
+@@ -9,7 +9,7 @@
+ 
+ install: make-bcache probe-bcache bcache-super-show
+ 	$(INSTALL) -m0755 make-bcache bcache-super-show	$(DESTDIR)${PREFIX}/sbin/
+-	$(INSTALL) -m0755 probe-bcache bcache-register		$(DESTDIR)$(UDEVLIBDIR)/
++	$(INSTALL) -m0755 probe-bcache bcache-register bcache-export-cached $(DESTDIR)$(UDEVLIBDIR)/
+ 	$(INSTALL) -m0644 69-bcache.rules	$(DESTDIR)$(UDEVLIBDIR)/rules.d/
+ 	$(INSTALL) -m0644 -- *.8 $(DESTDIR)${PREFIX}/share/man/man8/
+ 	$(INSTALL) -D -m0755 initramfs/hook	$(DESTDIR)/usr/share/initramfs-tools/hooks/bcache
+--- /dev/null
++++ b/bcache-export-cached
+@@ -0,0 +1,31 @@
++#!/bin/sh
++#
++# This program reads the bcache superblock on bcacheX slaves to extract the
++# dev.uuid and dev.label which refer to a specific backing device.
++#
++# It integrates with udev 'import' by writing CACHED_UUID=X and optionally
++# CACHED_LABEL=X for the backing device of the provided bcache device.
++# Ignore caching devices by skipping unless sb.version=1
++#
++# There is 1 and only 1 backing device (slaves/*) for a bcache device.
++
++TEMPNODE=${1}               # /dev/bcacheN
++DEVNAME=${TEMPNODE##*/}     # /dev/bcacheN -> bcacheN
++
++for slave in "/sys/class/block/$DEVNAME/slaves"/*; do
++    [ -d "$slave" ] || continue
++    /usr/sbin/bcache-super-show "/dev/${slave##*/}" |
++       gawk '$1 == "sb.version" { sbver=$2; }
++            $1 == "dev.uuid" { uuid=$2; }
++            $1 == "dev.label" && $2 != "(empty)" { label=$2; }
++            END {
++                if (sbver == 1 && uuid) {
++                    print("CACHED_UUID=" uuid)
++                    if (label) print("CACHED_LABEL=" label)
++                    exit(0)
++                }
++                exit(1);
++            }'
++    # gawk exits 0 if it found a backing device.
++    [ $? -eq 0 ] && exit 0
++done
+--- a/initcpio/install
++++ b/initcpio/install
+@@ -1,6 +1,7 @@
+ #!/bin/bash
+ build() {
+     add_module bcache
++    add_binary /usr/lib/udev/bcache-export-cached
+     add_binary /usr/lib/udev/bcache-register
+     add_binary /usr/lib/udev/probe-bcache
+     add_file /usr/lib/udev/rules.d/69-bcache.rules
+--- a/initramfs/hook
++++ b/initramfs/hook
+@@ -22,6 +22,7 @@
+     cp -pt "${DESTDIR}/lib/udev/rules.d" /lib/udev/rules.d/69-bcache.rules
+ fi
+ 
++copy_exec /lib/udev/bcache-export-cached
+ copy_exec /lib/udev/bcache-register
+ copy_exec /lib/udev/probe-bcache
+ manual_add_modules bcache
diff --git a/debian/patches/series b/debian/patches/series
index d487cf6..30402a4 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1,2 +1,3 @@
 0001-Clean-should-remove-bcache-register.patch
 0002-Don-t-inline-crc64-for-gcc-5-compatability.patch
+0003-Add-bcache-export-cached-helper.patch