[PATCH] f2fs: test for race condition in between atomic_write and gc

[PATCH] f2fs: test for race condition in between atomic_write and gc

[Chao Yu]

Test that we will simulate sqlite atomic write logic w/ below steps:
1. create a regular file, and initialize it w/ 0xff data
2. start transaction (via F2FS_IOC_START_ATOMIC_WRITE) on it
3. write transaction data
4. trigger foreground GC to migrate data block of the file
5. commit and end the transaction (via F2FS_IOC_COMMIT_ATOMIC_WRITE)
6. check consistency of transaction w/ in-memory and on-disk data
This is a regression test to check handling of race condition in
between atomic_write and GC.

Cc: Jaegeuk Kim <jaegeuk@kernel.org>
Cc: Daeho Jeong <daehojeong@google.com>
Signed-off-by: Chao Yu <chao@kernel.org>
---
 tests/f2fs/003     | 61 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/f2fs/003.out | 11 +++++++++
 2 files changed, 72 insertions(+)
 create mode 100755 tests/f2fs/003
 create mode 100644 tests/f2fs/003.out

diff --git a/tests/f2fs/003 b/tests/f2fs/003
new file mode 100755
index 00000000..d8311c4c
--- /dev/null
+++ b/tests/f2fs/003
@@ -0,0 +1,61 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2024 Oppo.  All Rights Reserved.
+#
+# FS QA Test No. f2fs/003
+#
+# Test that we will simulate sqlite atomic write logic w/ below steps:
+# 1. create a regular file, and initialize it w/ 0xff data
+# 2. start transaction (via F2FS_IOC_START_ATOMIC_WRITE) on it
+# 3. write transaction data
+# 4. trigger foreground GC to migrate data block of the file
+# 5. commit and end the transaction (via F2FS_IOC_COMMIT_ATOMIC_WRITE)
+# 6. check consistency of transaction w/ in-memory and on-disk data
+# This is a regression test to check handling of race condition in
+# between atomic_write and GC.
+#
+. ./common/preamble
+_begin_fstest auto quick
+
+. ./common/filter
+
+_supported_fs f2fs
+_require_scratch
+_require_xfs_io_command "pwrite"
+_require_xfs_io_command "fpunch"
+
+_scratch_mkfs >> $seqres.full
+_scratch_mount >> $seqres.full
+
+dbfile=$SCRATCH_MNT/dbfile
+foo=$SCRATCH_MNT/foo
+bar=$SCRATCH_MNT/bar
+
+$XFS_IO_PROG -c "pwrite 0 512k -S 0xff" -c "fsync" -f $dbfile >> $seqres.full
+$XFS_IO_PROG -c "pwrite 0 2m" -c "fsync" -f $foo >> $seqres.full
+sync
+
+# start atomic_write on dbfile & write data to dbfile
+$F2FS_IO_PROG write 1 0 32 zero atomic_commit $dbfile 3000 >> $seqres.full &
+$XFS_IO_PROG -c "fpunch 0 2m" $foo >> $seqres.full
+$XFS_IO_PROG -c "pwrite 0 2m" -c "fsync" -f $bar >> $seqres.full
+
+# persist all dirty data
+sync
+echo 3 > /proc/sys/vm/drop_caches
+
+# trigger foreground GC to migrate data block of atomic_file
+$F2FS_IO_PROG gc 1 $SCRATCH_MNT > /dev/null 2>&1
+
+# wait for atomic_write commit completion
+sleep 5
+# print in-memory data
+od -x $dbfile
+echo 3 > /proc/sys/vm/drop_caches
+# print on-disk data
+od -x $dbfile
+
+_scratch_unmount
+
+status=0
+exit
diff --git a/tests/f2fs/003.out b/tests/f2fs/003.out
new file mode 100644
index 00000000..d6c8a637
--- /dev/null
+++ b/tests/f2fs/003.out
@@ -0,0 +1,11 @@
+QA output created by 003
+0000000 0000 0000 0000 0000 0000 0000 0000 0000
+*
+0400000 ffff ffff ffff ffff ffff ffff ffff ffff
+*
+2000000
+0000000 0000 0000 0000 0000 0000 0000 0000 0000
+*
+0400000 ffff ffff ffff ffff ffff ffff ffff ffff
+*
+2000000
-- 
2.40.1

Re: [PATCH] f2fs: test for race condition in between atomic_write and gc

