[PATCH 1/9] e2scrub: check to make sure lvm2 is installed

[PATCH 1/9] e2scrub: check to make sure lvm2 is installed

[Theodore Ts'o]

Not all systems will have the lvm2 package installed, so check for
that.  Pretty much all systems should have util-linux installed, but
check for that as well.

Of course, if lvm2 is installed we shouldn't find any LVM devices ---
but eventually the Demon Murphy will find a way to make it happen. :-)

Also, set the PATH so we don't have to worry about the script failing
due to /sbin not being in the path.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 scrub/e2scrub.in     | 12 ++++++++++++
 scrub/e2scrub_all.in | 16 ++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/scrub/e2scrub.in b/scrub/e2scrub.in
index e1965db4e..51a909373 100644
--- a/scrub/e2scrub.in
+++ b/scrub/e2scrub.in
@@ -23,6 +23,8 @@
 # check filesystems in VGs that have at least 256MB (or so) of
 # free space.
 
+PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
+
 snap_size_mb=256
 fstrim=0
 reap=0
@@ -82,6 +84,16 @@ if [ -z "${arg}" ]; then
 	exitcode 1
 fi
 
+if ! type lsblk >& /dev/null ; then
+    echo "e2scrub: can't find lsblk --- is util-linux installed?"
+    exitcode 1
+fi
+
+if ! type lvcreate >& /dev/null ; then
+    echo "e2scrub: can't find lvcreate --- is lvm2 installed?"
+    exitcode 1
+fi
+
 # Find the device for a given mountpoint
 dev_from_mount() {
 	local mountpt="$(realpath "$1")"
diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
index 23d122d25..d725a7f2e 100644
--- a/scrub/e2scrub_all.in
+++ b/scrub/e2scrub_all.in
@@ -18,6 +18,8 @@
 #  along with this program; if not, write the Free Software Foundation,
 #  Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
 
+PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
+
 scrub_all=0
 conffile="@root_sysconfdir@/e2scrub.conf"
 
@@ -68,6 +70,20 @@ while getopts "ArV" opt; do
 done
 shift "$((OPTIND - 1))"
 
+# If some prerequisite packages are not installed, exit with a code
+# indicating success to avoid spamming the sysadmin with fail messages
+# when e2scrub_all is run out of cron or a systemd timer.
+
+if ! type lsblk >& /dev/null ; then
+    echo "e2scrub_all: can't find lsblk --- is util-linux installed?"
+    exitcode 0
+fi
+
+if ! type lvcreate >& /dev/null ; then
+    echo "e2scrub_all: can't find lvcreate --- is lvm2 installed?"
+    exitcode 0
+fi
+
 # Find scrub targets, make sure we only do this once.
 ls_scrub_targets() {
 	lsblk -o NAME,FSTYPE,MOUNTPOINT -p -P -n | while read vars; do
-- 
2.19.1

[PATCH 2/9] debian: drop lvm2 from the recommends line

[Theodore Ts'o]

If the user doesn't intend to use lvm2, and it's not installed,
installing e2fsprogs shouldn't drag it (and all of its dependencies)
into the system.

Addresses-Debian-Bug: 924275

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 debian/control | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/debian/control b/debian/control
index 1bb8adb15..e454687f9 100644
--- a/debian/control
+++ b/debian/control
@@ -191,7 +191,7 @@ XB-Important: yes
 Pre-Depends: ${shlibs:Depends}, ${misc:Depends}, libblkid1, libuuid1
 Multi-Arch: foreign
 Suggests: gpart, parted, fuse2fs, e2fsck-static
-Recommends: e2fsprogs-l10n, lvm2
+Recommends: e2fsprogs-l10n
 Architecture: any
 Description: ext2/ext3/ext4 file system utilities
  The ext2, ext3 and ext4 file systems are successors of the original ext
-- 
2.19.1

[PATCH 3/9] Fix "make install-strip"

[Theodore Ts'o]

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 lib/Makefile.bsd-lib     | 2 +-
 lib/Makefile.darwin-lib  | 2 +-
 lib/Makefile.elf-lib     | 2 +-
 lib/Makefile.library     | 2 ++
 lib/Makefile.solaris-lib | 2 +-
 5 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/lib/Makefile.bsd-lib b/lib/Makefile.bsd-lib
index db0947f9a..2792ba3df 100644
--- a/lib/Makefile.bsd-lib
+++ b/lib/Makefile.bsd-lib
@@ -36,7 +36,7 @@ install-shlibs install:: $(BSD_LIB)
 
 install-strip: install
 
-install-shlibs-strip: install-shlibs
+install-shlibs-strip:: install-shlibs
 
 uninstall-shlibs uninstall::
 	$(RM) -f $(DESTDIR)$(BSDLIB_INSTALL_DIR)/$(BSD_LIB)
diff --git a/lib/Makefile.darwin-lib b/lib/Makefile.darwin-lib
index c94a5e721..95cdd4b49 100644
--- a/lib/Makefile.darwin-lib
+++ b/lib/Makefile.darwin-lib
@@ -39,7 +39,7 @@ install-shlibs install:: $(BSD_LIB)
 
 install-strip: install
 
-install-shlibs-strip: install-shlibs
+install-shlibs-strip:: install-shlibs
 
 uninstall-shlibs uninstall::
 	$(RM) -f $(DESTDIR)$(BSDLIB_INSTALL_DIR)/$(BSD_LIB)
diff --git a/lib/Makefile.elf-lib b/lib/Makefile.elf-lib
index bd7b2b3c4..f850f3ddb 100644
--- a/lib/Makefile.elf-lib
+++ b/lib/Makefile.elf-lib
@@ -58,7 +58,7 @@ install-strip: install
 	$(Q) $(STRIP) --strip-unneeded --remove-section=.comment \
 		--remove-section=.note $(DESTDIR)$(ELF_INSTALL_DIR)/$(ELF_LIB)
 
-install-shlibs-strip: install-shlibs
+install-shlibs-strip:: install-shlibs
 	$(E) "	STRIP-LIB $(ELF_INSTALL_DIR)/$(ELF_LIB)"
 	$(Q) $(STRIP) --strip-unneeded --remove-section=.comment \
 		--remove-section=.note $(DESTDIR)$(ELF_INSTALL_DIR)/$(ELF_LIB)
diff --git a/lib/Makefile.library b/lib/Makefile.library
index 1b86b0235..f78467aa7 100644
--- a/lib/Makefile.library
+++ b/lib/Makefile.library
@@ -1,5 +1,7 @@
 all:: subdirs $(LIBRARY).a
 
+install-shlibs-strip::
+
 install-shlibs::
 
 uninstall-shlibs::
diff --git a/lib/Makefile.solaris-lib b/lib/Makefile.solaris-lib
index 304df7d40..1e636368f 100644
--- a/lib/Makefile.solaris-lib
+++ b/lib/Makefile.solaris-lib
@@ -50,7 +50,7 @@ install-shlibs install:: $(ELF_LIB) installdirs-elf-lib
 install-strip: install
 	$(STRIP) -x $(DESTDIR)$(ELF_INSTALL_DIR)/$(ELF_LIB)
 
-install-shlibs-strip: install-shlibs
+install-shlibs-strip:: install-shlibs
 	$(STRIP) -x $(DESTDIR)$(ELF_INSTALL_DIR)/$(ELF_LIB)
 
 uninstall-shlibs uninstall::
-- 
2.19.1

[PATCH 4/9] e2scrub: fix up "make install-strip" support

[Theodore Ts'o]

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 scrub/Makefile.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scrub/Makefile.in b/scrub/Makefile.in
index 2f6d63014..f1e917fcf 100644
--- a/scrub/Makefile.in
+++ b/scrub/Makefile.in
@@ -129,6 +129,8 @@ install-systemd: $(SERVICE_FILES)
 		$(INSTALL_DATA) $$i $(DESTDIR)$(SYSTEMD_SYSTEM_UNIT_DIR)/$$i; \
 	done
 
+install-strip: install
+
 install: $(PROGS) $(MANPAGES) $(FMANPAGES) installdirs $(INSTALL_TGT)
 	$(Q) for i in $(PROGS); do \
 		$(ES) "	INSTALL $(root_sbindir)/$$i"; \
-- 
2.19.1

[PATCH 5/9] e2fscrub: add the -n option which shows what commands e2scrub would execute

[Theodore Ts'o]

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 scrub/e2scrub.8.in | 5 +++++
 scrub/e2scrub.in   | 6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/scrub/e2scrub.8.in b/scrub/e2scrub.8.in
index ff03523e3..7342876a3 100644
--- a/scrub/e2scrub.8.in
+++ b/scrub/e2scrub.8.in
@@ -38,6 +38,11 @@ If the filesystem is not repaired,
 will be run before the next mount.
 .SH OPTIONS
 .TP
+\fB-n\fR
+Print what commands
+.B e2scrub
+would execute (but don't actually execute them).
+.TP
 \fB-r\fR
 Remove the e2scrub snapshot and exit without checking anything.
 .TP
diff --git a/scrub/e2scrub.in b/scrub/e2scrub.in
index 51a909373..301574968 100644
--- a/scrub/e2scrub.in
+++ b/scrub/e2scrub.in
@@ -37,6 +37,7 @@ print_help() {
 	echo "Usage: $0 [OPTIONS] mountpoint | device"
 	echo
 	echo "mountpoint must be on a LVM-managed block device"
+	echo "-n: Show what commands e2scrub would execute."
 	echo "-r: Remove e2scrub snapshot and exit, do not check anything."
 	echo "-t: Run fstrim if successful."
 	echo "-V: Print version information and exit."
@@ -68,8 +69,9 @@ exitcode() {
 	exit "${ret}"
 }
 
-while getopts "rtV" opt; do
-	case "${opt}" in
+while getopts "nrtV" opt; do
+    case "${opt}" in
+	"n") DBG="echo Would execute: " ;;
 	"r") reap=1;;
 	"t") fstrim=1;;
 	"V") print_version; exitcode 0;;
-- 
2.19.1

[PATCH 6/9] e2scrub_all: add the -n option which shows what e2scrub_all would do

[Theodore Ts'o]

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 scrub/e2scrub_all.8.in | 9 +++++++--
 scrub/e2scrub_all.in   | 8 +++++---
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/scrub/e2scrub_all.8.in b/scrub/e2scrub_all.8.in
index ba3b8735e..2787a7108 100644
--- a/scrub/e2scrub_all.8.in
+++ b/scrub/e2scrub_all.8.in
@@ -20,12 +20,17 @@ See the
 manual page for more information about how the checking is performed.
 .SH OPTIONS
 .TP
-\fB-A\fR
-Scrub all ext[234] filesystems even if they are not mounted.
+\fB-n\fR
+Print what commands
+.B e2scrub_all
+would execute (but don't actually execute them).
 .TP
 \fB-r\fR
 Remove e2scrub snapshots but do not check anything.
 .TP
+\fB-A\fR
+Scrub all ext[234] filesystems even if they are not mounted.
+.TP
 \fB-V\fR
 Print version information and exit.
 .SH SEE ALSO
diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
index d725a7f2e..8bc868aa0 100644
--- a/scrub/e2scrub_all.in
+++ b/scrub/e2scrub_all.in
@@ -29,8 +29,9 @@ scrub_args=""
 
 print_help() {
 	echo "Usage: $0 [OPTIONS]"
-	echo " -A: Scrub all ext[234] filesystems even if not mounted."
+	echo " -n: Show what commands e2scrub_all would execute."
 	echo " -r: Remove e2scrub snapshots."
+	echo " -A: Scrub all ext[234] filesystems even if not mounted."
 	echo " -V: Print version information and exit."
 }
 
@@ -60,10 +61,11 @@ exitcode() {
 	exit "${ret}"
 }
 
-while getopts "ArV" opt; do
+while getopts "nrAV" opt; do
 	case "${opt}" in
-	"A") scrub_all=1;;
+	"n") DBG="echo Would execute: " ;;
 	"r") scrub_args="${scrub_args} -r";;
+	"A") scrub_all=1;;
 	"V") print_version; exitcode 0;;
 	*) print_help; exitcode 2;;
 	esac
-- 
2.19.1

[PATCH 7/9] e2scrub_all: make sure there's enough free space for a snapshot

