Message-ID: <20180309233712.GC4856@magnolia>
Date:   Fri, 9 Mar 2018 15:37:12 -0800
From:   "Darrick J. Wong" <darrick.wong@...cle.com>
To:     Lukas Czerner <lczerner@...hat.com>
Cc:     tytso@....edu, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 2/4] e2scrub: create online fsck tool of sorts

On Fri, Mar 09, 2018 at 02:31:55PM +0100, Lukas Czerner wrote:
> 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 ?

I'd keep the e2scrub name and have it call the kernel instead of mucking
around with lv snapshots.

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

Ok.  You're right, sometimes I forget which one does what. :/

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

Ok.

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

That sounds like a good idea, but I think I'd prefer to leave that to
people who are more familar with dealing with dm-thin volumes.  AFAICT
it looks like we just change the options we hand to lvcreate, but I have
little experience with dm-thin management.

> > +.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.

Yes.  This is not really a bug either, so I'll just change the
description to say that it checks filesystems, not necessarily mounted
ones.

> > +.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 too have been running various versions of this script for years on all
of my computers and so far it mostly hasn't been an issue.

In the current iteration of the script it'll detect that the snapshot
went invalid and not flag that as a failure.

> 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

Hmm, good idea.


> 
> > +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 ?

I'd rather stick with the ${lv}.e2scrub name so that we have an
easy and predictable name to use in teardown.

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

Thank you.

> > +
> > +case "${fstype}" in
> > +"ext2"|"ext3"|"ext4")
> 
> ext[234]

Fixed.

> > +	;;
> > +*)
> > +	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 ?

There are a few problems (the ones with PR_PREEN_NO) that e2fsck won't
bother asking about unless we -y, so I pass -y to make sure we get
everything.

(The E2FSCK_FIXES_ONLY flag prevents e2fsck -fy from triggering on
purely optimization things.)

> > +	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.

As Andreas said, it's to get around the mount count / mount time checks.

> > +}
> > +
> > +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 ?

Ok.  The e2fsck itself is pretty chatty so adding another line won't
make much of a difference.

Thanks for the review!

--D

> > +		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