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

[Lukas Czerner <lczerner@redhat.com>]

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