[Theodore Ts'o]

If there isn't, skip the volume so we don't spam the system
administrator with error messages.  It's quite commkon that there is
is zero free space in the volume group.

Addresses-Debian-Bug: #924301

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 scrub/e2scrub_all.in | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
index 8bc868aa0..4cb90a0de 100644
--- a/scrub/e2scrub_all.in
+++ b/scrub/e2scrub_all.in
@@ -21,6 +21,7 @@
 PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
 
 scrub_all=0
+snap_size_mb=256
 conffile="@root_sysconfdir@/e2scrub.conf"
 
 test -f "${conffile}" && . "${conffile}"
@@ -108,6 +109,9 @@ ls_scrub_targets() {
 		eval "${lvm_vars}"
 		echo "${LVM2_LV_ROLE}" | grep -q "snapshot" && continue
 
+		free_space="$(vgs -o vg_free --units m --noheadings --no-suffix "${LVM2_VG_NAME}" 2> /dev/null | sed -e 's/\..*//')"
+		test "${snap_size_mb}" -gt "${free_space}" && continue
+
 		if [ -n "${MOUNTPOINT}" ]; then
 			echo "${MOUNTPOINT}"
 		else
-- 
2.19.1

[PATCH 8/9] e2scrub_all: refactor device probe loop

[Theodore Ts'o]

From: "Darrick J. Wong" <darrick.wong@oracle.com>

Paul Menzel reported that the e2scrub_all reaper service that runs at
startup takes a long time to run, and Ted Ts'o pointed out that we could
do a lot less work by using lvs as the outer loop in the ext4 filesystem
probe function so that we only have to lsblk the lvm devices containing
ext4 filesystems.

Therefore, refactor the loops to put lvs first, which should boost speed
a bit.

[ Made some of the further optimizations suggested by Lukas Czerner.  -- TYT ]

Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 scrub/e2scrub_all.in | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
index 4cb90a0de..cad232987 100644
--- a/scrub/e2scrub_all.in
+++ b/scrub/e2scrub_all.in
@@ -22,6 +22,7 @@ PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
 
 scrub_all=0
 snap_size_mb=256
+reap=0
 conffile="@root_sysconfdir@/e2scrub.conf"
 
 test -f "${conffile}" && . "${conffile}"
@@ -65,7 +66,7 @@ exitcode() {
 while getopts "nrAV" opt; do
 	case "${opt}" in
 	"n") DBG="echo Would execute: " ;;
-	"r") scrub_args="${scrub_args} -r";;
+	"r") scrub_args="${scrub_args} -r"; reap=1;;
 	"A") scrub_all=1;;
 	"V") print_version; exitcode 0;;
 	*) print_help; exitcode 2;;
@@ -88,9 +89,12 @@ if ! type lvcreate >& /dev/null ; then
 fi
 
 # Find scrub targets, make sure we only do this once.
-ls_scrub_targets() {
-	lsblk -o NAME,FSTYPE,MOUNTPOINT -p -P -n | while read vars; do
+ls_scan_targets() {
+	lvs --name-prefixes -o vg_name,lv_path \
+			-S lv_active=active,lv_role=public --noheadings | \
+	while read vars; do
 		eval "${vars}"
+		eval "$(lsblk -o FSTYPE,MOUNTPOINT -p -P -n "${LVM2_LV_PATH}")"
 
 		# Skip non-ext[234]
 		case "${FSTYPE}" in
@@ -103,12 +107,6 @@ ls_scrub_targets() {
 			continue;
 		fi
 
-		# Skip non-lvm devices and lvm snapshots
-		lvm_vars="$(lvs --nameprefixes -o vg_name,lv_name,lv_role --noheadings "${NAME}" 2> /dev/null)"
-		test $? -ne 0 && continue
-		eval "${lvm_vars}"
-		echo "${LVM2_LV_ROLE}" | grep -q "snapshot" && continue
-
 		free_space="$(vgs -o vg_free --units m --noheadings --no-suffix "${LVM2_VG_NAME}" 2> /dev/null | sed -e 's/\..*//')"
 		test "${snap_size_mb}" -gt "${free_space}" && continue
 
@@ -120,6 +118,20 @@ ls_scrub_targets() {
 	done | sort | uniq
 }
 
+# Find leftover scrub snapshots
+ls_reap_targets() {
+	lvs -o lv_path -S lv_role=snapshot -S lv_name=~\(e2scrub$\) --noheadings
+}
+
+# Figure out what we're targeting
+ls_targets() {
+	if [ "${reap}" -eq 1 ]; then
+		ls_reap_targets
+	else
+		ls_scan_targets
+	fi
+}
+
 # systemd doesn't know to do path escaping on the instance variable we pass
 # to the e2scrub service, which breaks things if there is a dash in the path
 # name.  Therefore, do the path escaping ourselves if needed.
@@ -140,10 +152,10 @@ escape_path_for_systemd() {
 
 # Scrub any mounted fs on lvm by creating a snapshot and fscking that.
 stdin="$(realpath /dev/stdin)"
-ls_scrub_targets | while read tgt; do
+ls_targets | while read tgt; do
 	# If we're not reaping and systemd is present, try invoking the
 	# systemd service.
-	if [ -z "${scrub_args}" ] && type systemctl > /dev/null 2>&1; then
+	if [ "${reap}" -ne 1 ] && type systemctl > /dev/null 2>&1; then
 		tgt_esc="$(escape_path_for_systemd "${tgt}")"
 		${DBG} systemctl start "e2scrub@${tgt_esc}" 2> /dev/null < "${stdin}"
 		res=$?
-- 
2.19.1

[PATCH 9/9] e2scrub,e2scrub_all: print a (more understandable) error if not run as root

[Theodore Ts'o]

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 scrub/e2scrub.in     | 5 +++++
 scrub/e2scrub_all.in | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/scrub/e2scrub.in b/scrub/e2scrub.in
index 301574968..666d6485a 100644
--- a/scrub/e2scrub.in
+++ b/scrub/e2scrub.in
@@ -25,6 +25,11 @@
 
 PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
 
+if (( $EUID != 0 )); then
+    echo "e2scrub must be run as root"
+    exit 1
+fi
+
 snap_size_mb=256
 fstrim=0
 reap=0
diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
index cad232987..a02ffe226 100644
--- a/scrub/e2scrub_all.in
+++ b/scrub/e2scrub_all.in
@@ -20,6 +20,11 @@
 
 PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
 
+if (( $EUID != 0 )); then
+    echo "e2scrub_all must be run as root"
+    exit 1
+fi
+
 scrub_all=0
 snap_size_mb=256
 reap=0
-- 
2.19.1

Re: [PATCH 1/9] e2scrub: check to make sure lvm2 is installed

[Darrick J. Wong]

On Wed, Mar 20, 2019 at 10:02:10PM -0400, Theodore Ts'o wrote:
> Not all systems will have the lvm2 package installed, so check for
> that.  Pretty much all systems should have util-linux installed, but
> check for that as well.
> 
> Of course, if lvm2 is installed we shouldn't find any LVM devices ---
> but eventually the Demon Murphy will find a way to make it happen. :-)
> 
> Also, set the PATH so we don't have to worry about the script failing
> due to /sbin not being in the path.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  scrub/e2scrub.in     | 12 ++++++++++++
>  scrub/e2scrub_all.in | 16 ++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/scrub/e2scrub.in b/scrub/e2scrub.in
> index e1965db4e..51a909373 100644
> --- a/scrub/e2scrub.in
> +++ b/scrub/e2scrub.in
> @@ -23,6 +23,8 @@
>  # check filesystems in VGs that have at least 256MB (or so) of
>  # free space.
>  
> +PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
> +
>  snap_size_mb=256
>  fstrim=0
>  reap=0
> @@ -82,6 +84,16 @@ if [ -z "${arg}" ]; then
>  	exitcode 1
>  fi
>  
> +if ! type lsblk >& /dev/null ; then
> +    echo "e2scrub: can't find lsblk --- is util-linux installed?"
> +    exitcode 1
> +fi
> +
> +if ! type lvcreate >& /dev/null ; then
> +    echo "e2scrub: can't find lvcreate --- is lvm2 installed?"
> +    exitcode 1
> +fi
> +
>  # Find the device for a given mountpoint
>  dev_from_mount() {
>  	local mountpt="$(realpath "$1")"
> diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
> index 23d122d25..d725a7f2e 100644
> --- a/scrub/e2scrub_all.in
> +++ b/scrub/e2scrub_all.in
> @@ -18,6 +18,8 @@
>  #  along with this program; if not, write the Free Software Foundation,
>  #  Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
>  
> +PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
> +
>  scrub_all=0
>  conffile="@root_sysconfdir@/e2scrub.conf"
>  
> @@ -68,6 +70,20 @@ while getopts "ArV" opt; do
>  done
>  shift "$((OPTIND - 1))"
>  
> +# If some prerequisite packages are not installed, exit with a code
> +# indicating success to avoid spamming the sysadmin with fail messages
> +# when e2scrub_all is run out of cron or a systemd timer.
> +
> +if ! type lsblk >& /dev/null ; then
> +    echo "e2scrub_all: can't find lsblk --- is util-linux installed?"
> +    exitcode 0
> +fi
> +
> +if ! type lvcreate >& /dev/null ; then
> +    echo "e2scrub_all: can't find lvcreate --- is lvm2 installed?"
> +    exitcode 0
> +fi
> +
>  # Find scrub targets, make sure we only do this once.
>  ls_scrub_targets() {
>  	lsblk -o NAME,FSTYPE,MOUNTPOINT -p -P -n | while read vars; do
> -- 
> 2.19.1
> 

Re: [PATCH 2/9] debian: drop lvm2 from the recommends line

[Darrick J. Wong]

On Wed, Mar 20, 2019 at 10:02:11PM -0400, Theodore Ts'o wrote:
> If the user doesn't intend to use lvm2, and it's not installed,
> installing e2fsprogs shouldn't drag it (and all of its dependencies)
> into the system.
> 
> Addresses-Debian-Bug: 924275
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  debian/control | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/debian/control b/debian/control
> index 1bb8adb15..e454687f9 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -191,7 +191,7 @@ XB-Important: yes
>  Pre-Depends: ${shlibs:Depends}, ${misc:Depends}, libblkid1, libuuid1
>  Multi-Arch: foreign
>  Suggests: gpart, parted, fuse2fs, e2fsck-static
> -Recommends: e2fsprogs-l10n, lvm2
> +Recommends: e2fsprogs-l10n
>  Architecture: any
>  Description: ext2/ext3/ext4 file system utilities
>   The ext2, ext3 and ext4 file systems are successors of the original ext
> -- 
> 2.19.1
> 

Re: [PATCH 5/9] e2fscrub: add the -n option which shows what commands e2scrub would execute

[Darrick J. Wong]

On Wed, Mar 20, 2019 at 10:02:14PM -0400, Theodore Ts'o wrote:
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  scrub/e2scrub.8.in | 5 +++++
>  scrub/e2scrub.in   | 6 ++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/scrub/e2scrub.8.in b/scrub/e2scrub.8.in
> index ff03523e3..7342876a3 100644
> --- a/scrub/e2scrub.8.in
> +++ b/scrub/e2scrub.8.in
> @@ -38,6 +38,11 @@ If the filesystem is not repaired,
>  will be run before the next mount.
>  .SH OPTIONS
>  .TP
> +\fB-n\fR
> +Print what commands
> +.B e2scrub
> +would execute (but don't actually execute them).
> +.TP
>  \fB-r\fR
>  Remove the e2scrub snapshot and exit without checking anything.
>  .TP
> diff --git a/scrub/e2scrub.in b/scrub/e2scrub.in
> index 51a909373..301574968 100644
> --- a/scrub/e2scrub.in
> +++ b/scrub/e2scrub.in
> @@ -37,6 +37,7 @@ print_help() {
>  	echo "Usage: $0 [OPTIONS] mountpoint | device"
>  	echo
>  	echo "mountpoint must be on a LVM-managed block device"
> +	echo "-n: Show what commands e2scrub would execute."
>  	echo "-r: Remove e2scrub snapshot and exit, do not check anything."
>  	echo "-t: Run fstrim if successful."
>  	echo "-V: Print version information and exit."
> @@ -68,8 +69,9 @@ exitcode() {
>  	exit "${ret}"
>  }
>  
> -while getopts "rtV" opt; do
> -	case "${opt}" in
> +while getopts "nrtV" opt; do
> +    case "${opt}" in
> +	"n") DBG="echo Would execute: " ;;
>  	"r") reap=1;;
>  	"t") fstrim=1;;
>  	"V") print_version; exitcode 0;;
> -- 
> 2.19.1
> 

Re: [PATCH 6/9] e2scrub_all: add the -n option which shows what e2scrub_all would do

[Darrick J. Wong]

On Wed, Mar 20, 2019 at 10:02:15PM -0400, Theodore Ts'o wrote:
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  scrub/e2scrub_all.8.in | 9 +++++++--
>  scrub/e2scrub_all.in   | 8 +++++---
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/scrub/e2scrub_all.8.in b/scrub/e2scrub_all.8.in
> index ba3b8735e..2787a7108 100644
> --- a/scrub/e2scrub_all.8.in
> +++ b/scrub/e2scrub_all.8.in
> @@ -20,12 +20,17 @@ See the
>  manual page for more information about how the checking is performed.
>  .SH OPTIONS
>  .TP
> -\fB-A\fR
> -Scrub all ext[234] filesystems even if they are not mounted.
> +\fB-n\fR
> +Print what commands
> +.B e2scrub_all
> +would execute (but don't actually execute them).
>  .TP
>  \fB-r\fR
>  Remove e2scrub snapshots but do not check anything.
>  .TP
> +\fB-A\fR
> +Scrub all ext[234] filesystems even if they are not mounted.
> +.TP

Nitpicking, but this is also rearranging the order of manpage and help
text.  The changes themselves look ok, but I don't know how picky you
are these days about this sort of thing being separate changes. :)

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  \fB-V\fR
>  Print version information and exit.
>  .SH SEE ALSO
> diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
> index d725a7f2e..8bc868aa0 100644
> --- a/scrub/e2scrub_all.in
> +++ b/scrub/e2scrub_all.in
> @@ -29,8 +29,9 @@ scrub_args=""
>  
>  print_help() {
>  	echo "Usage: $0 [OPTIONS]"
> -	echo " -A: Scrub all ext[234] filesystems even if not mounted."
> +	echo " -n: Show what commands e2scrub_all would execute."
>  	echo " -r: Remove e2scrub snapshots."
> +	echo " -A: Scrub all ext[234] filesystems even if not mounted."
>  	echo " -V: Print version information and exit."
>  }
>  
> @@ -60,10 +61,11 @@ exitcode() {
>  	exit "${ret}"
>  }
>  
> -while getopts "ArV" opt; do
> +while getopts "nrAV" opt; do
>  	case "${opt}" in
> -	"A") scrub_all=1;;
> +	"n") DBG="echo Would execute: " ;;
>  	"r") scrub_args="${scrub_args} -r";;
> +	"A") scrub_all=1;;
>  	"V") print_version; exitcode 0;;
>  	*) print_help; exitcode 2;;
>  	esac
> -- 
> 2.19.1
> 

