Re: [PATCH] iomap: fix zero padding data issue in concurrent append writes

[Long Li <leo.lilong@xxxxxxxxxx>]

On Sun, Nov 10, 2024 at 09:45:19PM -0800, Christoph Hellwig wrote:
> On Sat, Nov 09, 2024 at 03:13:53PM +0800, Long Li wrote:
> > > Oh, interesting one.  Do you have a reproducer we could wire up
> > > to xfstests?
> > > 
> > 
> > Yes, I have a simple reproducer, but it would require significant
> > work to incorporate it into xfstestis.
> 
> Can you at least shared it?  We might be able to help turning it into
> a test.
> 

At first, we used the following script to find the problem, but it was
difficult to reproduce the problem, run test.sh after system startup.

--------------------filesystem.sh---------------------
#!/bin/bash
index=$1
value=$2

while true; do
    echo "$value" >> /mnt/fs_"$index"/file1
    echo "$value" >> /mnt/fs_"$index"/file2
    cp /mnt/fs_"$index"/file1 /mnt/fs_"$index"/file3
    cat /mnt/fs_"$index"/file1 /mnt/fs_"$index"/file2
    mv /mnt/fs_"$index"/file3 /mnt/fs_"$index"/file1
done

--------------------test.sh--------------------------
#!/bin/bash
mount /dev/sda /mnt
cat -v /mnt/* | grep @
if [ $? == 0 ] ;then
        echo "find padding data"
        exit 1
fi

sh -x filesystem.sh 1 1111 &>/dev/null &
sh -x filesystem.sh 1 2222 &>/dev/null &
sh -x filesystem.sh 1 3333 &>/dev/null &
sleep $(($RANDOM%30))
echo "reboot..."
echo b > /proc/sysrq-trigger
------------------------------------------------------

I later reproduce it by adding a delay to the kernel code
and verified the fixed patch. 

1) add some sleep in xfs_end_ioend

--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -130,8 +130,10 @@ xfs_end_ioend(
        else if (ioend->io_type == IOMAP_UNWRITTEN)
                error = xfs_iomap_write_unwritten(ip, offset, size, false);
 
-       if (!error && xfs_ioend_is_append(ioend))
+       if (!error && xfs_ioend_is_append(ioend)) {
+               msleep(30000);
                error = xfs_setfilesize(ip, ioend->io_offset, ioend->io_size);
+       }
 done:
        iomap_finish_ioends(ioend, error);
        memalloc_nofs_restore(nofs_flag);


2) run rep.sh and reboot system
-----------------------rep.sh-------------------------
#!/bin/bash

mkfs.xfs -f /dev/sda
mount /dev/sda /mnt/test
touch /mnt/test/file
xfs_io -c "pwrite 0 20 -S 0x31" /mnt/test/file
sync &
sleep 5

echo 100000 > /proc/sys/vm/dirty_writeback_centisecs
echo 100000 > /proc/sys/vm/dirty_expire_centisecs
xfs_io -c "pwrite 20 20 -S 0x31" /mnt/test/file 
sleep 40

echo b > /proc/sysrq-trigger
------------------------------------------------------

3) after reboot, check file.

mount /dev/sda /mnt/test
cat -v /mnt/test/file | grep @

> > If we only use one size record, we can remove io_size and keep only
> > io_end to record the tail end of valid file data in ioend. Meanwhile,
> > we can add a wrapper function iomep_ioend_iosize() to get the extent
> > size of ioend, replacing the existing ioend->io_size. Would this work?
> 
> I'd probably still use offset + size to avoid churn because it feels
> more natural and causes less churn, but otherwise this sounds good to
> me.
> 

Ok, I got it. However, we need to change the meaning of "io_size" to
the size of the valid file data in ioend.

Thanks,
Long Li