Re: [PATCH v6 09/12] generic: Add sudden shutdown tests for multi block atomic writes

[John Garry]

On 17/09/2025 09:09, Ojaswin Mujoo wrote:
> > nit: I think that I mentioned this the last time - I would not use the word
> > "expected". We have old data, new data, and actual data. The only thing
> > which we expect is that actual data will be either all old or all new.
>   
> Hey John so I mentioned here [1] that the wording "expected new",
> "expected old", "actual" looked more clear to me than "new", "old" and
> "actual" and you replied with sure so I though we were good there 🙂
>
> But no worries I can make this change. I'll keep the wording as
> new, old and actual.

great, thanks

> > > +			echo "$expected_data_old"
> > > +			echo
> > > +			echo "Expected new: "
> > > +			echo "$expected_data_new"
> > > +			echo
> > > +			echo "Actual contents: "
> > > +			echo "$actual_data"
> > > +
> > > +			_fail
> > > +		fi
> > > +		echo -n "Check at offset $off succeeded! " >> $seqres.full
> > > +		if [[ "$actual_data" == "$expected_data_new" ]]
> > > +		then
> > > +			echo "matched new" >> $seqres.full
> > > +		elif [[ "$actual_data" == "$expected_data_old" ]]
> > > +		then
> > > +			echo "matched old" >> $seqres.full
> > > +		fi
> > > +		off=$(( off + awu_max ))
> > > +	done
> > > +}
> > > +
> > > +# test data integrity for file by shutting down in between atomic writes
> > > +test_data_integrity() {
> > > +	echo >> $seqres.full
> > > +	echo "# Writing atomically to file in background" >> $seqres.full
> > > +
> > > +	start_atomic_write_and_shutdown
> > > +
> > > +	last_offset=$(tail -n 1 $tmp.aw | cut -d" " -f4)
> > > +	if [[ -z $last_offset ]]
> > > +	then
> > > +		last_offset=0
> > > +	fi
> > > +
> > > +	echo >> $seqres.full
> > > +	echo "# Last offset of atomic write: $last_offset" >> $seqres.full
> > > +
> > > +	rm $tmp.aw
> > > +	sleep 0.5
> > > +
> > > +	_scratch_cycle_mount
> > > +
> > > +	# we want to verify all blocks around which the shutdown happened
> > > +	verify_start=$(( last_offset - (awu_max * 5)))
> > > +	if [[ $verify_start < 0 ]]
> > > +	then
> > > +		verify_start=0
> > > +	fi
> > > +
> > > +	verify_end=$(( last_offset + (awu_max * 5)))
> > > +	if [[ "$verify_end" -gt "$filesize" ]]
> > > +	then
> > > +		verify_end=$filesize
> > > +	fi
> > > +}
> > > +
> > > +# test data integrity for file with written and unwritten mappings
> > > +test_data_integrity_mixed() {
> > > +	$XFS_IO_PROG -fc "truncate 0" $testfile >> $seqres.full
> > > +
> > > +	echo >> $seqres.full
> > > +	echo "# Creating testfile with mixed mappings" >> $seqres.full
> > > +	create_mixed_mappings $testfile $filesize
> > > +
> > > +	test_data_integrity
> > > +
> > > +	verify_data_blocks $verify_start $verify_end "$expected_data_old_mixed" "$expected_data_new"
> > > +}
> > > +
> > > +# test data integrity for file with completely written mappings
> > > +test_data_integrity_written() {
> > nit: again, I am not so keen on using the word "integrity" at all.
> > "integrity" in storage world relates to T10 PI support in Linux. I know that
> > last time I mentioned it's ok to use "integrity" when close to words "atomic
> > write", but I still fear some doubt on whether we are talking about T10 PI
> > when we mention integrity.
> Okay got it, fine then how about using phrases like "test for torn
> data for file with completely written mapping" and such?

sure, it's ok

Thanks very much