On Thu, Feb 18, 2021 at 08:02:17AM -0500, Brian Foster wrote:
> 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.
Agreed. Admittedly, the test case I wrote for it that kills repair
after NR writes (where NR steadily increases) has opened my eyes to how
... stunningly awful xfs_repair (and e2fsck) can be.
(As in, you can *very easily* snatch death from the jaws of victory if
all you wanted was to fix a minor bitflip somewhere /and/ repair
dies...)
> > 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.
<nod>
> ...
> > > > 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?
Will do.
--D
> 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;
> > > > }
> > > >
> > >
> >
>