Re: [PATCH 7/9] e2scrub_all: make sure there's enough free space for a snapshot

[Darrick J. Wong]

On Wed, Mar 20, 2019 at 10:02:16PM -0400, Theodore Ts'o wrote:
> If there isn't, skip the volume so we don't spam the system
> administrator with error messages.  It's quite commkon that there is

'common'

> is zero free space in the volume group.
> 
> Addresses-Debian-Bug: #924301
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  scrub/e2scrub_all.in | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
> index 8bc868aa0..4cb90a0de 100644
> --- a/scrub/e2scrub_all.in
> +++ b/scrub/e2scrub_all.in
> @@ -21,6 +21,7 @@
>  PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
>  
>  scrub_all=0
> +snap_size_mb=256
>  conffile="@root_sysconfdir@/e2scrub.conf"
>  
>  test -f "${conffile}" && . "${conffile}"
> @@ -108,6 +109,9 @@ ls_scrub_targets() {
>  		eval "${lvm_vars}"
>  		echo "${LVM2_LV_ROLE}" | grep -q "snapshot" && continue
>  
> +		free_space="$(vgs -o vg_free --units m --noheadings --no-suffix "${LVM2_VG_NAME}" 2> /dev/null | sed -e 's/\..*//')"
> +		test "${snap_size_mb}" -gt "${free_space}" && continue
> +
>  		if [ -n "${MOUNTPOINT}" ]; then
>  			echo "${MOUNTPOINT}"
>  		else
> -- 
> 2.19.1
> 

Re: [PATCH 9/9] e2scrub,e2scrub_all: print a (more understandable) error if not run as root

[Darrick J. Wong]

On Wed, Mar 20, 2019 at 10:02:18PM -0400, Theodore Ts'o wrote:
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Oops, yeah, I forgot this. :/

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  scrub/e2scrub.in     | 5 +++++
>  scrub/e2scrub_all.in | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/scrub/e2scrub.in b/scrub/e2scrub.in
> index 301574968..666d6485a 100644
> --- a/scrub/e2scrub.in
> +++ b/scrub/e2scrub.in
> @@ -25,6 +25,11 @@
>  
>  PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
>  
> +if (( $EUID != 0 )); then
> +    echo "e2scrub must be run as root"
> +    exit 1
> +fi
> +
>  snap_size_mb=256
>  fstrim=0
>  reap=0
> diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
> index cad232987..a02ffe226 100644
> --- a/scrub/e2scrub_all.in
> +++ b/scrub/e2scrub_all.in
> @@ -20,6 +20,11 @@
>  
>  PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
>  
> +if (( $EUID != 0 )); then
> +    echo "e2scrub_all must be run as root"
> +    exit 1
> +fi
> +
>  scrub_all=0
>  snap_size_mb=256
>  reap=0
> -- 
> 2.19.1
> 

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Darrick J. Wong]

On Wed, Mar 20, 2019 at 10:02:17PM -0400, Theodore Ts'o wrote:
> From: "Darrick J. Wong" <darrick.wong@oracle.com>
> 
> Paul Menzel reported that the e2scrub_all reaper service that runs at
> startup takes a long time to run, and Ted Ts'o pointed out that we could
> do a lot less work by using lvs as the outer loop in the ext4 filesystem
> probe function so that we only have to lsblk the lvm devices containing
> ext4 filesystems.
> 
> Therefore, refactor the loops to put lvs first, which should boost speed
> a bit.
> 
> [ Made some of the further optimizations suggested by Lukas Czerner.  -- TYT ]

