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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 21 Mar 2019 14:24:56 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     Lukas Czerner <lczerner@...hat.com>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        darrick.wong@...cle.com
Subject: Re: [PATCH 8/9] e2scrub_all: refactor device probe loop

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@...v-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