[Zorro Lang]

On Tue, Jun 25, 2024 at 11:04:16AM +0800, Chao Yu wrote:
> Test that we will simulate sqlite atomic write logic w/ below steps:
> 1. create a regular file, and initialize it w/ 0xff data
> 2. start transaction (via F2FS_IOC_START_ATOMIC_WRITE) on it
> 3. write transaction data
> 4. trigger foreground GC to migrate data block of the file
> 5. commit and end the transaction (via F2FS_IOC_COMMIT_ATOMIC_WRITE)
> 6. check consistency of transaction w/ in-memory and on-disk data
> This is a regression test to check handling of race condition in
> between atomic_write and GC.

Hi Chao,

Sorry for the late reviewing. As this's a regression test, so which onne
kernel commit fix this known issue, please specify it by:

_fixed_by_kernel_commit $commit_id $commit_subject

> 
> Cc: Jaegeuk Kim <jaegeuk@kernel.org>
> Cc: Daeho Jeong <daehojeong@google.com>
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>  tests/f2fs/003     | 61 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/f2fs/003.out | 11 +++++++++
>  2 files changed, 72 insertions(+)
>  create mode 100755 tests/f2fs/003
>  create mode 100644 tests/f2fs/003.out
> 
> diff --git a/tests/f2fs/003 b/tests/f2fs/003
> new file mode 100755
> index 00000000..d8311c4c
> --- /dev/null
> +++ b/tests/f2fs/003
> @@ -0,0 +1,61 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2024 Oppo.  All Rights Reserved.
> +#
> +# FS QA Test No. f2fs/003
> +#
> +# Test that we will simulate sqlite atomic write logic w/ below steps:
> +# 1. create a regular file, and initialize it w/ 0xff data
> +# 2. start transaction (via F2FS_IOC_START_ATOMIC_WRITE) on it
> +# 3. write transaction data
> +# 4. trigger foreground GC to migrate data block of the file
> +# 5. commit and end the transaction (via F2FS_IOC_COMMIT_ATOMIC_WRITE)
> +# 6. check consistency of transaction w/ in-memory and on-disk data
> +# This is a regression test to check handling of race condition in
> +# between atomic_write and GC.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick
> +
> +. ./common/filter
> +
> +_supported_fs f2fs
    ^^^^^
This line is not necessary now.

> +_require_scratch
> +_require_xfs_io_command "pwrite"

pwrite is a basic command of xfs_io, so I think this line is not necessary.

> +_require_xfs_io_command "fpunch"
> +
> +_scratch_mkfs >> $seqres.full
> +_scratch_mount >> $seqres.full
> +
> +dbfile=$SCRATCH_MNT/dbfile
> +foo=$SCRATCH_MNT/foo
> +bar=$SCRATCH_MNT/bar
> +
> +$XFS_IO_PROG -c "pwrite 0 512k -S 0xff" -c "fsync" -f $dbfile >> $seqres.full
> +$XFS_IO_PROG -c "pwrite 0 2m" -c "fsync" -f $foo >> $seqres.full
> +sync
> +
> +# start atomic_write on dbfile & write data to dbfile
> +$F2FS_IO_PROG write 1 0 32 zero atomic_commit $dbfile 3000 >> $seqres.full &

As there's a background process, shouldn't we do kill and wait in _cleanup
to make sure unmount won't hit EBUSY?

> +$XFS_IO_PROG -c "fpunch 0 2m" $foo >> $seqres.full
> +$XFS_IO_PROG -c "pwrite 0 2m" -c "fsync" -f $bar >> $seqres.full
> +
> +# persist all dirty data
> +sync
> +echo 3 > /proc/sys/vm/drop_caches
> +
> +# trigger foreground GC to migrate data block of atomic_file
> +$F2FS_IO_PROG gc 1 $SCRATCH_MNT > /dev/null 2>&1
> +
> +# wait for atomic_write commit completion
> +sleep 5
> +# print in-memory data
> +od -x $dbfile
> +echo 3 > /proc/sys/vm/drop_caches
> +# print on-disk data
> +od -x $dbfile

There's a common helper named "_hexdump" in common/rc, can that be used?

> +
> +_scratch_unmount

The SCRATCH_DEV will be unmounted at the end of the test, don't need to
do this manually if it's not a necessary test step.

Thanks,
Zorro

