[PATCH] generic/new: drop caches while freeze

[PATCH] generic/new: drop caches while freeze

[Murphy Zhou]

Signed-off-by: Murphy Zhou <jencce.kernel@gmail.com>
---
 tests/generic/734     | 34 ++++++++++++++++++++++++++++++++++
 tests/generic/734.out |  2 ++
 2 files changed, 36 insertions(+)
 create mode 100755 tests/generic/734
 create mode 100644 tests/generic/734.out

diff --git a/tests/generic/734 b/tests/generic/734
new file mode 100755
index 00000000..8ca91930
--- /dev/null
+++ b/tests/generic/734
@@ -0,0 +1,34 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# FS QA Test 734
+#
+# Test possible deadlock of umount and reclaim memory
+# when there are EOF blocks in files.
+#
+. ./common/preamble
+_begin_fstest freeze auto quick
+
+_supported_fs generic
+_require_scratch
+
+$XFS_IO_PROG -fc "pwrite 0 64k" $SCRATCH_MNT/testfile >> $seqres.full
+cat $SCRATCH_MNT/testfile >> $SCRATCH_MNT/testfile1
+cat $SCRATCH_MNT/testfile >> $SCRATCH_MNT/testfile1
+
+sync
+
+fsfreeze -f $SCRATCH_MNT
+
+# This will hang if bug reproduces
+echo 3 > /proc/sys/vm/drop_caches &
+
+# Manually unfreeze after a while
+sleep 5
+fsfreeze -u $SCRATCH_MNT
+
+wait
+# success, all done
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/generic/734.out b/tests/generic/734.out
new file mode 100644
index 00000000..4299839b
--- /dev/null
+++ b/tests/generic/734.out
@@ -0,0 +1,2 @@
+QA output created by 734
+Silence is golden
-- 
2.31.1

Re: [PATCH] generic/new: drop caches while freeze

[Zorro Lang]

On Wed, Dec 13, 2023 at 05:20:43PM +0800, Murphy Zhou wrote:
> Signed-off-by: Murphy Zhou <jencce.kernel@gmail.com>
> ---
>  tests/generic/734     | 34 ++++++++++++++++++++++++++++++++++

Hi Murphy,


Thanks for this new test. Sorry that g/734 has been taken last weekend,
please rebase to the latest for-next branch.

>  tests/generic/734.out |  2 ++
>  2 files changed, 36 insertions(+)
>  create mode 100755 tests/generic/734
>  create mode 100644 tests/generic/734.out
> 
> diff --git a/tests/generic/734 b/tests/generic/734
> new file mode 100755
> index 00000000..8ca91930
> --- /dev/null
> +++ b/tests/generic/734
> @@ -0,0 +1,34 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# FS QA Test 734
> +#
> +# Test possible deadlock of umount and reclaim memory
> +# when there are EOF blocks in files.

Is there a known bug fix for this case? If yes, please specify that by:
"_fixed_by_kernel_commit xxxxxxxxxx xxxxxxxxxxxxxxxx".

> +#
> +. ./common/preamble
> +_begin_fstest freeze auto quick
> +
> +_supported_fs generic
> +_require_scratch

This's a geneirc case, not all fs support fsfreeze, so ...
_require_freeze

And ...

_scratch_mkfs >> $seqres.full ??
_scratch_mount ??

> +
> +$XFS_IO_PROG -fc "pwrite 0 64k" $SCRATCH_MNT/testfile >> $seqres.full

Could you explain why there're two duplicated lines below? If it's intended,
better to have a comment to explain why do it twice (not once or more times).

> +cat $SCRATCH_MNT/testfile >> $SCRATCH_MNT/testfile1
> +cat $SCRATCH_MNT/testfile >> $SCRATCH_MNT/testfile1
> +
> +sync
> +
> +fsfreeze -f $SCRATCH_MNT

All cases in fstests do fs freeze by xfs_freeze. If there's not special
reason, better to keep consistency. Due to the _require_freeze checks
that too.

> +
> +# This will hang if bug reproduces
> +echo 3 > /proc/sys/vm/drop_caches &

I'm wondering, if the data in cache has been written back and been cleared
before fsfreeze or this step, what will happen? If the cache data is necessary,
should we load/read more data/files to provide that?

> +
> +# Manually unfreeze after a while
> +sleep 5
> +fsfreeze -u $SCRATCH_MNT
> +
> +wait

As there's a fs freeze in this case, so we'd better to do a "forced" unfreeze
in _cleanup, to make sure the fs will be unfreezed 100%, no matter what. So
you can move these two lines to _cleanup.