Those chnages LGTM,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  scrub/e2scrub_all.in | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
> index 4cb90a0de..cad232987 100644
> --- a/scrub/e2scrub_all.in
> +++ b/scrub/e2scrub_all.in
> @@ -22,6 +22,7 @@ PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
>  
>  scrub_all=0
>  snap_size_mb=256
> +reap=0
>  conffile="@root_sysconfdir@/e2scrub.conf"
>  
>  test -f "${conffile}" && . "${conffile}"
> @@ -65,7 +66,7 @@ exitcode() {
>  while getopts "nrAV" opt; do
>  	case "${opt}" in
>  	"n") DBG="echo Would execute: " ;;
> -	"r") scrub_args="${scrub_args} -r";;
> +	"r") scrub_args="${scrub_args} -r"; reap=1;;
>  	"A") scrub_all=1;;
>  	"V") print_version; exitcode 0;;
>  	*) print_help; exitcode 2;;
> @@ -88,9 +89,12 @@ if ! type lvcreate >& /dev/null ; then
>  fi
>  
>  # Find scrub targets, make sure we only do this once.
> -ls_scrub_targets() {
> -	lsblk -o NAME,FSTYPE,MOUNTPOINT -p -P -n | while read vars; do
> +ls_scan_targets() {
> +	lvs --name-prefixes -o vg_name,lv_path \
> +			-S lv_active=active,lv_role=public --noheadings | \
> +	while read vars; do
>  		eval "${vars}"
> +		eval "$(lsblk -o FSTYPE,MOUNTPOINT -p -P -n "${LVM2_LV_PATH}")"
>  
>  		# Skip non-ext[234]
>  		case "${FSTYPE}" in
> @@ -103,12 +107,6 @@ ls_scrub_targets() {
>  			continue;
>  		fi
>  
> -		# Skip non-lvm devices and lvm snapshots
> -		lvm_vars="$(lvs --nameprefixes -o vg_name,lv_name,lv_role --noheadings "${NAME}" 2> /dev/null)"
> -		test $? -ne 0 && continue
> -		eval "${lvm_vars}"
> -		echo "${LVM2_LV_ROLE}" | grep -q "snapshot" && continue
> -
>  		free_space="$(vgs -o vg_free --units m --noheadings --no-suffix "${LVM2_VG_NAME}" 2> /dev/null | sed -e 's/\..*//')"
>  		test "${snap_size_mb}" -gt "${free_space}" && continue
>  
> @@ -120,6 +118,20 @@ ls_scrub_targets() {
>  	done | sort | uniq
>  }
>  
> +# Find leftover scrub snapshots
> +ls_reap_targets() {
> +	lvs -o lv_path -S lv_role=snapshot -S lv_name=~\(e2scrub$\) --noheadings
> +}
> +
> +# Figure out what we're targeting
> +ls_targets() {
> +	if [ "${reap}" -eq 1 ]; then
> +		ls_reap_targets
> +	else
> +		ls_scan_targets
> +	fi
> +}
> +
>  # systemd doesn't know to do path escaping on the instance variable we pass
>  # to the e2scrub service, which breaks things if there is a dash in the path
>  # name.  Therefore, do the path escaping ourselves if needed.
> @@ -140,10 +152,10 @@ escape_path_for_systemd() {
>  
>  # Scrub any mounted fs on lvm by creating a snapshot and fscking that.
>  stdin="$(realpath /dev/stdin)"
> -ls_scrub_targets | while read tgt; do
> +ls_targets | while read tgt; do
>  	# If we're not reaping and systemd is present, try invoking the
>  	# systemd service.
> -	if [ -z "${scrub_args}" ] && type systemctl > /dev/null 2>&1; then
> +	if [ "${reap}" -ne 1 ] && type systemctl > /dev/null 2>&1; then
>  		tgt_esc="$(escape_path_for_systemd "${tgt}")"
>  		${DBG} systemctl start "e2scrub@${tgt_esc}" 2> /dev/null < "${stdin}"
>  		res=$?
> -- 
> 2.19.1
> 

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Lukas Czerner]

On Wed, Mar 20, 2019 at 10:02:17PM -0400, Theodore Ts'o wrote:
> From: "Darrick J. Wong" <darrick.wong@oracle.com>
> 
> Paul Menzel reported that the e2scrub_all reaper service that runs at
> startup takes a long time to run, and Ted Ts'o pointed out that we could
> do a lot less work by using lvs as the outer loop in the ext4 filesystem
> probe function so that we only have to lsblk the lvm devices containing
> ext4 filesystems.
> 
> Therefore, refactor the loops to put lvs first, which should boost speed
> a bit.
> 
> [ Made some of the further optimizations suggested by Lukas Czerner.  -- TYT ]
> 
> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  scrub/e2scrub_all.in | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
> index 4cb90a0de..cad232987 100644
> --- a/scrub/e2scrub_all.in
> +++ b/scrub/e2scrub_all.in
> @@ -22,6 +22,7 @@ PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
>  
>  scrub_all=0
>  snap_size_mb=256
> +reap=0
>  conffile="@root_sysconfdir@/e2scrub.conf"
>  
>  test -f "${conffile}" && . "${conffile}"
> @@ -65,7 +66,7 @@ exitcode() {
>  while getopts "nrAV" opt; do
>  	case "${opt}" in
>  	"n") DBG="echo Would execute: " ;;
> -	"r") scrub_args="${scrub_args} -r";;
> +	"r") scrub_args="${scrub_args} -r"; reap=1;;
>  	"A") scrub_all=1;;
>  	"V") print_version; exitcode 0;;
>  	*) print_help; exitcode 2;;
> @@ -88,9 +89,12 @@ if ! type lvcreate >& /dev/null ; then
>  fi
>  
>  # Find scrub targets, make sure we only do this once.
> -ls_scrub_targets() {
> -	lsblk -o NAME,FSTYPE,MOUNTPOINT -p -P -n | while read vars; do
> +ls_scan_targets() {
> +	lvs --name-prefixes -o vg_name,lv_path \
> +			-S lv_active=active,lv_role=public --noheadings | \
> +	while read vars; do
>  		eval "${vars}"
> +		eval "$(lsblk -o FSTYPE,MOUNTPOINT -p -P -n "${LVM2_LV_PATH}")"
>  
>  		# Skip non-ext[234]
>  		case "${FSTYPE}" in
> @@ -103,12 +107,6 @@ ls_scrub_targets() {
>  			continue;
>  		fi
>  
> -		# Skip non-lvm devices and lvm snapshots
> -		lvm_vars="$(lvs --nameprefixes -o vg_name,lv_name,lv_role --noheadings "${NAME}" 2> /dev/null)"
> -		test $? -ne 0 && continue
> -		eval "${lvm_vars}"
> -		echo "${LVM2_LV_ROLE}" | grep -q "snapshot" && continue

This is wrong.

	lvs --name-prefixes -o vg_name,lv_path \
			-S lv_active=active,lv_role=public --noheadings | \

lv_role=public does not exclude snapshots and so it will fail later in
e2scrub when you try to create a snapshot of a thicksnapshot which is
not supported.

Snapshot of a thinspanshot is allowed though, so we might want to
include those. Not sure if it's wise to do it by default, but regardless
it's probably something for a separate change.

Also I am not sure what's the rush, but it seems you've ignored my other
suggestions.

-Lukas

> -
>  		free_space="$(vgs -o vg_free --units m --noheadings --no-suffix "${LVM2_VG_NAME}" 2> /dev/null | sed -e 's/\..*//')"
>  		test "${snap_size_mb}" -gt "${free_space}" && continue
>  
> @@ -120,6 +118,20 @@ ls_scrub_targets() {
>  	done | sort | uniq
>  }
>  
> +# Find leftover scrub snapshots
> +ls_reap_targets() {
> +	lvs -o lv_path -S lv_role=snapshot -S lv_name=~\(e2scrub$\) --noheadings
> +}
> +
> +# Figure out what we're targeting
> +ls_targets() {
> +	if [ "${reap}" -eq 1 ]; then
> +		ls_reap_targets
> +	else
> +		ls_scan_targets
> +	fi
> +}
> +
>  # systemd doesn't know to do path escaping on the instance variable we pass
>  # to the e2scrub service, which breaks things if there is a dash in the path
>  # name.  Therefore, do the path escaping ourselves if needed.
> @@ -140,10 +152,10 @@ escape_path_for_systemd() {
>  
>  # Scrub any mounted fs on lvm by creating a snapshot and fscking that.
>  stdin="$(realpath /dev/stdin)"
> -ls_scrub_targets | while read tgt; do
> +ls_targets | while read tgt; do
>  	# If we're not reaping and systemd is present, try invoking the
>  	# systemd service.
> -	if [ -z "${scrub_args}" ] && type systemctl > /dev/null 2>&1; then
> +	if [ "${reap}" -ne 1 ] && type systemctl > /dev/null 2>&1; then
>  		tgt_esc="$(escape_path_for_systemd "${tgt}")"
>  		${DBG} systemctl start "e2scrub@${tgt_esc}" 2> /dev/null < "${stdin}"
>  		res=$?
> -- 
> 2.19.1
> 

Re: [PATCH 5/9] e2fscrub: add the -n option which shows what commands e2scrub would execute

[Lukas Czerner]

On Wed, Mar 20, 2019 at 10:02:14PM -0400, Theodore Ts'o wrote:
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  scrub/e2scrub.8.in | 5 +++++
>  scrub/e2scrub.in   | 6 ++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/scrub/e2scrub.8.in b/scrub/e2scrub.8.in
> index ff03523e3..7342876a3 100644
> --- a/scrub/e2scrub.8.in
> +++ b/scrub/e2scrub.8.in
> @@ -38,6 +38,11 @@ If the filesystem is not repaired,
>  will be run before the next mount.
>  .SH OPTIONS
>  .TP
> +\fB-n\fR
> +Print what commands
> +.B e2scrub
> +would execute (but don't actually execute them).

We should be a bit more clear that it's not all the commands that will
be printed but not executed. Some of commands, like lsblk and lvs will
actually be executed so it's not like you can run this without proper
privledges.

It sounds nit-picky, and obvious to us, but I am sure someone will
complaing about this.

-Lukas

> +.TP
>  \fB-r\fR
>  Remove the e2scrub snapshot and exit without checking anything.
>  .TP
> diff --git a/scrub/e2scrub.in b/scrub/e2scrub.in
> index 51a909373..301574968 100644
> --- a/scrub/e2scrub.in
> +++ b/scrub/e2scrub.in
> @@ -37,6 +37,7 @@ print_help() {
>  	echo "Usage: $0 [OPTIONS] mountpoint | device"
>  	echo
>  	echo "mountpoint must be on a LVM-managed block device"
> +	echo "-n: Show what commands e2scrub would execute."
>  	echo "-r: Remove e2scrub snapshot and exit, do not check anything."
>  	echo "-t: Run fstrim if successful."
>  	echo "-V: Print version information and exit."
> @@ -68,8 +69,9 @@ exitcode() {
>  	exit "${ret}"
>  }
>  
> -while getopts "rtV" opt; do
> -	case "${opt}" in
> +while getopts "nrtV" opt; do
> +    case "${opt}" in
> +	"n") DBG="echo Would execute: " ;;
>  	"r") reap=1;;
>  	"t") fstrim=1;;
>  	"V") print_version; exitcode 0;;
> -- 
> 2.19.1
> 

Re: [PATCH 7/9] e2scrub_all: make sure there's enough free space for a snapshot

[Lukas Czerner]

On Wed, Mar 20, 2019 at 10:02:16PM -0400, Theodore Ts'o wrote:
> If there isn't, skip the volume so we don't spam the system
> administrator with error messages.  It's quite commkon that there is
> is zero free space in the volume group.
> 
> Addresses-Debian-Bug: #924301
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  scrub/e2scrub_all.in | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
> index 8bc868aa0..4cb90a0de 100644
> --- a/scrub/e2scrub_all.in
> +++ b/scrub/e2scrub_all.in
> @@ -21,6 +21,7 @@
>  PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
>  
>  scrub_all=0
> +snap_size_mb=256
>  conffile="@root_sysconfdir@/e2scrub.conf"
>  
>  test -f "${conffile}" && . "${conffile}"
> @@ -108,6 +109,9 @@ ls_scrub_targets() {
>  		eval "${lvm_vars}"
>  		echo "${LVM2_LV_ROLE}" | grep -q "snapshot" && continue
>  
> +		free_space="$(vgs -o vg_free --units m --noheadings --no-suffix "${LVM2_VG_NAME}" 2> /dev/null | sed -e 's/\..*//')"
> +		test "${snap_size_mb}" -gt "${free_space}" && continue

So you're going to be calling vgs for every lvs even though you can have
a whole bunch of them in the same vg. That's not very efficient.

You can just filter it out with the lvs at the start of the loop by
adding the proper select string.

-S vg_free\>${snap_size_mb}

-Lukas


> +
>  		if [ -n "${MOUNTPOINT}" ]; then
>  			echo "${MOUNTPOINT}"
>  		else
> -- 
> 2.19.1
> 

Re: [PATCH 9/9] e2scrub,e2scrub_all: print a (more understandable) error if not run as root

[Lukas Czerner]

On Wed, Mar 20, 2019 at 10:02:18PM -0400, Theodore Ts'o wrote:
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  scrub/e2scrub.in     | 5 +++++
>  scrub/e2scrub_all.in | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/scrub/e2scrub.in b/scrub/e2scrub.in
> index 301574968..666d6485a 100644
> --- a/scrub/e2scrub.in
> +++ b/scrub/e2scrub.in
> @@ -25,6 +25,11 @@
>  
>  PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
>  
> +if (( $EUID != 0 )); then

EUID is bashism and might not be available in a different shell. But I
am sure we're using a lot of those and we have "#!/bin/bash".

It's just somehting to consider, because I am not sure if we say
anywhere that those scripts will require bash. Personally I do not care
too much, but I am using bash exclusively :)

-Lukas

> +    echo "e2scrub must be run as root"
> +    exit 1
> +fi
> +
>  snap_size_mb=256
>  fstrim=0
>  reap=0
> diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
> index cad232987..a02ffe226 100644
> --- a/scrub/e2scrub_all.in
> +++ b/scrub/e2scrub_all.in
> @@ -20,6 +20,11 @@
>  
>  PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
>  
> +if (( $EUID != 0 )); then
> +    echo "e2scrub_all must be run as root"
> +    exit 1
> +fi
> +
>  scrub_all=0
>  snap_size_mb=256
>  reap=0
> -- 
> 2.19.1
> 

Re: [PATCH 7/9] e2scrub_all: make sure there's enough free space for a snapshot

[Theodore Ts'o]

On Thu, Mar 21, 2019 at 12:18:56PM +0100, Lukas Czerner wrote:
> 
> So you're going to be calling vgs for every lvs even though you can have
> a whole bunch of them in the same vg. That's not very efficient.
> 
> You can just filter it out with the lvs at the start of the loop by
> adding the proper select string.
> 
> -S vg_free\>${snap_size_mb}

Good point.  BTW, that's why I had ignored one of your other
suggestions --- I needed LVM_VG_NAME, which means I needed more than
one output from lvs, so I use the output directly as you suggested.

        	   	     	  - Ted
			  

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Theodore Ts'o]

On Thu, Mar 21, 2019 at 11:27:42AM +0100, Lukas Czerner wrote:
> 
> lv_role=public does not exclude snapshots and so it will fail later in
> e2scrub when you try to create a snapshot of a thicksnapshot which is
> not supported.

Ah, good point.  Thanks for pointing that out.

> Also I am not sure what's the rush, but it seems you've ignored my other
> suggestions.

I did consider them all, but there were reasons why I didn't use them.
One of them wasn't practical because I needed LVM2_VG_NAME; when you
made that suggestion, I hadn't published the patch needed to fix
Debian Bug #924301.  Of course, if we use your suggestion of using "-S
vg_free > ${snap_size}" it will obviate that need; so thanks for that
suggestion.

The reason why I didn't take one of your other suggestions is that we
need to check whether or not the file system is mounted, and so we
needed the mountpoint in lsblk, and once you ask for the mountpoint,
we can no longer use awk to select a field, since an unmounted file
system shows up as a an empty column.

% sudo lsblk -o MOUNTPOINT,FSTYPE,NAME -l
           LVM2_member nvme0n1p3_crypt
           ext4        lambda-uroot
[SWAP]     swap        lambda-swap_1
/          ext4        lambda-root
           ext4        lambda-old--files
           ext4        lambda-library
           ext4        lambda-test--4k
           ext4        lambda-scratch
           ext4        lambda-test--1k
                       lambda-scratch2
                       lambda-scratch3
           ext4        lambda-results
                       lambda-thinpool_tmeta
                       lambda-thinpool_tdata

I also really didn't like using grep to select the file system type
ext[234], since if it would falsely select a LV name that contained
"ext4", e.g.:

/home/dave       xfs        rhel-ext4--sucks

:-)

We could have used awk to select the field, but that still doesn't
deal with the mountpoint column being empty when it is unmounted.  I
did briefly consider using lsblk -J, but I didn't want to add a
dependency on the jq[1] package (and I didn't even know if RHEL/Fedora
packages jq).

[1] https://packages.debian.org/stretch/jq

Cheers,

					- Ted

Re: [PATCH 5/9] e2fscrub: add the -n option which shows what commands e2scrub would execute

[Theodore Ts'o]

On Thu, Mar 21, 2019 at 11:57:51AM +0100, Lukas Czerner wrote:
> On Wed, Mar 20, 2019 at 10:02:14PM -0400, Theodore Ts'o wrote:
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > ---
> >  scrub/e2scrub.8.in | 5 +++++
> >  scrub/e2scrub.in   | 6 ++++--
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/scrub/e2scrub.8.in b/scrub/e2scrub.8.in
> > index ff03523e3..7342876a3 100644
> > --- a/scrub/e2scrub.8.in
> > +++ b/scrub/e2scrub.8.in
> > @@ -38,6 +38,11 @@ If the filesystem is not repaired,
> >  will be run before the next mount.
> >  .SH OPTIONS
> >  .TP
> > +\fB-n\fR
> > +Print what commands
> > +.B e2scrub
> > +would execute (but don't actually execute them).
> 
> We should be a bit more clear that it's not all the commands that will
> be printed but not executed. Some of commands, like lsblk and lvs will
> actually be executed so it's not like you can run this without proper
> privledges.
> 
> It sounds nit-picky, and obvious to us, but I am sure someone will
> complaing about this.

Good point, I'll fix this.

					- Ted

Re: [PATCH 9/9] e2scrub,e2scrub_all: print a (more understandable) error if not run as root

[Theodore Ts'o]

On Thu, Mar 21, 2019 at 12:36:38PM +0100, Lukas Czerner wrote:
> 
> EUID is bashism and might not be available in a different shell. But I
> am sure we're using a lot of those and we have "#!/bin/bash".
> 
> It's just somehting to consider, because I am not sure if we say
> anywhere that those scripts will require bash. Personally I do not care
> too much, but I am using bash exclusively :)

Bash is considered "essential" by Debian so it's guaranteed to be
installed.  So I haven't really considered it high priority to try to
make it work on dash or pdksh or mksh or some other nitpicky POSIX.2
shell.

I assume that the right place to state this is in the RPM spec file
(it isn't not needed for debian/control since bash is an essential
package).

Shrug; I'd accept clean patches that allowed the use of a strict
POSIX.2-only shell, especially if there were distributions who cared
about this.  But I find it hard to get excited about it, personally.  :-)

					- Ted

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Lukas Czerner]

On Thu, Mar 21, 2019 at 10:31:41AM -0400, Theodore Ts'o wrote:
> On Thu, Mar 21, 2019 at 11:27:42AM +0100, Lukas Czerner wrote:
> > 
> > lv_role=public does not exclude snapshots and so it will fail later in
> > e2scrub when you try to create a snapshot of a thicksnapshot which is
> > not supported.
> 
> Ah, good point.  Thanks for pointing that out.
> 
> > Also I am not sure what's the rush, but it seems you've ignored my other
> > suggestions.
> 
> I did consider them all, but there were reasons why I didn't use them.
> One of them wasn't practical because I needed LVM2_VG_NAME; when you
> made that suggestion, I hadn't published the patch needed to fix
> Debian Bug #924301.  Of course, if we use your suggestion of using "-S
> vg_free > ${snap_size}" it will obviate that need; so thanks for that
> suggestion.
> 
> The reason why I didn't take one of your other suggestions is that we
> need to check whether or not the file system is mounted, and so we
> needed the mountpoint in lsblk, and once you ask for the mountpoint,
> we can no longer use awk to select a field, since an unmounted file
> system shows up as a an empty column.
> 
> % sudo lsblk -o MOUNTPOINT,FSTYPE,NAME -l
>            LVM2_member nvme0n1p3_crypt
>            ext4        lambda-uroot
> [SWAP]     swap        lambda-swap_1
> /          ext4        lambda-root
>            ext4        lambda-old--files
>            ext4        lambda-library
>            ext4        lambda-test--4k
>            ext4        lambda-scratch
>            ext4        lambda-test--1k
>                        lambda-scratch2
>                        lambda-scratch3
>            ext4        lambda-results
>                        lambda-thinpool_tmeta
>                        lambda-thinpool_tdata

So the good news is that it's not really a problem. What you really want
is the mountpoint if it exists and the device otherwise right ?

Then my suggestion will do just that becuase awk will separate the fieds
with FS (field separator) which by default is a single space. But space
is special in awk becuase the fields will be separated by a runs of
space, so if you pick the fileds just right you'll get what you want.

So if I run this on my system

# lsblk -o MOUNTPOINT,NAME,FSTYPE -p -n -l `lvs -o lv_path -S lv_active=active,lv_role=public,lv_role!=snapshot,vg_free\>256 --noheadings`

I'll get

            /dev/mapper/vg_perpetuum-ext4--suck
            /dev/mapper/vg_perpetuum-fedora29
            /dev/mapper/vg_perpetuum-fedora29--2
            /dev/mapper/vg_perpetuum-freebsd
/mnt/kernel /dev/mapper/vg_perpetuum-kernel      ext4
/home       /dev/mapper/vg_perpetuum-lv_home     xfs
/           /dev/mapper/vg_perpetuum-lv_root     xfs
[SWAP]      /dev/mapper/vg_perpetuum-lv_swap     swap
/var        /dev/mapper/vg_perpetuum-lv_var      xfs
            /dev/mapper/vg_perpetuum-lvol001     ext4
            /dev/mapper/vg_perpetuum-overlay     ext4
            /dev/mapper/vg_perpetuum-rhel7
            /dev/mapper/vg_perpetuum-rhel8
            /dev/mapper/vg_perpetuum-test0       ext3
            /dev/mapper/vg_perpetuum-test1       xfs
            /dev/mapper/vg_perpetuum-test10      ext4
            /dev/mapper/vg_perpetuum-test11      ext4
            /dev/mapper/vg_perpetuum-test12
            /dev/mapper/vg_perpetuum-test2
            /dev/mapper/vg_perpetuum-test3       ext4
            /dev/mapper/vg_perpetuum-test4       ext4
            /dev/mapper/vg_perpetuum-test5
            /dev/mapper/vg_perpetuum-test6       LVM2_member
            /dev/mapper/vg_perpetuum-test7
            /dev/mapper/vg_perpetuum-test8
            /dev/mapper/vg_perpetuum-test9       ext4
            /dev/mapper/vg_perpetuum-testing     ext4
            /dev/mapper/vg_perpetuum-testing1    xfs
            /dev/mapper/vg_perpetuum-thinvolume

now if I add 

| awk '{print $1}'

I'll get

/dev/mapper/vg_perpetuum-ext4--suck
/dev/mapper/vg_perpetuum-fedora29
/dev/mapper/vg_perpetuum-fedora29--2
/dev/mapper/vg_perpetuum-freebsd
/mnt/kernel
/home
/
[SWAP]
/var
/dev/mapper/vg_perpetuum-lvol001
/dev/mapper/vg_perpetuum-overlay
/dev/mapper/vg_perpetuum-rhel7
/dev/mapper/vg_perpetuum-rhel8
/dev/mapper/vg_perpetuum-test0
/dev/mapper/vg_perpetuum-test1
/dev/mapper/vg_perpetuum-test10
/dev/mapper/vg_perpetuum-test11
/dev/mapper/vg_perpetuum-test12
/dev/mapper/vg_perpetuum-test2
/dev/mapper/vg_perpetuum-test3
/dev/mapper/vg_perpetuum-test4
/dev/mapper/vg_perpetuum-test5
/dev/mapper/vg_perpetuum-test6
/dev/mapper/vg_perpetuum-test7
/dev/mapper/vg_perpetuum-test8
/dev/mapper/vg_perpetuum-test9
/dev/mapper/vg_perpetuum-testing
/dev/mapper/vg_perpetuum-testing1
/dev/mapper/vg_perpetuum-thinvolume


hence I get mountpoin where the volume is mounted and the device where
it is not. That's what we need right ?

What I did not consider was [SWAP] but we can get rid of that easily.

grep -v '[SWAP]'

> 
> I also really didn't like using grep to select the file system type
> ext[234], since if it would falsely select a LV name that contained
> "ext4", e.g.:
> 
> /home/dave       xfs        rhel-ext4--sucks

That can be dealt with as well as well just by grepping for ' ext[234]$'

So in the end we'll have something like

# lsblk -o MOUNTPOINT,NAME,FSTYPE -p -n -l `lvs -o lv_path -S lv_active=active,lv_role=public,lv_role!=snapshot,vg_free\>256 --noheadings` | grep -v '[SWAP]' | grep ' ext[234]$' | awk '{print $1}'

and so on my system this gives me

/mnt/kernel
/dev/mapper/vg_perpetuum-lvol001
/dev/mapper/vg_perpetuum-overlay
/dev/mapper/vg_perpetuum-test0
/dev/mapper/vg_perpetuum-test10
/dev/mapper/vg_perpetuum-test11
/dev/mapper/vg_perpetuum-test3
/dev/mapper/vg_perpetuum-test4
/dev/mapper/vg_perpetuum-test9
/dev/mapper/vg_perpetuum-testing

And leaving out mounted file systems is again as simple as

grep -v '^/dev/'

Have I missed something ?


Now for the performance. I drop caches, then run each twice

the ls_scan_targets function runs

real	0m0.575s
user	0m0.086s
sys	0m0.179s

real	0m0.241s
user	0m0.088s
sys	0m0.169s


My one-line suggestion runs

real	0m0.275s
user	0m0.027s
sys	0m0.047s

real	0m0.069s
user	0m0.015s
sys	0m0.052s


So the difference is significant, although I would not consider it
meaningful since it's really low anyway, but some people complained so I
guess someone cares and people do have different systems so...

> 
> :-)
> 
> We could have used awk to select the field, but that still doesn't
> deal with the mountpoint column being empty when it is unmounted.  I
> did briefly consider using lsblk -J, but I didn't want to add a
> dependency on the jq[1] package (and I didn't even know if RHEL/Fedora
> packages jq).
> 
> [1] https://packages.debian.org/stretch/jq

Yes Fedora does have it, but I agree that adding this dependency is not
ideal.

Thanks!
-Lukas

> 
> Cheers,
> 
> 					- Ted

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Lukas Czerner]

