Message-ID: <20180309133155.xy6sfi3r5reh5reu@rh_laptop>
Date:   Fri, 9 Mar 2018 14:31:55 +0100
From:   Lukas Czerner <lczerner@...hat.com>
To:     "Darrick J. Wong" <darrick.wong@...cle.com>
Cc:     tytso@....edu, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 2/4] e2scrub: create online fsck tool of sorts

On Thu, Mar 08, 2018 at 12:13:27PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@...cle.com>
> 
> Implement online fsck for ext* filesystems which live on LVM-managed
> logical volumes.  The basic strategy mirrors that of e2croncheck --
> create a snapshot, fsck the snapshot, report whatever errors appear,
> remove snapshot.  Unlike e2croncheck, this utility accepts any LVM
> device path, knows about snapshots running out of space, and can call
> fstrim having validated that the fs metadata is ok.

Thanks Darrick this looks good, I have couple of comments below. Also
it's not a big deal, but I have a slight issue with the name e2scrub.
What we will call it if/when we have a real in-kernel fs scrub ?

> 
> Signed-off-by: Darrick J. Wong <darrick.wong@...cle.com>
> ---
>  MCONFIG.in             |    3 +
>  Makefile.in            |    3 +
>  configure              |  149 +++++++++++++++++++++++++++++++++++++++++---
>  configure.ac           |   56 +++++++++++++++--
>  debian/control         |    2 -
>  debian/e2fsprogs.files |    1 
>  scrub/Makefile.in      |   97 +++++++++++++++++++++++++++++
>  scrub/e2scrub.8.in     |   34 ++++++++++
>  scrub/e2scrub.conf.in  |   10 +++
>  scrub/e2scrub.in       |  162 ++++++++++++++++++++++++++++++++++++++++++++++++
>  scrub/e2scrub.rules.in |    2 +
>  util/subst.conf.in     |    3 +
>  12 files changed, 505 insertions(+), 17 deletions(-)
>  create mode 100644 scrub/Makefile.in
>  create mode 100644 scrub/e2scrub.8.in
>  create mode 100644 scrub/e2scrub.conf.in
>  create mode 100644 scrub/e2scrub.in
>  create mode 100644 scrub/e2scrub.rules.in
> 
> 
> diff --git a/MCONFIG.in b/MCONFIG.in
> index 22b74eb..adeb5bd 100644
> --- a/MCONFIG.in
> +++ b/MCONFIG.in
> @@ -33,6 +33,9 @@ infodir = @infodir@
>  datadir = @datadir@
>  pkgconfigdir = $(libdir)/pkgconfig
>  
> +HAVE_UDEV = @have_udev@
> +UDEV_RULES_DIR = @pkg_udev_rules_dir@
> +
>  @SET_MAKE@
>  
>  @ifGNUmake@ V =
> diff --git a/Makefile.in b/Makefile.in
> index 37b6069..ddd94ec 100644
> --- a/Makefile.in
> +++ b/Makefile.in
> @@ -13,10 +13,11 @@ INSTALL = @INSTALL@
>  @DEBUGFS_CMT@...UGFS_DIR= debugfs
>  @UUID_CMT@...D_LIB_SUBDIR= lib/uuid
>  @BLKID_CMT@...ID_LIB_SUBDIR= lib/blkid
> +@...CRUB_CMT@...CRUB_DIR= scrub
>  SUPPORT_LIB_SUBDIR= lib/support
>  
>  LIB_SUBDIRS=lib/et lib/ss lib/e2p $(UUID_LIB_SUBDIR) $(BLKID_LIB_SUBDIR) $(SUPPORT_LIB_SUBDIR) lib/ext2fs intl
> -PROG_SUBDIRS=e2fsck $(DEBUGFS_DIR) misc $(RESIZE_DIR) tests/progs po
> +PROG_SUBDIRS=e2fsck $(DEBUGFS_DIR) misc $(RESIZE_DIR) tests/progs po $(E2SCRUB_DIR)
>  SUBDIRS=util $(LIB_SUBDIRS) $(PROG_SUBDIRS) tests
>  
>  SUBS= util/subst.conf lib/config.h $(top_builddir)/lib/dirpaths.h \
> diff --git a/configure b/configure
> index b2701d2..986e057 100755
> --- a/configure
> +++ b/configure
> @@ -625,6 +625,10 @@ gl_use_threads_default=
>  ac_func_list=
>  ac_subst_vars='LTLIBOBJS
>  LIBOBJS
> +pkg_udev_rules_dir
> +have_udev
> +udev_LIBS
> +udev_CFLAGS
>  LDFLAGS_SHLIB
>  CFLAGS_STLIB
>  CFLAGS_SHLIB
> @@ -639,6 +643,7 @@ root_libdir
>  root_sbindir
>  root_bindir
>  root_prefix
> +E2SCRUB_CMT
>  UNIX_CMT
>  CYGWIN_CMT
>  LINUX_CMT
> @@ -795,6 +800,7 @@ build_os
>  build_vendor
>  build_cpu
>  build
> +E2FSPROGS_DATE
>  E2FSPROGS_PKGVER
>  E2FSPROGS_VERSION
>  E2FSPROGS_DAY
> @@ -892,6 +898,7 @@ with_included_gettext
>  with_libintl_prefix
>  enable_fuse2fs
>  with_multiarch
> +with_udev_rules_dir
>  '
>        ac_precious_vars='build_alias
>  host_alias
> @@ -904,7 +911,9 @@ CPPFLAGS
>  CPP
>  PKG_CONFIG
>  PKG_CONFIG_PATH
> -PKG_CONFIG_LIBDIR'
> +PKG_CONFIG_LIBDIR
> +udev_CFLAGS
> +udev_LIBS'
>  
>  
>  # Initialize some variables set by options.
> @@ -1580,6 +1589,8 @@ Optional Packages:
>    --with-libintl-prefix[=DIR]  search for libintl in DIR/include and DIR/lib
>    --without-libintl-prefix     don't search for libintl in includedir and libdir
>    --with-multiarch=ARCH specify the multiarch triplet
> +  --with-udev-rules-dir[=DIR]
> +                          Install udev rules into DIR.
>  
>  Some influential environment variables:
>    CC          C compiler command
> @@ -1595,6 +1606,8 @@ Some influential environment variables:
>                directories to add to pkg-config's search path
>    PKG_CONFIG_LIBDIR
>                path overriding pkg-config's built-in search path
> +  udev_CFLAGS C compiler flags for udev, overriding pkg-config
> +  udev_LIBS   linker flags for udev, overriding pkg-config
>  
>  Use these variables to override the choices made by `configure' or to help
>  it to find libraries and programs with nonstandard names/locations.
> @@ -2758,11 +2771,11 @@ MCONFIG=./MCONFIG
>  BINARY_TYPE=bin
>  E2FSPROGS_VERSION=`grep E2FSPROGS_VERSION ${srcdir}/version.h  \
>  	| awk '{print $3}' | tr \" " " | awk '{print $1}'`
> -DATE=`grep E2FSPROGS_DATE ${srcdir}/version.h | awk '{print $3}' \
> -	| tr \" " "`
> -E2FSPROGS_DAY=$(echo $DATE | awk -F- '{print $1}' | sed -e '/^[1-9]$/s/^/0/')
> -MONTH=`echo $DATE | awk -F- '{print $2}'`
> -YEAR=`echo $DATE | awk -F- '{print $3}'`
> +E2FSPROGS_DATE=`grep E2FSPROGS_DATE ${srcdir}/version.h | awk '{print $3}' \
> +	| tr \" " " | awk '{print $1}'`
> +E2FSPROGS_DAY=$(echo $E2FSPROGS_DATE | awk -F- '{print $1}' | sed -e '/^[1-9]$/s/^/0/')
> +MONTH=`echo $E2FSPROGS_DATE | awk -F- '{print $2}'`
> +YEAR=`echo $E2FSPROGS_DATE | awk -F- '{print $3}'`
>  
>  if expr $YEAR ">" 1900 > /dev/null ; then
>  	E2FSPROGS_YEAR=$YEAR
> @@ -2813,6 +2826,7 @@ $as_echo "Release date is ${E2FSPROGS_MONTH}, ${E2FSPROGS_YEAR}" >&6; }
>  
>  
>  
> +
>  WITH_DIET_LIBC=
>  
>  # Check whether --with-diet-libc was given.
> @@ -7236,8 +7250,6 @@ main ()
>      if (*(data + i) != *(data3 + i))
>        return 14;
>    close (fd);
> -  free (data);
> -  free (data3);
>    return 0;
>  }
>  _ACEOF
> @@ -13713,6 +13725,8 @@ esac
>  
>  
>  
> +E2SCRUB_CMT="$LINUX_CMT"
> +
>  case "$host_os" in
>  linux* | gnu* | k*bsd*-gnu)
>  	if test "$prefix" = NONE -a "$root_prefix" = NONE ; then
> @@ -13878,6 +13892,123 @@ LDFLAGS_SHLIB=${LDFLAGS_SHLIB:-$LDFLAGS}
>  
>  
>  
> +
> +
> +# Check whether --with-udev_rules_dir was given.
> +if test "${with_udev_rules_dir+set}" = set; then :
> +  withval=$with_udev_rules_dir;
> +else
> +  with_udev_rules_dir=yes
> +fi
> +
> +if test "x${with_udev_rules_dir}" != "xno"; then :
> +
> +	if test "x${with_udev_rules_dir}" = "xyes"; then :
> +
> +
> +pkg_failed=no
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for udev" >&5
> +$as_echo_n "checking for udev... " >&6; }
> +
> +if test -n "$udev_CFLAGS"; then
> +    pkg_cv_udev_CFLAGS="$udev_CFLAGS"
> + elif test -n "$PKG_CONFIG"; then
> +    if test -n "$PKG_CONFIG" && \
> +    { { $as_echo "$as_me:${as_lineno-$LINENO}: \$PKG_CONFIG --exists --print-errors \"udev\""; } >&5
> +  ($PKG_CONFIG --exists --print-errors "udev") 2>&5
> +  ac_status=$?
> +  $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
> +  test $ac_status = 0; }; then
> +  pkg_cv_udev_CFLAGS=`$PKG_CONFIG --cflags "udev" 2>/dev/null`
> +		      test "x$?" != "x0" && pkg_failed=yes
> +else
> +  pkg_failed=yes
> +fi
> + else
> +    pkg_failed=untried
> +fi
> +if test -n "$udev_LIBS"; then
> +    pkg_cv_udev_LIBS="$udev_LIBS"
> + elif test -n "$PKG_CONFIG"; then
> +    if test -n "$PKG_CONFIG" && \
> +    { { $as_echo "$as_me:${as_lineno-$LINENO}: \$PKG_CONFIG --exists --print-errors \"udev\""; } >&5
> +  ($PKG_CONFIG --exists --print-errors "udev") 2>&5
> +  ac_status=$?
> +  $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
> +  test $ac_status = 0; }; then
> +  pkg_cv_udev_LIBS=`$PKG_CONFIG --libs "udev" 2>/dev/null`
> +		      test "x$?" != "x0" && pkg_failed=yes
> +else
> +  pkg_failed=yes
> +fi
> + else
> +    pkg_failed=untried
> +fi
> +
> +
> +
> +if test $pkg_failed = yes; then
> +   	{ $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
> +$as_echo "no" >&6; }
> +
> +if $PKG_CONFIG --atleast-pkgconfig-version 0.20; then
> +        _pkg_short_errors_supported=yes
> +else
> +        _pkg_short_errors_supported=no
> +fi
> +        if test $_pkg_short_errors_supported = yes; then
> +	        udev_PKG_ERRORS=`$PKG_CONFIG --short-errors --print-errors --cflags --libs "udev" 2>&1`
> +        else
> +	        udev_PKG_ERRORS=`$PKG_CONFIG --print-errors --cflags --libs "udev" 2>&1`
> +        fi
> +	# Put the nasty error message in config.log where it belongs
> +	echo "$udev_PKG_ERRORS" >&5
> +
> +
> +			with_udev_rules_dir=""
> +
> +elif test $pkg_failed = untried; then
> +     	{ $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
> +$as_echo "no" >&6; }
> +
> +			with_udev_rules_dir=""
> +
> +else
> +	udev_CFLAGS=$pkg_cv_udev_CFLAGS
> +	udev_LIBS=$pkg_cv_udev_LIBS
> +        { $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5
> +$as_echo "yes" >&6; }
> +
> +			with_udev_rules_dir="$($PKG_CONFIG --variable=udevdir udev)/rules.d"
> +
> +fi
> +
> +fi
> +	{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for udev rules dir" >&5
> +$as_echo_n "checking for udev rules dir... " >&6; }
> +	pkg_udev_rules_dir="${with_udev_rules_dir}"
> +	if test -n "${pkg_udev_rules_dir}"; then :
> +
> +		{ $as_echo "$as_me:${as_lineno-$LINENO}: result: ${pkg_udev_rules_dir}" >&5
> +$as_echo "${pkg_udev_rules_dir}" >&6; }
> +		have_udev="yes"
> +
> +else
> +
> +		{ $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
> +$as_echo "no" >&6; }
> +		have_udev="no"
> +
> +fi
> +
> +else
> +
> +	have_udev="disabled"
> +
> +fi
> +
> +
> +
>  test -d lib || mkdir lib
>  test -d include || mkdir include
>  test -d include/linux || mkdir include/linux
> @@ -13899,7 +14030,7 @@ for i in MCONFIG Makefile e2fsprogs.spec \
>  	misc/Makefile ext2ed/Makefile e2fsck/Makefile \
>  	debugfs/Makefile tests/Makefile tests/progs/Makefile \
>  	resize/Makefile doc/Makefile intl/Makefile \
> -	intl/libgnuintl.h po/Makefile.in ; do
> +	intl/libgnuintl.h po/Makefile.in scrub/Makefile; do
>  	if test -d `dirname ${srcdir}/$i` ; then
>  		outlist="$outlist $i"
>  	fi
> diff --git a/configure.ac b/configure.ac
> index 7392959..6e549c5 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -11,11 +11,11 @@ dnl This is to figure out the version number and the date....
>  dnl
>  E2FSPROGS_VERSION=`grep E2FSPROGS_VERSION ${srcdir}/version.h  \
>  	| awk '{print $3}' | tr \" " " | awk '{print $1}'`
> -DATE=`grep E2FSPROGS_DATE ${srcdir}/version.h | awk '{print $3}' \
> -	| tr \" " "`
> -E2FSPROGS_DAY=$(echo $DATE | awk -F- '{print $1}' | sed -e '/^[[1-9]]$/s/^/0/')
> -MONTH=`echo $DATE | awk -F- '{print $2}'`
> -YEAR=`echo $DATE | awk -F- '{print $3}'`
> +E2FSPROGS_DATE=`grep E2FSPROGS_DATE ${srcdir}/version.h | awk '{print $3}' \
> +	| tr \" " " | awk '{print $1}'`
> +E2FSPROGS_DAY=$(echo $E2FSPROGS_DATE | awk -F- '{print $1}' | sed -e '/^[[1-9]]$/s/^/0/')
> +MONTH=`echo $E2FSPROGS_DATE | awk -F- '{print $2}'`
> +YEAR=`echo $E2FSPROGS_DATE | awk -F- '{print $3}'`
>  
>  if expr $YEAR ">" 1900 > /dev/null ; then
>  	E2FSPROGS_YEAR=$YEAR
> @@ -63,6 +63,7 @@ AC_SUBST(E2FSPROGS_MONTH)
>  AC_SUBST(E2FSPROGS_DAY)
>  AC_SUBST(E2FSPROGS_VERSION)
>  AC_SUBST(E2FSPROGS_PKGVER)
> +AC_SUBST(E2FSPROGS_DATE)
>  dnl
>  dnl Use diet libc
>  dnl 
> @@ -1312,6 +1313,11 @@ AC_SUBST(LINUX_CMT)
>  AC_SUBST(CYGWIN_CMT)
>  AC_SUBST(UNIX_CMT)
>  dnl
> +dnl e2scrub only builds on linux
> +dnl
> +E2SCRUB_CMT="$LINUX_CMT"
> +AC_SUBST(E2SCRUB_CMT)
> +dnl
>  dnl Linux and Hurd places root files in the / by default
>  dnl
>  case "$host_os" in
> @@ -1469,6 +1475,44 @@ LDFLAGS_SHLIB=${LDFLAGS_SHLIB:-$LDFLAGS}
>  AC_SUBST(CFLAGS_SHLIB)
>  AC_SUBST(CFLAGS_STLIB)
>  AC_SUBST(LDFLAGS_SHLIB)
> +
> +dnl
> +dnl Where do udev rules go?
> +dnl
> +AC_ARG_WITH([udev_rules_dir],
> +  [AS_HELP_STRING([--with-udev-rules-dir@<:@=DIR@:>@],
> +	[Install udev rules into DIR.])],
> +  [],
> +  [with_udev_rules_dir=yes])
> +AS_IF([test "x${with_udev_rules_dir}" != "xno"],
> +  [
> +	AS_IF([test "x${with_udev_rules_dir}" = "xyes"],
> +	  [
> +		PKG_CHECK_MODULES([udev], [udev],
> +		  [
> +			with_udev_rules_dir="$($PKG_CONFIG --variable=udevdir udev)/rules.d"
> +		  ], [
> +			with_udev_rules_dir=""
> +		  ])
> +	  ])
> +	AC_MSG_CHECKING([for udev rules dir])
> +	pkg_udev_rules_dir="${with_udev_rules_dir}"
> +	AS_IF([test -n "${pkg_udev_rules_dir}"],
> +	  [
> +		AC_MSG_RESULT(${pkg_udev_rules_dir})
> +		have_udev="yes"
> +	  ],
> +	  [
> +		AC_MSG_RESULT(no)
> +		have_udev="no"
> +	  ])
> +  ],
> +  [
> +	have_udev="disabled"
> +  ])
> +AC_SUBST(have_udev)
> +AC_SUBST(pkg_udev_rules_dir)
> +
>  dnl
>  dnl Make our output files, being sure that we create the some miscellaneous 
>  dnl directories
> @@ -1494,7 +1538,7 @@ for i in MCONFIG Makefile e2fsprogs.spec \
>  	misc/Makefile ext2ed/Makefile e2fsck/Makefile \
>  	debugfs/Makefile tests/Makefile tests/progs/Makefile \
>  	resize/Makefile doc/Makefile intl/Makefile \
> -	intl/libgnuintl.h po/Makefile.in ; do
> +	intl/libgnuintl.h po/Makefile.in scrub/Makefile; do
>  	if test -d `dirname ${srcdir}/$i` ; then
>  		outlist="$outlist $i"
>  	fi
> diff --git a/debian/control b/debian/control
> index ee0f66b..3515d3d 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -189,7 +189,7 @@ Essential: yes
>  Pre-Depends: ${shlibs:Depends}, ${misc:Depends}, libblkid1, libuuid1
>  Multi-Arch: foreign
>  Suggests: gpart, parted, fuse2fs, e2fsck-static
> -Recommends: e2fsprogs-l10n
> +Recommends: e2fsprogs-l10n, lvm2, util-linux, coreutils
>  Architecture: any
>  Description: ext2/ext3/ext4 file system utilities
>   The ext2, ext3 and ext4 file systems are successors of the original ext
> diff --git a/debian/e2fsprogs.files b/debian/e2fsprogs.files
> index 0a22f31..78720fe 100644
> --- a/debian/e2fsprogs.files
> +++ b/debian/e2fsprogs.files
> @@ -3,3 +3,4 @@ usr/bin
>  usr/sbin
>  usr/share/man
>  etc
> +lib/udev/rules.d
> diff --git a/scrub/Makefile.in b/scrub/Makefile.in
> new file mode 100644
> index 0000000..a8bb06b
> --- /dev/null
> +++ b/scrub/Makefile.in
> @@ -0,0 +1,97 @@
> +#
> +# Makefile for e2scrub
> +#
> +
> +srcdir = @srcdir@
> +top_srcdir = @top_srcdir@
> +VPATH = @srcdir@
> +top_builddir = ..
> +my_dir = scrub
> +INSTALL = @INSTALL@
> +
> +@...NFIG@
> +
> +PROGS=		e2scrub
> +MANPAGES=	e2scrub.8
> +CONFFILES=	e2scrub.conf
> +
> +ifeq ($(HAVE_UDEV),yes)
> +UDEV_RULES	= e2scrub.rules
> +INSTALLDIRS_TGT	+= installdirs-udev
> +INSTALL_TGT	+= install-udev
> +UNINSTALL_TGT	+= uninstall-udev
> +endif
> +
> +all:: $(PROGS) $(MANPAGES) $(CONFFILES) $(UDEV_RULES)
> +
> +e2scrub: $(DEP_SUBSTITUTE) e2scrub.in
> +	$(E) "	SUBST $@"
> +	$(Q) $(SUBSTITUTE_UPTIME) $(srcdir)/e2scrub.in $@
> +	$(Q) chmod a+x $@
> +
> +%.8: %.8.in $(DEP_SUBSTITUTE)
> +	$(E) "	SUBST $@"
> +	$(Q) $(SUBSTITUTE_UPTIME) $< $@
> +
> +%.conf: %.conf.in $(DEP_SUBSTITUTE)
> +	$(E) "	SUBST $@"
> +	$(Q) $(SUBSTITUTE_UPTIME) $< $@
> +
> +%.rules: %.rules.in $(DEP_SUBSTITUTE)
> +	$(E) "	SUBST $@"
> +	$(Q) $(SUBSTITUTE_UPTIME) $< $@
> +
> +installdirs-udev:
> +	$(E) "	MKINSTALLDIRS $(UDEV_RULES_DIR)"
> +	$(Q) $(MKINSTALLDIRS) $(DESTDIR)$(UDEV_RULES_DIR)
> +
> +installdirs: $(INSTALLDIRS_TGT)
> +	$(E) "	MKINSTALLDIRS $(root_sbindir) $(man8dir) $(root_sysconfdir)"
> +	$(Q) $(MKINSTALLDIRS) $(DESTDIR)$(root_sbindir) \
> +		$(DESTDIR)$(man8dir) $(DESTDIR)$(root_sysconfdir)
> +
> +install-udev:
> +	$(Q) for i in $(UDEV_RULES); do \
> +		$(ES) "	INSTALL $(UDEV_RULES_DIR)/$$i"; \
> +		$(INSTALL_PROGRAM) $$i $(DESTDIR)$(UDEV_RULES_DIR)/96-$$i; \
> +	done
> +
> +install: $(PROGS) $(MANPAGES) $(FMANPAGES) installdirs $(INSTALL_TGT)
> +	$(Q) for i in $(PROGS); do \
> +		$(ES) "	INSTALL $(root_sbindir)/$$i"; \
> +		$(INSTALL_PROGRAM) $$i $(DESTDIR)$(root_sbindir)/$$i; \
> +	done
> +	$(Q) for i in $(MANPAGES); do \
> +		for j in $(COMPRESS_EXT); do \
> +			$(RM) -f $(DESTDIR)$(man8dir)/$$i.$$j; \
> +		done; \
> +		$(ES) "	INSTALL_DATA $(man8dir)/$$i"; \
> +		$(INSTALL_DATA) $$i $(DESTDIR)$(man8dir)/$$i; \
> +	done
> +	$(Q) for i in $(CONFFILES); do \
> +		$(ES) "	INSTALL_DATA $(root_sysconfdir)/$$i"; \
> +		$(INSTALL_DATA) $$i $(DESTDIR)$(root_sysconfdir)/$$i; \
> +	done
> +
> +uninstall-udev:
> +	for i in $(UDEV_RULES); do \
> +		$(RM) -f $(DESTDIR)$(UDEV_RULES_DIR)/96-$$i; \
> +	done
> +
> +uninstall: $(UNINSTALL_TGT)
> +	for i in $(PROGS); do \
> +		$(RM) -f $(DESTDIR)$(root_sbindir)/$$i; \
> +	done
> +	for i in $(MANPAGES); do \
> +		$(RM) -f $(DESTDIR)$(man8dir)/$$i; \
> +	done
> +	for i in $(CONFFILES); do \
> +		$(RM) -f $(DESTDIR)$(root_sysconfdir)/$$i; \
> +	done
> +
> +clean::
> +	$(RM) -f $(PROGS)
> +
> +mostlyclean: clean
> +distclean: clean
> +	$(RM) -f .depend Makefile $(srcdir)/TAGS $(srcdir)/Makefile.in.old
> diff --git a/scrub/e2scrub.8.in b/scrub/e2scrub.8.in
> new file mode 100644
> index 0000000..7cb6dfb
> --- /dev/null
> +++ b/scrub/e2scrub.8.in
> @@ -0,0 +1,34 @@
> +.TH E2SCRUB 8 "@E2FSPROGS_MONTH@ @E2FSPROGS_YEAR@" "E2fsprogs version @E2FSPROGS_VERSION@"
> +.SH NAME
> +e2scrub - check a mounted ext2/ext3/ext4 file system on an LVM volume for errors.
> +.SH SYNOPSYS
> +.B
> +e2scrub [OPTION] [LVM DEVICE PATH]
> +.SH DESCRIPTION
> +Given a live file system on a LVM volume, this program snapshots the
> +logical volume and runs a file system check to look for serious errors.
> +If no errors are found, fstrim can be called on the mounted file system.
> +However, if errors are found, the file system should be unmounted and
> +fixed.

