* [PATCH v3] generic: add a test to ensure that page is properly filled before write @ 2021-07-02 13:40 Jeff Layton 2021-07-05 11:04 ` Zorro Lang 0 siblings, 1 reply; 6+ messages in thread From: Jeff Layton @ 2021-07-02 13:40 UTC (permalink / raw) To: Eryu Guan; +Cc: fstests, aweits, dhowells, willy 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] generic: add a test to ensure that page is properly filled before write 2021-07-02 13:40 [PATCH v3] generic: add a test to ensure that page is properly filled before write Jeff Layton @ 2021-07-05 11:04 ` Zorro Lang 2021-07-05 11:10 ` Zorro Lang 2021-07-06 10:56 ` Jeff Layton 0 siblings, 2 replies; 6+ messages in thread From: Zorro Lang @ 2021-07-05 11:04 UTC (permalink / raw) To: Jeff Layton; +Cc: Eryu Guan, fstests, aweits, dhowells, willy 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] generic: add a test to ensure that page is properly filled before write 2021-07-05 11:04 ` Zorro Lang @ 2021-07-05 11:10 ` Zorro Lang 2021-07-06 10:56 ` Jeff Layton 1 sibling, 0 replies; 6+ messages in thread From: Zorro Lang @ 2021-07-05 11:10 UTC (permalink / raw) To: Jeff Layton, Eryu Guan, fstests 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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] generic: add a test to ensure that page is properly filled before write 2021-07-05 11:04 ` Zorro Lang 2021-07-05 11:10 ` Zorro Lang @ 2021-07-06 10:56 ` Jeff Layton 2021-07-07 5:27 ` Eryu Guan 1 sibling, 1 reply; 6+ messages in thread From: Jeff Layton @ 2021-07-06 10:56 UTC (permalink / raw) To: Zorro Lang; +Cc: Eryu Guan, fstests, aweits, dhowells, willy 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] generic: add a test to ensure that page is properly filled before write 2021-07-06 10:56 ` Jeff Layton @ 2021-07-07 5:27 ` Eryu Guan 2021-07-07 10:46 ` Jeff Layton 0 siblings, 1 reply; 6+ messages in thread From: Eryu Guan @ 2021-07-07 5:27 UTC (permalink / raw) To: Jeff Layton; +Cc: Zorro Lang, Eryu Guan, fstests, aweits, dhowells, willy 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] generic: add a test to ensure that page is properly filled before write 2021-07-07 5:27 ` Eryu Guan @ 2021-07-07 10:46 ` Jeff Layton 0 siblings, 0 replies; 6+ messages in thread From: Jeff Layton @ 2021-07-07 10:46 UTC (permalink / raw) To: Eryu Guan; +Cc: Zorro Lang, Eryu Guan, fstests, aweits, dhowells, willy 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-07 10:46 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-02 13:40 [PATCH v3] generic: add a test to ensure that page is properly filled before write Jeff Layton 2021-07-05 11:04 ` Zorro Lang 2021-07-05 11:10 ` Zorro Lang 2021-07-06 10:56 ` Jeff Layton 2021-07-07 5:27 ` Eryu Guan 2021-07-07 10:46 ` Jeff Layton
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.