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

[Ojaswin Mujoo]

On Tue, Sep 09, 2025 at 08:49:18AM +0100, John Garry wrote:
> On 09/09/2025 07:44, Ojaswin Mujoo wrote:
> > > > > > +create_mixed_mappings() {
> > > > > Is this same as patch 08/12?
> > > > I believe you mean the [D]SYNC tests, yes it is the same.
> > > then maybe factor out the test, if possible. I assume that this sort of
> > > approach is taken for xfstests.
> > > 
> > I'm not sure what you mean by factor out the*test*. We are testing
> > different things there and the only thing common in the tests is
> > creation of mixed mapping files and the check to ensure we didn't tear
> > data.
> > 
> > In case you mean to factor out the create_mixed_mappings() helper into
> > common/rc, sure I can do that but I'm unsure if at this point it would
> > be very useful for other tests.
> 
> above it was mentioned that the code in create_mixed_mappings was common, so
> that is why I suggested to factor it out. If it does not make sense, then
> fine (and don't).

Yes I mean Im unsure it will be useful for tests other than the two
tests in this patchset.

> 
> > 
> > > > > > +	local file=$1
> > > > > > +	local size_bytes=$2
> > > > > > +
> > > > > > +	echo "# Filling file $file with alternate mappings till size $size_bytes" >> $seqres.full
> > > > > > +	#Fill the file with alternate written and unwritten blocks
> > > > > > +	local off=0
> > > > > > +	local operations=("W" "U")
> > > > > > +
> > > > > > +	for ((i=0; i<$((size_bytes / blksz )); i++)); do
> > > > > > +		index=$(($i % ${#operations[@]}))
> > > > > > +		map="${operations[$index]}"
> > > > > > +
> > <...>
> 
> 
> > > > > > +	echo >> $seqres.full
> > > > > > +	echo "# Starting filesize integrity test for atomic writes" >> $seqres.full
> > > > > what does "Starting filesize integrity test" mean?
> > > > Basically other tests already truncate the file to a higher value and
> > > > then perform the shut down test. Here we actually do append atomic
> > > > writes since we want to also stress the i_size update paths during
> > > > shutdown to ensure that doesn't cause any tearing with atomic writes.
> > > > 
> > > > I can maybe rename it to:
> > > > 
> > > > 
> > > > echo "# Starting data integrity test for atomic append writes" >> $seqres.full
> > > > 
> > > > Thanks for the review!
> > > > 
> > > It's just the name "integrity" that throws me a bit..
> > So I mean integrity as in writes are not tearing after the shutdown.
> > That's how we have worded the other sub-tests above.
> 
> you could mention "shutdown" also in the print.

Umm, do you mean something like:

 "Starting shutdown data integrity tests ..."

 Regards,
 ojaswin
> 
> Thanks!
>