[PATCH v3] generic: add a test to ensure that page is properly filled before write

[PATCH v3] generic: add a test to ensure that page is properly filled before write

[Jeff Layton]

We had a broken optimization in cephfs and netfs lib that could cause
part of a page to be improperly zeroed-out when writing to an offset
that was beyond the EOF but in an existing page.

Add a simple test that would have caught this.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 tests/generic/639     | 39 +++++++++++++++++++++++++++++++++++++++
 tests/generic/639.out |  5 +++++
 2 files changed, 44 insertions(+)
 create mode 100755 tests/generic/639
 create mode 100644 tests/generic/639.out

v3: adapt to new test template

diff --git a/tests/generic/639 b/tests/generic/639
new file mode 100755
index 000000000000..c30f7644a2fc
--- /dev/null
+++ b/tests/generic/639
@@ -0,0 +1,39 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2021, Jeff Layton <jlayton@redhat.com>
+#
+# FS QA Test No. 639
+#
+# Open a file and write a little data to it. Unmount (to clean out the cache)
+# and then mount again. Then write some data to it beyond the EOF and ensure
+# the result is correct.
+#
+# Prompted by a bug in ceph_write_begin that was fixed by commit 827a746f405d.
+#
+. ./common/preamble
+_begin_fstest auto quick rw
+
+# Import common functions.
+. ./common/filter
+
+# real QA test starts here
+_supported_fs generic
+_require_test
+
+testfile="$TEST_DIR/test_write_begin.$$"
+
+# write some data to file and fsync it out
+$XFS_IO_PROG -f -c "pwrite -q 0 32" $testfile
+
+# cycle the mount to clean out the pagecache
+_test_cycle_mount
+
+# now, write to the file (near the end)
+$XFS_IO_PROG -c "pwrite -q 32 32" $testfile
+
+# dump what we think is in there
+echo "The result should be 64 bytes filled with 0xcd:"
+hexdump -C $testfile
+
+status=0
+exit
diff --git a/tests/generic/639.out b/tests/generic/639.out
new file mode 100644
index 000000000000..9bf0bac96864
--- /dev/null
+++ b/tests/generic/639.out
@@ -0,0 +1,5 @@
+QA output created by 639
+The result should be 64 bytes filled with 0xcd:
+00000000  cd cd cd cd cd cd cd cd  cd cd cd cd cd cd cd cd  |................|
+*
+00000040
-- 
2.31.1

Re: [PATCH v3] generic: add a test to ensure that page is properly filled before write

[Zorro Lang]

On Fri, Jul 02, 2021 at 09:40:24AM -0400, Jeff Layton wrote:
> We had a broken optimization in cephfs and netfs lib that could cause
> part of a page to be improperly zeroed-out when writing to an offset
> that was beyond the EOF but in an existing page.
> 
> Add a simple test that would have caught this.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  tests/generic/639     | 39 +++++++++++++++++++++++++++++++++++++++
>  tests/generic/639.out |  5 +++++
>  2 files changed, 44 insertions(+)
>  create mode 100755 tests/generic/639
>  create mode 100644 tests/generic/639.out
> 
> v3: adapt to new test template
> 
> diff --git a/tests/generic/639 b/tests/generic/639
> new file mode 100755
> index 000000000000..c30f7644a2fc
> --- /dev/null
> +++ b/tests/generic/639
> @@ -0,0 +1,39 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2021, Jeff Layton <jlayton@redhat.com>
> +#
> +# FS QA Test No. 639
> +#
> +# Open a file and write a little data to it. Unmount (to clean out the cache)
> +# and then mount again. Then write some data to it beyond the EOF and ensure
> +# the result is correct.
> +#
> +# Prompted by a bug in ceph_write_begin that was fixed by commit 827a746f405d.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick rw
> +
> +# Import common functions.
> +. ./common/filter
> +
> +# real QA test starts here
> +_supported_fs generic
> +_require_test
> +
> +testfile="$TEST_DIR/test_write_begin.$$"
> +
> +# write some data to file and fsync it out
> +$XFS_IO_PROG -f -c "pwrite -q 0 32" $testfile