e2scrub will _not_ fix you file system, I think we should make this part
really clear.

I am missing some more information on what are the requirements for this to
work - like free space in the vg.

How about few words about disaster recovery, I know you're trying to
always remove the snapshot, but what if it fails for whatever reason ? I
think there are things users should know about so that they can help
themselves.

Also in case this is run on dm-thin lv it's worth mentioning that lvm
will use the "old" snaphost instead if the thin snapshot. However this
is something we can recognize and use the right command.

> +.SH OPTIONS
> +.TP
> +\fB-t\fR
> +Run
> +.B
> +fstrim(1)
> +on the mounted filesystem if no errors are found.
> +.TP
> +\fB-V\fR
> +Print version information and exit.
> +.SH EXIT CODE
> +The exit codes are the same as in
> +.BR e2fsck (8)
> +.SH BUGS
> +This utility is capable of checking any ext* filesystem on an LVM volume,
> +regardless of whether it is mounted.

If it's not mounted it'll still use lvm snapshots right ? It's not
necessarily clear from the documentation.

> +.SH SEE ALSO
> +.BR e2fsck (8)
> +.SH AUTHOR
> +Darrick J. Wong <darrick.wong@...cle.com>
> +.SH COPYRIGHT
> +Copyright ©2018 Darrick J. Wong.  License is GPLv2+. <http://www.gnu.org/licenses/gpl-2.0.html>
> diff --git a/scrub/e2scrub.conf.in b/scrub/e2scrub.conf.in
> new file mode 100644
> index 0000000..fec828a
> --- /dev/null
> +++ b/scrub/e2scrub.conf.in
> @@ -0,0 +1,10 @@
> +# e2scrub configuration file
> +
> +# Snapshots will be created to run fsck; the snapshot will be of this size.
> +# snap_size_mb=256