> +
> +status=0
> +exit
> diff --git a/tests/f2fs/003.out b/tests/f2fs/003.out
> new file mode 100644
> index 00000000..d6c8a637
> --- /dev/null
> +++ b/tests/f2fs/003.out
> @@ -0,0 +1,11 @@
> +QA output created by 003
> +0000000 0000 0000 0000 0000 0000 0000 0000 0000
> +*
> +0400000 ffff ffff ffff ffff ffff ffff ffff ffff
> +*
> +2000000
> +0000000 0000 0000 0000 0000 0000 0000 0000 0000
> +*
> +0400000 ffff ffff ffff ffff ffff ffff ffff ffff
> +*
> +2000000
> -- 
> 2.40.1
> 

Re: [PATCH] f2fs: test for race condition in between atomic_write and gc

[Chao Yu]

On 2024/7/16 18:57, Zorro Lang wrote:
> On Tue, Jun 25, 2024 at 11:04:16AM +0800, Chao Yu wrote:
>> Test that we will simulate sqlite atomic write logic w/ below steps:
>> 1. create a regular file, and initialize it w/ 0xff data
>> 2. start transaction (via F2FS_IOC_START_ATOMIC_WRITE) on it
>> 3. write transaction data
>> 4. trigger foreground GC to migrate data block of the file
>> 5. commit and end the transaction (via F2FS_IOC_COMMIT_ATOMIC_WRITE)
>> 6. check consistency of transaction w/ in-memory and on-disk data
>> This is a regression test to check handling of race condition in
>> between atomic_write and GC.
> 
> Hi Chao,
> 
> Sorry for the late reviewing. As this's a regression test, so which onne
> kernel commit fix this known issue, please specify it by:
> 
> _fixed_by_kernel_commit $commit_id $commit_subject

Hi Zorro,

Since the fixed patch has not been merged yet, so what about adding this line
after merging the fix?

> 
>>
>> Cc: Jaegeuk Kim <jaegeuk@kernel.org>
>> Cc: Daeho Jeong <daehojeong@google.com>
>> Signed-off-by: Chao Yu <chao@kernel.org>
>> ---
>>   tests/f2fs/003     | 61 ++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/f2fs/003.out | 11 +++++++++
>>   2 files changed, 72 insertions(+)
>>   create mode 100755 tests/f2fs/003
>>   create mode 100644 tests/f2fs/003.out
>>
>> diff --git a/tests/f2fs/003 b/tests/f2fs/003
>> new file mode 100755
>> index 00000000..d8311c4c
>> --- /dev/null
>> +++ b/tests/f2fs/003
>> @@ -0,0 +1,61 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2024 Oppo.  All Rights Reserved.
>> +#
>> +# FS QA Test No. f2fs/003
>> +#
>> +# Test that we will simulate sqlite atomic write logic w/ below steps:
>> +# 1. create a regular file, and initialize it w/ 0xff data
>> +# 2. start transaction (via F2FS_IOC_START_ATOMIC_WRITE) on it
>> +# 3. write transaction data
>> +# 4. trigger foreground GC to migrate data block of the file
>> +# 5. commit and end the transaction (via F2FS_IOC_COMMIT_ATOMIC_WRITE)
>> +# 6. check consistency of transaction w/ in-memory and on-disk data
>> +# This is a regression test to check handling of race condition in
>> +# between atomic_write and GC.
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto quick
>> +
>> +. ./common/filter
>> +
>> +_supported_fs f2fs
>      ^^^^^
> This line is not necessary now.
> 
>> +_require_scratch
>> +_require_xfs_io_command "pwrite"
> 
> pwrite is a basic command of xfs_io, so I think this line is not necessary.
> 
>> +_require_xfs_io_command "fpunch"
>> +
>> +_scratch_mkfs >> $seqres.full
>> +_scratch_mount >> $seqres.full
>> +
>> +dbfile=$SCRATCH_MNT/dbfile
>> +foo=$SCRATCH_MNT/foo
>> +bar=$SCRATCH_MNT/bar
>> +
>> +$XFS_IO_PROG -c "pwrite 0 512k -S 0xff" -c "fsync" -f $dbfile >> $seqres.full
>> +$XFS_IO_PROG -c "pwrite 0 2m" -c "fsync" -f $foo >> $seqres.full
>> +sync
>> +
>> +# start atomic_write on dbfile & write data to dbfile
>> +$F2FS_IO_PROG write 1 0 32 zero atomic_commit $dbfile 3000 >> $seqres.full &
> 
> As there's a background process, shouldn't we do kill and wait in _cleanup
> to make sure unmount won't hit EBUSY?
> 
>> +$XFS_IO_PROG -c "fpunch 0 2m" $foo >> $seqres.full
>> +$XFS_IO_PROG -c "pwrite 0 2m" -c "fsync" -f $bar >> $seqres.full
>> +
>> +# persist all dirty data
>> +sync
>> +echo 3 > /proc/sys/vm/drop_caches
>> +
>> +# trigger foreground GC to migrate data block of atomic_file
>> +$F2FS_IO_PROG gc 1 $SCRATCH_MNT > /dev/null 2>&1
>> +
>> +# wait for atomic_write commit completion
>> +sleep 5
>> +# print in-memory data
>> +od -x $dbfile
>> +echo 3 > /proc/sys/vm/drop_caches
>> +# print on-disk data
>> +od -x $dbfile
> 
> There's a common helper named "_hexdump" in common/rc, can that be used?
> 
>> +
>> +_scratch_unmount
> 
> The SCRATCH_DEV will be unmounted at the end of the test, don't need to
> do this manually if it's not a necessary test step.