On Wed, Mar 20, 2019 at 10:02:17PM -0400, Theodore Ts'o wrote:
> From: "Darrick J. Wong" <darrick.wong@oracle.com>
> 
> Paul Menzel reported that the e2scrub_all reaper service that runs at
> startup takes a long time to run, and Ted Ts'o pointed out that we could
> do a lot less work by using lvs as the outer loop in the ext4 filesystem
> probe function so that we only have to lsblk the lvm devices containing
> ext4 filesystems.
> 
> Therefore, refactor the loops to put lvs first, which should boost speed
> a bit.
> 
> [ Made some of the further optimizations suggested by Lukas Czerner.  -- TYT ]
> 
> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  scrub/e2scrub_all.in | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
> index 4cb90a0de..cad232987 100644
> --- a/scrub/e2scrub_all.in
> +++ b/scrub/e2scrub_all.in
> @@ -22,6 +22,7 @@ PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
>  
>  scrub_all=0
>  snap_size_mb=256
> +reap=0
>  conffile="@root_sysconfdir@/e2scrub.conf"
>  
>  test -f "${conffile}" && . "${conffile}"
> @@ -65,7 +66,7 @@ exitcode() {
>  while getopts "nrAV" opt; do
>  	case "${opt}" in
>  	"n") DBG="echo Would execute: " ;;
> -	"r") scrub_args="${scrub_args} -r";;
> +	"r") scrub_args="${scrub_args} -r"; reap=1;;
>  	"A") scrub_all=1;;
>  	"V") print_version; exitcode 0;;
>  	*) print_help; exitcode 2;;
> @@ -88,9 +89,12 @@ if ! type lvcreate >& /dev/null ; then
>  fi
>  
>  # Find scrub targets, make sure we only do this once.
> -ls_scrub_targets() {
> -	lsblk -o NAME,FSTYPE,MOUNTPOINT -p -P -n | while read vars; do
> +ls_scan_targets() {
> +	lvs --name-prefixes -o vg_name,lv_path \
> +			-S lv_active=active,lv_role=public --noheadings | \
> +	while read vars; do
>  		eval "${vars}"
> +		eval "$(lsblk -o FSTYPE,MOUNTPOINT -p -P -n "${LVM2_LV_PATH}")"

Btw, I know that you're going to change and resend this, but there is
another problem. You're missing NAME field and so the check that we have
later

 93 →       →       if [ -n "${MOUNTPOINT}" ]; then
 94 →       →       →       echo "${MOUNTPOINT}"
 95 →       →       else
 96 →       →       →       echo "${NAME}"
 97 →       →       fi

will never print the device name.

It almost feels like we need some regression testing for this ;) However
I agree that it's going to be PITA to create since historically our test
suite did not require elevated privledges, nor did it change the system
in any way.

I could bring some knowledge over from ssm and try to avoid system
changes by using loop devices and DM_DEV_DIR, but it will still require
a root privledges and will be significantly different from what we do in
tests/ today. Any ideas on how you want to do this ?

-Lukas


>  
>  		# Skip non-ext[234]
>  		case "${FSTYPE}" in
> @@ -103,12 +107,6 @@ ls_scrub_targets() {
>  			continue;
>  		fi
>  
> -		# Skip non-lvm devices and lvm snapshots
> -		lvm_vars="$(lvs --nameprefixes -o vg_name,lv_name,lv_role --noheadings "${NAME}" 2> /dev/null)"
> -		test $? -ne 0 && continue
> -		eval "${lvm_vars}"
> -		echo "${LVM2_LV_ROLE}" | grep -q "snapshot" && continue
> -
>  		free_space="$(vgs -o vg_free --units m --noheadings --no-suffix "${LVM2_VG_NAME}" 2> /dev/null | sed -e 's/\..*//')"
>  		test "${snap_size_mb}" -gt "${free_space}" && continue
>  
> @@ -120,6 +118,20 @@ ls_scrub_targets() {
>  	done | sort | uniq
>  }
>  
> +# Find leftover scrub snapshots
> +ls_reap_targets() {
> +	lvs -o lv_path -S lv_role=snapshot -S lv_name=~\(e2scrub$\) --noheadings
> +}
> +
> +# Figure out what we're targeting
> +ls_targets() {
> +	if [ "${reap}" -eq 1 ]; then
> +		ls_reap_targets
> +	else
> +		ls_scan_targets
> +	fi
> +}
> +
>  # systemd doesn't know to do path escaping on the instance variable we pass
>  # to the e2scrub service, which breaks things if there is a dash in the path
>  # name.  Therefore, do the path escaping ourselves if needed.
> @@ -140,10 +152,10 @@ escape_path_for_systemd() {
>  
>  # Scrub any mounted fs on lvm by creating a snapshot and fscking that.
>  stdin="$(realpath /dev/stdin)"
> -ls_scrub_targets | while read tgt; do
> +ls_targets | while read tgt; do
>  	# If we're not reaping and systemd is present, try invoking the
>  	# systemd service.
> -	if [ -z "${scrub_args}" ] && type systemctl > /dev/null 2>&1; then
> +	if [ "${reap}" -ne 1 ] && type systemctl > /dev/null 2>&1; then
>  		tgt_esc="$(escape_path_for_systemd "${tgt}")"
>  		${DBG} systemctl start "e2scrub@${tgt_esc}" 2> /dev/null < "${stdin}"
>  		res=$?
> -- 
> 2.19.1
> 

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Theodore Ts'o]

On Thu, Mar 21, 2019 at 11:27:42AM +0100, Lukas Czerner wrote:
> 
> Snapshot of a thinspanshot is allowed though, so we might want to
> include those. Not sure if it's wise to do it by default, but regardless
> it's probably something for a separate change.

Yeah, it's definitely a separate change.  One potential design
question is that for a thin volume, you can do both a thin or a think
snapshot, and in some cases one might succeed while the other will
fail.  So do we make this choice be a parameter that we set in the
config file, or do we try to see if there is sufficient spare
freespace for a thick snapshot (and then do that), or a thin snapshot
(and then do that) --- and which should use prefer?

The other thing I'll note is that in order for us to tell whether
something is a thin or thick LV, we're going to to need to ask lvs to
return multiple parameters, so the optimization of using:

	for NAME in $(lvs -o lv_path --noheadings -S...) ; do
	    ...
	done

will no longer work.  (Or we end up calling lvs a second time, which
is less efficient.)

Just curious --- do we know how commonly thin LV's are being used by
customers of various distros?  I assume enterprise distro users will
be the most conservative, but how common is the uptake of thin LV's by
Fedora and OpenSuSE users?

						- Ted

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Theodore Ts'o]

On Thu, Mar 21, 2019 at 04:57:03PM +0100, Lukas Czerner wrote:
> 
> hence I get mountpoin where the volume is mounted and the device where
> it is not. That's what we need right ?

Well, except by default we need to be able to determine whether or not
the volume is mounted, since by default e2scrub_all only runs on
mounted file systems (unless -A) is specified.

What I'm now doing is this, which I think is the simplest way to do things:

ls_scan_targets() {
	for NAME in $(lvs -o lv_path --noheadings \
		   -S "lv_active=active,lv_role=public,lv_role!=snapshot,vg_free>${snap_size_mb}") ; do
		# Skip non-ext[234]
		case "$(blkid -o value -s TYPE ${NAME})" in
		ext[234])	;;
		*)		continue;;
		esac

		if [ "${scrub_all}" -eq 1 ]; then
		    echo ${NAME}
		else
		    MOUNTPOINT="$(lsblk -o MOUNTPOINT --noheadings ${NAME})"

		    if [ -n "${MOUNTPOINT}" ]; then
			echo "${MOUNTPOINT}"
		    fi
		fi
	done | sort | uniq
}

This way we only bother to fetch the mountpoints for ext[234] file
systems, and only when -A is _not_ specified.

In fact, I'm actually thinking that we should just *always* just
return the device pathname in which case we can make this even
simpler:

ls_scan_targets() {
	for NAME in $(lvs -o lv_path --noheadings \
		   -S "lv_active=active,lv_role=public,lv_role!=snapshot,vg_free>${snap_size_mb}") ; do
		# Skip non-ext[234]
		case "$(blkid -o value -s TYPE ${NAME})" in
		ext[234])	;;
		*)		continue;;
		esac

		if [ "${scrub_all}" -eq 1 ] ||
		   [ -n "$(lsblk -o MOUNTPOINT --noheadings ${NAME})" ]; then
		    echo ${NAME}
		fi
	done | sort | uniq
}