Looks good to me, if you've used this case to reproduce that issue.

Just one question, the comment says "fsync it out", does that mean we need an
explicit "fsync" call (e.g. -c "pwrite -q 0 32" -c "fsync") to reproduce this
bug? Or there's an implicit "fsync" somewhere?

Thanks,
Zorro

> +
> +# cycle the mount to clean out the pagecache
> +_test_cycle_mount
> +
> +# now, write to the file (near the end)
> +$XFS_IO_PROG -c "pwrite -q 32 32" $testfile
> +
> +# dump what we think is in there
> +echo "The result should be 64 bytes filled with 0xcd:"
> +hexdump -C $testfile
> +
> +status=0
> +exit
> diff --git a/tests/generic/639.out b/tests/generic/639.out
> new file mode 100644
> index 000000000000..9bf0bac96864
> --- /dev/null
> +++ b/tests/generic/639.out
> @@ -0,0 +1,5 @@
> +QA output created by 639
> +The result should be 64 bytes filled with 0xcd:
> +00000000  cd cd cd cd cd cd cd cd  cd cd cd cd cd cd cd cd  |................|
> +*
> +00000040
> -- 
> 2.31.1
> 

Re: [PATCH v3] generic: add a test to ensure that page is properly filled before write

[Zorro Lang]

On Mon, Jul 05, 2021 at 07:04:34PM +0800, Zorro Lang wrote:
> On Fri, Jul 02, 2021 at 09:40:24AM -0400, Jeff Layton wrote:
> > We had a broken optimization in cephfs and netfs lib that could cause
> > part of a page to be improperly zeroed-out when writing to an offset
> > that was beyond the EOF but in an existing page.
> > 
> > Add a simple test that would have caught this.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  tests/generic/639     | 39 +++++++++++++++++++++++++++++++++++++++
> >  tests/generic/639.out |  5 +++++
> >  2 files changed, 44 insertions(+)
> >  create mode 100755 tests/generic/639
> >  create mode 100644 tests/generic/639.out
> > 
> > v3: adapt to new test template
> > 
> > diff --git a/tests/generic/639 b/tests/generic/639
> > new file mode 100755
> > index 000000000000..c30f7644a2fc
> > --- /dev/null
> > +++ b/tests/generic/639
> > @@ -0,0 +1,39 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2021, Jeff Layton <jlayton@redhat.com>
> > +#
> > +# FS QA Test No. 639
> > +#
> > +# Open a file and write a little data to it. Unmount (to clean out the cache)
> > +# and then mount again. Then write some data to it beyond the EOF and ensure
> > +# the result is correct.
> > +#
> > +# Prompted by a bug in ceph_write_begin that was fixed by commit 827a746f405d.
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick rw
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +
> > +# real QA test starts here
> > +_supported_fs generic
> > +_require_test
> > +
> > +testfile="$TEST_DIR/test_write_begin.$$"
> > +
> > +# write some data to file and fsync it out
> > +$XFS_IO_PROG -f -c "pwrite -q 0 32" $testfile
> 
> Looks good to me, if you've used this case to reproduce that issue.
> 
> Just one question, the comment says "fsync it out", does that mean we need an
> explicit "fsync" call (e.g. -c "pwrite -q 0 32" -c "fsync") to reproduce this
> bug? Or there's an implicit "fsync" somewhere?

Oh, this patch has just been merged, sorry for my late review. You can ignore
this message.

Thanks,
Zorro

