On Wed, Aug 02, 2017 at 08:52:57AM -0700, Darrick J. Wong wrote:
> On Wed, Aug 02, 2017 at 10:30:10AM -0400, Brian Foster wrote:
> > On Wed, Aug 02, 2017 at 04:37:42PM +0800, Eryu Guan wrote:
> > > On Fri, Jul 21, 2017 at 03:04:45PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxx>
> > >
> > > This looks good to me overall, but I still perfer let other people
> > > who're more familar with XFS errortag and error injection to review too.
> > > While I do have some questions/comments :)
> > >
> > > > ---
> > > > common/inject | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > > > tests/xfs/141 | 5 +++--
> > > > tests/xfs/196 | 17 ++++++-----------
> > > > 3 files changed, 65 insertions(+), 15 deletions(-)
> > > >
> > > >
> > > > diff --git a/common/inject b/common/inject
> > > > index 8ecc290..9aa24de 100644
> > > > --- a/common/inject
> > > > +++ b/common/inject
> > > > @@ -35,10 +35,46 @@ _require_error_injection()
> > > > esac
> > > > }
> > > >
> > > > +# Find a given xfs mount's errortag injection knob in sysfs
> > > > +_find_xfs_mount_errortag_knob()
> > >
> > > While The function name and comment both indicate it needs a mounted
> > > XFS, it seems weird that the first argument is expected to be a block
> > > device. And do we need to check if the given device is really mounted?
> > >
> >
> > The xfs per-mount sysfs knobs distinguish between mounts via the
> > block device name.
>
> What if we rename the helper and change its documentation as such?
>
> # Find the errortag injection knob in sysfs for a given xfs mount's
> # block device.
> _find_xfs_mountdev_errortag_knob()
This looks better to me, thanks!
> {
> ...
> }
>
> > ...
> > > > # 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_mount_errortag_knob "${TEST_DEV}" "${type}")" && return
> > > > +
> > >
> > > As this check goes prior to the _require_error_injection check, so I
> > > assume this new errortag framework doesn't depend on a debug built XFS,
> > > can I?
>
> It depends on the sysfs knobs, and the sysfs knobs in turn depend on
> XFS_DEBUG=y.
>
> > It does depend on debug mode so it probably makes sense to push this
> > after the _require_error_injection check. That way the DEBUG=0 message
> > has precedent over a message regarding lack of support for a particular
> > knob.
>
> Hm? This is what _require_xfs_io_error_injection does:
>
> First we compute the path to the knob if it exists
> (_find_xfs_mount_errortag_knob).
>
> Then we check if that path is writable. If it is, error injection is
> enabled (which presumably means XFS_DEBUG=y) and we can continue.
>
> If not, then we use _require_error_injection to bail out if XFS_DEBUG=n.
>
> If we don't bail out, XFS_DEBUG=y and so we check if the xfs_io inject
> help page knows about the error type, and bail out if it doesn't.
>
>
> In the end it doesn't really matter if we look for XFS_DEBUG=y before or
> after we look for the new sysfs knob. I figured it was simple enough to
> assume that if the knob is present and writable, then our preconditions
> are satisfied and it's ok to proceed with the injection test.
Agreed, the order doesn't really matter here, I'm fine with either way.
Thanks,
Eryu
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
