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

["Theodore Ts'o" <tytso@mit.edu>]

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?

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).

      	    	      	   	    	- Ted