> +# success, all done
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/generic/734.out b/tests/generic/734.out
> new file mode 100644
> index 00000000..4299839b
> --- /dev/null
> +++ b/tests/generic/734.out
> @@ -0,0 +1,2 @@
> +QA output created by 734
> +Silence is golden
> -- 
> 2.31.1
> 
> 

Re: [PATCH] generic/new: drop caches while freeze

[Murphy Zhou]

Hi Zorro,

On Wed, Dec 13, 2023 at 11:34 PM Zorro Lang <zlang@redhat.com> wrote:
>
> On Wed, Dec 13, 2023 at 05:20:43PM +0800, Murphy Zhou wrote:
> > Signed-off-by: Murphy Zhou <jencce.kernel@gmail.com>
> > ---
> >  tests/generic/734     | 34 ++++++++++++++++++++++++++++++++++
>
> Hi Murphy,
>
>
> Thanks for this new test. Sorry that g/734 has been taken last weekend,
> please rebase to the latest for-next branch.

Got it.
>
> >  tests/generic/734.out |  2 ++
> >  2 files changed, 36 insertions(+)
> >  create mode 100755 tests/generic/734
> >  create mode 100644 tests/generic/734.out
> >
> > diff --git a/tests/generic/734 b/tests/generic/734
> > new file mode 100755
> > index 00000000..8ca91930
> > --- /dev/null
> > +++ b/tests/generic/734
> > @@ -0,0 +1,34 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# FS QA Test 734
> > +#
> > +# Test possible deadlock of umount and reclaim memory
> > +# when there are EOF blocks in files.
>
> Is there a known bug fix for this case? If yes, please specify that by:
> "_fixed_by_kernel_commit xxxxxxxxxx xxxxxxxxxxxxxxxx".

Maybe a patchset but I'm not sure of it. In this situation I think
we can skip this part?

>
> > +#
> > +. ./common/preamble
> > +_begin_fstest freeze auto quick
> > +
> > +_supported_fs generic
> > +_require_scratch
>
> This's a geneirc case, not all fs support fsfreeze, so ...
> _require_freeze

Got it.
>
> And ...
>
> _scratch_mkfs >> $seqres.full ??
> _scratch_mount ??

OK.
>
> > +
> > +$XFS_IO_PROG -fc "pwrite 0 64k" $SCRATCH_MNT/testfile >> $seqres.full
>
> Could you explain why there're two duplicated lines below? If it's intended,
> better to have a comment to explain why do it twice (not once or more times).

OK.
>
> > +cat $SCRATCH_MNT/testfile >> $SCRATCH_MNT/testfile1
> > +cat $SCRATCH_MNT/testfile >> $SCRATCH_MNT/testfile1
> > +
> > +sync
> > +
> > +fsfreeze -f $SCRATCH_MNT
>
> All cases in fstests do fs freeze by xfs_freeze. If there's not special
> reason, better to keep consistency. Due to the _require_freeze checks
> that too.

OK.
>
> > +
> > +# This will hang if bug reproduces
> > +echo 3 > /proc/sys/vm/drop_caches &
>
> I'm wondering, if the data in cache has been written back and been cleared
> before fsfreeze or this step, what will happen? If the cache data is necessary,
> should we load/read more data/files to provide that?

Agree. I'll add more files.
>
> > +
> > +# Manually unfreeze after a while
> > +sleep 5
> > +fsfreeze -u $SCRATCH_MNT
> > +
> > +wait
>
> As there's a fs freeze in this case, so we'd better to do a "forced" unfreeze
> in _cleanup, to make sure the fs will be unfreezed 100%, no matter what. So
> you can move these two lines to _cleanup.

Good idea!

Thanks for reviewing!

Murphy
>
> > +# success, all done
> > +echo "Silence is golden"
> > +status=0
> > +exit
> > diff --git a/tests/generic/734.out b/tests/generic/734.out
> > new file mode 100644
> > index 00000000..4299839b
> > --- /dev/null
> > +++ b/tests/generic/734.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 734
> > +Silence is golden
> > --
> > 2.31.1
> >
> >
>

[PATCH v2] generic/new: drop caches while freeze

[Murphy Zhou]

Signed-off-by: Murphy Zhou <jencce.kernel@gmail.com>
---
v2:
  re-number to 738
  mkfs and mount scratch in the begining
  create more files and comment why
  unfreeze in cleanup

 tests/generic/738     | 54 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/738.out |  2 ++
 2 files changed, 56 insertions(+)
 create mode 100755 tests/generic/738
 create mode 100644 tests/generic/738.out