This means that we always run e2scrub on the device name, which in
some cases might result in some ugliness, e.g.

	systemctl start e2scrub@-dev-lambda-test\\x2d1k

But I think I can live with that.  (However, the fact that
systemd-escape will create Unicode characters which themselves have to
be escaped is, well, sad....)

What do you see on your system when you benchmark the above?  The fact
that we only determine the mountpoints on ext[234] file systems should
save some time.  We are sheling out to blkid for each device but
that's probably not a huge overhead.

My before (v1.45.0 plus support for -n so we can have comparable
times) and after times (with all of the changes):

0.16user 0.15system 0:00.83elapsed 38%CPU (0avgtext+0avgdata 13384maxresident)k

0.12user 0.11system 0:00.36elapsed 64%CPU (0avgtext+0avgdata 13420maxresident)k

Your one-linder is a bit faster:

0.03user 0.04system 0:00.23elapsed 31%CPU (0avgtext+0avgdata 13316maxresident)k

But if we need to determine thick versus thin LV's so we can
potentially do thin snapshots, a bunch of these optimizations are
going to go away anyway.  And realistically, so long as we're fast in
the "no LV's" and "LV's exist but there is no free space" cases, that
should avoid most user complaints, since if we *do* trigger e2scrub,
the cost of running ls_scan_targets will be in the noise.

					- Ted

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Lukas Czerner]

On Thu, Mar 21, 2019 at 01:48:11PM -0400, Theodore Ts'o wrote:
> On Thu, Mar 21, 2019 at 11:27:42AM +0100, Lukas Czerner wrote:
> > 
> > Snapshot of a thinspanshot is allowed though, so we might want to
> > include those. Not sure if it's wise to do it by default, but regardless
> > it's probably something for a separate change.
> 
> Yeah, it's definitely a separate change.  One potential design
> question is that for a thin volume, you can do both a thin or a think
> snapshot, and in some cases one might succeed while the other will
> fail.  So do we make this choice be a parameter that we set in the
> config file, or do we try to see if there is sufficient spare
> freespace for a thick snapshot (and then do that), or a thin snapshot
> (and then do that) --- and which should use prefer?

That's a good question. I am not sure it makes sense to allow choice
between thin/thick snapshots of thin volume. Just pick one, ideally it
would be thin snapshot of thin volume.

> 
> The other thing I'll note is that in order for us to tell whether
> something is a thin or thick LV, we're going to to need to ask lvs to
> return multiple parameters, so the optimization of using:
> 
> 	for NAME in $(lvs -o lv_path --noheadings -S...) ; do
> 	    ...
> 	done
> 
> will no longer work.  (Or we end up calling lvs a second time, which
> is less efficient.)

Yeah, however calling lvs twice might still be better then calling
lsblk for every lv ? This can be easily benchmarked.

first it would be for a non-thin volumes so

-S lv_active=active,lv_role=public,lv_role!=snapshot,segtype!=thin

and second for thin volumes and thin snapshots

-S lv_active=active,lv_role=public,segtype=thin

Note that by default thin snapshots are not active so they will not be
checked unless user activated them.

> 
> Just curious --- do we know how commonly thin LV's are being used by
> customers of various distros?  I assume enterprise distro users will
> be the most conservative, but how common is the uptake of thin LV's by
> Fedora and OpenSuSE users?

I wish I knew. I think that for workstation users it's the defaults that
matters the most so I would not expect thin LV's to be used much there.
Qubes OS being most notable exception I think.
I'd think that thin would be mostly used for virtualization and storage
appliances. I'll ask around, Fedora should have some stats.

-Lukas

> 
> 						- Ted

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 4998 bytes --]

On Mar 21, 2019, at 9:57 AM, Lukas Czerner <lczerner@redhat.com> wrote:
> 
> So the good news is that it's not really a problem. What you really want
> is the mountpoint if it exists and the device otherwise right ?
> 
> Then my suggestion will do just that becuase awk will separate the fieds
> with FS (field separator) which by default is a single space. But space
> is special in awk becuase the fields will be separated by a runs of
> space, so if you pick the fileds just right you'll get what you want.
> 
> So if I run this on my system
> 
> # lsblk -o MOUNTPOINT,NAME,FSTYPE -p -n -l `lvs -o lv_path -S lv_active=active,lv_role=public,lv_role!=snapshot,vg_free\>256 --noheadings`
> 
> I'll get
> 
>            /dev/mapper/vg_perpetuum-ext4--suck
>            /dev/mapper/vg_perpetuum-fedora29
>            /dev/mapper/vg_perpetuum-fedora29--2
>            /dev/mapper/vg_perpetuum-freebsd
> /mnt/kernel /dev/mapper/vg_perpetuum-kernel      ext4
> /home       /dev/mapper/vg_perpetuum-lv_home     xfs
> /           /dev/mapper/vg_perpetuum-lv_root     xfs
> [SWAP]      /dev/mapper/vg_perpetuum-lv_swap     swap
> /var        /dev/mapper/vg_perpetuum-lv_var      xfs
>            /dev/mapper/vg_perpetuum-lvol001     ext4
>            /dev/mapper/vg_perpetuum-overlay     ext4
>            /dev/mapper/vg_perpetuum-rhel7
>            /dev/mapper/vg_perpetuum-rhel8
>            /dev/mapper/vg_perpetuum-test0       ext3
>            /dev/mapper/vg_perpetuum-testing     ext4
>            /dev/mapper/vg_perpetuum-testing1    xfs
>            /dev/mapper/vg_perpetuum-thinvolume
> 
> now if I add
> 
> | awk '{print $1}'
> 
> I'll get
> 
> /dev/mapper/vg_perpetuum-ext4--suck
> /dev/mapper/vg_perpetuum-fedora29
> /dev/mapper/vg_perpetuum-fedora29--2
> /dev/mapper/vg_perpetuum-freebsd
> /mnt/kernel
> /home
> /
> [SWAP]
> /var
> /dev/mapper/vg_perpetuum-lvol001
> /dev/mapper/vg_perpetuum-overlay
> /dev/mapper/vg_perpetuum-rhel7
> /dev/mapper/vg_perpetuum-rhel8
> /dev/mapper/vg_perpetuum-test0
> /dev/mapper/vg_perpetuum-testing
> /dev/mapper/vg_perpetuum-testing1
> /dev/mapper/vg_perpetuum-thinvolume
> 
> 
> hence I get mountpoin where the volume is mounted and the device where
> it is not. That's what we need right ?
> 
> What I did not consider was [SWAP] but we can get rid of that easily.
> 
> grep -v '[SWAP]'
> 
> So in the end we'll have something like
> 
> # lsblk -o MOUNTPOINT,NAME,FSTYPE -p -n -l `lvs -o lv_path -S lv_active=active,lv_role=public,lv_role!=snapshot,vg_free\>256 --noheadings` | grep -v '[SWAP]' | grep ' ext[234]$' | awk '{print $1}'

