Re: [RFC PATCH] check: try to fix the test device if it gets corrupted

[Eryu Guan <eguan@redhat.com>]

On Tue, Jul 18, 2017 at 01:30:09PM -0400, Theodore Ts'o wrote:
> On Tue, Jul 18, 2017 at 01:05:07PM +0800, Eryu Guan wrote:
> > > >  	if [ -f ${RESULT_DIR}/require_test ]; then
> > > > -		_check_test_fs || err=true
> > > > +		if ! _check_test_fs ; then
> > > > +			err=true
> > > > +			echo "Trying to repair broken TEST_DEV file system"
> > > > +			_repair_test_fs
> > 
> > Should we stop running if _repair_test_fs failed to fix TEST_DEV? I
> > think that was what Darrick first suggested. And I think unfixable
> > errors will cause subsequent tests to fail too.
> 
> Seems reasonable to me.  I'll take a look at it.
> 
> > > > +_repair_test_fs()
> > > > +{
> > > > +	case $FSTYP in
> > > > +	xfs)
> > > > +		_repair_xfs_test_fs "$@" >$tmp.repair 2>&1
> > > > +		res=$?
> > 
> > Better to declare res as a local variable. I know this was copied from
> > _repair_scratch_fs, but better to get it improved in new code :)
> 
> Ack.
> 
> > > Structurally this all looks ok, but it's a little weird that we have
> > > _scratch_xfs_repair for the scratch device (object-verb) but
> > > _repair_test_fs (verb-object) for the test device.
> > 
> > My best guess is that _repair_scratch_fs and _repair_test_fs are helpers
> > in a higher level, and are defined in common/rc, they deal with all
> > filesystem types.
> > 
> > _scratch_xfs_repair is xfs specific, and defined in common/xfs, it only
> > deals with xfs. So the naming schema is different but they are also in
> > different "namespace"s, which looks fine to me :)
> 
> I also didn't like _test_repair_fs because it's not clear whether
> "test" is a verb or a noun.
> 
> So one thing we could do is _xfs_scratch_repair and _xfs_test_repair?

Then all existing _scratch_xfs_repair calls need a rename, but I'm fine
with the rename.

> 
> The other thing is how much of the cleanup in common/xfs should be
> segregated into a separate commit (and I'm not sure how competent I'm
> going to be ate doing that cleanup, but I'm willing to give it a go).

I think we can do minimal cleanup in this patch (like the rename of
_scratch_xfs_repair to _xfs_scratch_repair) and do other cleanups in
separate commits (if there's still any).

Thanks!

Eryu