> 
> Thanks,
> Zorro
> 
> > +
> > +# cycle the mount to clean out the pagecache
> > +_test_cycle_mount
> > +
> > +# now, write to the file (near the end)
> > +$XFS_IO_PROG -c "pwrite -q 32 32" $testfile
> > +
> > +# dump what we think is in there
> > +echo "The result should be 64 bytes filled with 0xcd:"
> > +hexdump -C $testfile
> > +
> > +status=0
> > +exit
> > diff --git a/tests/generic/639.out b/tests/generic/639.out
> > new file mode 100644
> > index 000000000000..9bf0bac96864
> > --- /dev/null
> > +++ b/tests/generic/639.out
> > @@ -0,0 +1,5 @@
> > +QA output created by 639
> > +The result should be 64 bytes filled with 0xcd:
> > +00000000  cd cd cd cd cd cd cd cd  cd cd cd cd cd cd cd cd  |................|
> > +*
> > +00000040
> > -- 
> > 2.31.1
> > 

Re: [PATCH v3] generic: add a test to ensure that page is properly filled before write

[Jeff Layton]

On Mon, 2021-07-05 at 19:04 +0800, Zorro Lang wrote:
> On Fri, Jul 02, 2021 at 09:40:24AM -0400, Jeff Layton wrote:
> > We had a broken optimization in cephfs and netfs lib that could cause
> > part of a page to be improperly zeroed-out when writing to an offset
> > that was beyond the EOF but in an existing page.
> > 
> > Add a simple test that would have caught this.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  tests/generic/639     | 39 +++++++++++++++++++++++++++++++++++++++
> >  tests/generic/639.out |  5 +++++
> >  2 files changed, 44 insertions(+)
> >  create mode 100755 tests/generic/639
> >  create mode 100644 tests/generic/639.out
> > 
> > v3: adapt to new test template
> > 
> > diff --git a/tests/generic/639 b/tests/generic/639
> > new file mode 100755
> > index 000000000000..c30f7644a2fc
> > --- /dev/null
> > +++ b/tests/generic/639
> > @@ -0,0 +1,39 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2021, Jeff Layton <jlayton@redhat.com>
> > +#
> > +# FS QA Test No. 639
> > +#
> > +# Open a file and write a little data to it. Unmount (to clean out the cache)
> > +# and then mount again. Then write some data to it beyond the EOF and ensure
> > +# the result is correct.
> > +#
> > +# Prompted by a bug in ceph_write_begin that was fixed by commit 827a746f405d.
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick rw
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +
> > +# real QA test starts here
> > +_supported_fs generic
> > +_require_test
> > +
> > +testfile="$TEST_DIR/test_write_begin.$$"
> > +
> > +# write some data to file and fsync it out
> > +$XFS_IO_PROG -f -c "pwrite -q 0 32" $testfile
> 
> Looks good to me, if you've used this case to reproduce that issue.
> 
> Just one question, the comment says "fsync it out", does that mean we need an
> explicit "fsync" call (e.g. -c "pwrite -q 0 32" -c "fsync") to reproduce this
> bug? Or there's an implicit "fsync" somewhere?
> 
> 

No, there is no implicit fsync. This is holdover from copy/paste that I
did from another test. The patch is already merged now, but if we expand
this test to cover other cases, I'll plan to clean up the comment then.

Thanks,
Jeff

> 
> > +
> > +# cycle the mount to clean out the pagecache
> > +_test_cycle_mount
> > +
> > +# now, write to the file (near the end)
> > +$XFS_IO_PROG -c "pwrite -q 32 32" $testfile
> > +
> > +# dump what we think is in there
> > +echo "The result should be 64 bytes filled with 0xcd:"
> > +hexdump -C $testfile
> > +
> > +status=0
> > +exit
> > diff --git a/tests/generic/639.out b/tests/generic/639.out
> > new file mode 100644
> > index 000000000000..9bf0bac96864
> > --- /dev/null
> > +++ b/tests/generic/639.out
> > @@ -0,0 +1,5 @@
> > +QA output created by 639
> > +The result should be 64 bytes filled with 0xcd:
> > +00000000  cd cd cd cd cd cd cd cd  cd cd cd cd cd cd cd cd  |................|
> > +*
> > +00000040
> > -- 
> > 2.31.1
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3] generic: add a test to ensure that page is properly filled before write

[Eryu Guan]