This doesn't need to skip [SWAP] explicitly because it will not have an ext[234]
filesystem mounted on it, so would be excluded by the next "grep" anyway.

However, if you are using "awk" then there is no need for the extra grep(2) I think.
Something like the following looks like it will work:

  lsblk -o MOUNTPOINT,NAME,FSTYPE -p -n -l $(lvs -o lv_path \
      -S lv_active=active,lv_role=public,lv_role!=snapshot,vg_free\>256 --noheadings) |
      awk '($NF > 1 && $2 == ext[234]) || ($NF > 2 && $3 == ext[234]) { print $1 }'

The "$NF" checks appear to be needed because "$3 = ext[234]" seems to match everything
if "$3" is empty, though I can't understand why that would be desirable.

> and so on my system this gives me
> 
> /mnt/kernel
> /dev/mapper/vg_perpetuum-lvol001
> /dev/mapper/vg_perpetuum-overlay
> /dev/mapper/vg_perpetuum-test0
> /dev/mapper/vg_perpetuum-test10
> /dev/mapper/vg_perpetuum-test11
> /dev/mapper/vg_perpetuum-test3
> /dev/mapper/vg_perpetuum-test4
> /dev/mapper/vg_perpetuum-test9
> /dev/mapper/vg_perpetuum-testing
> 
> And leaving out mounted file systems is again as simple as
> 
> grep -v '^/dev/'
> 
> Have I missed something ?
> 
> 
> Now for the performance. I drop caches, then run each twice
> 
> the ls_scan_targets function runs
> 
> real	0m0.575s
> user	0m0.086s
> sys	0m0.179s
> 
> real	0m0.241s
> user	0m0.088s
> sys	0m0.169s
> 
> 
> My one-line suggestion runs
> 
> real	0m0.275s
> user	0m0.027s
> sys	0m0.047s
> 
> real	0m0.069s
> user	0m0.015s
> sys	0m0.052s
> 
> 
> So the difference is significant, although I would not consider it
> meaningful since it's really low anyway, but some people complained so I
> guess someone cares and people do have different systems so...
> 
>> 
>> :-)
>> 
>> We could have used awk to select the field, but that still doesn't
>> deal with the mountpoint column being empty when it is unmounted.  I
>> did briefly consider using lsblk -J, but I didn't want to add a
>> dependency on the jq[1] package (and I didn't even know if RHEL/Fedora
>> packages jq).
>> 
>> [1] https://packages.debian.org/stretch/jq
> 
> Yes Fedora does have it, but I agree that adding this dependency is not
> ideal.
> 
> Thanks!
> -Lukas
> 
>> 
>> Cheers,
>> 
>> 					- Ted


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Lukas Czerner]

On Thu, Mar 21, 2019 at 02:24:56PM -0400, Theodore Ts'o wrote:
> On Thu, Mar 21, 2019 at 04:57:03PM +0100, Lukas Czerner wrote:
> > 
> > hence I get mountpoin where the volume is mounted and the device where
> > it is not. That's what we need right ?
> 
> Well, except by default we need to be able to determine whether or not
> the volume is mounted, since by default e2scrub_all only runs on
> mounted file systems (unless -A) is specified.

Right, I did mention it later in the reply. It can be filtered

grep -v '^/dev/'

> 
> What I'm now doing is this, which I think is the simplest way to do things:
> 
> ls_scan_targets() {
> 	for NAME in $(lvs -o lv_path --noheadings \
> 		   -S "lv_active=active,lv_role=public,lv_role!=snapshot,vg_free>${snap_size_mb}") ; do
> 		# Skip non-ext[234]
> 		case "$(blkid -o value -s TYPE ${NAME})" in
> 		ext[234])	;;
> 		*)		continue;;
> 		esac
> 
> 		if [ "${scrub_all}" -eq 1 ]; then
> 		    echo ${NAME}
> 		else
> 		    MOUNTPOINT="$(lsblk -o MOUNTPOINT --noheadings ${NAME})"
> 
> 		    if [ -n "${MOUNTPOINT}" ]; then
> 			echo "${MOUNTPOINT}"
> 		    fi
> 		fi
> 	done | sort | uniq
> }
> 
> This way we only bother to fetch the mountpoints for ext[234] file
> systems, and only when -A is _not_ specified.
> 
> In fact, I'm actually thinking that we should just *always* just
> return the device pathname in which case we can make this even
> simpler:
> 
> ls_scan_targets() {
> 	for NAME in $(lvs -o lv_path --noheadings \
> 		   -S "lv_active=active,lv_role=public,lv_role!=snapshot,vg_free>${snap_size_mb}") ; do
> 		# Skip non-ext[234]
> 		case "$(blkid -o value -s TYPE ${NAME})" in
> 		ext[234])	;;
> 		*)		continue;;
> 		esac
> 
> 		if [ "${scrub_all}" -eq 1 ] ||
> 		   [ -n "$(lsblk -o MOUNTPOINT --noheadings ${NAME})" ]; then
> 		    echo ${NAME}
> 		fi
> 	done | sort | uniq
> }
> 
> This means that we always run e2scrub on the device name, which in
> some cases might result in some ugliness, e.g.
> 
> 	systemctl start e2scrub@-dev-lambda-test\\x2d1k
> 
> But I think I can live with that.  (However, the fact that
> systemd-escape will create Unicode characters which themselves have to
> be escaped is, well, sad....)
> 
> What do you see on your system when you benchmark the above?  The fact
> that we only determine the mountpoints on ext[234] file systems should
> save some time.  We are sheling out to blkid for each device but
> that's probably not a huge overhead.
> 
> My before (v1.45.0 plus support for -n so we can have comparable
> times) and after times (with all of the changes):
> 
> 0.16user 0.15system 0:00.83elapsed 38%CPU (0avgtext+0avgdata 13384maxresident)k
> 
> 0.12user 0.11system 0:00.36elapsed 64%CPU (0avgtext+0avgdata 13420maxresident)k

For me this new function is the wors of all.

cold cache:
real	0m2.115s
user	0m0.040s
sys	0m0.154s

second time:
real	0m1.100s
user	0m0.037s
sys	0m0.122s

But that's because of blkid which is terribly slow for some reason.
Replacing it with lsblk I get much better results

cold cache:
real	0m0.383s
user	0m0.043s
sys	0m0.112s

second time:
real	0m0.153s
user	0m0.048s
sys	0m0.102s

-Lukas

> 
> Your one-linder is a bit faster:
> 
> 0.03user 0.04system 0:00.23elapsed 31%CPU (0avgtext+0avgdata 13316maxresident)k
> 
> But if we need to determine thick versus thin LV's so we can
> potentially do thin snapshots, a bunch of these optimizations are
> going to go away anyway.  And realistically, so long as we're fast in
> the "no LV's" and "LV's exist but there is no free space" cases, that
> should avoid most user complaints, since if we *do* trigger e2scrub,
> the cost of running ls_scan_targets will be in the noise.
> 
> 					- Ted

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Theodore Ts'o]

On Thu, Mar 21, 2019 at 08:49:01PM +0100, Lukas Czerner wrote:
> I wish I knew. I think that for workstation users it's the defaults that
> matters the most so I would not expect thin LV's to be used much there.
> Qubes OS being most notable exception I think.

Yeah, especially since the lvcreate command is so complicated.  Even
if there's only one thinpool in a VG, you have to specify it
explicitly:

	lvcreate -V 5G --thinpool mythinpool -n thinlv lambda

Even though mythinpool is the only possible thinpool to use in VG
lambda, and even though it's clear that a thinLV is desired due to the
-V option, you still have to pass in the --thinpool option, and there
is no convenient short option character for it.  :-(

Given that the devicemapper people are saying that thick snapshots are
legacy and deprecated, and everyone should be using the new coolness,
I'm pretty sure most users haven't gotten the message....

    	   	     	   	   	  - Ted

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Theodore Ts'o]

On Thu, Mar 21, 2019 at 09:17:10PM +0100, Lukas Czerner wrote:
> 
> Right, I did mention it later in the reply. It can be filtered
> 
> grep -v '^/dev/'

Well, that assumes all device nodes are in /dev.  Which is not
necessarily always the case, especially in some of the more _whacky_
container setups which I've seen.   (Hmm, is whacky redundant here?)

I suppose can test whether or not the path is a block device or a
directory.....

> 
> For me this new function is the wors of all.
> 
> cold cache:
> real	0m2.115s
> user	0m0.040s
> sys	0m0.154s
> 
> second time:
> real	0m1.100s
> user	0m0.037s
> sys	0m0.122s
> 
> But that's because of blkid which is terribly slow for some reason.

I ran my test on a system with a NVMe SSD, and no HDD's attached.  I
just did an strace, and I see the util-linux folks have really done a
great job of pessimizing blkid.  :-(

My version used to just pull the information from the blkid cache file
and then verified the results, but it looks like the new, improved
util-linux version of blkid scans the /dev directory and opens and
reads from each device node, even when we're querying a single block
device.  <groan>

Even lsblk is *amazingly* inefficient in terms of the number of
useless file opens which it performs, although at least they are all
/sysfs files.

I'm half tempted to create and ship a binary which just calls
ext2fs_check_mount_point() and returns the value, since it's the most
efficient.  This command

sudo strace -o /tmp/st /build/e2fsprogs-maint/lib/ext2fs/tst_ismounted  /dev/lambda/tp

Opens /proc/mounts and does a trial mount of /dev/lambda/tp to make
sure it's actually busy, and that's it.

Sigh...

						- Ted

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Lukas Czerner]

On Thu, Mar 21, 2019 at 04:48:23PM -0400, Theodore Ts'o wrote:
> On Thu, Mar 21, 2019 at 09:17:10PM +0100, Lukas Czerner wrote:
> > 
> > Right, I did mention it later in the reply. It can be filtered
> > 
> > grep -v '^/dev/'
> 
> Well, that assumes all device nodes are in /dev.  Which is not
> necessarily always the case, especially in some of the more _whacky_
> container setups which I've seen.   (Hmm, is whacky redundant here?)

Fair enough, I've never seen this outside dm/lvm testing.

> 
> I suppose can test whether or not the path is a block device or a
> directory.....
> 
> > 
> > For me this new function is the wors of all.
> > 
> > cold cache:
> > real	0m2.115s
> > user	0m0.040s
> > sys	0m0.154s
> > 
> > second time:
> > real	0m1.100s
> > user	0m0.037s
> > sys	0m0.122s
> > 
> > But that's because of blkid which is terribly slow for some reason.
> 
> I ran my test on a system with a NVMe SSD, and no HDD's attached.  I
> just did an strace, and I see the util-linux folks have really done a
> great job of pessimizing blkid.  :-(

Yeah, all I have on that system is spinning rust :)
lsblk works good enough for me so I am not sure how I feel about special
binary to check the mount point :)

-Lukas

> 
> My version used to just pull the information from the blkid cache file
> and then verified the results, but it looks like the new, improved
> util-linux version of blkid scans the /dev directory and opens and
> reads from each device node, even when we're querying a single block
> device.  <groan>
> 
> Even lsblk is *amazingly* inefficient in terms of the number of
> useless file opens which it performs, although at least they are all
> /sysfs files.
> 
> I'm half tempted to create and ship a binary which just calls
> ext2fs_check_mount_point() and returns the value, since it's the most
> efficient.  This command
> 
> sudo strace -o /tmp/st /build/e2fsprogs-maint/lib/ext2fs/tst_ismounted  /dev/lambda/tp
> 
> Opens /proc/mounts and does a trial mount of /dev/lambda/tp to make
> sure it's actually busy, and that's it.
> 
> Sigh...
> 
> 						- Ted

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Theodore Ts'o]

OK, I've reworked the function to read:

ls_scan_targets() {
	lsblk -o NAME,MOUNTPOINT,FSTYPE -P -n -p \
	      $(lvs -o lv_path --noheadings -S "lv_active=active,lv_role=public,lv_role!=snapshot,vg_free>${snap_size_mb}") | \
	    grep FSTYPE=\"ext\[234\]\" | while read vars ; do
		eval "${vars}"

		if [ "${scrub_all}" -eq 1 ] || [ -n "${MOUNTPOINT}" ]; then
		    echo ${MOUNTPOINT:-${NAME}}
		fi
	done | sort | uniq
}