Do you have any idea what it takes to fill this up ? What kind of
workload for how long ? Why do you think it's a sane default ?

I am well aware that there is no right answer I am just curious.

> +
> +# Set this to 1 to enable fstrim for everyone
> +# fstrim=0
> +
> +# Arguments passed into e2fsck
> +# e2fsck_opts="-vtt"
> diff --git a/scrub/e2scrub.in b/scrub/e2scrub.in
> new file mode 100644
> index 0000000..e26b449
> --- /dev/null
> +++ b/scrub/e2scrub.in
> @@ -0,0 +1,162 @@
> +#!/bin/bash
> +
> +#  Copyright (C) 2018 Oracle.  All Rights Reserved.
> +#
> +#  Author: Darrick J. Wong <darrick.wong@...cle.com>
> +#
> +#  This program is free software; you can redistribute it and/or
> +#  modify it under the terms of the GNU General Public License
> +#  as published by the Free Software Foundation; either version 2
> +#  of the License, or (at your option) any later version.
> +#
> +#  This program is distributed in the hope that it would be useful,
> +#  but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +#  GNU General Public License for more details.
> +#
> +#  You should have received a copy of the GNU General Public License
> +#  along with this program; if not, write the Free Software Foundation,
> +#  Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
> +
> +# Automatically check a LVM-managed filesystem online.
> +# We use lvm snapshots to do this, which means that we can only
> +# check filesystems in VGs that have at least 256mb (or so) of
> +# free space.
> +
> +snap_size_mb=256
> +fstrim=0
> +e2fsck_opts=""
> +conffile="@root_sysconfdir@...scrub.conf"
> +
> +test -f "${conffile}" && . "${conffile}"
> +
> +print_help() {
> +	echo "Usage: $0 [-t] device"
> +	echo
> +	echo "device must be a LVM-managed block device"
> +	echo "-t: Run fstrim if successful."
> +}
> +
> +print_version() {
> +	echo "e2scrub @E2FSPROGS_VERSION@ (@E2FSPROGS_DATE@)"
> +}
> +
> +while getopts "tV" opt; do
> +	case "${opt}" in
> +	"t") fstrim=1;;
> +	"V") print_version; exit 0;;
> +	*) print_help; exit 2;;
> +	esac
> +done
> +shift "$((OPTIND - 1))"
> +
> +dev="$1"
> +if [ -z "${dev}" ]; then
> +	print_help
> +	exit 1
> +elif [ ! -b "${dev}" ]; then
> +	echo "${dev}: Not a block device?"
> +	print_help
> +	exit 16
> +fi
> +
> +# Make sure this is an LVM device we can snapshot
> +vg="$(lvs --noheadings -o vg_name "${dev}" 2> /dev/null | sed -e 's/^  //g')"
> +lv="$(lvs --noheadings -o lv_name "${dev}" 2> /dev/null | sed -e 's/^  //g')"

