Re: [PATCH RFC 3/3] fstests: generic: Check the fs after each FUA writes

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4731 bytes --]



On 2018年03月16日 12:01, Eryu Guan wrote:
> On Wed, Mar 14, 2018 at 05:02:30PM +0800, Qu Wenruo wrote:
>> Basic test case which triggers fsstress with dm-log-writes, and then
>> check the fs after each FUA writes.
>> With needed infrastructure and special handlers for journal based fs.
> 
> It's not clear to me why the existing infrastructure is not sufficient
> for the test. It'd be great if you could provide more information and/or
> background in commit log.

The main problem of current infrastructure is we don't have the
following things:

1) Way to take full advantage of dm-log-writes
   The main thing is, we don't have test cases to check each FUA (this
   patch) and flush (later patch after clearing all the RFC comments).

   We have some dm-flakey test cases to emulate power loss, but they are
   mostly for fsync.
   Here we are not only testing fsync, but also every superblock update.
   Which should be a super set of dm-flakey tests.

2) Workaround for journal replay
   In fact, if we only test btrfs, we don't even need such complicated
   work, just 'replay-log --fsck "btrfs check" --check fua' will be
   enough. As btrfs check doesn't report dirty journal (log tree) as
   problem.
   But for journal based fses, their fsck all report dirty journal as
   error, which needs current snapshot works to replay them before
   running fsck.

I would add them in next version if there is no extra comment on this.

> 
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> In my test, xfs and btrfs survives while ext4 would report error during fsck.
>>
>> My current biggest concern is, we abuse $TEST_DEV and mkfs on it all by
>> ourselves. Not sure if it's allowed.
> 
> As Amir already replied, that's not allowed, any destructive operations
> should be done on $SCRATCH_DEV.

Yep, I'm looking for similar case who uses $SCRATCH_DEV as LVM pv do get
extra device.

Or can we reuse the scratch_dev_pool even for ext4/xfs?

> 
>> ---
>>  common/dmlogwrites    | 119 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/generic/481     | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/generic/481.out |   2 +
>>  tests/generic/group   |   1 +
>>  4 files changed, 246 insertions(+)
>>  create mode 100755 tests/generic/481
>>  create mode 100644 tests/generic/481.out
>>
>> diff --git a/common/dmlogwrites b/common/dmlogwrites
>> index 467b872e..258f5887 100644
>> --- a/common/dmlogwrites
>> +++ b/common/dmlogwrites
>> @@ -126,3 +126,122 @@ _log_writes_cleanup()
>>  	$UDEV_SETTLE_PROG >/dev/null 2>&1
>>  	_log_writes_remove
>>  }
>> +
>> +# Convert log writes mark to entry number
>> +# Result entry number is output to stdout, could be empty if not found
>> +_log_writes_mark_to_entry_number()
>> +{
>> +	local _mark=$1
> 
> No need to name a local variable with leading underscore.
> 
>> +	local ret
>> +
>> +	[ -z "$_mark" ] && _fatal \
>> +		"mark must be given for _log_writes_mark_to_entry_number"
> 
> Please use _fail instead of _fatal (and all the other places in this
> patch). I think _fatal is only used at the initializing phase of the
> test harness (e.g. check required tools, env etc.) and abort the test
> run in early stage. _fail is used to indicate a failure in test phase,
> it also logs the error messages in $seqres.full.

Glad to know the difference.

> 
>> +
>> +	ret=$($here/src/log-writes/replay-log --find --log $LOGWRITES_DEV \
>> +		--end-mark $_mark 2> /dev/null)
> 
> Is this output long or short? If it's a short one, e.g. a number or a
> simple string, I think it's OK to save it in a variable. But if it's
> long/complex enough and/or has multiple lines, I think we'd better to
> save it with a tmp file, e.g. $tmp.<suffix>.

Very very short, just one line like '1024@2048'.

> 
>> +	[ -z "$ret" ] && return
>> +	ret=$(echo "$ret" | cut -f1 -d\@)
> 
> Because I want to avoid echoing such a variable to a pipe (even it's
> quoted), it used to cause subtle problems if not quoted properly.
> 
> Also, it'd be better to give a sample output of the replay-log command
> in comment if it's short enough, so people could have an idea what it
> looks like and know what you're looking for in the subsequent steps
> (cut -f1 -d\@, in this example), and it's easier to review.
> 
> Above comments apply to other helper functions too.
> 
> Thanks for all the work!

Thanks for your review too,
Qu

> 
> Eryu
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]