Let me update this patch according to your comments, thanks.

Thanks,

> 
> Thanks,
> Zorro
> 
>> +
>> +status=0
>> +exit
>> diff --git a/tests/f2fs/003.out b/tests/f2fs/003.out
>> new file mode 100644
>> index 00000000..d6c8a637
>> --- /dev/null
>> +++ b/tests/f2fs/003.out
>> @@ -0,0 +1,11 @@
>> +QA output created by 003
>> +0000000 0000 0000 0000 0000 0000 0000 0000 0000
>> +*
>> +0400000 ffff ffff ffff ffff ffff ffff ffff ffff
>> +*
>> +2000000
>> +0000000 0000 0000 0000 0000 0000 0000 0000 0000
>> +*
>> +0400000 ffff ffff ffff ffff ffff ffff ffff ffff
>> +*
>> +2000000
>> -- 
>> 2.40.1
>>
> 

Re: [PATCH] f2fs: test for race condition in between atomic_write and gc

[Zorro Lang]

On Wed, Jul 17, 2024 at 10:12:16AM +0800, Chao Yu wrote:
> On 2024/7/16 18:57, Zorro Lang wrote:
> > On Tue, Jun 25, 2024 at 11:04:16AM +0800, Chao Yu wrote:
> > > Test that we will simulate sqlite atomic write logic w/ below steps:
> > > 1. create a regular file, and initialize it w/ 0xff data
> > > 2. start transaction (via F2FS_IOC_START_ATOMIC_WRITE) on it
> > > 3. write transaction data
> > > 4. trigger foreground GC to migrate data block of the file
> > > 5. commit and end the transaction (via F2FS_IOC_COMMIT_ATOMIC_WRITE)
> > > 6. check consistency of transaction w/ in-memory and on-disk data
> > > This is a regression test to check handling of race condition in
> > > between atomic_write and GC.
> > 
> > Hi Chao,
> > 
> > Sorry for the late reviewing. As this's a regression test, so which onne
> > kernel commit fix this known issue, please specify it by:
> > 
> > _fixed_by_kernel_commit $commit_id $commit_subject
> 
> Hi Zorro,
> 
> Since the fixed patch has not been merged yet, so what about adding this line
> after merging the fix?

Oh, sure, but generally they'd like to use "xxxxxxxxxxxx" to be the $commit_id
(as a reminder) if there's not an assured id number. Then replace the "xxxxxx...."
with the right id number after the patch merged. But it depends on you, just
don't forget that :)

Thanks,
Zorro