You could use

eval $(lvs --nameprefixes -o name,vgname --noheadings "${dev}")

and have lvm set up $LVM2_VG_NAME and $LVM2_LV_NAME for you

> +if [ -z "${vg}" ] || [ -z "${lv}" ]; then
> +	echo "${dev}: Not a LVM device."
> +	exit 16
> +fi
> +start_time="$(date +'%Y%m%d%H%M%S')"
> +snap="${lv}.e2scrub"

Let's hope people are not creating these intentionally. Maybe using a
timestamp might be worthwhile ?

> +snap_dev="/dev/${vg}/${snap}"
> +fstype="$(blkid -p -s TYPE "${dev}" | sed -e 's/^.*TYPE="\(.*\)".*$/\1/g')"

blkid -o value -p -s TYPE "${dev}"

> +
> +case "${fstype}" in
> +"ext2"|"ext3"|"ext4")

ext[234]

> +	;;
> +*)
> +	echo "${dev}: Filesystem of type ${fstype} not supported."
> +	exit 16
> +	;;
> +esac
> +
> +teardown() {
> +	# Remove and wait for removal to succeed.
> +	lvremove -f "${vg}/${snap}" 3>&-
> +	while [ -b "${snap_dev}" ] && [ "$?" -eq "5" ]; do
> +		sleep 0.5
> +		lvremove -f "${vg}/${snap}" 3>&-
> +	done
> +}
> +
> +check() {
> +	# First we recover the journal, then we see if e2fsck tries any
> +	# non-optimization repairs.  If either of these two returns a
> +	# non-zero status (errors fixed or remaining) then this fs is bad.
> +	E2FSCK_FIXES_ONLY=1
> +	export E2FSCK_FIXES_ONLY
> +	${DBG} "@root_sbindir@...fsck" -E journal_only -p ${e2fsck_opts} "${snap_dev}" || return 1
> +	${DBG} "@root_sbindir@...fsck" -fy ${e2fsck_opts} "${snap_dev}" || return 1

Why to use -y ? Wouldn't it be better to just run with -p ?

> +	return 0
> +}
> +
> +mark_clean() {
> +	${DBG} "@root_sbindir@...ne2fs" -C 0 -T "${start_time}" "${dev}"

I am not sure about -C 0. I understand that it's there to elliminate the
mount-count based fsck, but it also means the mount-count is not going
to be reliable, not sure if that matters much.

> +}
> +
> +mark_corrupt() {
> +	${DBG} "@root_sbindir@...ne2fs" -E force_fsck "${dev}"
> +}
> +
> +setup() {
> +	# Create the snapshot, wait for device to appear
> +	teardown > /dev/null 2> /dev/null
> +	lvcreate -s -L "${snap_size_mb}m" -n "${snap}" "${vg}/${lv}" 3>&-
> +	test $? -ne 0 && return 1
> +	udevadm settle 2> /dev/null
> +	return 0
> +}
> +
> +trap "teardown; exit 1" EXIT INT QUIT TERM
> +if ! setup; then
> +	echo "Snapshot of ${dev} FAILED, will not check!"
> +	exit 1
> +fi
> +
> +# Check and react
> +if check; then
> +	echo "Scrub of ${dev} succeeded."
> +	mark_clean
> +
> +	if [ "${fstrim}" -eq 1 ] && type fstrim > /dev/null 2>&1; then
> +		dir="$(lsblk -o MOUNTPOINT -n "${dev}")"
> +		if [ -d "${dir}" ]; then
> +			# NB: fstrim fails with snapshot present
> +			trap '' EXIT
> +			teardown
> +			fstrim -v "${dir}"

