[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <254C3560-67C3-4A7A-A1AC-3A04ED7828AB@dilger.ca>
Date: Thu, 1 Mar 2018 15:42:52 -0700
From: Andreas Dilger <adilger@...ger.ca>
To: "Darrick J. Wong" <darrick.wong@...cle.com>
Cc: tytso@....edu, djwong@...nel.org, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 2/7] e2scrub: create a script to scrub all ext*
filesystems
On Mar 1, 2018, at 11:23 AM, Darrick J. Wong <darrick.wong@...cle.com> wrote:
>
> From: Darrick J. Wong <darrick.wong@...cle.com>
>
> Create an e2scrub_all command to find all ext* filesystems
> and run an online scrub against them all.
Is fstrim a no-op on HDD and SSD devices that don't support it?
> diff --git a/scrub/e2scrub_all.8.in b/scrub/e2scrub_all.8.in
> new file mode 100644
> index 0000000..a57d4aa
> --- /dev/null
> +++ b/scrub/e2scrub_all.8.in
> @@ -0,0 +1,14 @@
> +.TH E2SCRUB 8 "@E2FSPROGS_MONTH@ @E2FSPROGS_YEAR@" "E2fsprogs version @E2FSPROGS_VERSION@"
> +.SH NAME
> +e2scrub_all - Check all mounted btrfs/ext4/XFS LVM file systems for errors.
Does it actually check btrfs/XFS filesystems? I'd guess not.
> diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
> new file mode 100644
> index 0000000..ff9eb8f
> --- /dev/null
> +++ b/scrub/e2scrub_all.in
> @@ -0,0 +1,57 @@
> +#!/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.
> +
> +# Check all ext[234] filesystems mounted on LVM
> +types="ext2,ext3,ext4"
> +
> +exitcode() {
> + ret="$1"
> +
> + exit "${ret}"
> +}
I don't understand the benefit of this at all... Why not "exit" directly?
> +prog_path() {
> + path="$1"
> + displayname="$2"
> +
> + if ! type -P "${path}" && [ -n "${displayname}" ]; then
> + echo "${displayname}: Command not found."
> + exitcode 8
> + fi
> +}
> +
> +LVS_PROG="$(prog_path "@root_sbindir@...s" "lvs")"
> +BLKID_PROG="$(prog_path "@root_sbindir@...kid" "blkid")"
This is confusing. You are forcing the path to be one thing, then failing if
the command is not at the fully-specified path. Why not just use:
PATH=/bin:/sbin:/usr/bin:/usr/sbin
lvs ...
blkid ...
and then no need to make the script ugly by using $LVS_PROG and $BLKID_PROG
everywhere, secondly it will work regardless of where the programs are installed
(for any sane system), and finally no need to subst every one of your scripts.
Cheers, Andreas
> +
> +# Scrub any fs on lvm by creating a snapshot and fscking that.
> +"${LVS_PROG}" -o vg_name,lv_name,lv_role --noheadings 2> /dev/null | while read vg lv role extra; do
> + # parsing error?
> + test -n "${extra}" && continue
> + # Skip snapshots
> + echo "${role}" | grep -q "snapshot" && continue
> +
> + dev="/dev/${vg}/${lv}"
> + # Skip non-ext[234]
> + "${BLKID_PROG}" -p -n "${types}" "${dev}" > /dev/null 2>&1 || continue
> +
> + ${DBG} "@root_sbindir@...scrub" "${dev}"
> +done
> +
> +exitcode 0
>
Cheers, Andreas
Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)
Powered by blists - more mailing lists