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

[Lukas Czerner <lczerner@redhat.com>]

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