I think that's the final answer....

						- Ted

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Theodore Ts'o]

On Thu, Mar 21, 2019 at 06:04:40PM -0400, Theodore Ts'o wrote:
> OK, I've reworked the function to read:
> 
> ls_scan_targets() {
> 	lsblk -o NAME,MOUNTPOINT,FSTYPE -P -n -p \
> 	      $(lvs -o lv_path --noheadings -S "lv_active=active,lv_role=public,lv_role!=snapshot,vg_free>${snap_size_mb}") | \
> 	    grep FSTYPE=\"ext\[234\]\" | while read vars ; do
> 		eval "${vars}"
> 
> 		if [ "${scrub_all}" -eq 1 ] || [ -n "${MOUNTPOINT}" ]; then
> 		    echo ${MOUNTPOINT:-${NAME}}
> 		fi
> 	done | sort | uniq
> }
> 
> I think that's the final answer....

And I just saw your e-mail about dropping the sort and uniq calls.
OK, I'll take care of that too.

					- Ted
					

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Lukas Czerner]

On Thu, Mar 21, 2019 at 06:08:19PM -0400, Theodore Ts'o wrote:
> On Thu, Mar 21, 2019 at 06:04:40PM -0400, Theodore Ts'o wrote:
> > OK, I've reworked the function to read:
> > 
> > ls_scan_targets() {
> > 	lsblk -o NAME,MOUNTPOINT,FSTYPE -P -n -p \
> > 	      $(lvs -o lv_path --noheadings -S "lv_active=active,lv_role=public,lv_role!=snapshot,vg_free>${snap_size_mb}") | \
> > 	    grep FSTYPE=\"ext\[234\]\" | while read vars ; do
> > 		eval "${vars}"
> > 
> > 		if [ "${scrub_all}" -eq 1 ] || [ -n "${MOUNTPOINT}" ]; then
> > 		    echo ${MOUNTPOINT:-${NAME}}
> > 		fi
> > 	done | sort | uniq
> > }
> > 
> > I think that's the final answer....
> 
> And I just saw your e-mail about dropping the sort and uniq calls.
> OK, I'll take care of that too.
> 
> 					- Ted

Great, I like it and it runs very fast on my system.

cold cache
real	0m0.268s
user	0m0.011s
sys	0m0.036s

second run
real	0m0.053s
user	0m0.013s
sys	0m0.031s

Thanks for working on this.
-Lukas

Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

[Lukas Czerner]

On Thu, Mar 21, 2019 at 04:25:12PM -0400, Theodore Ts'o wrote:
> From: "Darrick J. Wong" <darrick.wong@oracle.com>
> 
> Paul Menzel reported that the e2scrub_all reaper service that runs at
> startup takes a long time to run, and Ted Ts'o pointed out that we could
> do a lot less work by using lvs as the outer loop in the ext4 filesystem
> probe function so that we only have to lsblk the lvm devices containing
> ext4 filesystems.
> 
> Therefore, refactor the loops to put lvs first, which should boost speed
> a bit.
> 
> [ Made some of the further optimizations suggested by Lukas Czerner.  -- TYT ]
> 
> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  scrub/e2scrub_all.in | 49 ++++++++++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
> index 4cb90a0de..81340b76f 100644
> --- a/scrub/e2scrub_all.in
> +++ b/scrub/e2scrub_all.in
> @@ -22,6 +22,7 @@ PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
>  
>  scrub_all=0
>  snap_size_mb=256
> +reap=0
>  conffile="@root_sysconfdir@/e2scrub.conf"
>  
>  test -f "${conffile}" && . "${conffile}"
> @@ -65,7 +66,7 @@ exitcode() {
>  while getopts "nrAV" opt; do
>  	case "${opt}" in
>  	"n") DBG="echo Would execute: " ;;
> -	"r") scrub_args="${scrub_args} -r";;
> +	"r") scrub_args="${scrub_args} -r"; reap=1;;
>  	"A") scrub_all=1;;
>  	"V") print_version; exitcode 0;;
>  	*) print_help; exitcode 2;;
> @@ -88,36 +89,34 @@ if ! type lvcreate >& /dev/null ; then
>  fi
>  
>  # Find scrub targets, make sure we only do this once.
> -ls_scrub_targets() {
> -	lsblk -o NAME,FSTYPE,MOUNTPOINT -p -P -n | while read vars; do
> -		eval "${vars}"
> -
> +ls_scan_targets() {
> +	for NAME in $(lvs -o lv_path --noheadings \
> +		   -S "lv_active=active,lv_role=public,lv_role!=snapshot,vg_free>${snap_size_mb}") ; do
>  		# Skip non-ext[234]
> -		case "${FSTYPE}" in
> +		case "$(blkid -o value -s TYPE ${NAME})" in

as I said in previous email for some reason blkid makes it terribly slow
for me.

	lsblk -o FSTYPE -n ${NAME}

lsblk makes is almost 9 times faster on my system. Not really sure what's
going on with blkid in my setup.

>  		ext[234])	;;
>  		*)		continue;;
>  		esac
>  
> -		# Skip unmounted filesystems unless -A
> -		if [ "${scrub_all}" -eq 0 ] && [ -z "${MOUNTPOINT}" ]; then
> -			continue;
> +		if [ "${scrub_all}" -eq 1 ] ||
> +		   [ -n "$(lsblk -o MOUNTPOINT --noheadings ${NAME})" ]; then
> +		    echo ${NAME}
>  		fi
> +	done | sort | uniq

It's already sorted and unique from lvs.

-Lukas

> +}
>  
> -		# Skip non-lvm devices and lvm snapshots
> -		lvm_vars="$(lvs --nameprefixes -o vg_name,lv_name,lv_role --noheadings "${NAME}" 2> /dev/null)"
> -		test $? -ne 0 && continue
> -		eval "${lvm_vars}"
> -		echo "${LVM2_LV_ROLE}" | grep -q "snapshot" && continue
> -
> -		free_space="$(vgs -o vg_free --units m --noheadings --no-suffix "${LVM2_VG_NAME}" 2> /dev/null | sed -e 's/\..*//')"
> -		test "${snap_size_mb}" -gt "${free_space}" && continue
> +# Find leftover scrub snapshots
> +ls_reap_targets() {
> +	lvs -o lv_path -S lv_role=snapshot -S lv_name=~\(e2scrub$\) --noheadings
> +}
>  
> -		if [ -n "${MOUNTPOINT}" ]; then
> -			echo "${MOUNTPOINT}"
> -		else
> -			echo "${NAME}"
> -		fi
> -	done | sort | uniq
> +# Figure out what we're targeting
> +ls_targets() {
> +	if [ "${reap}" -eq 1 ]; then
> +		ls_reap_targets
> +	else
> +		ls_scan_targets
> +	fi
>  }
>  
>  # systemd doesn't know to do path escaping on the instance variable we pass
> @@ -140,10 +139,10 @@ escape_path_for_systemd() {
>  
>  # Scrub any mounted fs on lvm by creating a snapshot and fscking that.
>  stdin="$(realpath /dev/stdin)"
> -ls_scrub_targets | while read tgt; do
> +ls_targets | while read tgt; do
>  	# If we're not reaping and systemd is present, try invoking the
>  	# systemd service.
> -	if [ -z "${scrub_args}" ] && type systemctl > /dev/null 2>&1; then
> +	if [ "${reap}" -ne 1 ] && type systemctl > /dev/null 2>&1; then
>  		tgt_esc="$(escape_path_for_systemd "${tgt}")"
>  		${DBG} systemctl start "e2scrub@${tgt_esc}" 2> /dev/null < "${stdin}"
>  		res=$?
> -- 
> 2.19.1
> 

[PATCH 8/9] e2scrub_all: refactor device probe loop

[Theodore Ts'o]

From: "Darrick J. Wong" <darrick.wong@oracle.com>

Paul Menzel reported that the e2scrub_all reaper service that runs at
startup takes a long time to run, and Ted Ts'o pointed out that we could
do a lot less work by using lvs as the outer loop in the ext4 filesystem
probe function so that we only have to lsblk the lvm devices containing
ext4 filesystems.

Therefore, refactor the loops to put lvs first, which should boost speed
a bit.

[ Made some of the further optimizations suggested by Lukas Czerner.  -- TYT ]

Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 scrub/e2scrub_all.in | 49 ++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
index 4cb90a0de..81340b76f 100644
--- a/scrub/e2scrub_all.in
+++ b/scrub/e2scrub_all.in
@@ -22,6 +22,7 @@ PATH=/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
 
 scrub_all=0
 snap_size_mb=256
+reap=0
 conffile="@root_sysconfdir@/e2scrub.conf"
 
 test -f "${conffile}" && . "${conffile}"
@@ -65,7 +66,7 @@ exitcode() {
 while getopts "nrAV" opt; do
 	case "${opt}" in
 	"n") DBG="echo Would execute: " ;;
-	"r") scrub_args="${scrub_args} -r";;
+	"r") scrub_args="${scrub_args} -r"; reap=1;;
 	"A") scrub_all=1;;
 	"V") print_version; exitcode 0;;
 	*) print_help; exitcode 2;;
@@ -88,36 +89,34 @@ if ! type lvcreate >& /dev/null ; then
 fi
 
 # Find scrub targets, make sure we only do this once.
-ls_scrub_targets() {
-	lsblk -o NAME,FSTYPE,MOUNTPOINT -p -P -n | while read vars; do
-		eval "${vars}"
-
+ls_scan_targets() {
+	for NAME in $(lvs -o lv_path --noheadings \
+		   -S "lv_active=active,lv_role=public,lv_role!=snapshot,vg_free>${snap_size_mb}") ; do
 		# Skip non-ext[234]
-		case "${FSTYPE}" in
+		case "$(blkid -o value -s TYPE ${NAME})" in
 		ext[234])	;;
 		*)		continue;;
 		esac
 
-		# Skip unmounted filesystems unless -A
-		if [ "${scrub_all}" -eq 0 ] && [ -z "${MOUNTPOINT}" ]; then
-			continue;
+		if [ "${scrub_all}" -eq 1 ] ||
+		   [ -n "$(lsblk -o MOUNTPOINT --noheadings ${NAME})" ]; then
+		    echo ${NAME}
 		fi
+	done | sort | uniq
+}
 
-		# Skip non-lvm devices and lvm snapshots
-		lvm_vars="$(lvs --nameprefixes -o vg_name,lv_name,lv_role --noheadings "${NAME}" 2> /dev/null)"
-		test $? -ne 0 && continue
-		eval "${lvm_vars}"
-		echo "${LVM2_LV_ROLE}" | grep -q "snapshot" && continue
-
-		free_space="$(vgs -o vg_free --units m --noheadings --no-suffix "${LVM2_VG_NAME}" 2> /dev/null | sed -e 's/\..*//')"
-		test "${snap_size_mb}" -gt "${free_space}" && continue
+# Find leftover scrub snapshots
+ls_reap_targets() {
+	lvs -o lv_path -S lv_role=snapshot -S lv_name=~\(e2scrub$\) --noheadings
+}
 
-		if [ -n "${MOUNTPOINT}" ]; then
-			echo "${MOUNTPOINT}"
-		else
-			echo "${NAME}"
-		fi
-	done | sort | uniq
+# Figure out what we're targeting
+ls_targets() {
+	if [ "${reap}" -eq 1 ]; then
+		ls_reap_targets
+	else
+		ls_scan_targets
+	fi
 }
 
 # systemd doesn't know to do path escaping on the instance variable we pass
@@ -140,10 +139,10 @@ escape_path_for_systemd() {
 
 # Scrub any mounted fs on lvm by creating a snapshot and fscking that.
 stdin="$(realpath /dev/stdin)"
-ls_scrub_targets | while read tgt; do
+ls_targets | while read tgt; do
 	# If we're not reaping and systemd is present, try invoking the
 	# systemd service.
-	if [ -z "${scrub_args}" ] && type systemctl > /dev/null 2>&1; then
+	if [ "${reap}" -ne 1 ] && type systemctl > /dev/null 2>&1; then
 		tgt_esc="$(escape_path_for_systemd "${tgt}")"
 		${DBG} systemctl start "e2scrub@${tgt_esc}" 2> /dev/null < "${stdin}"
 		res=$?
-- 
2.19.1