DAX: Fix mmap-write not updating c/mtime
Subsystems (6)
cover DAX: Fix mmap-write not updating c/mtime
Hi
First submitted an xfstest (generic/080) that demonstrates our current problem.
When run on ext4-dax mount, and I'd imagine Dave's xfs-dax mount this test fails, on regular mount it works just fine.
then 2 patches to fix it.
The main problem is that current mm/memory.c will no call us with page_mkwrite if we do not have an actual page mapping, which is what DAX uses. The solution presented here introduces a new pfn_mkwrite to solve this problem. Please see patch-2 for details.
I've been running with this patch for 4 month both HW and VMs with no apparent danger, but see patch-2 I played it safe.
List of patches:
[PATCH 1/2] generic: 080 test that mmap-write updates c/mtime
This one is for Dave Chinner to submit to xfstests
[PATCH 1/2] mm: New pfn_mkwrite same as page_mkwrite for VM_PFNMAP
This patch I need help with please look into it?
[PATCH 2/2] dax: use pfn_mkwrite to update mtime
Please review and comment.
Thanks
Boaz
PATCH 1/3 xfstests: generic/080 test that mmap-write updates c/mtime
From: Yigal Korman <[email protected]>
when using mmap() for file i/o, writing to the file should update it's c/mtime. Specifically if we first mmap-read from a page, then memap-write to the same page.
This test was failing for the initial submission of DAX because pfn based mapping do not have an page_mkwrite called for them. The new Kernel patches that introduce pfn_mkwrite fixes this test.
--- .gitignore | 1 + src/Makefile | 2 +- src/mmap_mtime.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/080 | 53 ++++++++++++++++++++++ tests/generic/080.out | 4 ++ tests/generic/group | 1 + 6 files changed, 182 insertions(+), 1 deletion(-) create mode 100644 src/mmap_mtime.c create mode 100644 tests/generic/080 create mode 100644 tests/generic/080.out
.gitignore +1 -0
@@ -119,6 +119,7 @@ /src/cloner /src/renameat2 /src/t_rename_overwrite +/src/mmap_mtime # dmapi/ binaries /dmapi/src/common/cmd/read_invis
src/Makefile +1 -1
@@ -19,7 +19,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ bulkstat_unlink_test_modified t_dir_offset t_futimens t_immutable \ stale_handle pwrite_mmap_blocked t_dir_offset2 seek_sanity_test \ seek_copy_test t_readdir_1 t_readdir_2 fsync-tester nsexec cloner \ - renameat2 t_getcwd e4compact + renameat2 t_getcwd e4compact mmap_mtime SUBDIRS =
src/mmap_mtime.c new file +122 -0
122 lines not on this page · raw message
tests/generic/080 new file +53 -0
53 lines not on this page · raw message
tests/generic/080.out new file +4 -0
@@ -0,0 +1,4 @@ +QA output created by 080 +reading page...done +writing something...done +Success. mtime was updated as expected
tests/generic/group +1 -0
@@ -77,6 +77,7 @@ 076 metadata rw udf auto quick stress 077 acl attr auto enospc 079 acl attr ioctl metadata auto quick +080 auto quick 083 rw auto enospc stress 088 perms auto quick 089 metadata auto
Signature · 8 lines
1.9.3 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [PATCH 1/3] xfstests: generic/080 test that mmap-write updates c/mtime
Quoted 9 lines · Boaz Harrosh
From: Yigal Korman <[email protected]>
when using
mmap()for file i/o, writing to the file should update it's c/mtime. Specifically if we first mmap-read from a page, then memap-write to the same page.This test was failing for the initial submission of DAX because pfn based mapping do not have an
page_mkwritecalled for them. The new Kernel patches that introducepfn_mkwritefixes this test.
This is a lot more complex than it needs to be - xfs_io does everything we already need, so the test really just needs to follow the template set out by generic/309. i.e:
# pattern the file. $XFS_IO_PROG -f -c "pwrite 0 64k" -c fsync $testfile | _filter_xfs_io
# sample timestamps. mtime1=`stat -c %Y $testfile` ctime1=`stat -c %Z $testfile`
# map read followed by map write to trigger timestamp change sleep 2 $XFS_IO_PROG -c "mmap 0 64k" -c "mread 0 64k" -c "mwrite 0 4k" $testfile |_filter_xfs_io
# sample and check timestamps have changed. mtime2=`stat -c %Y $testsfile` ctime2=`stat -c %Z $testsfile`
if [ "$mtime1" == "$mtime2" ]; then
echo "mtime not updated"
let status=$status+1
fi
if [ "$ctime1" == "$ctime2" ]; then
echo "ctime not updated"
let status=$status+1
fi
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
PATCH v2 1/3 xfstest: generic/080 test that mmap-write updates c/mtime
From: Dave Chinner <[email protected]>
when using mmap() for file i/o, writing to the file should update it's c/mtime. Specifically if we first mmap-read from a page, then memap-write to the same page.
This test was failing for the initial submission of DAX because pfn based mapping do not have an page_mkwrite called for them. The new Kernel patches that introduce pfn_mkwrite fixes this test.
Written by Dave Chinner but edited and tested by:
Omer Zilberberg
---
Dave hands-up man, it looks like you edited this directly in the email, but there was not even a single typo.
We have tested this both with and without the pfn_mkwrite patch. And it works as expected fails without and success with.
Thanks
tests/generic/080 | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/080.out | 2 ++ tests/generic/group | 1 + 3 files changed, 82 insertions(+) create mode 100755 tests/generic/080 create mode 100644 tests/generic/080.out
tests/generic/080 new file +79 -0
@@ -0,0 +1,79 @@ +#! /bin/bash +# FS QA Test No. 080 +# +# Verify that mtime is updated when writing to mmap-ed pages +# +#----------------------------------------------------------------------- +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- +# + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=0 +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* + rm -f $TEST_DIR/mmap_mtime_testfile +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# real QA test starts here + +# Modify as appropriate. +_supported_fs generic +_supported_os IRIX Linux +_require_test + +echo "Silence is golden." +rm -f $seqres.full + +# pattern the file. +testfile=$TEST_DIR/mmap_mtime_testfile +$XFS_IO_PROG -f -c "pwrite 0 4k" -c fsync $testfile >> $seqres.full + +# sample timestamps. +mtime1=`stat -c %Y $testfile` +ctime1=`stat -c %Z $testfile` +echo "before mwrite: $mtime1 $ctime1" >> $seqres.full + +# map read followed by map write to trigger timestamp change +sleep 2 +$XFS_IO_PROG -c "mmap 0 4k" -c "mread 0 4k" -c "mwrite 0 4k" $testfile |_filter_xfs_io >> $seqres.full + +# sample and verify that timestamps have changed. +mtime2=`stat -c %Y $testfile` +ctime2=`stat -c %Z $testfile` +echo "after mwrite : $mtime2 $ctime2" >> $seqres.full + +if [ "$mtime1" == "$mtime2" ]; then + echo "mtime not updated" + let status=$status+1 +fi +if [ "$ctime1" == "$ctime2" ]; then + echo "ctime not updated" + let status=$status+1 +fi + +exit
tests/generic/080.out new file +2 -0
@@ -0,0 +1,2 @@ +QA output created by 080 +Silence is golden.
tests/generic/group +1 -0
@@ -77,6 +77,7 @@ 076 metadata rw udf auto quick stress 077 acl attr auto enospc 079 acl attr ioctl metadata auto quick +080 auto quick 083 rw auto enospc stress 088 perms auto quick 089 metadata auto
Signature · 1 line
1.9.3
Re: [PATCH 1/3 v2] xfstest: generic/080 test that mmap-write updates c/mtime
I again forgot to CC: [email protected]
Thanks
Boaz
Quoted 138 lines · Boaz Harrosh
From: Dave Chinner <[email protected]>
when using
mmap()for file i/o, writing to the file should update it's c/mtime. Specifically if we first mmap-read from a page, then memap-write to the same page.This test was failing for the initial submission of DAX because pfn based mapping do not have an
page_mkwritecalled for them. The new Kernel patches that introducepfn_mkwritefixes this test.Written by Dave Chinner but edited and tested by:
Omer ZilberbergTested-by Omer Zilberberg <[email protected]>Signed-off-by Omer Zilberberg <[email protected]>Signed-off-by Boaz Harrosh <[email protected]>---
Dave hands-up man, it looks like you edited this directly in the email, but there was not even a single typo.We have tested this both with and without the
pfn_mkwritepatch. And it works as expected fails without and success with.Thanks
tests/generic/080 | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/080.out | 2 ++ tests/generic/group | 1 + 3 files changed, 82 insertions(+) create mode 100755 tests/generic/080 create mode 100644 tests/generic/080.outtests/generic/080 new file +79
-0@@ -0,0 +1,79 @@ +#! /bin/bash +# FS QA Test No. 080 +# +# Verify that mtime is updated when writing to mmap-ed pages +# +#----------------------------------------------------------------------- +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- +# + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=0 +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* + rm -f $TEST_DIR/mmap_mtime_testfile +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# real QA test starts here + +# Modify as appropriate. +_supported_fs generic +_supported_os IRIX Linux +_require_test + +echo "Silence is golden." +rm -f $seqres.full + +# pattern the file. +testfile=$TEST_DIR/mmap_mtime_testfile +$XFS_IO_PROG -f -c "pwrite 0 4k" -c fsync $testfile >> $seqres.full + +# sample timestamps. +mtime1=`stat -c %Y $testfile` +ctime1=`stat -c %Z $testfile` +echo "before mwrite: $mtime1 $ctime1" >> $seqres.full + +# map read followed by map write to trigger timestamp change +sleep 2 +$XFS_IO_PROG -c "mmap 0 4k" -c "mread 0 4k" -c "mwrite 0 4k" $testfile |_filter_xfs_io >> $seqres.full + +# sample and verify that timestamps have changed. +mtime2=`stat -c %Y $testfile` +ctime2=`stat -c %Z $testfile` +echo "after mwrite : $mtime2 $ctime2" >> $seqres.full + +if [ "$mtime1" == "$mtime2" ]; then + echo "mtime not updated" + let status=$status+1 +fi +if [ "$ctime1" == "$ctime2" ]; then + echo "ctime not updated" + let status=$status+1 +fi + +exittests/generic/080.out new file +2
-0@@ -0,0 +1,2 @@ +QA output created by 080 +Silence is golden.tests/generic/group +1
-0@@ -77,6 +77,7 @@ 076 metadata rw udf auto quick stress 077 acl attr auto enospc 079 acl attr ioctl metadata auto quick +080 auto quick 083 rw auto enospc stress 088 perms auto quick 089 metadata auto
PATCH RESEND v2 1/3 xfstest: generic/080 test that mmap-write updates c/mtime
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
when using mmap() for file i/o, writing to the file should update it's c/mtime. Specifically if we first mmap-read from a page, then memap-write to the same page.
This test was failing for the initial submission of DAX because pfn based mapping do not have an page_mkwrite called for them. The new Kernel patches that introduce pfn_mkwrite fixes this test.
Written by Dave Chinner but edited and tested by:
Omer Zilberberg
---
Dave hands-up man, it looks like you edited this directly in the email, but there was not even a single typo.
We have tested this both with and without the pfn_mkwrite patch. And it works as expected fails without and success with.
Thanks
tests/generic/080 | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/080.out | 2 ++ tests/generic/group | 1 + 3 files changed, 82 insertions(+) create mode 100755 tests/generic/080 create mode 100644 tests/generic/080.out
tests/generic/080 new file +79 -0
@@ -0,0 +1,79 @@ +#! /bin/bash +# FS QA Test No. 080 +# +# Verify that mtime is updated when writing to mmap-ed pages +# +#----------------------------------------------------------------------- +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- +# + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=0 +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* + rm -f $TEST_DIR/mmap_mtime_testfile +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# real QA test starts here + +# Modify as appropriate. +_supported_fs generic +_supported_os IRIX Linux +_require_test + +echo "Silence is golden." +rm -f $seqres.full + +# pattern the file. +testfile=$TEST_DIR/mmap_mtime_testfile +$XFS_IO_PROG -f -c "pwrite 0 4k" -c fsync $testfile >> $seqres.full + +# sample timestamps. +mtime1=`stat -c %Y $testfile` +ctime1=`stat -c %Z $testfile` +echo "before mwrite: $mtime1 $ctime1" >> $seqres.full + +# map read followed by map write to trigger timestamp change +sleep 2 +$XFS_IO_PROG -c "mmap 0 4k" -c "mread 0 4k" -c "mwrite 0 4k" $testfile |_filter_xfs_io >> $seqres.full + +# sample and verify that timestamps have changed. +mtime2=`stat -c %Y $testfile` +ctime2=`stat -c %Z $testfile` +echo "after mwrite : $mtime2 $ctime2" >> $seqres.full + +if [ "$mtime1" == "$mtime2" ]; then + echo "mtime not updated" + let status=$status+1 +fi +if [ "$ctime1" == "$ctime2" ]; then + echo "ctime not updated" + let status=$status+1 +fi + +exit
tests/generic/080.out new file +2 -0
@@ -0,0 +1,2 @@ +QA output created by 080 +Silence is golden.
tests/generic/group +1 -0
@@ -77,6 +77,7 @@ 076 metadata rw udf auto quick stress 077 acl attr auto enospc 079 acl attr ioctl metadata auto quick +080 auto quick 083 rw auto enospc stress 088 perms auto quick 089 metadata auto
Signature · 1 line
1.9.3
Re: [PATCH 1/3 v2 resend] xfstest: generic/080 test that mmap-write updates c/mtime
Quoted 21 lines · Boaz Harrosh
From: Dave Chinner <[email protected]>
when using
mmap()for file i/o, writing to the file should update it's c/mtime. Specifically if we first mmap-read from a page, then memap-write to the same page.This test was failing for the initial submission of DAX because pfn based mapping do not have an
page_mkwritecalled for them. The new Kernel patches that introducepfn_mkwritefixes this test.Written by Dave Chinner but edited and tested by:
Omer ZilberbergTested-by Omer Zilberberg <[email protected]>Tested-by Boaz Harrosh <[email protected]>Signed-off-by Omer Zilberberg <[email protected]>Signed-off-by Boaz Harrosh <[email protected]>---
Dave hands-up man, it looks like you edited this directly in the email, but there was not even a single typo.
Quoted 8 lines
We have tested this both with and without the
pfn_mkwritepatch. And it works as expected fails without and success with.Thanks
tests/generic/080 | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/080.out | 2 ++ tests/generic/group | 1 +
This update to group file makes git am to fail on current HEAD
7a1ad74 common: fix "utility required warning" with empty utility name
Applying: xfstest: generic/080 test that mmap-write updates c/mtime
error: patch failed: tests/generic/group:77
error: tests/generic/group: patch does not apply
Patch failed at 0001 xfstest: generic/080 test that mmap-write updates c/mtime
Quoted 35 lines
3 files changed, 82 insertions(+) create mode 100755 tests/generic/080 create mode 100644 tests/generic/080.outtests/generic/080 new file +25
-0@@ -0,0 +1,79 @@ +#! /bin/bash +# FS QA Test No. 080 +# +# Verify that mtime is updated when writing to mmap-ed pages +# +#----------------------------------------------------------------------- +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- +# + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" +
tests/generic/080 line 35
Quoted 10 lines
tests/generic/080 new file +10
-0+here=`pwd` +tmp=/tmp/$$ +status=0 +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* + rm -f $TEST_DIR/mmap_mtime_testfile
rm -f $testfile ? as you have testfile defined
Also please use single tab to indent, not 4 spaces (the 'new' script has been fixed too)
tests/generic/080 line 44
Quoted 9 lines
tests/generic/080 new file +9
-0+} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# real QA test starts here + +# Modify as appropriate.
This comment can be removed.
Quoted 9 lines
tests/generic/080 new file +9
-0+_supported_fs generic +_supported_os IRIX Linux +_require_test + +echo "Silence is golden." +rm -f $seqres.full + +# pattern the file. +testfile=$TEST_DIR/mmap_mtime_testfile
tests/generic/080 line 63
Quoted 10 lines
tests/generic/080 new file +10
-0+$XFS_IO_PROG -f -c "pwrite 0 4k" -c fsync $testfile >> $seqres.full + +# sample timestamps. +mtime1=`stat -c %Y $testfile` +ctime1=`stat -c %Z $testfile` +echo "before mwrite: $mtime1 $ctime1" >> $seqres.full + +# map read followed by map write to trigger timestamp change +sleep 2 +$XFS_IO_PROG -c "mmap 0 4k" -c "mread 0 4k" -c "mwrite 0 4k" $testfile |_filter_xfs_io >> $seqres.full
_filter_xfs_io seems unnecessary, the output is not going to .out file but to $seqres.full
Thanks,
Eryu Guan
Quoted 43 lines
tests/generic/080 new file +16
-0+ +# sample and verify that timestamps have changed. +mtime2=`stat -c %Y $testfile` +ctime2=`stat -c %Z $testfile` +echo "after mwrite : $mtime2 $ctime2" >> $seqres.full + +if [ "$mtime1" == "$mtime2" ]; then + echo "mtime not updated" + let status=$status+1 +fi +if [ "$ctime1" == "$ctime2" ]; then + echo "ctime not updated" + let status=$status+1 +fi + +exittests/generic/080.out new file +2
-0@@ -0,0 +1,2 @@ +QA output created by 080 +Silence is golden.tests/generic/group +1
-0@@ -77,6 +77,7 @@ 076 metadata rw udf auto quick stress 077 acl attr auto enospc 079 acl attr ioctl metadata auto quick +080 auto quick 083 rw auto enospc stress 088 perms auto quick 089 metadata autoSignature · 6 lines
1.9.3 -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v3 1/1 xfstest: generic/080 test that mmap-write updates c/mtime
From: Dave Chinner <[email protected]>
when using mmap() for file i/o, writing to the file should update it's c/mtime. Specifically if we first mmap-read from a page, then memap-write to the same page.
This test was failing for the initial submission of DAX because pfn based mapping do not have an page_mkwrite called for them. The new Kernel patches that introduce pfn_mkwrite fixes this test.
Written by Dave Chinner but edited and tested by:
Omer Zilberberg
Dave hands-up man, it looks like you edited this directly in the email, but there was not even a single typo.
--- tests/generic/080 | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/080.out | 2 ++ tests/generic/group | 1 + 3 files changed, 80 insertions(+) create mode 100755 tests/generic/080 create mode 100644 tests/generic/080.out
tests/generic/080 new file +77 -0
@@ -0,0 +1,77 @@ +#! /bin/bash +# FS QA Test No. 080 +# +# Verify that mtime is updated when writing to mmap-ed pages +# +#----------------------------------------------------------------------- +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- +# + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=0 +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* + rm -f $testfile +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# real QA test starts here +_supported_fs generic +_supported_os IRIX Linux +_require_test + +echo "Silence is golden." +rm -f $seqres.full + +# pattern the file. +testfile=$TEST_DIR/mmap_mtime_testfile +$XFS_IO_PROG -f -c "pwrite 0 4k" -c fsync $testfile >> $seqres.full + +# sample timestamps. +mtime1=`stat -c %Y $testfile` +ctime1=`stat -c %Z $testfile` +echo "before mwrite: $mtime1 $ctime1" >> $seqres.full + +# map read followed by map write to trigger timestamp change +sleep 2 +$XFS_IO_PROG -c "mmap 0 4k" -c "mread 0 4k" -c "mwrite 0 4k" $testfile >> $seqres.full + +# sample and verify that timestamps have changed. +mtime2=`stat -c %Y $testfile` +ctime2=`stat -c %Z $testfile` +echo "after mwrite : $mtime2 $ctime2" >> $seqres.full + +if [ "$mtime1" == "$mtime2" ]; then + echo "mtime not updated" + let status=$status+1 +fi +if [ "$ctime1" == "$ctime2" ]; then + echo "ctime not updated" + let status=$status+1 +fi + +exit
tests/generic/080.out new file +2 -0
@@ -0,0 +1,2 @@ +QA output created by 080 +Silence is golden.
tests/generic/group +1 -0
@@ -79,6 +79,7 @@ 077 acl attr auto enospc 078 auto quick metadata 079 acl attr ioctl metadata auto quick +080 auto quick 083 rw auto enospc stress 088 perms auto quick 089 metadata auto
Signature · 1 line
1.9.3
Re: [PATCH v3] xfstest: generic/080 test that mmap-write updates c/mtime
Quoted 13 lines · Boaz Harrosh
From: Dave Chinner <[email protected]>
when using
mmap()for file i/o, writing to the file should update it's c/mtime. Specifically if we first mmap-read from a page, then memap-write to the same page.This test was failing for the initial submission of DAX because pfn based mapping do not have an
page_mkwritecalled for them. The new Kernel patches that introducepfn_mkwritefixes this test.Written by Dave Chinner but edited and tested by:
Omer Zilberberg
Quoted 8 lines
Dave hands-up man, it looks like you edited this directly in the email, but there was not even a single typo.
Tested-by Omer Zilberberg <[email protected]>Tested-by Boaz Harrosh <[email protected]>Signed-off-by Omer Zilberberg <[email protected]>Signed-off-by Boaz Harrosh <[email protected]>Need-Reviewed-by Eryu Guan <[email protected]>
Hi Eryu
Thank you for your review.
I have tried to incorporate your comments, I hope its what you meant.
Thanks
Boaz
Re: [PATCH v3] xfstest: generic/080 test that mmap-write updates c/mtime
In reply to Boaz HarroshQuoted 13 lines · Boaz Harrosh
From: Dave Chinner <[email protected]>
when using
mmap()for file i/o, writing to the file should update it's c/mtime. Specifically if we first mmap-read from a page, then memap-write to the same page.This test was failing for the initial submission of DAX because pfn based mapping do not have an
page_mkwritecalled for them. The new Kernel patches that introducepfn_mkwritefixes this test.Written by Dave Chinner but edited and tested by:
Omer Zilberberg
Quoted 8 lines
Dave hands-up man, it looks like you edited this directly in the email, but there was not even a single typo.
Tested-by Omer Zilberberg <[email protected]>Tested-by Boaz Harrosh <[email protected]>Signed-off-by Omer Zilberberg <[email protected]>Signed-off-by Boaz Harrosh <[email protected]>Need-Reviewed-by Eryu Guan <[email protected]>
checkpatch.pl complains :)
WARNING: 'Need-reviewed-by:' is the preferred signature form
I tested it with xfs/ext4/btrfs/nfs and cifs, cifs failed the test.
The test looks good to me (with one nitpick below).
Quoted 65 lines
--- tests/generic/080 | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/080.out | 2 ++ tests/generic/group | 1 + 3 files changed, 80 insertions(+) create mode 100755 tests/generic/080 create mode 100644 tests/generic/080.outtests/generic/080 new file +51
-0@@ -0,0 +1,77 @@ +#! /bin/bash +# FS QA Test No. 080 +# +# Verify that mtime is updated when writing to mmap-ed pages +# +#----------------------------------------------------------------------- +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- +# + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=0 +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* + rm -f $testfile +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# real QA test starts here +_supported_fs generic +_supported_os IRIX Linux +_require_test + +echo "Silence is golden." +rm -f $seqres.full + +# pattern the file. +testfile=$TEST_DIR/mmap_mtime_testfile
tests/generic/080 line 61
Quoted 10 lines
tests/generic/080 new file +10
-0+$XFS_IO_PROG -f -c "pwrite 0 4k" -c fsync $testfile >> $seqres.full + +# sample timestamps. +mtime1=`stat -c %Y $testfile` +ctime1=`stat -c %Z $testfile` +echo "before mwrite: $mtime1 $ctime1" >> $seqres.full + +# map read followed by map write to trigger timestamp change +sleep 2 +$XFS_IO_PROG -c "mmap 0 4k" -c "mread 0 4k" -c "mwrite 0 4k" $testfile >> $seqres.full
Better to keep the line within 80 columns.
Thanks,
Eryu Guan
Quoted 40 lines
tests/generic/080 new file +16
-0+ +# sample and verify that timestamps have changed. +mtime2=`stat -c %Y $testfile` +ctime2=`stat -c %Z $testfile` +echo "after mwrite : $mtime2 $ctime2" >> $seqres.full + +if [ "$mtime1" == "$mtime2" ]; then + echo "mtime not updated" + let status=$status+1 +fi +if [ "$ctime1" == "$ctime2" ]; then + echo "ctime not updated" + let status=$status+1 +fi + +exittests/generic/080.out new file +2
-0@@ -0,0 +1,2 @@ +QA output created by 080 +Silence is golden.tests/generic/group +1
-0@@ -79,6 +79,7 @@ 077 acl attr auto enospc 078 auto quick metadata 079 acl attr ioctl metadata auto quick +080 auto quick 083 rw auto enospc stress 088 perms auto quick 089 metadata autoSignature · 1 line
1.9.3
Re: [PATCH v4] xfstest: generic/080 test that mmap-write updates c/mtime
From: Dave Chinner <[email protected]>
when using mmap() for file i/o, writing to the file should update it's c/mtime. Specifically if we first mmap-read from a page, then memap-write to the same page.
This test was failing for the initial submission of DAX because pfn based mapping do not have an page_mkwrite called for them. The new Kernel patches that introduce pfn_mkwrite fixes this test.
Written by Dave Chinner but edited and tested by:
Omer Zilberberg
Dave hands-up man, it looks like you edited this directly in the email, but there was not even a single typo.
--- tests/generic/080 | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/080.out | 2 ++ tests/generic/group | 1 + 3 files changed, 81 insertions(+) create mode 100755 tests/generic/080 create mode 100644 tests/generic/080.out
tests/generic/080 new file +78 -0
@@ -0,0 +1,78 @@ +#! /bin/bash +# FS QA Test No. 080 +# +# Verify that mtime is updated when writing to mmap-ed pages +# +#----------------------------------------------------------------------- +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- +# + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=0 +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* + rm -f $testfile +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# real QA test starts here +_supported_fs generic +_supported_os IRIX Linux +_require_test + +echo "Silence is golden." +rm -f $seqres.full + +# pattern the file. +testfile=$TEST_DIR/mmap_mtime_testfile +$XFS_IO_PROG -f -c "pwrite 0 4k" -c fsync $testfile >> $seqres.full + +# sample timestamps. +mtime1=`stat -c %Y $testfile` +ctime1=`stat -c %Z $testfile` +echo "before mwrite: $mtime1 $ctime1" >> $seqres.full + +# map read followed by map write to trigger timestamp change +sleep 2 +$XFS_IO_PROG -c "mmap 0 4k" -c "mread 0 4k" -c "mwrite 0 4k" $testfile \ + >> $seqres.full + +# sample and verify that timestamps have changed. +mtime2=`stat -c %Y $testfile` +ctime2=`stat -c %Z $testfile` +echo "after mwrite : $mtime2 $ctime2" >> $seqres.full + +if [ "$mtime1" == "$mtime2" ]; then + echo "mtime not updated" + let status=$status+1 +fi +if [ "$ctime1" == "$ctime2" ]; then + echo "ctime not updated" + let status=$status+1 +fi + +exit
tests/generic/080.out new file +2 -0
@@ -0,0 +1,2 @@ +QA output created by 080 +Silence is golden.
tests/generic/group +1 -0
@@ -79,6 +79,7 @@ 077 acl attr auto enospc 078 auto quick metadata 079 acl attr ioctl metadata auto quick +080 auto quick 083 rw auto enospc stress 088 perms auto quick 089 metadata auto
Signature · 1 line
1.9.3
Re: [PATCH v3] xfstest: generic/080 test that mmap-write updates c/mtime
In reply to Eryu GuanOn 03/19/2015 05:30 PM, Eryu Guan wrote:
<>
Quoted 3 lines
I tested it with xfs/ext4/btrfs/nfs and cifs, cifs failed the test.
Ha there you go. I think git has some operation that do this and the date gets screwed.
Quoted 2 lines
The test looks good to me (with one nitpick below).
I have sent v4 that fixes this
Quoted 2 lines
Reviewed-by Eryu Guan <[email protected]>
And also added this one.
Thank you sir Eryu
Boaz
PATCH 2/3 mm: New pfn_mkwrite same as page_mkwrite for VM_PFNMAP
In reply to Boaz HarroshFrom: Yigal Korman <[email protected]>
This will allow FS that uses VM_PFNMAP | VM_MIXEDMAP (no page structs) to get notified when access is a write to a read-only PFN.
This can happen if we mmap() a file then first mmap-read from it to page-in a read-only PFN, than we mmap-write to the same page.
We need this functionality to fix a DAX bug, where in the scenario above we fail to set ctime/mtime though we modified the file. An xfstest is attached to this patchset that shows the failure and the fix. (A DAX patch will follow)
This functionality is extra important for us, because upon dirtying of a pmem page we also want to RDMA the page to a remote cluster node.
We define a new pfn_mkwrite and do not reuse page_mkwrite because
1 - The name ;-)
2 - But mainly because it would take a very long and tedious
audit of all page_mkwrite functions of VM_MIXEDMAP/VM_PFNMAP users. To make sure they do not now CRASH. For example current DAX code (which this is for) would crash.
If we would want to reuse page_mkwrite, We will need to first patch all users, so to not-crash-on-no-page. Then enable this patch. But even if I did that I would not sleep so well at night. Adding a new vector is the safest thing to do, and is not that expensive. an extra pointer at a static function vector per driver. Also the new vector is better for performance, because else we Will call all current Kernel vectors, so to:
check-ha-no-page-do-nothing and return.
No need to call it from do_shared_fault because do_wp_page is called to change pte permissions anyway.
--- include/linux/mm.h | 2 ++ mm/memory.c | 27 ++++++++++++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-)
include/linux/mm.h +2 -0
@@ -250,6 +250,8 @@ struct vm_operations_struct {/* notification that a previously read-only page is about to become * writable, if an error is returned it will cause a SIGBUS */ int (*page_mkwrite)(struct vm_area_struct *vma,struct vm_fault *vmf); +/* same as page_mkwrite when using VM_PFNMAP|VM_MIXEDMAP */ +int (*pfn_mkwrite)(struct vm_area_struct *vma,struct vm_fault *vmf);/* called by access_process_vm when get_user_pages() fails, typically * for use by special VMAs that can switch between memory and hardware
mm/memory.c +26 -1
@@ -1982,6 +1982,22 @@ static int do_page_mkwrite(struct vm_area_struct *vma, struct page *page,return ret; } +static int do_pfn_mkwrite (struct vm_area_struct *vma,unsigned long address) +{ +struct vm_fault vmf; + +if (!vma->vm_ops || !vma->vm_ops->pfn_mkwrite) +return 0 ; + + vmf.page =0 ; + vmf.pgoff = (((address &PAGE_MASK ) - vma->vm_start) >>PAGE_SHIFT ) + + vma->vm_pgoff; + vmf.virtual_address = (void __user *)(address &PAGE_MASK ); + vmf.flags =FAULT_FLAG_WRITE |FAULT_FLAG_MKWRITE ; + +return vma->vm_ops->pfn_mkwrite (vma, &vmf); +} +/* * This routine handles present pages, when users try to write * to a shared page. It is done by copying the page to a new address
@@ -2025,8 +2041,17 @@ static int do_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,* accounting on raw pfn maps. */ if ((vma->vm_flags & (VM_WRITE |VM_SHARED )) == - (VM_WRITE |VM_SHARED )) + (VM_WRITE |VM_SHARED )) { +pte_unmap_unlock (page_table, ptl); + ret =do_pfn_mkwrite (vma, address); +if (ret &VM_FAULT_ERROR ) +return ret; + page_table =pte_offset_map_lock (mm, pmd, address, + &ptl); +if (!pte_same (*page_table, orig_pte)) + goto unlock; goto reuse; + } goto gotten; }
Signature · 8 lines
1.9.3 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
PATCH 3/3 DAX: use pfn_mkwrite to update c/mtime
In reply to Boaz HarroshFrom: Yigal Korman <[email protected]>
Without this patch, c/mtime is not updated correctly when mmap'ed page is first read from and then written to.
A new xfstest is submitted for testing this (generic/080)
--- fs/dax.c | 13 +++++++++++++ fs/ext2/file.c | 1 + fs/ext4/file.c | 1 + include/linux/fs.h | 1 + 4 files changed, 16 insertions(+)
fs/dax.c +13 -0
@@ -464,6 +464,19 @@ int dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf,EXPORT_SYMBOL_GPL (dax_fault);/** +* dax_pfn_mkwrite - handle first write to DAX page +* @vma: The virtual memory area where the fault occurred +* @vmf: The description of the fault +* +*/ +int dax_pfn_mkwrite (struct vm_area_struct *vma,struct vm_fault *vmf) +{ +file_update_time (vma->vm_file); +return VM_FAULT_NOPAGE ; +} +EXPORT_SYMBOL_GPL (dax_pfn_mkwrite); + +/** * dax_zero_page_range - zero a range within a page of a DAX file * @inode: The file being truncated * @from: The file offset that is being truncated to
fs/ext2/file.c +1 -0
@@ -39,6 +39,7 @@ static int ext2_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)static const struct vm_operations_struct ext2_dax_vm_ops = { .fault = ext2_dax_fault, .page_mkwrite = ext2_dax_mkwrite, + .pfn_mkwrite = dax_pfn_mkwrite, };static int ext2_file_mmap (struct file *file,struct vm_area_struct *vma)
fs/ext4/file.c +1 -0
@@ -206,6 +206,7 @@ static int ext4_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)static const struct vm_operations_struct ext4_dax_vm_ops = { .fault = ext4_dax_fault, .page_mkwrite = ext4_dax_mkwrite, + .pfn_mkwrite = dax_pfn_mkwrite, };#else #define ext4_dax_vm_ops ext4_file_vm_ops
include/linux/fs.h +1 -0
@@ -2597,6 +2597,7 @@ int dax_clear_blocks(struct inode *, sector_t block, long size);int dax_zero_page_range (struct inode *,loff_t from,unsigned len,get_block_t );int dax_truncate_page (struct inode *,loff_t from,get_block_t );int dax_fault (struct vm_area_struct *,struct vm_fault *,get_block_t ); +int dax_pfn_mkwrite (struct vm_area_struct *,struct vm_fault *);#define dax_mkwrite (vma, vmf, gb) dax_fault(vma, vmf, gb)#ifdef CONFIG_BLOCK
Signature · 8 lines
1.9.3 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Jan Kara
reviewerRe: [PATCH 3/3] DAX: use pfn_mkwrite to update c/mtime
Quoted 25 lines · Boaz Harrosh
From: Yigal Korman <[email protected]>
Without this patch, c/mtime is not updated correctly when mmap'ed page is first read from and then written to.
A new xfstest is submitted for testing this (generic/080)
Signed-off-by Yigal Korman <[email protected]>Signed-off-by Boaz Harrosh <[email protected]>--- fs/dax.c | 13 +++++++++++++ fs/ext2/file.c | 1 + fs/ext4/file.c | 1 + include/linux/fs.h | 1 + 4 files changed, 16 insertions(+)fs/dax.c +1
-0@@ -464,6 +464,19 @@ int dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf,EXPORT_SYMBOL_GPL (dax_fault);/** +* dax_pfn_mkwrite - handle first write to DAX page
fs/dax.c int dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf, · line 477
Quoted 10 lines
fs/dax.c +10
-0+* @vma: The virtual memory area where the fault occurred +* @vmf: The description of the fault +* +*/ +int dax_pfn_mkwrite (struct vm_area_struct *vma,struct vm_fault *vmf) +{ +file_update_time (vma->vm_file); +return VM_FAULT_NOPAGE ; +} +EXPORT_SYMBOL_GPL (dax_pfn_mkwrite);
Hum, you likely want a freeze protection in there as well
(sb_start_pagefault() and sb_end_pagefault()). Don't you? Otherwise user could start writing to the page while filesystem is frozen. That's another bug you could mention in your description.
Honza
Quoted 45 lines
fs/dax.c +2
-0+ +/** * dax_zero_page_range - zero a range within a page of a DAX file * @inode: The file being truncated * @from: The file offset that is being truncated to fs/ext2/file.c +1
-0@@ -39,6 +39,7 @@ static int ext2_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)static const struct vm_operations_struct ext2_dax_vm_ops = { .fault = ext2_dax_fault, .page_mkwrite = ext2_dax_mkwrite, + .pfn_mkwrite = dax_pfn_mkwrite, };static int ext2_file_mmap (struct file *file,struct vm_area_struct *vma)fs/ext4/file.c +1
-0@@ -206,6 +206,7 @@ static int ext4_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)static const struct vm_operations_struct ext4_dax_vm_ops = { .fault = ext4_dax_fault, .page_mkwrite = ext4_dax_mkwrite, + .pfn_mkwrite = dax_pfn_mkwrite, };#else #define ext4_dax_vm_ops ext4_file_vm_opsinclude/linux/fs.h +1
-0@@ -2597,6 +2597,7 @@ int dax_clear_blocks(struct inode *, sector_t block, long size);int dax_zero_page_range (struct inode *,loff_t from,unsigned len,get_block_t );int dax_truncate_page (struct inode *,loff_t from,get_block_t );int dax_fault (struct vm_area_struct *,struct vm_fault *,get_block_t ); +int dax_pfn_mkwrite (struct vm_area_struct *,struct vm_fault *);#define dax_mkwrite (vma, vmf, gb) dax_fault(vma, vmf, gb)#ifdef CONFIG_BLOCK Signature · 1 line
1.9.3
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH v2 3/3 dax: use pfn_mkwrite to update c/mtime + freeze protection
From: Yigal Korman <[email protected]>
[v1]
Without this patch, c/mtime is not updated correctly when mmap'ed page is first read from and then written to.
A new xfstest is submitted for testing this (generic/080)
[v2]
Jan Kara has pointed out that if we add the sb_start/end_pagefault pair in the new pfn_mkwrite we are then fixing another bug where: A user could start writing to the page while filesystem is frozen.
--- fs/dax.c | 17 +++++++++++++++++ fs/ext2/file.c | 1 + fs/ext4/file.c | 1 + include/linux/fs.h | 1 + 4 files changed, 20 insertions(+)
fs/dax.c +17 -0
@@ -464,6 +464,23 @@ int dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf,EXPORT_SYMBOL_GPL (dax_fault);/** +* dax_pfn_mkwrite - handle first write to DAX page +* @vma: The virtual memory area where the fault occurred +* @vmf: The description of the fault +* +*/ +int dax_pfn_mkwrite (struct vm_area_struct *vma,struct vm_fault *vmf) +{ +struct super_block *sb =file_inode (vma->vm_file)->i_sb; + +sb_start_pagefault (sb); +file_update_time (vma->vm_file); +sb_end_pagefault (sb); +return VM_FAULT_NOPAGE ; +} +EXPORT_SYMBOL_GPL (dax_pfn_mkwrite); + +/** * dax_zero_page_range - zero a range within a page of a DAX file * @inode: The file being truncated * @from: The file offset that is being truncated to
fs/ext2/file.c +1 -0
@@ -39,6 +39,7 @@ static int ext2_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)static const struct vm_operations_struct ext2_dax_vm_ops = { .fault = ext2_dax_fault, .page_mkwrite = ext2_dax_mkwrite, + .pfn_mkwrite = dax_pfn_mkwrite, };static int ext2_file_mmap (struct file *file,struct vm_area_struct *vma)
fs/ext4/file.c +1 -0
@@ -206,6 +206,7 @@ static int ext4_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)static const struct vm_operations_struct ext4_dax_vm_ops = { .fault = ext4_dax_fault, .page_mkwrite = ext4_dax_mkwrite, + .pfn_mkwrite = dax_pfn_mkwrite, };#else #define ext4_dax_vm_ops ext4_file_vm_ops
include/linux/fs.h +1 -0
@@ -2597,6 +2597,7 @@ int dax_clear_blocks(struct inode *, sector_t block, long size);int dax_zero_page_range (struct inode *,loff_t from,unsigned len,get_block_t );int dax_truncate_page (struct inode *,loff_t from,get_block_t );int dax_fault (struct vm_area_struct *,struct vm_fault *,get_block_t ); +int dax_pfn_mkwrite (struct vm_area_struct *,struct vm_fault *);#define dax_mkwrite (vma, vmf, gb) dax_fault(vma, vmf, gb)#ifdef CONFIG_BLOCK
Signature · 1 line
1.9.3
Re: [PATCH 3/3 v2] dax: use pfn_mkwrite to update c/mtime + freeze protection
Quoted 5 lines · Boaz Harrosh
[v1]
Without this patch, c/mtime is not updated correctly when mmap'ed page is first read from and then written to.
Quoted 8 lines
A new xfstest is submitted for testing this (generic/080)
[v2]
Jan Kara has pointed out that if we add the sb_start/end_pagefault pair in the newpfn_mkwritewe are then fixing another bug where: A user could start writing to the page while filesystem is frozen.
Thanks Jan.
Just as curiosity, does the freezing code goes and turns all mappings into read-only, Also for pfn mapping?
Do you think there is already an xfstest freezing test that should now fail, and will succeed after this patch (v2). Something like:
* mmap-read/write before the freeze
* freeze the fs
* Another thread tries to mmap-write, should get stuck
* unfreeze the fs
* Now mmap-writer continues
Thanks again
Boaz
Quoted 3 lines
CC Jan Kara <[email protected]>Signed-off-by Yigal Korman <[email protected]>Signed-off-by Boaz Harrosh <[email protected]>
<>
Jan Kara
reviewerRe: [PATCH 3/3 v2] dax: use pfn_mkwrite to update c/mtime + freeze protection
Quoted 15 lines · Boaz Harrosh
Quoted 5 lines · Boaz Harrosh
[v1]
Without this patch, c/mtime is not updated correctly when mmap'ed page is first read from and then written to.Quoted 8 lines
A new xfstest is submitted for testing this (generic/080)
[v2]
Jan Kara has pointed out that if we add the sb_start/end_pagefault pair in the newpfn_mkwritewe are then fixing another bug where: A user could start writing to the page while filesystem is frozen.
Quoted 4 lines
Thanks Jan.
Just as curiosity, does the freezing code goes and turns all mappings into read-only, Also for pfn mapping?
Hum, that's a good question. Probably we don't end up doing that. For
normal filesystems we sync all inodes which also writeprotects all pages (in clear_page_dirty_for_io() - for normal filesystems we know that if page is writeably mapped it is dirty). However this won't happen for pfn mapping as we don't have dirty pages. So we probably need dax_freeze() implementation that will walk through all inodes with writeable mappings and writeprotect them.
Quoted 7 lines
Do you think there is already an xfstest freezing test that should now fail, and will succeed after this patch (v2). Something like:
* mmap-read/write before the freeze
* freeze the fs
* Another thread tries to mmap-write, should get stuck
* unfreeze the fs
* Now mmap-writer continues
I don't remember there would be any test to specifically test this.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 3/3 v2] dax: use pfn_mkwrite to update c/mtime + freeze protection
Quoted 2 lines · Jan Kara
Quoted 1 line · Boaz Harrosh
On 03/05/2015 11:24 AM, Boaz Harrosh wrote:
<>
Quoted 12 lines
Quoted 3 lines
Just as curiosity, does the freezing code goes and turns all mappings into read-only, Also for pfn mapping?
Hum, that's a good question. Probably we don't end up doing that. For
normal filesystems we sync all inodes which also writeprotects all pages (in
clear_page_dirty_for_io()- for normal filesystems we know that if page is writeably mapped it is dirty). However this won't happen for pfn mapping as we don't have dirty pages. So we probably needdax_freeze()implementation that will walk through all inodes with writeable mappings and writeprotect them.
I'll go head and try my shot on implementing a dax_freeze(). But I will please need help with where to call it from.
Probably something like:
if (IS_DAX(inode)) dax_freeze(inode); else sync(inode)
So to share the for-all-inodes-in-sb loop. And also the IS_DAX(inode) is per inode, for example dirs need the regular sync (if they are using page cache)
Quoted 9 lines
Quoted 7 lines
Do you think there is already an xfstest freezing test that should now fail, and will succeed after this patch (v2). Something like:
* mmap-read/write before the freeze
* freeze the fs
* Another thread tries to mmap-write, should get stuck
* unfreeze the fs
* Now mmap-writer continuesI don't remember there would be any test to specifically test this.
OK Thanks, I was hopping we should already test for mmap vs freeze. This is not special for our case. Actually mmap is the most fragile access.
Quoted 2 lines
Honza
Thanks
Boaz
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Jan Kara
reviewerRe: [PATCH 3/3 v2] dax: use pfn_mkwrite to update c/mtime + freeze protection
Quoted 17 lines · Boaz Harrosh
Quoted 2 lines · Jan Kara
Quoted 1 line · Boaz Harrosh
On 03/05/2015 11:24 AM, Boaz Harrosh wrote:
<>
Quoted 12 lines
Quoted 3 lines
Just as curiosity, does the freezing code goes and turns all mappings into read-only, Also for pfn mapping?
Hum, that's a good question. Probably we don't end up doing that. For
normal filesystems we sync all inodes which also writeprotects all pages (in
clear_page_dirty_for_io()- for normal filesystems we know that if page is writeably mapped it is dirty). However this won't happen for pfn mapping as we don't have dirty pages. So we probably needdax_freeze()implementation that will walk through all inodes with writeable mappings and writeprotect them.
Quoted 8 lines
I'll go head and try my shot on implementing a
dax_freeze(). But I will please need help with where to call it from.Probably something like:
if (IS_DAX(inode)) dax_freeze(inode); else sync(inode)
We normally call sync_filesystem() from fs/superblock:freeze_super(). For
DAX filesystems we'd also need to call the special function after that. Maybe dax_freeze() isn't the best name. It could be called something like dax_writeprotect(sb) or something like that.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR