[8/9] e2scrub_all: refactor device probe loop
diff mbox series

Message ID 20190321020218.5154-8-tytso@mit.edu
State Accepted
Headers show
Series
  • [1/9] e2scrub: check to make sure lvm2 is installed
Related show

Commit Message

Theodore Y. Ts'o March 21, 2019, 2:02 a.m. UTC
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(-)

Comments

Darrick J. Wong March 21, 2019, 4:05 a.m. UTC | #1
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
>
Lukas Czerner March 21, 2019, 10:27 a.m. UTC | #2
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
>
Theodore Y. Ts'o March 21, 2019, 2:31 p.m. UTC | #3
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
Lukas Czerner March 21, 2019, 3:57 p.m. UTC | #4
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
Lukas Czerner March 21, 2019, 4:10 p.m. UTC | #5
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
>
Theodore Y. Ts'o March 21, 2019, 5:48 p.m. UTC | #6
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
Theodore Y. Ts'o March 21, 2019, 6:24 p.m. UTC | #7
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
Lukas Czerner March 21, 2019, 7:49 p.m. UTC | #8
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
Andreas Dilger March 21, 2019, 8:09 p.m. UTC | #9
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
Lukas Czerner March 21, 2019, 8:17 p.m. UTC | #10
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
Theodore Y. Ts'o March 21, 2019, 8:23 p.m. UTC | #11
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
Theodore Y. Ts'o March 21, 2019, 8:48 p.m. UTC | #12
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
Lukas Czerner March 21, 2019, 9:14 p.m. UTC | #13
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
Theodore Y. Ts'o March 21, 2019, 10:04 p.m. UTC | #14
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
Theodore Y. Ts'o March 21, 2019, 10:08 p.m. UTC | #15
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
Lukas Czerner March 22, 2019, 9:38 a.m. UTC | #16
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

Patch
diff mbox series

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=$?