On Tue, Jul 06, 2021 at 06:56:41AM -0400, Jeff Layton wrote:
> On Mon, 2021-07-05 at 19:04 +0800, Zorro Lang wrote:
> > On Fri, Jul 02, 2021 at 09:40:24AM -0400, Jeff Layton wrote:
> > > We had a broken optimization in cephfs and netfs lib that could cause
> > > part of a page to be improperly zeroed-out when writing to an offset
> > > that was beyond the EOF but in an existing page.
> > > 
> > > Add a simple test that would have caught this.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  tests/generic/639     | 39 +++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/639.out |  5 +++++
> > >  2 files changed, 44 insertions(+)
> > >  create mode 100755 tests/generic/639
> > >  create mode 100644 tests/generic/639.out
> > > 
> > > v3: adapt to new test template
> > > 
> > > diff --git a/tests/generic/639 b/tests/generic/639
> > > new file mode 100755
> > > index 000000000000..c30f7644a2fc
> > > --- /dev/null
> > > +++ b/tests/generic/639
> > > @@ -0,0 +1,39 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2021, Jeff Layton <jlayton@redhat.com>
> > > +#
> > > +# FS QA Test No. 639
> > > +#
> > > +# Open a file and write a little data to it. Unmount (to clean out the cache)
> > > +# and then mount again. Then write some data to it beyond the EOF and ensure
> > > +# the result is correct.
> > > +#
> > > +# Prompted by a bug in ceph_write_begin that was fixed by commit 827a746f405d.
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto quick rw
> > > +
> > > +# Import common functions.
> > > +. ./common/filter
> > > +
> > > +# real QA test starts here
> > > +_supported_fs generic
> > > +_require_test
> > > +
> > > +testfile="$TEST_DIR/test_write_begin.$$"
> > > +
> > > +# write some data to file and fsync it out
> > > +$XFS_IO_PROG -f -c "pwrite -q 0 32" $testfile
> > 
> > Looks good to me, if you've used this case to reproduce that issue.
> > 
> > Just one question, the comment says "fsync it out", does that mean we need an
> > explicit "fsync" call (e.g. -c "pwrite -q 0 32" -c "fsync") to reproduce this
> > bug? Or there's an implicit "fsync" somewhere?
> > 
> > 
> 
> No, there is no implicit fsync. This is holdover from copy/paste that I
> did from another test. The patch is already merged now, but if we expand
> this test to cover other cases, I'll plan to clean up the comment then.
> 
> Thanks,
> Jeff
> 
> > 
> > > +
> > > +# cycle the mount to clean out the pagecache
> > > +_test_cycle_mount

I think this cycle mount accounts as an implicit fsync?

Thanks,
Eryu

> > > +
> > > +# now, write to the file (near the end)
> > > +$XFS_IO_PROG -c "pwrite -q 32 32" $testfile
> > > +
> > > +# dump what we think is in there
> > > +echo "The result should be 64 bytes filled with 0xcd:"
> > > +hexdump -C $testfile
> > > +
> > > +status=0
> > > +exit
> > > diff --git a/tests/generic/639.out b/tests/generic/639.out
> > > new file mode 100644
> > > index 000000000000..9bf0bac96864
> > > --- /dev/null
> > > +++ b/tests/generic/639.out
> > > @@ -0,0 +1,5 @@
> > > +QA output created by 639
> > > +The result should be 64 bytes filled with 0xcd:
> > > +00000000  cd cd cd cd cd cd cd cd  cd cd cd cd cd cd cd cd  |................|
> > > +*
> > > +00000040
> > > -- 
> > > 2.31.1
> > > 
> > 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3] generic: add a test to ensure that page is properly filled before write

[Jeff Layton]

