* [PATCH] iomap: iomap that extends beyond EOF should be marked dirty
@ 2019-10-16 5:11 Dave Chinner
2019-10-16 5:25 ` Dave Chinner
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Dave Chinner @ 2019-10-16 5:11 UTC (permalink / raw)
To: linux-xfs; +Cc: linux-ext4, linux-fsdevel
From: Dave Chinner <dchinner@redhat.com>
When doing a direct IO that spans the current EOF, and there are
written blocks beyond EOF that extend beyond the current write, the
only metadata update that needs to be done is a file size extension.
However, we don't mark such iomaps as IOMAP_F_DIRTY to indicate that
there is IO completion metadata updates required, and hence we may
fail to correctly sync file size extensions made in IO completion
when O_DSYNC writes are beingt used and the hardware supports FUA.
Hence when setting IOMAP_F_DIRTY, we need to also take into account
whether the iomap spans the current EOF. If it does, then we need to
mark it dirty so that IO completion will call generic_write_sync()
to flush the inode size update to stable storage correctly.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
fs/ext4/inode.c | 9 ++++++++-
fs/xfs/xfs_iomap.c | 8 ++++++++
include/linux/iomap.h | 2 ++
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 516faa280ced..e9dc52537e5b 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3523,9 +3523,16 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
return ret;
}
+ /*
+ * Writes that span EOF might trigger an IO size update on completion,
+ * so consider them to be dirty for the purposes of O_DSYNC even if
+ * there is no other metadata changes being made or are pending here.
+ */
iomap->flags = 0;
- if (ext4_inode_datasync_dirty(inode))
+ if (ext4_inode_datasync_dirty(inode) ||
+ offset + length > i_size_read(inode))
iomap->flags |= IOMAP_F_DIRTY;
+
iomap->bdev = inode->i_sb->s_bdev;
iomap->dax_dev = sbi->s_daxdev;
iomap->offset = (u64)first_block << blkbits;
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index f780e223b118..38be06f19ea2 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -722,6 +722,14 @@ xfs_file_iomap_begin_delay(
xfs_trim_extent(&imap, cmap.br_startoff, cmap.br_blockcount);
shared = true;
}
+
+ /*
+ * Writes that span EOF might trigger an IO size update on completion,
+ * so consider them to be dirty for the purposes of O_DSYNC even if
+ * there is no other metadata changes being made or are pending here.
+ */
+ if (offset + count > i_size_read(inode))
+ iomap->flags |= IOMAP_F_DIRTY;
error = xfs_bmbt_to_iomap(ip, iomap, &imap, shared);
out_unlock:
xfs_iunlock(ip, XFS_ILOCK_EXCL);
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 7aa5d6117936..24bd227d59f9 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -32,6 +32,8 @@ struct vm_fault;
*
* IOMAP_F_DIRTY indicates the inode has uncommitted metadata needed to access
* written data and requires fdatasync to commit them to persistent storage.
+ * This needs to take into account metadata changes that *may* be made at IO
+ * completion, such as file size updates from direct IO.
*/
#define IOMAP_F_NEW 0x01 /* blocks have been newly allocated */
#define IOMAP_F_DIRTY 0x02 /* uncommitted metadata */
--
2.23.0.rc1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] iomap: iomap that extends beyond EOF should be marked dirty
2019-10-16 5:11 [PATCH] iomap: iomap that extends beyond EOF should be marked dirty Dave Chinner
@ 2019-10-16 5:25 ` Dave Chinner
2019-10-16 5:27 ` Darrick J. Wong
2019-10-16 6:06 ` [PATCH v2] " Dave Chinner
2 siblings, 0 replies; 9+ messages in thread
From: Dave Chinner @ 2019-10-16 5:25 UTC (permalink / raw)
To: linux-xfs; +Cc: linux-ext4, linux-fsdevel
On Wed, Oct 16, 2019 at 04:11:01PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> When doing a direct IO that spans the current EOF, and there are
> written blocks beyond EOF that extend beyond the current write, the
> only metadata update that needs to be done is a file size extension.
>
> However, we don't mark such iomaps as IOMAP_F_DIRTY to indicate that
> there is IO completion metadata updates required, and hence we may
> fail to correctly sync file size extensions made in IO completion
> when O_DSYNC writes are beingt used and the hardware supports FUA.
>
> Hence when setting IOMAP_F_DIRTY, we need to also take into account
> whether the iomap spans the current EOF. If it does, then we need to
> mark it dirty so that IO completion will call generic_write_sync()
> to flush the inode size update to stable storage correctly.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> fs/ext4/inode.c | 9 ++++++++-
> fs/xfs/xfs_iomap.c | 8 ++++++++
> include/linux/iomap.h | 2 ++
> 3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 516faa280ced..e9dc52537e5b 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3523,9 +3523,16 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> return ret;
> }
>
> + /*
> + * Writes that span EOF might trigger an IO size update on completion,
> + * so consider them to be dirty for the purposes of O_DSYNC even if
> + * there is no other metadata changes being made or are pending here.
> + */
> iomap->flags = 0;
> - if (ext4_inode_datasync_dirty(inode))
> + if (ext4_inode_datasync_dirty(inode) ||
> + offset + length > i_size_read(inode))
> iomap->flags |= IOMAP_F_DIRTY;
> +
> iomap->bdev = inode->i_sb->s_bdev;
> iomap->dax_dev = sbi->s_daxdev;
> iomap->offset = (u64)first_block << blkbits;
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index f780e223b118..38be06f19ea2 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -722,6 +722,14 @@ xfs_file_iomap_begin_delay(
> xfs_trim_extent(&imap, cmap.br_startoff, cmap.br_blockcount);
> shared = true;
> }
> +
> + /*
> + * Writes that span EOF might trigger an IO size update on completion,
> + * so consider them to be dirty for the purposes of O_DSYNC even if
> + * there is no other metadata changes being made or are pending here.
> + */
> + if (offset + count > i_size_read(inode))
> + iomap->flags |= IOMAP_F_DIRTY;
> error = xfs_bmbt_to_iomap(ip, iomap, &imap, shared);
> out_unlock:
> xfs_iunlock(ip, XFS_ILOCK_EXCL);
Urk, self-nack.
I put this in the wrong function. Too much spaghetti that all looks
similar in the XFS iomap code now. Will repost in a minute.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iomap: iomap that extends beyond EOF should be marked dirty
2019-10-16 5:11 [PATCH] iomap: iomap that extends beyond EOF should be marked dirty Dave Chinner
2019-10-16 5:25 ` Dave Chinner
@ 2019-10-16 5:27 ` Darrick J. Wong
2019-10-16 6:40 ` Christoph Hellwig
2019-10-16 6:06 ` [PATCH v2] " Dave Chinner
2 siblings, 1 reply; 9+ messages in thread
From: Darrick J. Wong @ 2019-10-16 5:27 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, linux-ext4, linux-fsdevel, Christoph Hellwig
On Wed, Oct 16, 2019 at 04:11:01PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> When doing a direct IO that spans the current EOF, and there are
> written blocks beyond EOF that extend beyond the current write, the
> only metadata update that needs to be done is a file size extension.
>
> However, we don't mark such iomaps as IOMAP_F_DIRTY to indicate that
> there is IO completion metadata updates required, and hence we may
> fail to correctly sync file size extensions made in IO completion
> when O_DSYNC writes are beingt used and the hardware supports FUA.
>
> Hence when setting IOMAP_F_DIRTY, we need to also take into account
> whether the iomap spans the current EOF. If it does, then we need to
> mark it dirty so that IO completion will call generic_write_sync()
> to flush the inode size update to stable storage correctly.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> fs/ext4/inode.c | 9 ++++++++-
> fs/xfs/xfs_iomap.c | 8 ++++++++
> include/linux/iomap.h | 2 ++
> 3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 516faa280ced..e9dc52537e5b 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3523,9 +3523,16 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> return ret;
> }
>
> + /*
> + * Writes that span EOF might trigger an IO size update on completion,
> + * so consider them to be dirty for the purposes of O_DSYNC even if
> + * there is no other metadata changes being made or are pending here.
> + */
> iomap->flags = 0;
> - if (ext4_inode_datasync_dirty(inode))
> + if (ext4_inode_datasync_dirty(inode) ||
> + offset + length > i_size_read(inode))
> iomap->flags |= IOMAP_F_DIRTY;
> +
> iomap->bdev = inode->i_sb->s_bdev;
> iomap->dax_dev = sbi->s_daxdev;
> iomap->offset = (u64)first_block << blkbits;
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index f780e223b118..38be06f19ea2 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -722,6 +722,14 @@ xfs_file_iomap_begin_delay(
> xfs_trim_extent(&imap, cmap.br_startoff, cmap.br_blockcount);
> shared = true;
> }
> +
> + /*
> + * Writes that span EOF might trigger an IO size update on completion,
> + * so consider them to be dirty for the purposes of O_DSYNC even if
> + * there is no other metadata changes being made or are pending here.
> + */
> + if (offset + count > i_size_read(inode))
> + iomap->flags |= IOMAP_F_DIRTY;
This ought to be in xfs_direct_write_iomap_begin(), right?
(Hoping to see another rev of Christoph's iomap cleanup series... ;))
--D
> error = xfs_bmbt_to_iomap(ip, iomap, &imap, shared);
> out_unlock:
> xfs_iunlock(ip, XFS_ILOCK_EXCL);
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index 7aa5d6117936..24bd227d59f9 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -32,6 +32,8 @@ struct vm_fault;
> *
> * IOMAP_F_DIRTY indicates the inode has uncommitted metadata needed to access
> * written data and requires fdatasync to commit them to persistent storage.
> + * This needs to take into account metadata changes that *may* be made at IO
> + * completion, such as file size updates from direct IO.
> */
> #define IOMAP_F_NEW 0x01 /* blocks have been newly allocated */
> #define IOMAP_F_DIRTY 0x02 /* uncommitted metadata */
> --
> 2.23.0.rc1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] iomap: iomap that extends beyond EOF should be marked dirty
2019-10-16 5:11 [PATCH] iomap: iomap that extends beyond EOF should be marked dirty Dave Chinner
2019-10-16 5:25 ` Dave Chinner
2019-10-16 5:27 ` Darrick J. Wong
@ 2019-10-16 6:06 ` Dave Chinner
2019-10-16 6:43 ` Christoph Hellwig
2019-10-17 12:29 ` Theodore Y. Ts'o
2 siblings, 2 replies; 9+ messages in thread
From: Dave Chinner @ 2019-10-16 6:06 UTC (permalink / raw)
To: linux-xfs; +Cc: linux-ext4, linux-fsdevel
From: Dave Chinner <dchinner@redhat.com>
When doing a direct IO that spans the current EOF, and there are
written blocks beyond EOF that extend beyond the current write, the
only metadata update that needs to be done is a file size extension.
However, we don't mark such iomaps as IOMAP_F_DIRTY to indicate that
there is IO completion metadata updates required, and hence we may
fail to correctly sync file size extensions made in IO completion
when O_DSYNC writes are being used and the hardware supports FUA.
Hence when setting IOMAP_F_DIRTY, we need to also take into account
whether the iomap spans the current EOF. If it does, then we need to
mark it dirty so that IO completion will call generic_write_sync()
to flush the inode size update to stable storage correctly.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
V2: put the XFS code in the right function, tested on non-FUA
capable hardware, too.
fs/ext4/inode.c | 9 ++++++++-
fs/xfs/xfs_iomap.c | 7 +++++++
include/linux/iomap.h | 2 ++
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 516faa280ced..e9dc52537e5b 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3523,9 +3523,16 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
return ret;
}
+ /*
+ * Writes that span EOF might trigger an IO size update on completion,
+ * so consider them to be dirty for the purposes of O_DSYNC even if
+ * there is no other metadata changes being made or are pending here.
+ */
iomap->flags = 0;
- if (ext4_inode_datasync_dirty(inode))
+ if (ext4_inode_datasync_dirty(inode) ||
+ offset + length > i_size_read(inode))
iomap->flags |= IOMAP_F_DIRTY;
+
iomap->bdev = inode->i_sb->s_bdev;
iomap->dax_dev = sbi->s_daxdev;
iomap->offset = (u64)first_block << blkbits;
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index f780e223b118..32993c2acbd9 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1049,6 +1049,13 @@ xfs_file_iomap_begin(
trace_xfs_iomap_alloc(ip, offset, length, XFS_DATA_FORK, &imap);
out_finish:
+ /*
+ * Writes that span EOF might trigger an IO size update on completion,
+ * so consider them to be dirty for the purposes of O_DSYNC even if
+ * there is no other metadata changes pending or have been made here.
+ */
+ if ((flags & IOMAP_WRITE) && offset + length > i_size_read(inode))
+ iomap->flags |= IOMAP_F_DIRTY;
return xfs_bmbt_to_iomap(ip, iomap, &imap, shared);
out_found:
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 7aa5d6117936..24bd227d59f9 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -32,6 +32,8 @@ struct vm_fault;
*
* IOMAP_F_DIRTY indicates the inode has uncommitted metadata needed to access
* written data and requires fdatasync to commit them to persistent storage.
+ * This needs to take into account metadata changes that *may* be made at IO
+ * completion, such as file size updates from direct IO.
*/
#define IOMAP_F_NEW 0x01 /* blocks have been newly allocated */
#define IOMAP_F_DIRTY 0x02 /* uncommitted metadata */
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] iomap: iomap that extends beyond EOF should be marked dirty
2019-10-16 5:27 ` Darrick J. Wong
@ 2019-10-16 6:40 ` Christoph Hellwig
0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2019-10-16 6:40 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Dave Chinner, linux-xfs, linux-ext4, linux-fsdevel, Christoph Hellwig
On Tue, Oct 15, 2019 at 10:27:13PM -0700, Darrick J. Wong wrote:
> > + /*
> > + * Writes that span EOF might trigger an IO size update on completion,
> > + * so consider them to be dirty for the purposes of O_DSYNC even if
> > + * there is no other metadata changes being made or are pending here.
> > + */
> > + if (offset + count > i_size_read(inode))
> > + iomap->flags |= IOMAP_F_DIRTY;
>
> This ought to be in xfs_direct_write_iomap_begin(), right?
>
> (Hoping to see another rev of Christoph's iomap cleanup series... ;))
I need to finish off all the nitpicks on the first iomap series..
Also we'll want this patch in first as it is 5.4 / -stable material,
so I'll need to rebase on top of that as well.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iomap: iomap that extends beyond EOF should be marked dirty
2019-10-16 6:06 ` [PATCH v2] " Dave Chinner
@ 2019-10-16 6:43 ` Christoph Hellwig
2019-10-17 12:29 ` Theodore Y. Ts'o
1 sibling, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2019-10-16 6:43 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, linux-ext4, linux-fsdevel
I wonder if the i_size check should be done in core, similar to the
iomap_block_needs_zeroing helper. But independent of what is nicer
this version does look correct to me:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iomap: iomap that extends beyond EOF should be marked dirty
2019-10-16 6:06 ` [PATCH v2] " Dave Chinner
2019-10-16 6:43 ` Christoph Hellwig
@ 2019-10-17 12:29 ` Theodore Y. Ts'o
2019-10-17 14:17 ` Christoph Hellwig
1 sibling, 1 reply; 9+ messages in thread
From: Theodore Y. Ts'o @ 2019-10-17 12:29 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, linux-ext4, linux-fsdevel, Matthew Bobrowski
On Wed, Oct 16, 2019 at 05:06:04PM +1100, Dave Chinner wrote:
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 516faa280ced..e9dc52537e5b 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3523,9 +3523,16 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> return ret;
> }
>
> + /*
> + * Writes that span EOF might trigger an IO size update on completion,
> + * so consider them to be dirty for the purposes of O_DSYNC even if
> + * there is no other metadata changes being made or are pending here.
> + */
> iomap->flags = 0;
> - if (ext4_inode_datasync_dirty(inode))
> + if (ext4_inode_datasync_dirty(inode) ||
> + offset + length > i_size_read(inode))
> iomap->flags |= IOMAP_F_DIRTY;
> +
> iomap->bdev = inode->i_sb->s_bdev;
> iomap->dax_dev = sbi->s_daxdev;
> iomap->offset = (u64)first_block << blkbits;
Ext4 is not currently using iomap for any kind of writing right now,
so perhaps this should land via Matthew's patchset?
- Ted
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iomap: iomap that extends beyond EOF should be marked dirty
2019-10-17 12:29 ` Theodore Y. Ts'o
@ 2019-10-17 14:17 ` Christoph Hellwig
2019-10-17 14:48 ` Theodore Y. Ts'o
0 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2019-10-17 14:17 UTC (permalink / raw)
To: Theodore Y. Ts'o
Cc: Dave Chinner, linux-xfs, linux-ext4, linux-fsdevel, Matthew Bobrowski
On Thu, Oct 17, 2019 at 08:29:11AM -0400, Theodore Y. Ts'o wrote:
> > + /*
> > + * Writes that span EOF might trigger an IO size update on completion,
> > + * so consider them to be dirty for the purposes of O_DSYNC even if
> > + * there is no other metadata changes being made or are pending here.
> > + */
> > iomap->flags = 0;
> > - if (ext4_inode_datasync_dirty(inode))
> > + if (ext4_inode_datasync_dirty(inode) ||
> > + offset + length > i_size_read(inode))
> > iomap->flags |= IOMAP_F_DIRTY;
> > +
> > iomap->bdev = inode->i_sb->s_bdev;
> > iomap->dax_dev = sbi->s_daxdev;
> > iomap->offset = (u64)first_block << blkbits;
>
> Ext4 is not currently using iomap for any kind of writing right now,
> so perhaps this should land via Matthew's patchset?
It does for DAX, which is one of the consumers of IOMAP_F_DIRTY.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iomap: iomap that extends beyond EOF should be marked dirty
2019-10-17 14:17 ` Christoph Hellwig
@ 2019-10-17 14:48 ` Theodore Y. Ts'o
0 siblings, 0 replies; 9+ messages in thread
From: Theodore Y. Ts'o @ 2019-10-17 14:48 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Dave Chinner, linux-xfs, linux-ext4, linux-fsdevel, Matthew Bobrowski
On Thu, Oct 17, 2019 at 07:17:05AM -0700, Christoph Hellwig wrote:
> >
> > Ext4 is not currently using iomap for any kind of writing right now,
> > so perhaps this should land via Matthew's patchset?
>
> It does for DAX, which is one of the consumers of IOMAP_F_DIRTY.
Ah, right, I had forgotten about DAX.
- Ted
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-10-17 14:48 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-16 5:11 [PATCH] iomap: iomap that extends beyond EOF should be marked dirty Dave Chinner
2019-10-16 5:25 ` Dave Chinner
2019-10-16 5:27 ` Darrick J. Wong
2019-10-16 6:40 ` Christoph Hellwig
2019-10-16 6:06 ` [PATCH v2] " Dave Chinner
2019-10-16 6:43 ` Christoph Hellwig
2019-10-17 12:29 ` Theodore Y. Ts'o
2019-10-17 14:17 ` Christoph Hellwig
2019-10-17 14:48 ` Theodore Y. Ts'o
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).