iomap fixes

iomap fixes

[Christoph Hellwig]

A few iomap fixes for 4.16-rc and -stable:

 - don't allocate blocks for zeroing holes or unwritten extents on files
   with the reflink bit set
 - don't take the exlusive ilock when no needed in iomap_begin
 - don't block on the ilock for RWF_NOWAIT

[PATCH 1/3] xfs: don't allocate COW blocks for zeroing holes or unwritten extents

[Christoph Hellwig]

The iomap zeroing interface is smart enough to skip zeroing holes or
unwritten extents.  Don't subvert this logic for reflink files.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_iomap.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 66e1edbfb2b2..4e771e0f1170 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -955,6 +955,13 @@ static inline bool imap_needs_alloc(struct inode *inode,
 		(IS_DAX(inode) && imap->br_state == XFS_EXT_UNWRITTEN);
 }
 
+static inline bool needs_cow_for_zeroing(struct xfs_bmbt_irec *imap, int nimaps)
+{
+	return nimaps &&
+		imap->br_startblock != HOLESTARTBLOCK &&
+		imap->br_state != XFS_EXT_UNWRITTEN;
+}
+
 static inline bool need_excl_ilock(struct xfs_inode *ip, unsigned flags)
 {
 	/*
@@ -1024,7 +1031,9 @@ xfs_file_iomap_begin(
 			goto out_unlock;
 	}
 
-	if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
+	if (xfs_is_reflink_inode(ip) &&
+	    ((flags & IOMAP_WRITE) ||
+	     ((flags & IOMAP_ZERO) && needs_cow_for_zeroing(&imap, nimaps)))) {
 		if (flags & IOMAP_DIRECT) {
 			/*
 			 * A reflinked inode will result in CoW alloc.
-- 
2.14.2

Re: [PATCH 1/3] xfs: don't allocate COW blocks for zeroing holes or unwritten extents

[Dave Chinner]

On Wed, Feb 28, 2018 at 09:45:50AM -0800, Christoph Hellwig wrote:
> The iomap zeroing interface is smart enough to skip zeroing holes or
> unwritten extents.  Don't subvert this logic for reflink files.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good,

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/3] xfs: don't allocate COW blocks for zeroing holes or unwritten extents

[Darrick J. Wong]

On Fri, Mar 02, 2018 at 09:01:20AM +1100, Dave Chinner wrote:
> On Wed, Feb 28, 2018 at 09:45:50AM -0800, Christoph Hellwig wrote:
> > The iomap zeroing interface is smart enough to skip zeroing holes or
> > unwritten extents.  Don't subvert this logic for reflink files.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Looks good,
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

Looks ok, will test.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Oracle mail and vger are having some sort of idiotic slapfest, which
means I haven't received any list traffic in 2 days.  Please cc me on
patches so that review isn't delayed. :)

--D

> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 2/3] xfs: don't start out with the exclusive ilock for direct I/O

[Christoph Hellwig]

There is no reason to take the ilock exclusively at the start of
xfs_file_iomap_begin for direct I/O, given that it will be demoted
just before calling xfs_iomap_write_direct anyway.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_iomap.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 4e771e0f1170..ee01859b77a5 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -965,13 +965,11 @@ static inline bool needs_cow_for_zeroing(struct xfs_bmbt_irec *imap, int nimaps)
 static inline bool need_excl_ilock(struct xfs_inode *ip, unsigned flags)
 {
 	/*
-	 * COW writes will allocate delalloc space, so we need to make sure
-	 * to take the lock exclusively here.
+	 * COW writes may allocate delalloc space or convert unwritten COW
+	 * extents, so we need to make sure to take the lock exclusively here.
 	 */
 	if (xfs_is_reflink_inode(ip) && (flags & (IOMAP_WRITE | IOMAP_ZERO)))
 		return true;
-	if ((flags & IOMAP_DIRECT) && (flags & IOMAP_WRITE))
-		return true;
 	return false;
 }
 
-- 
2.14.2

Re: [PATCH 2/3] xfs: don't start out with the exclusive ilock for direct I/O

[Dave Chinner]

On Wed, Feb 28, 2018 at 09:45:51AM -0800, Christoph Hellwig wrote:
> There is no reason to take the ilock exclusively at the start of
> xfs_file_iomap_begin for direct I/O, given that it will be demoted
> just before calling xfs_iomap_write_direct anyway.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Seems to work fine, and reverts us back to the per-iomap behaviour.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

[PATCH 3/3] xfs: don't block on the ilock for RWF_NOWAIT

[Christoph Hellwig]

Fix xfs_file_iomap_begin to trylock the ilock if IOMAP_NOWAIT is passed,
so that we don't block io_submit callers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_iomap.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index ee01859b77a5..046469fcc1b8 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -970,6 +970,15 @@ static inline bool need_excl_ilock(struct xfs_inode *ip, unsigned flags)
 	 */
 	if (xfs_is_reflink_inode(ip) && (flags & (IOMAP_WRITE | IOMAP_ZERO)))
 		return true;
+
+	/*
+	 * Extents not yet cached requires exclusive access, don't block.
+	 * This is an opencoded xfs_ilock_data_map_shared() to cater for the
+	 * non-blocking behaviour.
+	 */
+	if (ip->i_d.di_format == XFS_DINODE_FMT_BTREE &&
+	    !(ip->i_df.if_flags & XFS_IFEXTENTS))
+		return true;
 	return false;
 }
 
@@ -998,16 +1007,18 @@ xfs_file_iomap_begin(
 		return xfs_file_iomap_begin_delay(inode, offset, length, iomap);
 	}
 
-	if (need_excl_ilock(ip, flags)) {
+	if (need_excl_ilock(ip, flags))
 		lockmode = XFS_ILOCK_EXCL;
-		xfs_ilock(ip, XFS_ILOCK_EXCL);
-	} else {
-		lockmode = xfs_ilock_data_map_shared(ip);
-	}
+	else
+		lockmode = XFS_ILOCK_SHARED;
 
-	if ((flags & IOMAP_NOWAIT) && !(ip->i_df.if_flags & XFS_IFEXTENTS)) {
-		error = -EAGAIN;
-		goto out_unlock;
+	if (flags & IOMAP_NOWAIT) {
+		if (!(ip->i_df.if_flags & XFS_IFEXTENTS))
+			return -EAGAIN;
+		if (!xfs_ilock_nowait(ip, lockmode))
+			return -EAGAIN;
+	} else {
+		xfs_ilock(ip, lockmode);
 	}
 
 	ASSERT(offset <= mp->m_super->s_maxbytes);
-- 
2.14.2

Re: [PATCH 3/3] xfs: don't block on the ilock for RWF_NOWAIT

[Dave Chinner]

On Wed, Feb 28, 2018 at 09:45:52AM -0800, Christoph Hellwig wrote:
> Fix xfs_file_iomap_begin to trylock the ilock if IOMAP_NOWAIT is passed,
> so that we don't block io_submit callers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I'm not really happy with the locking mess we've created, but I have
patches on top of this to clean it up a bit. SO as a stepping
stone,

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com