From: Brian Foster <bfoster@redhat.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: eguan@redhat.com, linux-xfs@vger.kernel.org, fstests@vger.kernel.org
Subject: Re: [PATCH 11/8] common/inject: refactor helpers to use new errortag interface
Date: Thu, 6 Jul 2017 06:07:50 -0400 [thread overview]
Message-ID: <20170706100749.GA56359@bfoster.bfoster> (raw)
In-Reply-To: <20170705210501.GA6331@magnolia>
On Wed, Jul 05, 2017 at 02:05:01PM -0700, Darrick J. Wong wrote:
> On Wed, Jul 05, 2017 at 12:45:35PM -0400, Brian Foster wrote:
> > On Wed, Jul 05, 2017 at 09:29:14AM -0700, Darrick J. Wong wrote:
> > > On Wed, Jul 05, 2017 at 08:16:11AM -0400, Brian Foster wrote:
> > > > On Thu, Jun 29, 2017 at 09:13:08PM -0700, Darrick J. Wong wrote:
> > > > > Refactor the XFS error injection helpers to use the new errortag
> > > > > interface to configure error injection. If that isn't present, fall
> > > > > back either to the xfs_io/ioctl based injection or the older sysfs
> > > > > knobs. Refactor existing testcases to use the new helpers.
> > > > >
> > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > ---
> > > > > common/inject | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > > > > tests/xfs/051 | 7 ++++--
> > > > > tests/xfs/141 | 5 +++--
> > > > > tests/xfs/196 | 17 ++++++----------
> > > > > 4 files changed, 73 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/common/inject b/common/inject
> > > > > index 8ecc290..1f12a2b 100644
> > > > > --- a/common/inject
> > > > > +++ b/common/inject
> > > > > @@ -35,10 +35,50 @@ _require_error_injection()
> > > > > esac
> > > > > }
> > > > >
> > > > > +# Find a given xfs mount's errortag injection knob in sysfs
> > > > > +_find_xfs_errortag_knob()
> > > > > +{
> > > > > + dev="$1"
> > > > > + knob="$2"
> > > > > + shortdev="$(_short_dev "${dev}")"
> > > > > + tagfile="/sys/fs/xfs/${shortdev}/errortag/${knob}"
> > > > > +
> > > > > + # Some of the new sysfs errortag knobs were previously available via
> > > > > + # another sysfs path.
> > > > > + case "${knob}" in
> > > > > + "log_bad_crc")
> > > > > + if [ ! -w "${tagfile}" ]; then
> > > > > + tagfile="/sys/fs/xfs/${shortdev}/log/log_badcrc_factor"
> > > > > + fi
> > > > > + ;;
> > > > > + "drop_writes")
> > > > > + if [ ! -w "${tagfile}" ]; then
> > > > > + tagfile="/sys/fs/xfs/${shortdev}/drop_writes"
> > > > > + fi
> > > > > + if [ ! -w "${tagfile}" ]; then
> > > > > + tagfile="/sys/fs/xfs/${shortdev}/fail_writes"
> > > > > + fi
> > > > > + ;;
> > > > > + "log_recovery_delay"|"bug_on_assert")
> > > >
> > > > What's the purpose for having these here? I'm not sure that either is
> > > > really related to error injection (which I find a little confusing
> > > > despite that this code can be reused to generically send a value to a
> > > > sysfs knob).
> > >
> > > I was entertaining the thought of making all the debugging knobs
> > > available through a single helper function, though I concede that these
> > > two affect all of XFS and are thus a little weird.
> > >
> > > Perhaps a second helper function for module-wide knobs?
> > >
> >
> > Yeah, though I'm not sure we need to factor the helpers based on whether
> > a knob is per-fs or global. I was thinking about a generic set of
> > helpers to handle sysfs knobs, but in poking around it appears we
> > already have something like that. See xfs/264 and _set_fs_sysfs_attr()
> > and friends. These currently expect a device param, but we should be
> > able to fix that up to support either. Perhaps we could also reuse those
> > to implement the error injection helpers?
>
> It's definitely possible, but I'm not convinced we'd gain enough from
> wrapping "echo value > key". For error injection we have to check that
> the sysfs path exists; if so, then write to the path, and if not, call
> the ioctl. Therefore, we'd have to add a _check_fs_sysfs or something
> that returns whether or not a sysfs path exists (as opposed to
> _require_fs_sysfs which will _notrun the test) and rewrite
> _scratch_inject_error to call _check_fs_sysfs and _set_fs_sysfs_attr.
> It's certainly doable, but that seems like a lot of work to encapsulate
> test -w and echo.
>
Indeed, it's probably not be worth it if the common code ends up as
basically just the knob update. Perhaps we should just update the
existing _set_fs_sysfs_attr() api to support a NULL device for such
global knobs and call it a day (and this patch can just drop the bits
unrelated to error injection).
Brian
> --D
>
> >
> > Brian
> >
> > > >
> > > > > + # These apply to all xfs filesystems
> > > > > + tagfile="/sys/fs/xfs/debug/${knob}"
> > > > > + ;;
> > > > > + *)
> > > > > + ;;
> > > > > + esac
> > > > > +
> > > > > + echo "${tagfile}"
> > > > > +}
> > > > > +
> > > > > # Requires that xfs_io inject command knows about this error type
> > > > > _require_xfs_io_error_injection()
> > > > > {
> > > > > type="$1"
> > > > > +
> > > > > + # Can we find the error injection knobs via the new errortag
> > > > > + # configuration mechanism?
> > > > > + test -w "$(_find_xfs_errortag_knob "${TEST_DEV}" "${type}")" && return
> > > > > +
> > > > > _require_error_injection
> > > > >
> > > > > # NOTE: We can't actually test error injection here because xfs
> > > > ...
> > > > > diff --git a/tests/xfs/196 b/tests/xfs/196
> > > > > index e9b0649..5afc343 100755
> > > > > --- a/tests/xfs/196
> > > > > +++ b/tests/xfs/196
> > > > ...
> > > > > @@ -53,13 +54,7 @@ rm -f $seqres.full
> > > > > _supported_fs generic
> > > > > _supported_os Linux
> > > > > _require_scratch
> > > > > -
> > > > > -DROP_WRITES="drop_writes"
> > > > > -# replace "drop_writes" with "fail_writes" for old kernel
> > > > > -if [ -f /sys/fs/xfs/$(_short_dev $TEST_DEV)/fail_writes ];then
> > > > > - DROP_WRITES="fail_writes"
> > > > > -fi
> > > > > -_require_xfs_sysfs $(_short_dev $TEST_DEV)/${DROP_WRITES}
> > > > > +_require_xfs_io_error_injection "drop_writes"
> > > > >
> > > > > _scratch_mkfs >/dev/null 2>&1
> > > > > _scratch_mount
> > > > > @@ -72,7 +67,7 @@ bytes=$((64 * 1024))
> > > > > $XFS_IO_PROG -f -c "pwrite 0 $bytes" $file >> $seqres.full 2>&1
> > > > >
> > > > > # Enable write drops. All buffered writes are dropped from this point on.
> > > > > -echo 1 > /sys/fs/xfs/$sdev/$DROP_WRITES
> > > > > +_scratch_inject_error "drop_writes"
> > > >
> > > > I'd prefer to keep the parameter in this case (and below) just as a
> > > > defense against future changes in the default value. Otherwise the rest
> > > > looks pretty good.
> > >
> > > Ok.
> > >
> > > >
> > > > Brian
> > > >
> > > > >
> > > > > # Write every other 4k range to split the larger delalloc extent into many more
> > > > > # smaller extents. Use pwrite because with write failures enabled, all
> > > > > @@ -89,7 +84,7 @@ for i in $(seq 4096 8192 $endoff); do
> > > > > $XFS_IO_PROG -c "pwrite $i 4k" $file >> $seqres.full 2>&1
> > > > > done
> > > > >
> > > > > -echo 0 > /sys/fs/xfs/$sdev/$DROP_WRITES
> > > > > +_scratch_inject_error "drop_writes" 0
> > > > >
> > > > > _scratch_cycle_mount
> > > > > $XFS_IO_PROG -c 'bmap -vp' $file | _filter_bmap
> > > > > @@ -104,9 +99,9 @@ for offset in $(seq 0 100 500); do
> > > > > $XFS_IO_PROG -fc "pwrite ${offset}m 100m" $file >> $seqres.full 2>&1
> > > > >
> > > > > punchoffset=$((offset + 75))
> > > > > - echo 1 > /sys/fs/xfs/$sdev/$DROP_WRITES
> > > > > + _scratch_inject_error "drop_writes"
> > > > > $XFS_IO_PROG -c "pwrite ${punchoffset}m 4k" $file >> $seqres.full 2>&1
> > > > > - echo 0 > /sys/fs/xfs/$sdev/$DROP_WRITES
> > > > > + _scratch_inject_error "drop_writes" 0
> > > > > done
> > > > >
> > > > > echo "Silence is golden."
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2017-07-06 10:07 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-21 21:57 [PATCH 0/8] miscellaneous tests Darrick J. Wong
2017-06-21 21:57 ` [PATCH 1/8] ext4: fsmap tests Darrick J. Wong
2017-06-23 7:41 ` Eryu Guan
2017-06-23 15:35 ` Darrick J. Wong
2017-06-21 21:57 ` [PATCH 2/8] xfs/274: flip shared bits to reflect xfsprogs usage Darrick J. Wong
2017-06-21 21:57 ` [PATCH 3/8] xfs: don't allow realtime swap files Darrick J. Wong
2017-06-21 21:57 ` [PATCH 4/8] xfs/040: use compare-libxfs in xfsprogs Darrick J. Wong
2017-06-21 21:57 ` [PATCH 5/8] reflink: test unlinking a huge extent with a lot of refcount adjustments Darrick J. Wong
2017-06-29 9:36 ` Eryu Guan
2017-06-29 16:07 ` Darrick J. Wong
2017-06-29 17:19 ` Eryu Guan
2017-06-21 21:57 ` [PATCH 6/8] xfs: test that we can SEEK_HOLE/DATA data and holes that are in the CoW fork only Darrick J. Wong
2017-06-21 21:58 ` [PATCH 7/8] xfs: test freeze/rmap repair race Darrick J. Wong
2017-06-29 9:47 ` Eryu Guan
2017-06-29 17:17 ` Darrick J. Wong
2017-06-21 21:58 ` [PATCH 8/8] xfs: scrub while appending to a file Darrick J. Wong
2017-06-22 6:26 ` [PATCH 9/8] common/populate: remember multi-device configurations Darrick J. Wong
2017-06-23 7:49 ` Eryu Guan
2017-06-23 15:34 ` Darrick J. Wong
2017-06-30 4:12 ` [PATCH v2 " Darrick J. Wong
2017-06-23 7:59 ` [PATCH 0/8] miscellaneous tests Eryu Guan
2017-06-30 4:12 ` [PATCH 10/8] common/rc: test that the xfs_io scrub/repair commands actually work Darrick J. Wong
2017-06-30 4:13 ` [PATCH 11/8] common/inject: refactor helpers to use new errortag interface Darrick J. Wong
2017-07-05 12:16 ` Brian Foster
2017-07-05 16:29 ` Darrick J. Wong
2017-07-05 16:45 ` Brian Foster
2017-07-05 21:05 ` Darrick J. Wong
2017-07-06 10:07 ` Brian Foster [this message]
2017-06-30 4:13 ` [PATCH 12/8] ext4: don't online scrub ever Darrick J. Wong
2017-06-30 4:22 ` Eryu Guan
2017-06-30 4:26 ` Darrick J. Wong
2017-06-30 4:33 ` Eryu Guan
2017-06-30 4:58 ` [PATCH v2 " Darrick J. Wong
2017-06-30 12:32 ` Eryu Guan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170706100749.GA56359@bfoster.bfoster \
--to=bfoster@redhat.com \
--cc=darrick.wong@oracle.com \
--cc=eguan@redhat.com \
--cc=fstests@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).