On Wed, 2021-07-07 at 13:27 +0800, Eryu Guan wrote:
> On Tue, Jul 06, 2021 at 06:56:41AM -0400, Jeff Layton wrote:
> > On Mon, 2021-07-05 at 19:04 +0800, Zorro Lang wrote:
> > > On Fri, Jul 02, 2021 at 09:40:24AM -0400, Jeff Layton wrote:
> > > > We had a broken optimization in cephfs and netfs lib that could cause
> > > > part of a page to be improperly zeroed-out when writing to an offset
> > > > that was beyond the EOF but in an existing page.
> > > > 
> > > > Add a simple test that would have caught this.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  tests/generic/639     | 39 +++++++++++++++++++++++++++++++++++++++
> > > >  tests/generic/639.out |  5 +++++
> > > >  2 files changed, 44 insertions(+)
> > > >  create mode 100755 tests/generic/639
> > > >  create mode 100644 tests/generic/639.out
> > > > 
> > > > v3: adapt to new test template
> > > > 
> > > > diff --git a/tests/generic/639 b/tests/generic/639
> > > > new file mode 100755
> > > > index 000000000000..c30f7644a2fc
> > > > --- /dev/null
> > > > +++ b/tests/generic/639
> > > > @@ -0,0 +1,39 @@
> > > > +#! /bin/bash
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +# Copyright (c) 2021, Jeff Layton <jlayton@redhat.com>
> > > > +#
> > > > +# FS QA Test No. 639
> > > > +#
> > > > +# Open a file and write a little data to it. Unmount (to clean out the cache)
> > > > +# and then mount again. Then write some data to it beyond the EOF and ensure
> > > > +# the result is correct.
> > > > +#
> > > > +# Prompted by a bug in ceph_write_begin that was fixed by commit 827a746f405d.
> > > > +#
> > > > +. ./common/preamble
> > > > +_begin_fstest auto quick rw
> > > > +
> > > > +# Import common functions.
> > > > +. ./common/filter
> > > > +
> > > > +# real QA test starts here
> > > > +_supported_fs generic
> > > > +_require_test
> > > > +
> > > > +testfile="$TEST_DIR/test_write_begin.$$"
> > > > +
> > > > +# write some data to file and fsync it out
> > > > +$XFS_IO_PROG -f -c "pwrite -q 0 32" $testfile
> > > 
> > > Looks good to me, if you've used this case to reproduce that issue.
> > > 
> > > Just one question, the comment says "fsync it out", does that mean we need an
> > > explicit "fsync" call (e.g. -c "pwrite -q 0 32" -c "fsync") to reproduce this
> > > bug? Or there's an implicit "fsync" somewhere?
> > > 
> > > 
> > 
> > No, there is no implicit fsync. This is holdover from copy/paste that I
> > did from another test. The patch is already merged now, but if we expand
> > this test to cover other cases, I'll plan to clean up the comment then.
> > 
> > Thanks,
> > Jeff
> > 
> > > 
> > > > +
> > > > +# cycle the mount to clean out the pagecache
> > > > +_test_cycle_mount
> 
> I think this cycle mount accounts as an implicit fsync?
> 

Yes, the data should get synced out naturally during a umount. The
comment is still a bit misleading though since it may not happen until
this point.

> > > > +
> > > > +# now, write to the file (near the end)
> > > > +$XFS_IO_PROG -c "pwrite -q 32 32" $testfile
> > > > +
> > > > +# dump what we think is in there
> > > > +echo "The result should be 64 bytes filled with 0xcd:"
> > > > +hexdump -C $testfile
> > > > +
> > > > +status=0
> > > > +exit
> > > > diff --git a/tests/generic/639.out b/tests/generic/639.out
> > > > new file mode 100644
> > > > index 000000000000..9bf0bac96864
> > > > --- /dev/null
> > > > +++ b/tests/generic/639.out
> > > > @@ -0,0 +1,5 @@
> > > > +QA output created by 639
> > > > +The result should be 64 bytes filled with 0xcd:
> > > > +00000000  cd cd cd cd cd cd cd cd  cd cd cd cd cd cd cd cd  |................|
> > > > +*
> > > > +00000040
> > > > -- 
> > > > 2.31.1
> > > > 
> > > 
> > 
> > -- 
> > Jeff Layton <jlayton@kernel.org>

-- 
Jeff Layton <jlayton@kernel.org>