This can take a long time, how about some information for the user what
we're doing ?

> +		fi
> +	fi
> +
> +	ret=0
> +else
> +	# fsck failed.  Check if the snapshot is invalid; if so, make a
> +	# note of that at the end of the log.  This isn't necessarily a
> +	# failure because the mounted fs could have overflowed the
> +	# snapshot with regular disk writes /or/ our repair process
> +	# could have done it by repairing too much.
> +	#
> +	# If it's really corrupt we ought to fsck at next boot.
> +	is_invalid="$(lvs -o lv_snapshot_invalid --noheadings "${snap_dev}" | awk '{print $1}')"
> +	if [ -n "${is_invalid}" ]; then
> +		echo "Scrub of ${dev} FAILED due to invalid snapshot."
> +		ret=8
> +	else
> +		echo "Scrub of ${dev} FAILED!  Reboot soon to fsck."
> +		mark_corrupt
> +		ret=6
> +	fi
> +fi
> +
> +exit "${ret}"
> diff --git a/scrub/e2scrub.rules.in b/scrub/e2scrub.rules.in
> new file mode 100644
> index 0000000..5e1b35b
> --- /dev/null
> +++ b/scrub/e2scrub.rules.in
> @@ -0,0 +1,2 @@
> +# Try to hide our fsck snapshots from udev's /dev/disk linking...
> +ACTION=="add|change", ENV{DM_LV_NAME}=="*.e2scrub", OPTIONS="link_priority=-100"
> diff --git a/util/subst.conf.in b/util/subst.conf.in
> index fbc044d..6bf658d 100644
> --- a/util/subst.conf.in
> +++ b/util/subst.conf.in
> @@ -2,6 +2,7 @@ AWK			@AWK@
>  SED			@SED@
>  E2FSPROGS_MONTH		@E2FSPROGS_MONTH@
>  E2FSPROGS_YEAR		@E2FSPROGS_YEAR@
> +E2FSPROGS_DATE		@E2FSPROGS_DATE@
>  E2FSPROGS_VERSION	@E2FSPROGS_VERSION@
>  SIZEOF_LONG_LONG	@SIZEOF_LONG_LONG@
>  SIZEOF_LONG		@SIZEOF_LONG@
> @@ -18,3 +19,5 @@ $prefix			@prefix@
>  JDEV			
>  # Enable the documentation for the tdb profile in e2fsck.conf's man page
>  TDB_MAN_COMMENT		@TDB_MAN_COMMENT@
> +root_sbindir		@root_sbindir@
> +root_bindir		@root_bindir@
> 

Powered by blists - more mailing lists