diff --git a/tests/generic/738 b/tests/generic/738
new file mode 100755
index 00000000..9f1f9d6b
--- /dev/null
+++ b/tests/generic/738
@@ -0,0 +1,54 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# FS QA Test 738
+#
+# Test possible deadlock of umount and reclaim memory
+# when there are EOF blocks in files.
+#
+. ./common/preamble
+_begin_fstest freeze auto quick
+
+_cleanup()
+{
+	xfs_freeze -u $SCRATCH_MNT
+	wait
+	cd /
+	rm -r -f $tmp.*
+}
+
+_supported_fs generic
+_require_scratch
+_require_freeze
+
+_scratch_mkfs >> $seqres.full
+_scratch_mount
+
+create_eof_block_file()
+{
+	# Create several EOF blocks in the new file
+	for j in $(seq 1 5); do
+		cat $SCRATCH_MNT/testfile >> $1
+	done
+}
+
+$XFS_IO_PROG -fc "pwrite 0 64k" $SCRATCH_MNT/testfile >> $seqres.full
+
+# Create enough files to make sure there is enough cache
+for i in $(seq 0 1024); do
+	create_eof_block_file $SCRATCH_MNT/$i
+done
+
+sync
+
+xfs_freeze -f $SCRATCH_MNT
+
+# This will hang if bug reproduces
+echo 3 > /proc/sys/vm/drop_caches &
+
+# Wait a while before exiting and unfreezing.
+sleep 3
+# success, all done
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/generic/738.out b/tests/generic/738.out
new file mode 100644
index 00000000..5719515a
--- /dev/null
+++ b/tests/generic/738.out
@@ -0,0 +1,2 @@
+QA output created by 738
+Silence is golden
-- 
2.31.1

Re: [PATCH v2] generic/new: drop caches while freeze

[Zorro Lang]

On Fri, Dec 15, 2023 at 07:55:05PM +0800, Murphy Zhou wrote:
> Signed-off-by: Murphy Zhou <jencce.kernel@gmail.com>
> ---
> v2:
>   re-number to 738
>   mkfs and mount scratch in the begining
>   create more files and comment why
>   unfreeze in cleanup

This version looks better to me. I'll merge this patch with a few of
small changes. Thanks for this new coverage!

Reviewed-by: Zorro Lang <zlang@redhat.com>

> 
>  tests/generic/738     | 54 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/738.out |  2 ++
>  2 files changed, 56 insertions(+)
>  create mode 100755 tests/generic/738
>  create mode 100644 tests/generic/738.out
> 
> diff --git a/tests/generic/738 b/tests/generic/738
> new file mode 100755
> index 00000000..9f1f9d6b
> --- /dev/null
> +++ b/tests/generic/738
> @@ -0,0 +1,54 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# FS QA Test 738
> +#
> +# Test possible deadlock of umount and reclaim memory
> +# when there are EOF blocks in files.
> +#
> +. ./common/preamble
> +_begin_fstest freeze auto quick
> +
> +_cleanup()
> +{
> +	xfs_freeze -u $SCRATCH_MNT
> +	wait
> +	cd /
> +	rm -r -f $tmp.*
> +}
> +
> +_supported_fs generic
> +_require_scratch
> +_require_freeze
> +
> +_scratch_mkfs >> $seqres.full
> +_scratch_mount
> +
> +create_eof_block_file()
> +{
> +	# Create several EOF blocks in the new file
> +	for j in $(seq 1 5); do
> +		cat $SCRATCH_MNT/testfile >> $1
> +	done
> +}
> +
> +$XFS_IO_PROG -fc "pwrite 0 64k" $SCRATCH_MNT/testfile >> $seqres.full
> +
> +# Create enough files to make sure there is enough cache
> +for i in $(seq 0 1024); do
> +	create_eof_block_file $SCRATCH_MNT/$i
> +done
> +
> +sync
> +
> +xfs_freeze -f $SCRATCH_MNT
> +
> +# This will hang if bug reproduces
> +echo 3 > /proc/sys/vm/drop_caches &
> +
> +# Wait a while before exiting and unfreezing.
> +sleep 3
> +# success, all done
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/generic/738.out b/tests/generic/738.out
> new file mode 100644
> index 00000000..5719515a
> --- /dev/null
> +++ b/tests/generic/738.out
> @@ -0,0 +1,2 @@
> +QA output created by 738
> +Silence is golden
> -- 
> 2.31.1
>