From: Ira Weiny <[email protected]>
When zeroing the end of a file we must account for bytes contained in
the final page which are past EOF.
Extend the range passed to iomap_zero_range() to reach LLONG_MAX which
will include all bytes of the final page.
Signed-off-by: Ira Weiny <[email protected]>
---
fs/xfs/xfs_iops.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index a2f2604c3187..a34b04e8ac9c 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -910,7 +910,7 @@ xfs_setattr_size(
*/
if (newsize > oldsize) {
trace_xfs_zero_eof(ip, oldsize, newsize - oldsize);
- error = iomap_zero_range(inode, oldsize, newsize - oldsize,
+ error = iomap_zero_range(inode, oldsize, LLONG_MAX - oldsize,
&did_zeroing, &xfs_buffered_write_iomap_ops);
} else {
error = iomap_truncate_page(inode, newsize, &did_zeroing,
--
2.21.0
On Fri, Jan 10, 2020 at 11:29:40AM -0800, [email protected] wrote:
> From: Ira Weiny <[email protected]>
>
> When zeroing the end of a file we must account for bytes contained in
> the final page which are past EOF.
>
> Extend the range passed to iomap_zero_range() to reach LLONG_MAX which
> will include all bytes of the final page.
>
> Signed-off-by: Ira Weiny <[email protected]>
> ---
> fs/xfs/xfs_iops.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index a2f2604c3187..a34b04e8ac9c 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -910,7 +910,7 @@ xfs_setattr_size(
> */
> if (newsize > oldsize) {
> trace_xfs_zero_eof(ip, oldsize, newsize - oldsize);
> - error = iomap_zero_range(inode, oldsize, newsize - oldsize,
> + error = iomap_zero_range(inode, oldsize, LLONG_MAX - oldsize,
Huh? Won't this cause the file size to be set to LLONG_MAX?
--D
> &did_zeroing, &xfs_buffered_write_iomap_ops);
> } else {
> error = iomap_truncate_page(inode, newsize, &did_zeroing,
> --
> 2.21.0
>
On Mon, Jan 13, 2020 at 02:27:55PM -0800, Darrick J. Wong wrote:
> On Fri, Jan 10, 2020 at 11:29:40AM -0800, [email protected] wrote:
> > From: Ira Weiny <[email protected]>
> >
> > When zeroing the end of a file we must account for bytes contained in
> > the final page which are past EOF.
> >
> > Extend the range passed to iomap_zero_range() to reach LLONG_MAX which
> > will include all bytes of the final page.
> >
> > Signed-off-by: Ira Weiny <[email protected]>
> > ---
> > fs/xfs/xfs_iops.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index a2f2604c3187..a34b04e8ac9c 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -910,7 +910,7 @@ xfs_setattr_size(
> > */
> > if (newsize > oldsize) {
> > trace_xfs_zero_eof(ip, oldsize, newsize - oldsize);
> > - error = iomap_zero_range(inode, oldsize, newsize - oldsize,
> > + error = iomap_zero_range(inode, oldsize, LLONG_MAX - oldsize,
>
> Huh? Won't this cause the file size to be set to LLONG_MAX?
Not as I understand the code. But as I said in the cover I am not 100% sure of
this fix.
From what I can tell xfs_ioctl_setattr_dax_invalidate() should invalidate the
mappings and the page cache and the traces I have indicate that the DAX mode
is not changing or was properly held off.
Any other suggestions as to the problem are welcome.
Ira
>
> --D
>
> > &did_zeroing, &xfs_buffered_write_iomap_ops);
> > } else {
> > error = iomap_truncate_page(inode, newsize, &did_zeroing,
> > --
> > 2.21.0
> >
On Mon, Jan 13, 2020 at 04:40:47PM -0800, Ira Weiny wrote:
> On Mon, Jan 13, 2020 at 02:27:55PM -0800, Darrick J. Wong wrote:
> > On Fri, Jan 10, 2020 at 11:29:40AM -0800, [email protected] wrote:
> > > From: Ira Weiny <[email protected]>
> > >
> > > When zeroing the end of a file we must account for bytes contained in
> > > the final page which are past EOF.
> > >
> > > Extend the range passed to iomap_zero_range() to reach LLONG_MAX which
> > > will include all bytes of the final page.
> > >
> > > Signed-off-by: Ira Weiny <[email protected]>
> > > ---
> > > fs/xfs/xfs_iops.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > index a2f2604c3187..a34b04e8ac9c 100644
> > > --- a/fs/xfs/xfs_iops.c
> > > +++ b/fs/xfs/xfs_iops.c
> > > @@ -910,7 +910,7 @@ xfs_setattr_size(
> > > */
> > > if (newsize > oldsize) {
> > > trace_xfs_zero_eof(ip, oldsize, newsize - oldsize);
> > > - error = iomap_zero_range(inode, oldsize, newsize - oldsize,
> > > + error = iomap_zero_range(inode, oldsize, LLONG_MAX - oldsize,
> >
> > Huh? Won't this cause the file size to be set to LLONG_MAX?
>
> Not as I understand the code.
iomap_zero_range uses the standard iomap_write_{begin,end} functions,
which means that if you pass it an (offset, length) that extend beyond
EOF it will change isize to offset+length.
> But as I said in the cover I am not 100% sure of
> this fix.
> From what I can tell xfs_ioctl_setattr_dax_invalidate() should invalidate the
> mappings and the page cache and the traces I have indicate that the DAX mode
> is not changing or was properly held off.
Hmm, that implies the invalidation didn't work. Can you find a way to
report the contents of the page cache after the dax mode change
invalidation fails? I wonder if this is something dorky like rounding
down such that the EOF page doesn't actually get invalidated?
Hmm, no, xfs_ioctl_setattr_dax_invalidate should be nuking all the
pages... do you have a quick reproducer?
--D
> Any other suggestions as to the problem are welcome.
>
> Ira
>
>
> >
> > --D
> >
> > > &did_zeroing, &xfs_buffered_write_iomap_ops);
> > > } else {
> > > error = iomap_truncate_page(inode, newsize, &did_zeroing,
> > > --
> > > 2.21.0
> > >
On Mon, Jan 13, 2020 at 05:14:07PM -0800, Darrick J. Wong wrote:
> On Mon, Jan 13, 2020 at 04:40:47PM -0800, Ira Weiny wrote:
> > On Mon, Jan 13, 2020 at 02:27:55PM -0800, Darrick J. Wong wrote:
> > > On Fri, Jan 10, 2020 at 11:29:40AM -0800, [email protected] wrote:
> > > > From: Ira Weiny <[email protected]>
> > > >
> > > > When zeroing the end of a file we must account for bytes contained in
> > > > the final page which are past EOF.
> > > >
> > > > Extend the range passed to iomap_zero_range() to reach LLONG_MAX which
> > > > will include all bytes of the final page.
> > > >
> > > > Signed-off-by: Ira Weiny <[email protected]>
> > > > ---
> > > > fs/xfs/xfs_iops.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > > index a2f2604c3187..a34b04e8ac9c 100644
> > > > --- a/fs/xfs/xfs_iops.c
> > > > +++ b/fs/xfs/xfs_iops.c
> > > > @@ -910,7 +910,7 @@ xfs_setattr_size(
> > > > */
> > > > if (newsize > oldsize) {
> > > > trace_xfs_zero_eof(ip, oldsize, newsize - oldsize);
> > > > - error = iomap_zero_range(inode, oldsize, newsize - oldsize,
> > > > + error = iomap_zero_range(inode, oldsize, LLONG_MAX - oldsize,
> > >
> > > Huh? Won't this cause the file size to be set to LLONG_MAX?
> >
> > Not as I understand the code.
>
> iomap_zero_range uses the standard iomap_write_{begin,end} functions,
> which means that if you pass it an (offset, length) that extend beyond
> EOF it will change isize to offset+length.
I don't see that but I'll take your word for it... That is unfortunate because
I would have thought that the full page would have been zero'ed by something
already.
I found code in xfs_free_file_space() with this comment:
/*
* If we zeroed right up to EOF and EOF straddles a page boundary we
* must make sure that the post-EOF area is also zeroed because the
* page could be mmap'd and iomap_zero_range doesn't do that for us.
* Writeback of the eof page will do this, albeit clumsily.
*/
But that just calls filemap_write_and_wait_range()... :-/
>
> > But as I said in the cover I am not 100% sure of
> > this fix.
>
> > From what I can tell xfs_ioctl_setattr_dax_invalidate() should invalidate the
> > mappings and the page cache and the traces I have indicate that the DAX mode
> > is not changing or was properly held off.
>
> Hmm, that implies the invalidation didn't work. Can you find a way to
> report the contents of the page cache after the dax mode change
> invalidation fails? I wonder if this is something dorky like rounding
> down such that the EOF page doesn't actually get invalidated?
>
> Hmm, no, xfs_ioctl_setattr_dax_invalidate should be nuking all the
> pages... do you have a quick reproducer?
I thought I did...
What I have done is take this patch:
https://www.spinics.net/lists/fstests/msg13313.html
and put [run_fsx ""] in a loop... (diff below) And without this truncate fix
patch it was failing in about 5 - 10 iterations. But I'm running it right now
and it has gone for 30+... :-(
I am 90% confident that this series works for 100% of the use cases we have. I
think this is an existing bug which I've just managed to find. And again I'm
not comfortable with this patch either. So I'm not trying to argue for it but
I just don't know what could be wrong...
Ira
diff --git a/tests/generic/999 b/tests/generic/999
index 6dd5529dbc65..929c20c6db04 100755
--- a/tests/generic/999
+++ b/tests/generic/999
@@ -274,7 +274,9 @@ function run_fsx {
pid=""
}
-run_fsx ""
+while [ 1 ]; do
+ run_fsx ""
+done
run_fsx "-A"
run_fsx "-Z -r 4096 -w 4096"
On Tue, Jan 14, 2020 at 11:00:57AM -0800, 'Ira Weiny' wrote:
> On Mon, Jan 13, 2020 at 05:14:07PM -0800, Darrick J. Wong wrote:
> > On Mon, Jan 13, 2020 at 04:40:47PM -0800, Ira Weiny wrote:
> > > On Mon, Jan 13, 2020 at 02:27:55PM -0800, Darrick J. Wong wrote:
> > > > On Fri, Jan 10, 2020 at 11:29:40AM -0800, [email protected] wrote:
> > > > > From: Ira Weiny <[email protected]>
[snip]
>
> >
> > > But as I said in the cover I am not 100% sure of
> > > this fix.
> >
> > > From what I can tell xfs_ioctl_setattr_dax_invalidate() should invalidate the
> > > mappings and the page cache and the traces I have indicate that the DAX mode
> > > is not changing or was properly held off.
> >
> > Hmm, that implies the invalidation didn't work. Can you find a way to
> > report the contents of the page cache after the dax mode change
> > invalidation fails? I wonder if this is something dorky like rounding
> > down such that the EOF page doesn't actually get invalidated?
> >
> > Hmm, no, xfs_ioctl_setattr_dax_invalidate should be nuking all the
> > pages... do you have a quick reproducer?
>
> I thought I did...
>
> What I have done is take this patch:
>
> https://www.spinics.net/lists/fstests/msg13313.html
>
> and put [run_fsx ""] in a loop... (diff below) And without this truncate fix
> patch it was failing in about 5 - 10 iterations. But I'm running it right now
> and it has gone for 30+... :-(
Ok... perhaps this is a qemu problem? In qemu I had a slightly different
script; 'test-suite.sh' (attached). This was copied to create the xfstest I
sent. Without this 'fix truncate patch I get something like the following
after in just a few iterations.
...
*** run 'fsx ' racing with setting/clearing the DAX flag
*** run 'fsx ' racing with setting/clearing the DAX flag
FAILED: fsx exited with status : 205
see trace_output
zero_range to largest ever: 0x19867
truncating to largest ever: 0x3fb6d
zero_range to largest ever: 0x40000
Mapped Write: non-zero data past EOF (0x3ab88) page offset 0xb89 is 0xe71c
LOG DUMP (15817 total operations):
15818(202 mod 256): SKIPPED (no operation)
15819(203 mod 256): ZERO 0x2ae9f thru 0x3346c (0x85ce bytes)
15820(204 mod 256): READ 0x3637 thru 0x91e6 (0x5bb0 bytes)
15821(205 mod 256): MAPREAD 0x38a80 thru 0x3d014 (0x4595 bytes)
15822(206 mod 256): INSERT 0x28000 thru 0x29fff (0x2000 bytes)
Ira
>
> I am 90% confident that this series works for 100% of the use cases we have. I
> think this is an existing bug which I've just managed to find. And again I'm
> not comfortable with this patch either. So I'm not trying to argue for it but
> I just don't know what could be wrong...
>
> Ira
>
> diff --git a/tests/generic/999 b/tests/generic/999
> index 6dd5529dbc65..929c20c6db04 100755
> --- a/tests/generic/999
> +++ b/tests/generic/999
> @@ -274,7 +274,9 @@ function run_fsx {
> pid=""
> }
>
> -run_fsx ""
> +while [ 1 ]; do
> + run_fsx ""
> +done
> run_fsx "-A"
> run_fsx "-Z -r 4096 -w 4096"
>