> 
> > 
> > > 
> > > Cc: Jaegeuk Kim <jaegeuk@kernel.org>
> > > Cc: Daeho Jeong <daehojeong@google.com>
> > > Signed-off-by: Chao Yu <chao@kernel.org>
> > > ---
> > >   tests/f2fs/003     | 61 ++++++++++++++++++++++++++++++++++++++++++++++
> > >   tests/f2fs/003.out | 11 +++++++++
> > >   2 files changed, 72 insertions(+)
> > >   create mode 100755 tests/f2fs/003
> > >   create mode 100644 tests/f2fs/003.out
> > > 
> > > diff --git a/tests/f2fs/003 b/tests/f2fs/003
> > > new file mode 100755
> > > index 00000000..d8311c4c
> > > --- /dev/null
> > > +++ b/tests/f2fs/003
> > > @@ -0,0 +1,61 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2024 Oppo.  All Rights Reserved.
> > > +#
> > > +# FS QA Test No. f2fs/003
> > > +#
> > > +# Test that we will simulate sqlite atomic write logic w/ below steps:
> > > +# 1. create a regular file, and initialize it w/ 0xff data
> > > +# 2. start transaction (via F2FS_IOC_START_ATOMIC_WRITE) on it
> > > +# 3. write transaction data
> > > +# 4. trigger foreground GC to migrate data block of the file
> > > +# 5. commit and end the transaction (via F2FS_IOC_COMMIT_ATOMIC_WRITE)
> > > +# 6. check consistency of transaction w/ in-memory and on-disk data
> > > +# This is a regression test to check handling of race condition in
> > > +# between atomic_write and GC.
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto quick
> > > +
> > > +. ./common/filter
> > > +
> > > +_supported_fs f2fs
> >      ^^^^^
> > This line is not necessary now.
> > 
> > > +_require_scratch
> > > +_require_xfs_io_command "pwrite"
> > 
> > pwrite is a basic command of xfs_io, so I think this line is not necessary.
> > 
> > > +_require_xfs_io_command "fpunch"
> > > +
> > > +_scratch_mkfs >> $seqres.full
> > > +_scratch_mount >> $seqres.full
> > > +
> > > +dbfile=$SCRATCH_MNT/dbfile
> > > +foo=$SCRATCH_MNT/foo
> > > +bar=$SCRATCH_MNT/bar
> > > +
> > > +$XFS_IO_PROG -c "pwrite 0 512k -S 0xff" -c "fsync" -f $dbfile >> $seqres.full
> > > +$XFS_IO_PROG -c "pwrite 0 2m" -c "fsync" -f $foo >> $seqres.full
> > > +sync
> > > +
> > > +# start atomic_write on dbfile & write data to dbfile
> > > +$F2FS_IO_PROG write 1 0 32 zero atomic_commit $dbfile 3000 >> $seqres.full &
> > 
> > As there's a background process, shouldn't we do kill and wait in _cleanup
> > to make sure unmount won't hit EBUSY?
> > 
> > > +$XFS_IO_PROG -c "fpunch 0 2m" $foo >> $seqres.full
> > > +$XFS_IO_PROG -c "pwrite 0 2m" -c "fsync" -f $bar >> $seqres.full
> > > +
> > > +# persist all dirty data
> > > +sync
> > > +echo 3 > /proc/sys/vm/drop_caches
> > > +
> > > +# trigger foreground GC to migrate data block of atomic_file
> > > +$F2FS_IO_PROG gc 1 $SCRATCH_MNT > /dev/null 2>&1
> > > +
> > > +# wait for atomic_write commit completion
> > > +sleep 5
> > > +# print in-memory data
> > > +od -x $dbfile
> > > +echo 3 > /proc/sys/vm/drop_caches
> > > +# print on-disk data
> > > +od -x $dbfile
> > 
> > There's a common helper named "_hexdump" in common/rc, can that be used?
> > 
> > > +
> > > +_scratch_unmount
> > 
> > The SCRATCH_DEV will be unmounted at the end of the test, don't need to
> > do this manually if it's not a necessary test step.
> 
> Let me update this patch according to your comments, thanks.
> 
> Thanks,
> 
> > 
> > Thanks,
> > Zorro
> > 
> > > +
> > > +status=0
> > > +exit
> > > diff --git a/tests/f2fs/003.out b/tests/f2fs/003.out
> > > new file mode 100644
> > > index 00000000..d6c8a637
> > > --- /dev/null
> > > +++ b/tests/f2fs/003.out
> > > @@ -0,0 +1,11 @@
> > > +QA output created by 003
> > > +0000000 0000 0000 0000 0000 0000 0000 0000 0000
> > > +*
> > > +0400000 ffff ffff ffff ffff ffff ffff ffff ffff
> > > +*
> > > +2000000
> > > +0000000 0000 0000 0000 0000 0000 0000 0000 0000
> > > +*
> > > +0400000 ffff ffff ffff ffff ffff ffff ffff ffff
> > > +*
> > > +2000000
> > > -- 
> > > 2.40.1
> > > 
> > 
>