On Wed, Feb 17, 2021 at 08:36:20PM -0800, Darrick J. Wong wrote:
> On Tue, Feb 16, 2021 at 06:56:45AM -0500, Brian Foster wrote:
> > On Fri, Feb 12, 2021 at 09:46:56PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > >
> > > Add an error injection knob so that we can simulate system failure after
> > > a certain number of disk writes. This knob is being added so that we
> > > can check repair's behavior after an arbitrary number of tests.
> > >
> > > Set LIBXFS_DEBUG_WRITE_CRASH={ddev,logdev,rtdev}=nn in the environment
> > > to make libxfs SIGKILL itself after nn writes to the data, log, or rt
> > > devices. Note that this only applies to xfs_buf writes and zero_range.
> > >
> > > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > ---
> > > libxfs/init.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++---
> > > libxfs/libxfs_io.h | 19 +++++++++++++++
> > > libxfs/rdwr.c | 6 ++++-
> > > 3 files changed, 88 insertions(+), 5 deletions(-)
> > >
> > >
> > > diff --git a/libxfs/init.c b/libxfs/init.c
> > > index 8a8ce3c4..1ec83791 100644
> > > --- a/libxfs/init.c
> > > +++ b/libxfs/init.c
> > ...
> > > @@ -614,6 +634,46 @@ libxfs_buftarg_init(
> > > dev_t logdev,
> > > dev_t rtdev)
> > > {
> > > + char *p = getenv("LIBXFS_DEBUG_WRITE_CRASH");
> > > + unsigned long dfail = 0, lfail = 0, rfail = 0;
> >
> > Was there a reason for using an environment variable now rather than the
> > original command line option?
>
> Well, you said you wanted a generic write error injection hook for
> libxfs, and this is the simplest way to add that, given that libraries
> don't have a direct means to parse argc and argv.
>
I think you're misinterpreting my previous feedback. ;) I thought the
injection mechanism was too closely tied to an implementation detail
(i.e. "fail after updating needsrepair bit") of the application.
Instead, I preferred a more generic mechanism (the "fail after so many
I/Os," "fail after phase N" approaches in these patches) that covers the
original use case. That broadens the potential test coverage and
usefulness of the mechanism.
> I mean... this /could/ take the form of an exposed library function that
> xfs utilities could opt into their own getopt loops, but that's even
> /more/ infrastructure code that I'd have to write.
>
In this case I was just curious why the interface was changed from the
previous approach. ISTM it didn't necessarily have to, but I'm not
concerned about it either way.
...
> > > diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
> > > index c80e2d59..85485257 100644
> > > --- a/libxfs/libxfs_io.h
> > > +++ b/libxfs/libxfs_io.h
> > ...
> > > @@ -30,6 +32,23 @@ struct xfs_buftarg {
> > > #define XFS_BUFTARG_LOST_WRITE (1 << 0)
> > > /* A dirty buffer failed the write verifier. */
> > > #define XFS_BUFTARG_CORRUPT_WRITE (1 << 1)
> > > +/* Simulate failure after a certain number of writes. */
> > > +#define XFS_BUFTARG_INJECT_WRITE_FAIL (1 << 2)
> > > +
> > > +/* Simulate the system crashing after a write. */
> > > +static inline void
> > > +xfs_buftarg_trip_write(
> > > + struct xfs_buftarg *btp)
> > > +{
> > > + if (!(btp->flags & XFS_BUFTARG_INJECT_WRITE_FAIL))
> > > + return;
> > > +
> > > + pthread_mutex_lock(&btp->lock);
> > > + btp->writes_left--;
> > > + if (!btp->writes_left)
> > > + kill(getpid(), SIGKILL);
> >
> > Can we just exit()?
> >
> > (Same questions for the next patch..)
>
> The goal of this generic write error injection framework is to simulate
> total system crashes immediately after a write.
>
> SIGKILL and exit are not the same, because atexit handlers don't run if
> the process forcibly kills itself.
>
Can you document this somewhere please?
Brian
> --D
>
> >
> > Brian
> >
> > > + pthread_mutex_unlock(&btp->lock);
> > > +}
> > >
> > > extern void libxfs_buftarg_init(struct xfs_mount *mp, dev_t ddev,
> > > dev_t logdev, dev_t rtdev);
> > > diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> > > index ca272387..fd456d6b 100644
> > > --- a/libxfs/rdwr.c
> > > +++ b/libxfs/rdwr.c
> > > @@ -74,8 +74,10 @@ libxfs_device_zero(struct xfs_buftarg *btp, xfs_daddr_t start, uint len)
> > > /* try to use special zeroing methods, fall back to writes if needed */
> > > len_bytes = LIBXFS_BBTOOFF64(len);
> > > error = platform_zero_range(fd, start_offset, len_bytes);
> > > - if (!error)
> > > + if (!error) {
> > > + xfs_buftarg_trip_write(btp);
> > > return 0;
> > > + }
> > >
> > > zsize = min(BDSTRAT_SIZE, BBTOB(len));
> > > if ((z = memalign(libxfs_device_alignment(), zsize)) == NULL) {
> > > @@ -105,6 +107,7 @@ libxfs_device_zero(struct xfs_buftarg *btp, xfs_daddr_t start, uint len)
> > > progname, __FUNCTION__);
> > > exit(1);
> > > }
> > > + xfs_buftarg_trip_write(btp);
> > > offset += bytes;
> > > }
> > > free(z);
> > > @@ -860,6 +863,7 @@ libxfs_bwrite(
> > > } else {
> > > bp->b_flags |= LIBXFS_B_UPTODATE;
> > > bp->b_flags &= ~(LIBXFS_B_DIRTY | LIBXFS_B_UNCHECKED);
> > > + xfs_buftarg_trip_write(bp->b_target);
> > > }
> > > return bp->b_error;
> > > }
> > >
> >
>
