linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* a few iomap / bmap cleanups
@ 2019-10-25 15:03 Christoph Hellwig
  2019-10-25 15:03 ` [PATCH 1/8] xfs: simplify xfs_iomap_eof_align_last_fsb Christoph Hellwig
                   ` (7 more replies)
  0 siblings, 8 replies; 21+ messages in thread
From: Christoph Hellwig @ 2019-10-25 15:03 UTC (permalink / raw)
  To: linux-xfs

Hi all,

this series cleans up a few lose ends through the iomap and bmap
write path.

^ permalink raw reply	[flat|nested] 21+ messages in thread
* [PATCH 1/8] xfs: simplify xfs_iomap_eof_align_last_fsb
  2019-10-25 15:03 a few iomap / bmap cleanups Christoph Hellwig
@ 2019-10-25 15:03 ` Christoph Hellwig
  2019-10-28 15:55   ` Darrick J. Wong
  2019-10-25 15:03 ` [PATCH 2/8] xfs: mark xfs_eof_alignment static Christoph Hellwig
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2019-10-25 15:03 UTC (permalink / raw)
  To: linux-xfs

By open coding xfs_bmap_last_extent instead of calling it through a
double indirection we don't need to handle an error return that
can't happen given that we are guaranteed to have the extent list in
memory already.  Also simplify the calling conventions a little and
move the extent list assert from the only caller into the function.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_bmap_util.c | 23 ---------------------
 fs/xfs/xfs_bmap_util.h |  2 --
 fs/xfs/xfs_iomap.c     | 47 ++++++++++++++++++++----------------------
 3 files changed, 22 insertions(+), 50 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 11658da40640..44d6b6469303 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -179,29 +179,6 @@ xfs_bmap_rtalloc(
 }
 #endif /* CONFIG_XFS_RT */
 
-/*
- * Check if the endoff is outside the last extent. If so the caller will grow
- * the allocation to a stripe unit boundary.  All offsets are considered outside
- * the end of file for an empty fork, so 1 is returned in *eof in that case.
- */
-int
-xfs_bmap_eof(
-	struct xfs_inode	*ip,
-	xfs_fileoff_t		endoff,
-	int			whichfork,
-	int			*eof)
-{
-	struct xfs_bmbt_irec	rec;
-	int			error;
-
-	error = xfs_bmap_last_extent(NULL, ip, whichfork, &rec, eof);
-	if (error || *eof)
-		return error;
-
-	*eof = endoff >= rec.br_startoff + rec.br_blockcount;
-	return 0;
-}
-
 /*
  * Extent tree block counting routines.
  */
diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
index 3e0fa0d363d1..9f993168b55b 100644
--- a/fs/xfs/xfs_bmap_util.h
+++ b/fs/xfs/xfs_bmap_util.h
@@ -30,8 +30,6 @@ xfs_bmap_rtalloc(struct xfs_bmalloca *ap)
 }
 #endif /* CONFIG_XFS_RT */
 
-int	xfs_bmap_eof(struct xfs_inode *ip, xfs_fileoff_t endoff,
-		     int whichfork, int *eof);
 int	xfs_bmap_punch_delalloc_range(struct xfs_inode *ip,
 		xfs_fileoff_t start_fsb, xfs_fileoff_t length);
 
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index c1063507e5fd..49fbc99c18ff 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -156,25 +156,33 @@ xfs_eof_alignment(
 	return align;
 }
 
-STATIC int
+/*
+ * Check if last_fsb is outside the last extent, and if so grow it to the next
+ * stripe unit boundary.
+ */
+static xfs_fileoff_t
 xfs_iomap_eof_align_last_fsb(
 	struct xfs_inode	*ip,
-	xfs_extlen_t		extsize,
-	xfs_fileoff_t		*last_fsb)
+	xfs_fileoff_t		end_fsb)
 {
-	xfs_extlen_t		align = xfs_eof_alignment(ip, extsize);
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
+	xfs_extlen_t		extsz = xfs_get_extsz_hint(ip);
+	xfs_extlen_t		align = xfs_eof_alignment(ip, extsz);
+	struct xfs_bmbt_irec	irec;
+	struct xfs_iext_cursor	icur;
+
+	ASSERT(ifp->if_flags & XFS_IFEXTENTS);
 
 	if (align) {
-		xfs_fileoff_t	new_last_fsb = roundup_64(*last_fsb, align);
-		int		eof, error;
+		xfs_fileoff_t	aligned_end_fsb = roundup_64(end_fsb, align);
 
-		error = xfs_bmap_eof(ip, new_last_fsb, XFS_DATA_FORK, &eof);
-		if (error)
-			return error;
-		if (eof)
-			*last_fsb = new_last_fsb;
+		xfs_iext_last(ifp, &icur);
+		if (!xfs_iext_get_extent(ifp, &icur, &irec) ||
+		    aligned_end_fsb >= irec.br_startoff + irec.br_blockcount)
+			return aligned_end_fsb;
 	}
-	return 0;
+
+	return end_fsb;
 }
 
 int
@@ -206,19 +214,8 @@ xfs_iomap_write_direct(
 
 	ASSERT(xfs_isilocked(ip, lockmode));
 
-	if ((offset + count) > XFS_ISIZE(ip)) {
-		/*
-		 * Assert that the in-core extent list is present since this can
-		 * call xfs_iread_extents() and we only have the ilock shared.
-		 * This should be safe because the lock was held around a bmapi
-		 * call in the caller and we only need it to access the in-core
-		 * list.
-		 */
-		ASSERT(XFS_IFORK_PTR(ip, XFS_DATA_FORK)->if_flags &
-								XFS_IFEXTENTS);
-		error = xfs_iomap_eof_align_last_fsb(ip, extsz, &last_fsb);
-		if (error)
-			goto out_unlock;
+	if (offset + count > XFS_ISIZE(ip)) {
+		last_fsb = xfs_iomap_eof_align_last_fsb(ip, last_fsb);
 	} else {
 		if (nmaps && (imap->br_startblock == HOLESTARTBLOCK))
 			last_fsb = min(last_fsb, (xfs_fileoff_t)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 2/8] xfs: mark xfs_eof_alignment static
  2019-10-25 15:03 a few iomap / bmap cleanups Christoph Hellwig
  2019-10-25 15:03 ` [PATCH 1/8] xfs: simplify xfs_iomap_eof_align_last_fsb Christoph Hellwig
@ 2019-10-25 15:03 ` Christoph Hellwig
  2019-10-28 15:55   ` Darrick J. Wong
  2019-10-25 15:03 ` [PATCH 3/8] xfs: remove the extsize argument to xfs_eof_alignment Christoph Hellwig
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2019-10-25 15:03 UTC (permalink / raw)
  To: linux-xfs

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

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 49fbc99c18ff..c803a8efa8ff 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -116,7 +116,7 @@ xfs_iomap_end_fsb(
 		   XFS_B_TO_FSB(mp, mp->m_super->s_maxbytes));
 }
 
-xfs_extlen_t
+static xfs_extlen_t
 xfs_eof_alignment(
 	struct xfs_inode	*ip,
 	xfs_extlen_t		extsize)
diff --git a/fs/xfs/xfs_iomap.h b/fs/xfs/xfs_iomap.h
index 7aed28275089..d5b292bdaf82 100644
--- a/fs/xfs/xfs_iomap.h
+++ b/fs/xfs/xfs_iomap.h
@@ -17,7 +17,6 @@ int xfs_iomap_write_unwritten(struct xfs_inode *, xfs_off_t, xfs_off_t, bool);
 
 int xfs_bmbt_to_iomap(struct xfs_inode *, struct iomap *,
 		struct xfs_bmbt_irec *, u16);
-xfs_extlen_t xfs_eof_alignment(struct xfs_inode *ip, xfs_extlen_t extsize);
 
 static inline xfs_filblks_t
 xfs_aligned_fsb_count(
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 3/8] xfs: remove the extsize argument to xfs_eof_alignment
  2019-10-25 15:03 a few iomap / bmap cleanups Christoph Hellwig
  2019-10-25 15:03 ` [PATCH 1/8] xfs: simplify xfs_iomap_eof_align_last_fsb Christoph Hellwig
  2019-10-25 15:03 ` [PATCH 2/8] xfs: mark xfs_eof_alignment static Christoph Hellwig
@ 2019-10-25 15:03 ` Christoph Hellwig
  2019-10-28 16:06   ` Darrick J. Wong
  2019-10-25 15:03 ` [PATCH 4/8] xfs: don't log the inode in xfs_fs_map_blocks if it wasn't modified Christoph Hellwig
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2019-10-25 15:03 UTC (permalink / raw)
  To: linux-xfs

And move the code dependent on it to the one caller that cares
instead.

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

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index c803a8efa8ff..e3b11cda447e 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -118,8 +118,7 @@ xfs_iomap_end_fsb(
 
 static xfs_extlen_t
 xfs_eof_alignment(
-	struct xfs_inode	*ip,
-	xfs_extlen_t		extsize)
+	struct xfs_inode	*ip)
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	xfs_extlen_t		align = 0;
@@ -142,17 +141,6 @@ xfs_eof_alignment(
 			align = 0;
 	}
 
-	/*
-	 * Always round up the allocation request to an extent boundary
-	 * (when file on a real-time subvolume or has di_extsize hint).
-	 */
-	if (extsize) {
-		if (align)
-			align = roundup_64(align, extsize);
-		else
-			align = extsize;
-	}
-
 	return align;
 }
 
@@ -167,12 +155,22 @@ xfs_iomap_eof_align_last_fsb(
 {
 	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
 	xfs_extlen_t		extsz = xfs_get_extsz_hint(ip);
-	xfs_extlen_t		align = xfs_eof_alignment(ip, extsz);
+	xfs_extlen_t		align = xfs_eof_alignment(ip);
 	struct xfs_bmbt_irec	irec;
 	struct xfs_iext_cursor	icur;
 
 	ASSERT(ifp->if_flags & XFS_IFEXTENTS);
 
+	/*
+	 * Always round up the allocation request to the extent hint boundary.
+	 */
+	if (extsz) {
+		if (align)
+			align = roundup_64(align, extsz);
+		else
+			align = extsz;
+	}
+
 	if (align) {
 		xfs_fileoff_t	aligned_end_fsb = roundup_64(end_fsb, align);
 
@@ -992,7 +990,7 @@ xfs_buffered_write_iomap_begin(
 			p_end_fsb = XFS_B_TO_FSBT(mp, end_offset) +
 					prealloc_blocks;
 
-			align = xfs_eof_alignment(ip, 0);
+			align = xfs_eof_alignment(ip);
 			if (align)
 				p_end_fsb = roundup_64(p_end_fsb, align);
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 4/8] xfs: don't log the inode in xfs_fs_map_blocks if it wasn't modified
  2019-10-25 15:03 a few iomap / bmap cleanups Christoph Hellwig
                   ` (2 preceding siblings ...)
  2019-10-25 15:03 ` [PATCH 3/8] xfs: remove the extsize argument to xfs_eof_alignment Christoph Hellwig
@ 2019-10-25 15:03 ` Christoph Hellwig
  2019-10-28 16:12   ` Darrick J. Wong
  2019-10-25 15:03 ` [PATCH 5/8] xfs: simplify the xfs_iomap_write_direct calling conventions Christoph Hellwig
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2019-10-25 15:03 UTC (permalink / raw)
  To: linux-xfs

Even if we are asked for a write layout there is no point in logging
the inode unless we actually modified it in some way.

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

diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
index 9c96493be9e0..fa90c6334c7c 100644
--- a/fs/xfs/xfs_pnfs.c
+++ b/fs/xfs/xfs_pnfs.c
@@ -147,32 +147,27 @@ xfs_fs_map_blocks(
 	if (error)
 		goto out_unlock;
 
-	if (write) {
-		enum xfs_prealloc_flags	flags = 0;
-
+	if (write &&
+	    (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) {
 		ASSERT(imap.br_startblock != DELAYSTARTBLOCK);
 
-		if (!nimaps || imap.br_startblock == HOLESTARTBLOCK) {
-			/*
-			 * xfs_iomap_write_direct() expects to take ownership of
-			 * the shared ilock.
-			 */
-			xfs_ilock(ip, XFS_ILOCK_SHARED);
-			error = xfs_iomap_write_direct(ip, offset, length,
-						       &imap, nimaps);
-			if (error)
-				goto out_unlock;
-
-			/*
-			 * Ensure the next transaction is committed
-			 * synchronously so that the blocks allocated and
-			 * handed out to the client are guaranteed to be
-			 * present even after a server crash.
-			 */
-			flags |= XFS_PREALLOC_SET | XFS_PREALLOC_SYNC;
-		}
-
-		error = xfs_update_prealloc_flags(ip, flags);
+		/*
+		 * xfs_iomap_write_direct() expects to take ownership of the
+		 * shared ilock.
+		 */
+		xfs_ilock(ip, XFS_ILOCK_SHARED);
+		error = xfs_iomap_write_direct(ip, offset, length, &imap,
+					       nimaps);
+		if (error)
+			goto out_unlock;
+
+		/*
+		 * Ensure the next transaction is committed synchronously so
+		 * that the blocks allocated and handed out to the client are
+		 * guaranteed to be present even after a server crash.
+		 */
+		error = xfs_update_prealloc_flags(ip,
+				XFS_PREALLOC_SET | XFS_PREALLOC_SYNC);
 		if (error)
 			goto out_unlock;
 	}
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 5/8] xfs: simplify the xfs_iomap_write_direct calling conventions
  2019-10-25 15:03 a few iomap / bmap cleanups Christoph Hellwig
                   ` (3 preceding siblings ...)
  2019-10-25 15:03 ` [PATCH 4/8] xfs: don't log the inode in xfs_fs_map_blocks if it wasn't modified Christoph Hellwig
@ 2019-10-25 15:03 ` Christoph Hellwig
  2019-10-28 16:25   ` Darrick J. Wong
  2019-10-25 15:03 ` [PATCH 6/8] xfs: refactor xfs_bmapi_allocate Christoph Hellwig
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2019-10-25 15:03 UTC (permalink / raw)
  To: linux-xfs

Move the EOF alignment and checking for the next allocated extent into
the callers to avoid the need to pass the byte based offset and count
as well as looking at the incoming imap.  The added benefit is that
the caller can unlock the incoming ilock and the function doesn't have
funny unbalanced locking contexts.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_iomap.c | 82 ++++++++++++++++------------------------------
 fs/xfs/xfs_iomap.h |  6 ++--
 fs/xfs/xfs_pnfs.c  | 25 +++++++-------
 3 files changed, 45 insertions(+), 68 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index e3b11cda447e..4af50b101d2b 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -148,7 +148,7 @@ xfs_eof_alignment(
  * Check if last_fsb is outside the last extent, and if so grow it to the next
  * stripe unit boundary.
  */
-static xfs_fileoff_t
+xfs_fileoff_t
 xfs_iomap_eof_align_last_fsb(
 	struct xfs_inode	*ip,
 	xfs_fileoff_t		end_fsb)
@@ -185,61 +185,36 @@ xfs_iomap_eof_align_last_fsb(
 
 int
 xfs_iomap_write_direct(
-	xfs_inode_t	*ip,
-	xfs_off_t	offset,
-	size_t		count,
-	xfs_bmbt_irec_t *imap,
-	int		nmaps)
+	struct xfs_inode	*ip,
+	xfs_fileoff_t		offset_fsb,
+	xfs_fileoff_t		count_fsb,
+	struct xfs_bmbt_irec	*imap)
 {
-	xfs_mount_t	*mp = ip->i_mount;
-	xfs_fileoff_t	offset_fsb = XFS_B_TO_FSBT(mp, offset);
-	xfs_fileoff_t	last_fsb = xfs_iomap_end_fsb(mp, offset, count);
-	xfs_filblks_t	count_fsb, resaligned;
-	xfs_extlen_t	extsz;
-	int		nimaps;
-	int		quota_flag;
-	int		rt;
-	xfs_trans_t	*tp;
-	uint		qblocks, resblks, resrtextents;
-	int		error;
-	int		lockmode;
-	int		bmapi_flags = XFS_BMAPI_PREALLOC;
-	uint		tflags = 0;
-
-	rt = XFS_IS_REALTIME_INODE(ip);
-	extsz = xfs_get_extsz_hint(ip);
-	lockmode = XFS_ILOCK_SHARED;	/* locked by caller */
-
-	ASSERT(xfs_isilocked(ip, lockmode));
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_trans	*tp;
+	xfs_filblks_t		resaligned;
+	int			nimaps;
+	int			quota_flag;
+	uint			qblocks, resblks;
+	unsigned int		resrtextents = 0;
+	int			error;
+	int			bmapi_flags = XFS_BMAPI_PREALLOC;
+	uint			tflags = 0;
 
-	if (offset + count > XFS_ISIZE(ip)) {
-		last_fsb = xfs_iomap_eof_align_last_fsb(ip, last_fsb);
-	} else {
-		if (nmaps && (imap->br_startblock == HOLESTARTBLOCK))
-			last_fsb = min(last_fsb, (xfs_fileoff_t)
-					imap->br_blockcount +
-					imap->br_startoff);
-	}
-	count_fsb = last_fsb - offset_fsb;
 	ASSERT(count_fsb > 0);
-	resaligned = xfs_aligned_fsb_count(offset_fsb, count_fsb, extsz);
 
-	if (unlikely(rt)) {
+	resaligned = xfs_aligned_fsb_count(offset_fsb, count_fsb,
+					   xfs_get_extsz_hint(ip));
+	if (unlikely(XFS_IS_REALTIME_INODE(ip))) {
 		resrtextents = qblocks = resaligned;
 		resrtextents /= mp->m_sb.sb_rextsize;
 		resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
 		quota_flag = XFS_QMOPT_RES_RTBLKS;
 	} else {
-		resrtextents = 0;
 		resblks = qblocks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
 		quota_flag = XFS_QMOPT_RES_REGBLKS;
 	}
 
-	/*
-	 * Drop the shared lock acquired by the caller, attach the dquot if
-	 * necessary and move on to transaction setup.
-	 */
-	xfs_iunlock(ip, lockmode);
 	error = xfs_qm_dqattach(ip);
 	if (error)
 		return error;
@@ -269,8 +244,7 @@ xfs_iomap_write_direct(
 	if (error)
 		return error;
 
-	lockmode = XFS_ILOCK_EXCL;
-	xfs_ilock(ip, lockmode);
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
 
 	error = xfs_trans_reserve_quota_nblks(tp, ip, qblocks, 0, quota_flag);
 	if (error)
@@ -307,7 +281,7 @@ xfs_iomap_write_direct(
 		error = xfs_alert_fsblock_zero(ip, imap);
 
 out_unlock:
-	xfs_iunlock(ip, lockmode);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	return error;
 
 out_res_cancel:
@@ -807,14 +781,16 @@ xfs_direct_write_iomap_begin(
 	 * lower level functions are updated.
 	 */
 	length = min_t(loff_t, length, 1024 * PAGE_SIZE);
+	end_fsb = xfs_iomap_end_fsb(mp, offset, length);
 
-	/*
-	 * xfs_iomap_write_direct() expects the shared lock. It is unlocked on
-	 * return.
-	 */
-	if (lockmode == XFS_ILOCK_EXCL)
-		xfs_ilock_demote(ip, lockmode);
-	error = xfs_iomap_write_direct(ip, offset, length, &imap, nimaps);
+	if (offset + length > XFS_ISIZE(ip))
+		end_fsb = xfs_iomap_eof_align_last_fsb(ip, end_fsb);
+	else if (nimaps && imap.br_startblock == HOLESTARTBLOCK)
+		end_fsb = min(end_fsb, imap.br_startoff + imap.br_blockcount);
+	xfs_iunlock(ip, lockmode);
+
+	error = xfs_iomap_write_direct(ip, offset_fsb, end_fsb - offset_fsb,
+			&imap);
 	if (error)
 		return error;
 
diff --git a/fs/xfs/xfs_iomap.h b/fs/xfs/xfs_iomap.h
index d5b292bdaf82..7d3703556d0e 100644
--- a/fs/xfs/xfs_iomap.h
+++ b/fs/xfs/xfs_iomap.h
@@ -11,9 +11,11 @@
 struct xfs_inode;
 struct xfs_bmbt_irec;
 
-int xfs_iomap_write_direct(struct xfs_inode *, xfs_off_t, size_t,
-			struct xfs_bmbt_irec *, int);
+int xfs_iomap_write_direct(struct xfs_inode *ip, xfs_fileoff_t offset_fsb,
+		xfs_fileoff_t count_fsb, struct xfs_bmbt_irec *imap);
 int xfs_iomap_write_unwritten(struct xfs_inode *, xfs_off_t, xfs_off_t, bool);
+xfs_fileoff_t xfs_iomap_eof_align_last_fsb(struct xfs_inode *ip,
+		xfs_fileoff_t end_fsb);
 
 int xfs_bmbt_to_iomap(struct xfs_inode *, struct iomap *,
 		struct xfs_bmbt_irec *, u16);
diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
index fa90c6334c7c..f64996ca4870 100644
--- a/fs/xfs/xfs_pnfs.c
+++ b/fs/xfs/xfs_pnfs.c
@@ -142,22 +142,19 @@ xfs_fs_map_blocks(
 	lock_flags = xfs_ilock_data_map_shared(ip);
 	error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb,
 				&imap, &nimaps, bmapi_flags);
-	xfs_iunlock(ip, lock_flags);
-
-	if (error)
-		goto out_unlock;
-
-	if (write &&
+	if (!error && write &&
 	    (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) {
 		ASSERT(imap.br_startblock != DELAYSTARTBLOCK);
 
-		/*
-		 * xfs_iomap_write_direct() expects to take ownership of the
-		 * shared ilock.
-		 */
-		xfs_ilock(ip, XFS_ILOCK_SHARED);
-		error = xfs_iomap_write_direct(ip, offset, length, &imap,
-					       nimaps);
+		if (offset + length > XFS_ISIZE(ip))
+			end_fsb = xfs_iomap_eof_align_last_fsb(ip, end_fsb);
+		else if (nimaps && imap.br_startblock == HOLESTARTBLOCK)
+			end_fsb = min(end_fsb, imap.br_startoff +
+					       imap.br_blockcount);
+		xfs_iunlock(ip, lock_flags);
+
+		error = xfs_iomap_write_direct(ip, offset_fsb,
+				end_fsb - offset_fsb, &imap);
 		if (error)
 			goto out_unlock;
 
@@ -170,6 +167,8 @@ xfs_fs_map_blocks(
 				XFS_PREALLOC_SET | XFS_PREALLOC_SYNC);
 		if (error)
 			goto out_unlock;
+	} else {
+		xfs_iunlock(ip, lock_flags);
 	}
 	xfs_iunlock(ip, XFS_IOLOCK_EXCL);
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 6/8] xfs: refactor xfs_bmapi_allocate
  2019-10-25 15:03 a few iomap / bmap cleanups Christoph Hellwig
                   ` (4 preceding siblings ...)
  2019-10-25 15:03 ` [PATCH 5/8] xfs: simplify the xfs_iomap_write_direct calling conventions Christoph Hellwig
@ 2019-10-25 15:03 ` Christoph Hellwig
  2019-10-28 16:29   ` Darrick J. Wong
  2019-10-25 15:03 ` [PATCH 7/8] xfs: move extent zeroing to xfs_bmapi_allocate Christoph Hellwig
  2019-10-25 15:03 ` [PATCH 8/8] xfs: cleanup use of the XFS_ALLOC_ flags Christoph Hellwig
  7 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2019-10-25 15:03 UTC (permalink / raw)
  To: linux-xfs

Avoid duplicate userdata and data fork checks by restructuring the code
so we only have a helper for userdata allocations that combines these
checks in a straight foward way.  That also helps to obsoletes the
comments explaining what the code does as it is now clearly obvious.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_bmap.c | 93 +++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 48 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index ef75e223cb70..c278eff29e82 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3641,20 +3641,6 @@ xfs_bmap_btalloc(
 	return 0;
 }
 
-/*
- * xfs_bmap_alloc is called by xfs_bmapi to allocate an extent for a file.
- * It figures out where to ask the underlying allocator to put the new extent.
- */
-STATIC int
-xfs_bmap_alloc(
-	struct xfs_bmalloca	*ap)	/* bmap alloc argument struct */
-{
-	if (XFS_IS_REALTIME_INODE(ap->ip) &&
-	    xfs_alloc_is_userdata(ap->datatype))
-		return xfs_bmap_rtalloc(ap);
-	return xfs_bmap_btalloc(ap);
-}
-
 /* Trim extent to fit a logical block range. */
 void
 xfs_trim_extent(
@@ -4010,6 +3996,42 @@ xfs_bmapi_reserve_delalloc(
 	return error;
 }
 
+static int
+xfs_bmap_alloc_userdata(
+	struct xfs_bmalloca	*bma)
+{
+	struct xfs_mount	*mp = bma->ip->i_mount;
+	int			whichfork = xfs_bmapi_whichfork(bma->flags);
+	int			error;
+
+	/*
+	 * Set the data type being allocated. For the data fork, the first data
+	 * in the file is treated differently to all other allocations. For the
+	 * attribute fork, we only need to ensure the allocated range is not on
+	 * the busy list.
+	 */
+	bma->datatype = XFS_ALLOC_NOBUSY;
+	if (bma->flags & XFS_BMAPI_ZERO)
+		bma->datatype |= XFS_ALLOC_USERDATA_ZERO;
+	if (whichfork == XFS_DATA_FORK) {
+		if (bma->offset == 0)
+			bma->datatype |= XFS_ALLOC_INITIAL_USER_DATA;
+		else
+			bma->datatype |= XFS_ALLOC_USERDATA;
+
+		if (mp->m_dalign && bma->length >= mp->m_dalign) {
+			error = xfs_bmap_isaeof(bma, whichfork);
+			if (error)
+				return error;
+		}
+
+		if (XFS_IS_REALTIME_INODE(bma->ip))
+			return xfs_bmap_rtalloc(bma);
+	}
+
+	return xfs_bmap_btalloc(bma);
+}
+
 static int
 xfs_bmapi_allocate(
 	struct xfs_bmalloca	*bma)
@@ -4037,43 +4059,18 @@ xfs_bmapi_allocate(
 					bma->got.br_startoff - bma->offset);
 	}
 
-	/*
-	 * Set the data type being allocated. For the data fork, the first data
-	 * in the file is treated differently to all other allocations. For the
-	 * attribute fork, we only need to ensure the allocated range is not on
-	 * the busy list.
-	 */
-	if (!(bma->flags & XFS_BMAPI_METADATA)) {
-		bma->datatype = XFS_ALLOC_NOBUSY;
-		if (whichfork == XFS_DATA_FORK) {
-			if (bma->offset == 0)
-				bma->datatype |= XFS_ALLOC_INITIAL_USER_DATA;
-			else
-				bma->datatype |= XFS_ALLOC_USERDATA;
-		}
-		if (bma->flags & XFS_BMAPI_ZERO)
-			bma->datatype |= XFS_ALLOC_USERDATA_ZERO;
-	}
-
-	bma->minlen = (bma->flags & XFS_BMAPI_CONTIG) ? bma->length : 1;
-
-	/*
-	 * Only want to do the alignment at the eof if it is userdata and
-	 * allocation length is larger than a stripe unit.
-	 */
-	if (mp->m_dalign && bma->length >= mp->m_dalign &&
-	    !(bma->flags & XFS_BMAPI_METADATA) && whichfork == XFS_DATA_FORK) {
-		error = xfs_bmap_isaeof(bma, whichfork);
-		if (error)
-			return error;
-	}
+	if (bma->flags & XFS_BMAPI_CONTIG)
+		bma->minlen = bma->length;
+	else
+		bma->minlen = 1;
 
-	error = xfs_bmap_alloc(bma);
-	if (error)
+	if (bma->flags & XFS_BMAPI_METADATA)
+		error = xfs_bmap_btalloc(bma);
+	else
+		error = xfs_bmap_alloc_userdata(bma);
+	if (error || bma->blkno == NULLFSBLOCK)
 		return error;
 
-	if (bma->blkno == NULLFSBLOCK)
-		return 0;
 	if ((ifp->if_flags & XFS_IFBROOT) && !bma->cur)
 		bma->cur = xfs_bmbt_init_cursor(mp, bma->tp, bma->ip, whichfork);
 	/*
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 7/8] xfs: move extent zeroing to xfs_bmapi_allocate
  2019-10-25 15:03 a few iomap / bmap cleanups Christoph Hellwig
                   ` (5 preceding siblings ...)
  2019-10-25 15:03 ` [PATCH 6/8] xfs: refactor xfs_bmapi_allocate Christoph Hellwig
@ 2019-10-25 15:03 ` Christoph Hellwig
  2019-10-28 16:31   ` Darrick J. Wong
  2019-10-25 15:03 ` [PATCH 8/8] xfs: cleanup use of the XFS_ALLOC_ flags Christoph Hellwig
  7 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2019-10-25 15:03 UTC (permalink / raw)
  To: linux-xfs

Move the extent zeroing case there for the XFS_BMAPI_ZERO flag outside
the low-level allocator and into xfs_bmapi_allocate, where is still
is in transaction context, but outside the very lowlevel code where
it doesn't belong.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_alloc.c |  7 -------
 fs/xfs/libxfs/xfs_alloc.h |  4 +---
 fs/xfs/libxfs/xfs_bmap.c  | 10 ++++++----
 fs/xfs/xfs_bmap_util.c    |  7 -------
 4 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 925eba9489d5..ff6454887ff3 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -3083,13 +3083,6 @@ xfs_alloc_vextent(
 			args->len);
 #endif
 
-		/* Zero the extent if we were asked to do so */
-		if (args->datatype & XFS_ALLOC_USERDATA_ZERO) {
-			error = xfs_zero_extent(args->ip, args->fsbno, args->len);
-			if (error)
-				goto error0;
-		}
-
 	}
 	xfs_perag_put(args->pag);
 	return 0;
diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
index d6ed5d2c07c2..626384d75c9c 100644
--- a/fs/xfs/libxfs/xfs_alloc.h
+++ b/fs/xfs/libxfs/xfs_alloc.h
@@ -54,7 +54,6 @@ typedef struct xfs_alloc_arg {
 	struct xfs_mount *mp;		/* file system mount point */
 	struct xfs_buf	*agbp;		/* buffer for a.g. freelist header */
 	struct xfs_perag *pag;		/* per-ag struct for this agno */
-	struct xfs_inode *ip;		/* for userdata zeroing method */
 	xfs_fsblock_t	fsbno;		/* file system block number */
 	xfs_agnumber_t	agno;		/* allocation group number */
 	xfs_agblock_t	agbno;		/* allocation group-relative block # */
@@ -83,8 +82,7 @@ typedef struct xfs_alloc_arg {
  */
 #define XFS_ALLOC_USERDATA		(1 << 0)/* allocation is for user data*/
 #define XFS_ALLOC_INITIAL_USER_DATA	(1 << 1)/* special case start of file */
-#define XFS_ALLOC_USERDATA_ZERO		(1 << 2)/* zero extent on allocation */
-#define XFS_ALLOC_NOBUSY		(1 << 3)/* Busy extents not allowed */
+#define XFS_ALLOC_NOBUSY		(1 << 2)/* Busy extents not allowed */
 
 static inline bool
 xfs_alloc_is_userdata(int datatype)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index c278eff29e82..6ec3c48abc1b 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3555,8 +3555,6 @@ xfs_bmap_btalloc(
 	args.wasdel = ap->wasdel;
 	args.resv = XFS_AG_RESV_NONE;
 	args.datatype = ap->datatype;
-	if (ap->datatype & XFS_ALLOC_USERDATA_ZERO)
-		args.ip = ap->ip;
 
 	error = xfs_alloc_vextent(&args);
 	if (error)
@@ -4011,8 +4009,6 @@ xfs_bmap_alloc_userdata(
 	 * the busy list.
 	 */
 	bma->datatype = XFS_ALLOC_NOBUSY;
-	if (bma->flags & XFS_BMAPI_ZERO)
-		bma->datatype |= XFS_ALLOC_USERDATA_ZERO;
 	if (whichfork == XFS_DATA_FORK) {
 		if (bma->offset == 0)
 			bma->datatype |= XFS_ALLOC_INITIAL_USER_DATA;
@@ -4071,6 +4067,12 @@ xfs_bmapi_allocate(
 	if (error || bma->blkno == NULLFSBLOCK)
 		return error;
 
+	if (bma->flags & XFS_BMAPI_ZERO) {
+		error = xfs_zero_extent(bma->ip, bma->blkno, bma->length);
+		if (error)
+			return error;
+	}
+
 	if ((ifp->if_flags & XFS_IFBROOT) && !bma->cur)
 		bma->cur = xfs_bmbt_init_cursor(mp, bma->tp, bma->ip, whichfork);
 	/*
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 44d6b6469303..e418f9922bb1 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -165,13 +165,6 @@ xfs_bmap_rtalloc(
 		xfs_trans_mod_dquot_byino(ap->tp, ap->ip,
 			ap->wasdel ? XFS_TRANS_DQ_DELRTBCOUNT :
 					XFS_TRANS_DQ_RTBCOUNT, (long) ralen);
-
-		/* Zero the extent if we were asked to do so */
-		if (ap->datatype & XFS_ALLOC_USERDATA_ZERO) {
-			error = xfs_zero_extent(ap->ip, ap->blkno, ap->length);
-			if (error)
-				return error;
-		}
 	} else {
 		ap->length = 0;
 	}
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 8/8] xfs: cleanup use of the XFS_ALLOC_ flags
  2019-10-25 15:03 a few iomap / bmap cleanups Christoph Hellwig
                   ` (6 preceding siblings ...)
  2019-10-25 15:03 ` [PATCH 7/8] xfs: move extent zeroing to xfs_bmapi_allocate Christoph Hellwig
@ 2019-10-25 15:03 ` Christoph Hellwig
  2019-10-28 16:32   ` Darrick J. Wong
  7 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2019-10-25 15:03 UTC (permalink / raw)
  To: linux-xfs

Always set XFS_ALLOC_USERDATA for data fork allocations, and check it
in xfs_alloc_is_userdata instead of the current obsfucated check.
Also remove the xfs_alloc_is_userdata and xfs_alloc_allow_busy_reuse
helpers to make the code a little easier to understand.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_alloc.c |  8 ++++----
 fs/xfs/libxfs/xfs_alloc.h | 12 ------------
 fs/xfs/libxfs/xfs_bmap.c  | 11 +++++------
 fs/xfs/xfs_extent_busy.c  |  2 +-
 fs/xfs/xfs_filestream.c   |  2 +-
 5 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index ff6454887ff3..4a6d6a1ad9f3 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -331,7 +331,7 @@ xfs_alloc_compute_diff(
 	xfs_extlen_t	newlen1=0;	/* length with newbno1 */
 	xfs_extlen_t	newlen2=0;	/* length with newbno2 */
 	xfs_agblock_t	wantend;	/* end of target extent */
-	bool		userdata = xfs_alloc_is_userdata(datatype);
+	bool		userdata = datatype & XFS_ALLOC_USERDATA;
 
 	ASSERT(freelen >= wantlen);
 	freeend = freebno + freelen;
@@ -1041,9 +1041,9 @@ xfs_alloc_ag_vextent_small(
 		goto out;
 
 	xfs_extent_busy_reuse(args->mp, args->agno, fbno, 1,
-			      xfs_alloc_allow_busy_reuse(args->datatype));
+			      (args->datatype & XFS_ALLOC_NOBUSY));
 
-	if (xfs_alloc_is_userdata(args->datatype)) {
+	if (args->datatype & XFS_ALLOC_USERDATA) {
 		struct xfs_buf	*bp;
 
 		bp = xfs_btree_get_bufs(args->mp, args->tp, args->agno, fbno);
@@ -2380,7 +2380,7 @@ xfs_alloc_fix_freelist(
 	 * somewhere else if we are not being asked to try harder at this
 	 * point
 	 */
-	if (pag->pagf_metadata && xfs_alloc_is_userdata(args->datatype) &&
+	if (pag->pagf_metadata && (args->datatype & XFS_ALLOC_USERDATA) &&
 	    (flags & XFS_ALLOC_FLAG_TRYLOCK)) {
 		ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
 		goto out_agbp_relse;
diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
index 626384d75c9c..7380fbe4a3ff 100644
--- a/fs/xfs/libxfs/xfs_alloc.h
+++ b/fs/xfs/libxfs/xfs_alloc.h
@@ -84,18 +84,6 @@ typedef struct xfs_alloc_arg {
 #define XFS_ALLOC_INITIAL_USER_DATA	(1 << 1)/* special case start of file */
 #define XFS_ALLOC_NOBUSY		(1 << 2)/* Busy extents not allowed */
 
-static inline bool
-xfs_alloc_is_userdata(int datatype)
-{
-	return (datatype & ~XFS_ALLOC_NOBUSY) != 0;
-}
-
-static inline bool
-xfs_alloc_allow_busy_reuse(int datatype)
-{
-	return (datatype & XFS_ALLOC_NOBUSY) == 0;
-}
-
 /* freespace limit calculations */
 #define XFS_ALLOC_AGFL_RESERVE	4
 unsigned int xfs_alloc_set_aside(struct xfs_mount *mp);
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 6ec3c48abc1b..f62f66863801 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3059,7 +3059,7 @@ xfs_bmap_adjacent(
 	mp = ap->ip->i_mount;
 	nullfb = ap->tp->t_firstblock == NULLFSBLOCK;
 	rt = XFS_IS_REALTIME_INODE(ap->ip) &&
-		xfs_alloc_is_userdata(ap->datatype);
+		(ap->datatype & XFS_ALLOC_USERDATA);
 	fb_agno = nullfb ? NULLAGNUMBER : XFS_FSB_TO_AGNO(mp,
 							ap->tp->t_firstblock);
 	/*
@@ -3412,7 +3412,7 @@ xfs_bmap_btalloc(
 
 	if (ap->flags & XFS_BMAPI_COWFORK)
 		align = xfs_get_cowextsz_hint(ap->ip);
-	else if (xfs_alloc_is_userdata(ap->datatype))
+	else if (ap->datatype & XFS_ALLOC_USERDATA)
 		align = xfs_get_extsz_hint(ap->ip);
 	if (align) {
 		error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
@@ -3427,7 +3427,7 @@ xfs_bmap_btalloc(
 	fb_agno = nullfb ? NULLAGNUMBER : XFS_FSB_TO_AGNO(mp,
 							ap->tp->t_firstblock);
 	if (nullfb) {
-		if (xfs_alloc_is_userdata(ap->datatype) &&
+		if ((ap->datatype & XFS_ALLOC_USERDATA) &&
 		    xfs_inode_is_filestream(ap->ip)) {
 			ag = xfs_filestream_lookup_ag(ap->ip);
 			ag = (ag != NULLAGNUMBER) ? ag : 0;
@@ -3467,7 +3467,7 @@ xfs_bmap_btalloc(
 		 * enough for the request.  If one isn't found, then adjust
 		 * the minimum allocation size to the largest space found.
 		 */
-		if (xfs_alloc_is_userdata(ap->datatype) &&
+		if ((ap->datatype & XFS_ALLOC_USERDATA) &&
 		    xfs_inode_is_filestream(ap->ip))
 			error = xfs_bmap_btalloc_filestreams(ap, &args, &blen);
 		else
@@ -4010,10 +4010,9 @@ xfs_bmap_alloc_userdata(
 	 */
 	bma->datatype = XFS_ALLOC_NOBUSY;
 	if (whichfork == XFS_DATA_FORK) {
+		bma->datatype |= XFS_ALLOC_USERDATA;
 		if (bma->offset == 0)
 			bma->datatype |= XFS_ALLOC_INITIAL_USER_DATA;
-		else
-			bma->datatype |= XFS_ALLOC_USERDATA;
 
 		if (mp->m_dalign && bma->length >= mp->m_dalign) {
 			error = xfs_bmap_isaeof(bma, whichfork);
diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
index 2183d87be4cf..3991e59cfd18 100644
--- a/fs/xfs/xfs_extent_busy.c
+++ b/fs/xfs/xfs_extent_busy.c
@@ -367,7 +367,7 @@ xfs_extent_busy_trim(
 		 * If this is a metadata allocation, try to reuse the busy
 		 * extent instead of trimming the allocation.
 		 */
-		if (!xfs_alloc_is_userdata(args->datatype) &&
+		if (!(args->datatype & XFS_ALLOC_USERDATA) &&
 		    !(busyp->flags & XFS_EXTENT_BUSY_DISCARDED)) {
 			if (!xfs_extent_busy_update_extent(args->mp, args->pag,
 							  busyp, fbno, flen,
diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
index 574a7a8b4736..2ae356775f63 100644
--- a/fs/xfs/xfs_filestream.c
+++ b/fs/xfs/xfs_filestream.c
@@ -374,7 +374,7 @@ xfs_filestream_new_ag(
 		startag = (item->ag + 1) % mp->m_sb.sb_agcount;
 	}
 
-	if (xfs_alloc_is_userdata(ap->datatype))
+	if (ap->datatype & XFS_ALLOC_USERDATA)
 		flags |= XFS_PICK_USERDATA;
 	if (ap->tp->t_flags & XFS_TRANS_LOWMODE)
 		flags |= XFS_PICK_LOWSPACE;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* Re: [PATCH 1/8] xfs: simplify xfs_iomap_eof_align_last_fsb
  2019-10-25 15:03 ` [PATCH 1/8] xfs: simplify xfs_iomap_eof_align_last_fsb Christoph Hellwig
@ 2019-10-28 15:55   ` Darrick J. Wong
  0 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2019-10-28 15:55 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Fri, Oct 25, 2019 at 05:03:29PM +0200, Christoph Hellwig wrote:
> By open coding xfs_bmap_last_extent instead of calling it through a
> double indirection we don't need to handle an error return that
> can't happen given that we are guaranteed to have the extent list in
> memory already.  Also simplify the calling conventions a little and
> move the extent list assert from the only caller into the function.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_bmap_util.c | 23 ---------------------
>  fs/xfs/xfs_bmap_util.h |  2 --
>  fs/xfs/xfs_iomap.c     | 47 ++++++++++++++++++++----------------------
>  3 files changed, 22 insertions(+), 50 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 11658da40640..44d6b6469303 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -179,29 +179,6 @@ xfs_bmap_rtalloc(
>  }
>  #endif /* CONFIG_XFS_RT */
>  
> -/*
> - * Check if the endoff is outside the last extent. If so the caller will grow
> - * the allocation to a stripe unit boundary.  All offsets are considered outside
> - * the end of file for an empty fork, so 1 is returned in *eof in that case.
> - */
> -int
> -xfs_bmap_eof(
> -	struct xfs_inode	*ip,
> -	xfs_fileoff_t		endoff,
> -	int			whichfork,
> -	int			*eof)
> -{
> -	struct xfs_bmbt_irec	rec;
> -	int			error;
> -
> -	error = xfs_bmap_last_extent(NULL, ip, whichfork, &rec, eof);
> -	if (error || *eof)
> -		return error;
> -
> -	*eof = endoff >= rec.br_startoff + rec.br_blockcount;
> -	return 0;
> -}
> -
>  /*
>   * Extent tree block counting routines.
>   */
> diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
> index 3e0fa0d363d1..9f993168b55b 100644
> --- a/fs/xfs/xfs_bmap_util.h
> +++ b/fs/xfs/xfs_bmap_util.h
> @@ -30,8 +30,6 @@ xfs_bmap_rtalloc(struct xfs_bmalloca *ap)
>  }
>  #endif /* CONFIG_XFS_RT */
>  
> -int	xfs_bmap_eof(struct xfs_inode *ip, xfs_fileoff_t endoff,
> -		     int whichfork, int *eof);
>  int	xfs_bmap_punch_delalloc_range(struct xfs_inode *ip,
>  		xfs_fileoff_t start_fsb, xfs_fileoff_t length);
>  
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index c1063507e5fd..49fbc99c18ff 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -156,25 +156,33 @@ xfs_eof_alignment(
>  	return align;
>  }
>  
> -STATIC int
> +/*
> + * Check if last_fsb is outside the last extent, and if so grow it to the next
> + * stripe unit boundary.
> + */
> +static xfs_fileoff_t
>  xfs_iomap_eof_align_last_fsb(
>  	struct xfs_inode	*ip,
> -	xfs_extlen_t		extsize,
> -	xfs_fileoff_t		*last_fsb)
> +	xfs_fileoff_t		end_fsb)
>  {
> -	xfs_extlen_t		align = xfs_eof_alignment(ip, extsize);
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> +	xfs_extlen_t		extsz = xfs_get_extsz_hint(ip);
> +	xfs_extlen_t		align = xfs_eof_alignment(ip, extsz);
> +	struct xfs_bmbt_irec	irec;
> +	struct xfs_iext_cursor	icur;
> +
> +	ASSERT(ifp->if_flags & XFS_IFEXTENTS);
>  
>  	if (align) {
> -		xfs_fileoff_t	new_last_fsb = roundup_64(*last_fsb, align);
> -		int		eof, error;
> +		xfs_fileoff_t	aligned_end_fsb = roundup_64(end_fsb, align);
>  
> -		error = xfs_bmap_eof(ip, new_last_fsb, XFS_DATA_FORK, &eof);
> -		if (error)
> -			return error;
> -		if (eof)
> -			*last_fsb = new_last_fsb;
> +		xfs_iext_last(ifp, &icur);
> +		if (!xfs_iext_get_extent(ifp, &icur, &irec) ||
> +		    aligned_end_fsb >= irec.br_startoff + irec.br_blockcount)
> +			return aligned_end_fsb;
>  	}
> -	return 0;
> +
> +	return end_fsb;
>  }
>  
>  int
> @@ -206,19 +214,8 @@ xfs_iomap_write_direct(
>  
>  	ASSERT(xfs_isilocked(ip, lockmode));
>  
> -	if ((offset + count) > XFS_ISIZE(ip)) {
> -		/*
> -		 * Assert that the in-core extent list is present since this can
> -		 * call xfs_iread_extents() and we only have the ilock shared.
> -		 * This should be safe because the lock was held around a bmapi
> -		 * call in the caller and we only need it to access the in-core
> -		 * list.
> -		 */
> -		ASSERT(XFS_IFORK_PTR(ip, XFS_DATA_FORK)->if_flags &
> -								XFS_IFEXTENTS);
> -		error = xfs_iomap_eof_align_last_fsb(ip, extsz, &last_fsb);
> -		if (error)
> -			goto out_unlock;
> +	if (offset + count > XFS_ISIZE(ip)) {
> +		last_fsb = xfs_iomap_eof_align_last_fsb(ip, last_fsb);
>  	} else {
>  		if (nmaps && (imap->br_startblock == HOLESTARTBLOCK))
>  			last_fsb = min(last_fsb, (xfs_fileoff_t)
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 2/8] xfs: mark xfs_eof_alignment static
  2019-10-25 15:03 ` [PATCH 2/8] xfs: mark xfs_eof_alignment static Christoph Hellwig
@ 2019-10-28 15:55   ` Darrick J. Wong
  0 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2019-10-28 15:55 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Fri, Oct 25, 2019 at 05:03:30PM +0200, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_iomap.c | 2 +-
>  fs/xfs/xfs_iomap.h | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 49fbc99c18ff..c803a8efa8ff 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -116,7 +116,7 @@ xfs_iomap_end_fsb(
>  		   XFS_B_TO_FSB(mp, mp->m_super->s_maxbytes));
>  }
>  
> -xfs_extlen_t
> +static xfs_extlen_t
>  xfs_eof_alignment(
>  	struct xfs_inode	*ip,
>  	xfs_extlen_t		extsize)
> diff --git a/fs/xfs/xfs_iomap.h b/fs/xfs/xfs_iomap.h
> index 7aed28275089..d5b292bdaf82 100644
> --- a/fs/xfs/xfs_iomap.h
> +++ b/fs/xfs/xfs_iomap.h
> @@ -17,7 +17,6 @@ int xfs_iomap_write_unwritten(struct xfs_inode *, xfs_off_t, xfs_off_t, bool);
>  
>  int xfs_bmbt_to_iomap(struct xfs_inode *, struct iomap *,
>  		struct xfs_bmbt_irec *, u16);
> -xfs_extlen_t xfs_eof_alignment(struct xfs_inode *ip, xfs_extlen_t extsize);
>  
>  static inline xfs_filblks_t
>  xfs_aligned_fsb_count(
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 3/8] xfs: remove the extsize argument to xfs_eof_alignment
  2019-10-25 15:03 ` [PATCH 3/8] xfs: remove the extsize argument to xfs_eof_alignment Christoph Hellwig
@ 2019-10-28 16:06   ` Darrick J. Wong
  2019-10-29  7:57     ` Christoph Hellwig
  0 siblings, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2019-10-28 16:06 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Fri, Oct 25, 2019 at 05:03:31PM +0200, Christoph Hellwig wrote:
> And move the code dependent on it to the one caller that cares
> instead.

Hmm, so if I'm understanding this correctly, now xfs_eof_alignment
rounds alignment up to the stripe width (or dalign) for files on the
data device?  And the alignment number it produces is further rounded up
to the extent hint size which is then used to round up a space
allocation (directio writes) or used to round up the speculative
preallocation window (buffered writes)?

Why does it make more sense to do the inode extsize roundup only for
direct writes and not as an intermediate step of determining the
speculative preallocation size than what the code does now?

--D

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_iomap.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index c803a8efa8ff..e3b11cda447e 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -118,8 +118,7 @@ xfs_iomap_end_fsb(
>  
>  static xfs_extlen_t
>  xfs_eof_alignment(
> -	struct xfs_inode	*ip,
> -	xfs_extlen_t		extsize)
> +	struct xfs_inode	*ip)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	xfs_extlen_t		align = 0;
> @@ -142,17 +141,6 @@ xfs_eof_alignment(
>  			align = 0;
>  	}
>  
> -	/*
> -	 * Always round up the allocation request to an extent boundary
> -	 * (when file on a real-time subvolume or has di_extsize hint).
> -	 */
> -	if (extsize) {
> -		if (align)
> -			align = roundup_64(align, extsize);
> -		else
> -			align = extsize;
> -	}
> -
>  	return align;
>  }
>  
> @@ -167,12 +155,22 @@ xfs_iomap_eof_align_last_fsb(
>  {
>  	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
>  	xfs_extlen_t		extsz = xfs_get_extsz_hint(ip);
> -	xfs_extlen_t		align = xfs_eof_alignment(ip, extsz);
> +	xfs_extlen_t		align = xfs_eof_alignment(ip);
>  	struct xfs_bmbt_irec	irec;
>  	struct xfs_iext_cursor	icur;
>  
>  	ASSERT(ifp->if_flags & XFS_IFEXTENTS);
>  
> +	/*
> +	 * Always round up the allocation request to the extent hint boundary.
> +	 */
> +	if (extsz) {
> +		if (align)
> +			align = roundup_64(align, extsz);
> +		else
> +			align = extsz;
> +	}
> +
>  	if (align) {
>  		xfs_fileoff_t	aligned_end_fsb = roundup_64(end_fsb, align);
>  
> @@ -992,7 +990,7 @@ xfs_buffered_write_iomap_begin(
>  			p_end_fsb = XFS_B_TO_FSBT(mp, end_offset) +
>  					prealloc_blocks;
>  
> -			align = xfs_eof_alignment(ip, 0);
> +			align = xfs_eof_alignment(ip);
>  			if (align)
>  				p_end_fsb = roundup_64(p_end_fsb, align);
>  
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 4/8] xfs: don't log the inode in xfs_fs_map_blocks if it wasn't modified
  2019-10-25 15:03 ` [PATCH 4/8] xfs: don't log the inode in xfs_fs_map_blocks if it wasn't modified Christoph Hellwig
@ 2019-10-28 16:12   ` Darrick J. Wong
  2019-10-29  7:58     ` Christoph Hellwig
  0 siblings, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2019-10-28 16:12 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Fri, Oct 25, 2019 at 05:03:32PM +0200, Christoph Hellwig wrote:
> Even if we are asked for a write layout there is no point in logging
> the inode unless we actually modified it in some way.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_pnfs.c | 43 +++++++++++++++++++------------------------
>  1 file changed, 19 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
> index 9c96493be9e0..fa90c6334c7c 100644
> --- a/fs/xfs/xfs_pnfs.c
> +++ b/fs/xfs/xfs_pnfs.c
> @@ -147,32 +147,27 @@ xfs_fs_map_blocks(
>  	if (error)
>  		goto out_unlock;
>  
> -	if (write) {
> -		enum xfs_prealloc_flags	flags = 0;
> -
> +	if (write &&
> +	    (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) {
>  		ASSERT(imap.br_startblock != DELAYSTARTBLOCK);

The change in code flow makes this assert rather useless, I think, since
we only end up in this branch if we have a write and a hole.  If the
condition that it checks is important (and it seems to be?) then it
ought to be hoisted up a level and turned into:

ASSERT(!write || !nimaps || imap.br_startblock != DELAYSTARTBLOCK);

Right?

Otherwise looks ok.

--D

>  
> -		if (!nimaps || imap.br_startblock == HOLESTARTBLOCK) {
> -			/*
> -			 * xfs_iomap_write_direct() expects to take ownership of
> -			 * the shared ilock.
> -			 */
> -			xfs_ilock(ip, XFS_ILOCK_SHARED);
> -			error = xfs_iomap_write_direct(ip, offset, length,
> -						       &imap, nimaps);
> -			if (error)
> -				goto out_unlock;
> -
> -			/*
> -			 * Ensure the next transaction is committed
> -			 * synchronously so that the blocks allocated and
> -			 * handed out to the client are guaranteed to be
> -			 * present even after a server crash.
> -			 */
> -			flags |= XFS_PREALLOC_SET | XFS_PREALLOC_SYNC;
> -		}
> -
> -		error = xfs_update_prealloc_flags(ip, flags);
> +		/*
> +		 * xfs_iomap_write_direct() expects to take ownership of the
> +		 * shared ilock.
> +		 */
> +		xfs_ilock(ip, XFS_ILOCK_SHARED);
> +		error = xfs_iomap_write_direct(ip, offset, length, &imap,
> +					       nimaps);
> +		if (error)
> +			goto out_unlock;
> +
> +		/*
> +		 * Ensure the next transaction is committed synchronously so
> +		 * that the blocks allocated and handed out to the client are
> +		 * guaranteed to be present even after a server crash.
> +		 */
> +		error = xfs_update_prealloc_flags(ip,
> +				XFS_PREALLOC_SET | XFS_PREALLOC_SYNC);
>  		if (error)
>  			goto out_unlock;
>  	}
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 5/8] xfs: simplify the xfs_iomap_write_direct calling conventions
  2019-10-25 15:03 ` [PATCH 5/8] xfs: simplify the xfs_iomap_write_direct calling conventions Christoph Hellwig
@ 2019-10-28 16:25   ` Darrick J. Wong
  0 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2019-10-28 16:25 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Fri, Oct 25, 2019 at 05:03:33PM +0200, Christoph Hellwig wrote:
> Move the EOF alignment and checking for the next allocated extent into
> the callers to avoid the need to pass the byte based offset and count
> as well as looking at the incoming imap.  The added benefit is that
> the caller can unlock the incoming ilock and the function doesn't have
> funny unbalanced locking contexts.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok, though it took me a while to figure out the old unlocking
strategy well enough to compare it to the new one. :)

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_iomap.c | 82 ++++++++++++++++------------------------------
>  fs/xfs/xfs_iomap.h |  6 ++--
>  fs/xfs/xfs_pnfs.c  | 25 +++++++-------
>  3 files changed, 45 insertions(+), 68 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index e3b11cda447e..4af50b101d2b 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -148,7 +148,7 @@ xfs_eof_alignment(
>   * Check if last_fsb is outside the last extent, and if so grow it to the next
>   * stripe unit boundary.
>   */
> -static xfs_fileoff_t
> +xfs_fileoff_t
>  xfs_iomap_eof_align_last_fsb(
>  	struct xfs_inode	*ip,
>  	xfs_fileoff_t		end_fsb)
> @@ -185,61 +185,36 @@ xfs_iomap_eof_align_last_fsb(
>  
>  int
>  xfs_iomap_write_direct(
> -	xfs_inode_t	*ip,
> -	xfs_off_t	offset,
> -	size_t		count,
> -	xfs_bmbt_irec_t *imap,
> -	int		nmaps)
> +	struct xfs_inode	*ip,
> +	xfs_fileoff_t		offset_fsb,
> +	xfs_fileoff_t		count_fsb,
> +	struct xfs_bmbt_irec	*imap)
>  {
> -	xfs_mount_t	*mp = ip->i_mount;
> -	xfs_fileoff_t	offset_fsb = XFS_B_TO_FSBT(mp, offset);
> -	xfs_fileoff_t	last_fsb = xfs_iomap_end_fsb(mp, offset, count);
> -	xfs_filblks_t	count_fsb, resaligned;
> -	xfs_extlen_t	extsz;
> -	int		nimaps;
> -	int		quota_flag;
> -	int		rt;
> -	xfs_trans_t	*tp;
> -	uint		qblocks, resblks, resrtextents;
> -	int		error;
> -	int		lockmode;
> -	int		bmapi_flags = XFS_BMAPI_PREALLOC;
> -	uint		tflags = 0;
> -
> -	rt = XFS_IS_REALTIME_INODE(ip);
> -	extsz = xfs_get_extsz_hint(ip);
> -	lockmode = XFS_ILOCK_SHARED;	/* locked by caller */
> -
> -	ASSERT(xfs_isilocked(ip, lockmode));
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_trans	*tp;
> +	xfs_filblks_t		resaligned;
> +	int			nimaps;
> +	int			quota_flag;
> +	uint			qblocks, resblks;
> +	unsigned int		resrtextents = 0;
> +	int			error;
> +	int			bmapi_flags = XFS_BMAPI_PREALLOC;
> +	uint			tflags = 0;
>  
> -	if (offset + count > XFS_ISIZE(ip)) {
> -		last_fsb = xfs_iomap_eof_align_last_fsb(ip, last_fsb);
> -	} else {
> -		if (nmaps && (imap->br_startblock == HOLESTARTBLOCK))
> -			last_fsb = min(last_fsb, (xfs_fileoff_t)
> -					imap->br_blockcount +
> -					imap->br_startoff);
> -	}
> -	count_fsb = last_fsb - offset_fsb;
>  	ASSERT(count_fsb > 0);
> -	resaligned = xfs_aligned_fsb_count(offset_fsb, count_fsb, extsz);
>  
> -	if (unlikely(rt)) {
> +	resaligned = xfs_aligned_fsb_count(offset_fsb, count_fsb,
> +					   xfs_get_extsz_hint(ip));
> +	if (unlikely(XFS_IS_REALTIME_INODE(ip))) {
>  		resrtextents = qblocks = resaligned;
>  		resrtextents /= mp->m_sb.sb_rextsize;
>  		resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
>  		quota_flag = XFS_QMOPT_RES_RTBLKS;
>  	} else {
> -		resrtextents = 0;
>  		resblks = qblocks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
>  		quota_flag = XFS_QMOPT_RES_REGBLKS;
>  	}
>  
> -	/*
> -	 * Drop the shared lock acquired by the caller, attach the dquot if
> -	 * necessary and move on to transaction setup.
> -	 */
> -	xfs_iunlock(ip, lockmode);
>  	error = xfs_qm_dqattach(ip);
>  	if (error)
>  		return error;
> @@ -269,8 +244,7 @@ xfs_iomap_write_direct(
>  	if (error)
>  		return error;
>  
> -	lockmode = XFS_ILOCK_EXCL;
> -	xfs_ilock(ip, lockmode);
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  
>  	error = xfs_trans_reserve_quota_nblks(tp, ip, qblocks, 0, quota_flag);
>  	if (error)
> @@ -307,7 +281,7 @@ xfs_iomap_write_direct(
>  		error = xfs_alert_fsblock_zero(ip, imap);
>  
>  out_unlock:
> -	xfs_iunlock(ip, lockmode);
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  	return error;
>  
>  out_res_cancel:
> @@ -807,14 +781,16 @@ xfs_direct_write_iomap_begin(
>  	 * lower level functions are updated.
>  	 */
>  	length = min_t(loff_t, length, 1024 * PAGE_SIZE);
> +	end_fsb = xfs_iomap_end_fsb(mp, offset, length);
>  
> -	/*
> -	 * xfs_iomap_write_direct() expects the shared lock. It is unlocked on
> -	 * return.
> -	 */
> -	if (lockmode == XFS_ILOCK_EXCL)
> -		xfs_ilock_demote(ip, lockmode);
> -	error = xfs_iomap_write_direct(ip, offset, length, &imap, nimaps);
> +	if (offset + length > XFS_ISIZE(ip))
> +		end_fsb = xfs_iomap_eof_align_last_fsb(ip, end_fsb);
> +	else if (nimaps && imap.br_startblock == HOLESTARTBLOCK)
> +		end_fsb = min(end_fsb, imap.br_startoff + imap.br_blockcount);
> +	xfs_iunlock(ip, lockmode);
> +
> +	error = xfs_iomap_write_direct(ip, offset_fsb, end_fsb - offset_fsb,
> +			&imap);
>  	if (error)
>  		return error;
>  
> diff --git a/fs/xfs/xfs_iomap.h b/fs/xfs/xfs_iomap.h
> index d5b292bdaf82..7d3703556d0e 100644
> --- a/fs/xfs/xfs_iomap.h
> +++ b/fs/xfs/xfs_iomap.h
> @@ -11,9 +11,11 @@
>  struct xfs_inode;
>  struct xfs_bmbt_irec;
>  
> -int xfs_iomap_write_direct(struct xfs_inode *, xfs_off_t, size_t,
> -			struct xfs_bmbt_irec *, int);
> +int xfs_iomap_write_direct(struct xfs_inode *ip, xfs_fileoff_t offset_fsb,
> +		xfs_fileoff_t count_fsb, struct xfs_bmbt_irec *imap);
>  int xfs_iomap_write_unwritten(struct xfs_inode *, xfs_off_t, xfs_off_t, bool);
> +xfs_fileoff_t xfs_iomap_eof_align_last_fsb(struct xfs_inode *ip,
> +		xfs_fileoff_t end_fsb);
>  
>  int xfs_bmbt_to_iomap(struct xfs_inode *, struct iomap *,
>  		struct xfs_bmbt_irec *, u16);
> diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
> index fa90c6334c7c..f64996ca4870 100644
> --- a/fs/xfs/xfs_pnfs.c
> +++ b/fs/xfs/xfs_pnfs.c
> @@ -142,22 +142,19 @@ xfs_fs_map_blocks(
>  	lock_flags = xfs_ilock_data_map_shared(ip);
>  	error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb,
>  				&imap, &nimaps, bmapi_flags);
> -	xfs_iunlock(ip, lock_flags);
> -
> -	if (error)
> -		goto out_unlock;
> -
> -	if (write &&
> +	if (!error && write &&
>  	    (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) {
>  		ASSERT(imap.br_startblock != DELAYSTARTBLOCK);
>  
> -		/*
> -		 * xfs_iomap_write_direct() expects to take ownership of the
> -		 * shared ilock.
> -		 */
> -		xfs_ilock(ip, XFS_ILOCK_SHARED);
> -		error = xfs_iomap_write_direct(ip, offset, length, &imap,
> -					       nimaps);
> +		if (offset + length > XFS_ISIZE(ip))
> +			end_fsb = xfs_iomap_eof_align_last_fsb(ip, end_fsb);
> +		else if (nimaps && imap.br_startblock == HOLESTARTBLOCK)
> +			end_fsb = min(end_fsb, imap.br_startoff +
> +					       imap.br_blockcount);
> +		xfs_iunlock(ip, lock_flags);
> +
> +		error = xfs_iomap_write_direct(ip, offset_fsb,
> +				end_fsb - offset_fsb, &imap);
>  		if (error)
>  			goto out_unlock;
>  
> @@ -170,6 +167,8 @@ xfs_fs_map_blocks(
>  				XFS_PREALLOC_SET | XFS_PREALLOC_SYNC);
>  		if (error)
>  			goto out_unlock;
> +	} else {
> +		xfs_iunlock(ip, lock_flags);
>  	}
>  	xfs_iunlock(ip, XFS_IOLOCK_EXCL);
>  
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 6/8] xfs: refactor xfs_bmapi_allocate
  2019-10-25 15:03 ` [PATCH 6/8] xfs: refactor xfs_bmapi_allocate Christoph Hellwig
@ 2019-10-28 16:29   ` Darrick J. Wong
  0 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2019-10-28 16:29 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Fri, Oct 25, 2019 at 05:03:34PM +0200, Christoph Hellwig wrote:
> Avoid duplicate userdata and data fork checks by restructuring the code
> so we only have a helper for userdata allocations that combines these
> checks in a straight foward way.  That also helps to obsoletes the
> comments explaining what the code does as it is now clearly obvious.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

LGTM,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_bmap.c | 93 +++++++++++++++++++---------------------
>  1 file changed, 45 insertions(+), 48 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index ef75e223cb70..c278eff29e82 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3641,20 +3641,6 @@ xfs_bmap_btalloc(
>  	return 0;
>  }
>  
> -/*
> - * xfs_bmap_alloc is called by xfs_bmapi to allocate an extent for a file.
> - * It figures out where to ask the underlying allocator to put the new extent.
> - */
> -STATIC int
> -xfs_bmap_alloc(
> -	struct xfs_bmalloca	*ap)	/* bmap alloc argument struct */
> -{
> -	if (XFS_IS_REALTIME_INODE(ap->ip) &&
> -	    xfs_alloc_is_userdata(ap->datatype))
> -		return xfs_bmap_rtalloc(ap);
> -	return xfs_bmap_btalloc(ap);
> -}
> -
>  /* Trim extent to fit a logical block range. */
>  void
>  xfs_trim_extent(
> @@ -4010,6 +3996,42 @@ xfs_bmapi_reserve_delalloc(
>  	return error;
>  }
>  
> +static int
> +xfs_bmap_alloc_userdata(
> +	struct xfs_bmalloca	*bma)
> +{
> +	struct xfs_mount	*mp = bma->ip->i_mount;
> +	int			whichfork = xfs_bmapi_whichfork(bma->flags);
> +	int			error;
> +
> +	/*
> +	 * Set the data type being allocated. For the data fork, the first data
> +	 * in the file is treated differently to all other allocations. For the
> +	 * attribute fork, we only need to ensure the allocated range is not on
> +	 * the busy list.
> +	 */
> +	bma->datatype = XFS_ALLOC_NOBUSY;
> +	if (bma->flags & XFS_BMAPI_ZERO)
> +		bma->datatype |= XFS_ALLOC_USERDATA_ZERO;
> +	if (whichfork == XFS_DATA_FORK) {
> +		if (bma->offset == 0)
> +			bma->datatype |= XFS_ALLOC_INITIAL_USER_DATA;
> +		else
> +			bma->datatype |= XFS_ALLOC_USERDATA;
> +
> +		if (mp->m_dalign && bma->length >= mp->m_dalign) {
> +			error = xfs_bmap_isaeof(bma, whichfork);
> +			if (error)
> +				return error;
> +		}
> +
> +		if (XFS_IS_REALTIME_INODE(bma->ip))
> +			return xfs_bmap_rtalloc(bma);
> +	}
> +
> +	return xfs_bmap_btalloc(bma);
> +}
> +
>  static int
>  xfs_bmapi_allocate(
>  	struct xfs_bmalloca	*bma)
> @@ -4037,43 +4059,18 @@ xfs_bmapi_allocate(
>  					bma->got.br_startoff - bma->offset);
>  	}
>  
> -	/*
> -	 * Set the data type being allocated. For the data fork, the first data
> -	 * in the file is treated differently to all other allocations. For the
> -	 * attribute fork, we only need to ensure the allocated range is not on
> -	 * the busy list.
> -	 */
> -	if (!(bma->flags & XFS_BMAPI_METADATA)) {
> -		bma->datatype = XFS_ALLOC_NOBUSY;
> -		if (whichfork == XFS_DATA_FORK) {
> -			if (bma->offset == 0)
> -				bma->datatype |= XFS_ALLOC_INITIAL_USER_DATA;
> -			else
> -				bma->datatype |= XFS_ALLOC_USERDATA;
> -		}
> -		if (bma->flags & XFS_BMAPI_ZERO)
> -			bma->datatype |= XFS_ALLOC_USERDATA_ZERO;
> -	}
> -
> -	bma->minlen = (bma->flags & XFS_BMAPI_CONTIG) ? bma->length : 1;
> -
> -	/*
> -	 * Only want to do the alignment at the eof if it is userdata and
> -	 * allocation length is larger than a stripe unit.
> -	 */
> -	if (mp->m_dalign && bma->length >= mp->m_dalign &&
> -	    !(bma->flags & XFS_BMAPI_METADATA) && whichfork == XFS_DATA_FORK) {
> -		error = xfs_bmap_isaeof(bma, whichfork);
> -		if (error)
> -			return error;
> -	}
> +	if (bma->flags & XFS_BMAPI_CONTIG)
> +		bma->minlen = bma->length;
> +	else
> +		bma->minlen = 1;
>  
> -	error = xfs_bmap_alloc(bma);
> -	if (error)
> +	if (bma->flags & XFS_BMAPI_METADATA)
> +		error = xfs_bmap_btalloc(bma);
> +	else
> +		error = xfs_bmap_alloc_userdata(bma);
> +	if (error || bma->blkno == NULLFSBLOCK)
>  		return error;
>  
> -	if (bma->blkno == NULLFSBLOCK)
> -		return 0;
>  	if ((ifp->if_flags & XFS_IFBROOT) && !bma->cur)
>  		bma->cur = xfs_bmbt_init_cursor(mp, bma->tp, bma->ip, whichfork);
>  	/*
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 7/8] xfs: move extent zeroing to xfs_bmapi_allocate
  2019-10-25 15:03 ` [PATCH 7/8] xfs: move extent zeroing to xfs_bmapi_allocate Christoph Hellwig
@ 2019-10-28 16:31   ` Darrick J. Wong
  0 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2019-10-28 16:31 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Fri, Oct 25, 2019 at 05:03:35PM +0200, Christoph Hellwig wrote:
> Move the extent zeroing case there for the XFS_BMAPI_ZERO flag outside
> the low-level allocator and into xfs_bmapi_allocate, where is still
> is in transaction context, but outside the very lowlevel code where
> it doesn't belong.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/libxfs/xfs_alloc.c |  7 -------
>  fs/xfs/libxfs/xfs_alloc.h |  4 +---
>  fs/xfs/libxfs/xfs_bmap.c  | 10 ++++++----
>  fs/xfs/xfs_bmap_util.c    |  7 -------
>  4 files changed, 7 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 925eba9489d5..ff6454887ff3 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -3083,13 +3083,6 @@ xfs_alloc_vextent(
>  			args->len);
>  #endif
>  
> -		/* Zero the extent if we were asked to do so */
> -		if (args->datatype & XFS_ALLOC_USERDATA_ZERO) {
> -			error = xfs_zero_extent(args->ip, args->fsbno, args->len);
> -			if (error)
> -				goto error0;
> -		}
> -
>  	}
>  	xfs_perag_put(args->pag);
>  	return 0;
> diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
> index d6ed5d2c07c2..626384d75c9c 100644
> --- a/fs/xfs/libxfs/xfs_alloc.h
> +++ b/fs/xfs/libxfs/xfs_alloc.h
> @@ -54,7 +54,6 @@ typedef struct xfs_alloc_arg {
>  	struct xfs_mount *mp;		/* file system mount point */
>  	struct xfs_buf	*agbp;		/* buffer for a.g. freelist header */
>  	struct xfs_perag *pag;		/* per-ag struct for this agno */
> -	struct xfs_inode *ip;		/* for userdata zeroing method */
>  	xfs_fsblock_t	fsbno;		/* file system block number */
>  	xfs_agnumber_t	agno;		/* allocation group number */
>  	xfs_agblock_t	agbno;		/* allocation group-relative block # */
> @@ -83,8 +82,7 @@ typedef struct xfs_alloc_arg {
>   */
>  #define XFS_ALLOC_USERDATA		(1 << 0)/* allocation is for user data*/
>  #define XFS_ALLOC_INITIAL_USER_DATA	(1 << 1)/* special case start of file */
> -#define XFS_ALLOC_USERDATA_ZERO		(1 << 2)/* zero extent on allocation */
> -#define XFS_ALLOC_NOBUSY		(1 << 3)/* Busy extents not allowed */
> +#define XFS_ALLOC_NOBUSY		(1 << 2)/* Busy extents not allowed */
>  
>  static inline bool
>  xfs_alloc_is_userdata(int datatype)
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index c278eff29e82..6ec3c48abc1b 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3555,8 +3555,6 @@ xfs_bmap_btalloc(
>  	args.wasdel = ap->wasdel;
>  	args.resv = XFS_AG_RESV_NONE;
>  	args.datatype = ap->datatype;
> -	if (ap->datatype & XFS_ALLOC_USERDATA_ZERO)
> -		args.ip = ap->ip;
>  
>  	error = xfs_alloc_vextent(&args);
>  	if (error)
> @@ -4011,8 +4009,6 @@ xfs_bmap_alloc_userdata(
>  	 * the busy list.
>  	 */
>  	bma->datatype = XFS_ALLOC_NOBUSY;
> -	if (bma->flags & XFS_BMAPI_ZERO)
> -		bma->datatype |= XFS_ALLOC_USERDATA_ZERO;
>  	if (whichfork == XFS_DATA_FORK) {
>  		if (bma->offset == 0)
>  			bma->datatype |= XFS_ALLOC_INITIAL_USER_DATA;
> @@ -4071,6 +4067,12 @@ xfs_bmapi_allocate(
>  	if (error || bma->blkno == NULLFSBLOCK)
>  		return error;
>  
> +	if (bma->flags & XFS_BMAPI_ZERO) {
> +		error = xfs_zero_extent(bma->ip, bma->blkno, bma->length);
> +		if (error)
> +			return error;
> +	}
> +
>  	if ((ifp->if_flags & XFS_IFBROOT) && !bma->cur)
>  		bma->cur = xfs_bmbt_init_cursor(mp, bma->tp, bma->ip, whichfork);
>  	/*
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 44d6b6469303..e418f9922bb1 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -165,13 +165,6 @@ xfs_bmap_rtalloc(
>  		xfs_trans_mod_dquot_byino(ap->tp, ap->ip,
>  			ap->wasdel ? XFS_TRANS_DQ_DELRTBCOUNT :
>  					XFS_TRANS_DQ_RTBCOUNT, (long) ralen);
> -
> -		/* Zero the extent if we were asked to do so */
> -		if (ap->datatype & XFS_ALLOC_USERDATA_ZERO) {
> -			error = xfs_zero_extent(ap->ip, ap->blkno, ap->length);
> -			if (error)
> -				return error;
> -		}
>  	} else {
>  		ap->length = 0;
>  	}
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 8/8] xfs: cleanup use of the XFS_ALLOC_ flags
  2019-10-25 15:03 ` [PATCH 8/8] xfs: cleanup use of the XFS_ALLOC_ flags Christoph Hellwig
@ 2019-10-28 16:32   ` Darrick J. Wong
  0 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2019-10-28 16:32 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Fri, Oct 25, 2019 at 05:03:36PM +0200, Christoph Hellwig wrote:
> Always set XFS_ALLOC_USERDATA for data fork allocations, and check it
> in xfs_alloc_is_userdata instead of the current obsfucated check.
> Also remove the xfs_alloc_is_userdata and xfs_alloc_allow_busy_reuse
> helpers to make the code a little easier to understand.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/libxfs/xfs_alloc.c |  8 ++++----
>  fs/xfs/libxfs/xfs_alloc.h | 12 ------------
>  fs/xfs/libxfs/xfs_bmap.c  | 11 +++++------
>  fs/xfs/xfs_extent_busy.c  |  2 +-
>  fs/xfs/xfs_filestream.c   |  2 +-
>  5 files changed, 11 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index ff6454887ff3..4a6d6a1ad9f3 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -331,7 +331,7 @@ xfs_alloc_compute_diff(
>  	xfs_extlen_t	newlen1=0;	/* length with newbno1 */
>  	xfs_extlen_t	newlen2=0;	/* length with newbno2 */
>  	xfs_agblock_t	wantend;	/* end of target extent */
> -	bool		userdata = xfs_alloc_is_userdata(datatype);
> +	bool		userdata = datatype & XFS_ALLOC_USERDATA;
>  
>  	ASSERT(freelen >= wantlen);
>  	freeend = freebno + freelen;
> @@ -1041,9 +1041,9 @@ xfs_alloc_ag_vextent_small(
>  		goto out;
>  
>  	xfs_extent_busy_reuse(args->mp, args->agno, fbno, 1,
> -			      xfs_alloc_allow_busy_reuse(args->datatype));
> +			      (args->datatype & XFS_ALLOC_NOBUSY));
>  
> -	if (xfs_alloc_is_userdata(args->datatype)) {
> +	if (args->datatype & XFS_ALLOC_USERDATA) {
>  		struct xfs_buf	*bp;
>  
>  		bp = xfs_btree_get_bufs(args->mp, args->tp, args->agno, fbno);
> @@ -2380,7 +2380,7 @@ xfs_alloc_fix_freelist(
>  	 * somewhere else if we are not being asked to try harder at this
>  	 * point
>  	 */
> -	if (pag->pagf_metadata && xfs_alloc_is_userdata(args->datatype) &&
> +	if (pag->pagf_metadata && (args->datatype & XFS_ALLOC_USERDATA) &&
>  	    (flags & XFS_ALLOC_FLAG_TRYLOCK)) {
>  		ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
>  		goto out_agbp_relse;
> diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
> index 626384d75c9c..7380fbe4a3ff 100644
> --- a/fs/xfs/libxfs/xfs_alloc.h
> +++ b/fs/xfs/libxfs/xfs_alloc.h
> @@ -84,18 +84,6 @@ typedef struct xfs_alloc_arg {
>  #define XFS_ALLOC_INITIAL_USER_DATA	(1 << 1)/* special case start of file */
>  #define XFS_ALLOC_NOBUSY		(1 << 2)/* Busy extents not allowed */
>  
> -static inline bool
> -xfs_alloc_is_userdata(int datatype)
> -{
> -	return (datatype & ~XFS_ALLOC_NOBUSY) != 0;
> -}
> -
> -static inline bool
> -xfs_alloc_allow_busy_reuse(int datatype)
> -{
> -	return (datatype & XFS_ALLOC_NOBUSY) == 0;
> -}
> -
>  /* freespace limit calculations */
>  #define XFS_ALLOC_AGFL_RESERVE	4
>  unsigned int xfs_alloc_set_aside(struct xfs_mount *mp);
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 6ec3c48abc1b..f62f66863801 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3059,7 +3059,7 @@ xfs_bmap_adjacent(
>  	mp = ap->ip->i_mount;
>  	nullfb = ap->tp->t_firstblock == NULLFSBLOCK;
>  	rt = XFS_IS_REALTIME_INODE(ap->ip) &&
> -		xfs_alloc_is_userdata(ap->datatype);
> +		(ap->datatype & XFS_ALLOC_USERDATA);
>  	fb_agno = nullfb ? NULLAGNUMBER : XFS_FSB_TO_AGNO(mp,
>  							ap->tp->t_firstblock);
>  	/*
> @@ -3412,7 +3412,7 @@ xfs_bmap_btalloc(
>  
>  	if (ap->flags & XFS_BMAPI_COWFORK)
>  		align = xfs_get_cowextsz_hint(ap->ip);
> -	else if (xfs_alloc_is_userdata(ap->datatype))
> +	else if (ap->datatype & XFS_ALLOC_USERDATA)
>  		align = xfs_get_extsz_hint(ap->ip);
>  	if (align) {
>  		error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
> @@ -3427,7 +3427,7 @@ xfs_bmap_btalloc(
>  	fb_agno = nullfb ? NULLAGNUMBER : XFS_FSB_TO_AGNO(mp,
>  							ap->tp->t_firstblock);
>  	if (nullfb) {
> -		if (xfs_alloc_is_userdata(ap->datatype) &&
> +		if ((ap->datatype & XFS_ALLOC_USERDATA) &&
>  		    xfs_inode_is_filestream(ap->ip)) {
>  			ag = xfs_filestream_lookup_ag(ap->ip);
>  			ag = (ag != NULLAGNUMBER) ? ag : 0;
> @@ -3467,7 +3467,7 @@ xfs_bmap_btalloc(
>  		 * enough for the request.  If one isn't found, then adjust
>  		 * the minimum allocation size to the largest space found.
>  		 */
> -		if (xfs_alloc_is_userdata(ap->datatype) &&
> +		if ((ap->datatype & XFS_ALLOC_USERDATA) &&
>  		    xfs_inode_is_filestream(ap->ip))
>  			error = xfs_bmap_btalloc_filestreams(ap, &args, &blen);
>  		else
> @@ -4010,10 +4010,9 @@ xfs_bmap_alloc_userdata(
>  	 */
>  	bma->datatype = XFS_ALLOC_NOBUSY;
>  	if (whichfork == XFS_DATA_FORK) {
> +		bma->datatype |= XFS_ALLOC_USERDATA;
>  		if (bma->offset == 0)
>  			bma->datatype |= XFS_ALLOC_INITIAL_USER_DATA;
> -		else
> -			bma->datatype |= XFS_ALLOC_USERDATA;
>  
>  		if (mp->m_dalign && bma->length >= mp->m_dalign) {
>  			error = xfs_bmap_isaeof(bma, whichfork);
> diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
> index 2183d87be4cf..3991e59cfd18 100644
> --- a/fs/xfs/xfs_extent_busy.c
> +++ b/fs/xfs/xfs_extent_busy.c
> @@ -367,7 +367,7 @@ xfs_extent_busy_trim(
>  		 * If this is a metadata allocation, try to reuse the busy
>  		 * extent instead of trimming the allocation.
>  		 */
> -		if (!xfs_alloc_is_userdata(args->datatype) &&
> +		if (!(args->datatype & XFS_ALLOC_USERDATA) &&
>  		    !(busyp->flags & XFS_EXTENT_BUSY_DISCARDED)) {
>  			if (!xfs_extent_busy_update_extent(args->mp, args->pag,
>  							  busyp, fbno, flen,
> diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
> index 574a7a8b4736..2ae356775f63 100644
> --- a/fs/xfs/xfs_filestream.c
> +++ b/fs/xfs/xfs_filestream.c
> @@ -374,7 +374,7 @@ xfs_filestream_new_ag(
>  		startag = (item->ag + 1) % mp->m_sb.sb_agcount;
>  	}
>  
> -	if (xfs_alloc_is_userdata(ap->datatype))
> +	if (ap->datatype & XFS_ALLOC_USERDATA)
>  		flags |= XFS_PICK_USERDATA;
>  	if (ap->tp->t_flags & XFS_TRANS_LOWMODE)
>  		flags |= XFS_PICK_LOWSPACE;
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 3/8] xfs: remove the extsize argument to xfs_eof_alignment
  2019-10-28 16:06   ` Darrick J. Wong
@ 2019-10-29  7:57     ` Christoph Hellwig
  0 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2019-10-29  7:57 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs

On Mon, Oct 28, 2019 at 09:06:38AM -0700, Darrick J. Wong wrote:
> Why does it make more sense to do the inode extsize roundup only for
> direct writes and not as an intermediate step of determining the
> speculative preallocation size than what the code does now?

No behavior change in this patch.  xfs_buffered_write_iomap_begin
already passes a 0 extsize to xfs_eof_alignment, making the code moved
from xfs_eof_alignment to xfs_iomap_eof_align_last_fsb a no-op for
the buffered write path.

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 4/8] xfs: don't log the inode in xfs_fs_map_blocks if it wasn't modified
  2019-10-28 16:12   ` Darrick J. Wong
@ 2019-10-29  7:58     ` Christoph Hellwig
  2019-10-30 16:12       ` Darrick J. Wong
  0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2019-10-29  7:58 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs

On Mon, Oct 28, 2019 at 09:12:45AM -0700, Darrick J. Wong wrote:
> On Fri, Oct 25, 2019 at 05:03:32PM +0200, Christoph Hellwig wrote:
> > Even if we are asked for a write layout there is no point in logging
> > the inode unless we actually modified it in some way.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/xfs/xfs_pnfs.c | 43 +++++++++++++++++++------------------------
> >  1 file changed, 19 insertions(+), 24 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
> > index 9c96493be9e0..fa90c6334c7c 100644
> > --- a/fs/xfs/xfs_pnfs.c
> > +++ b/fs/xfs/xfs_pnfs.c
> > @@ -147,32 +147,27 @@ xfs_fs_map_blocks(
> >  	if (error)
> >  		goto out_unlock;
> >  
> > -	if (write) {
> > -		enum xfs_prealloc_flags	flags = 0;
> > -
> > +	if (write &&
> > +	    (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) {
> >  		ASSERT(imap.br_startblock != DELAYSTARTBLOCK);
> 
> The change in code flow makes this assert rather useless, I think, since
> we only end up in this branch if we have a write and a hole.  If the
> condition that it checks is important (and it seems to be?) then it
> ought to be hoisted up a level and turned into:
> 
> ASSERT(!write || !nimaps || imap.br_startblock != DELAYSTARTBLOCK);
> 
> Right?

Actually even for !write we should not see delalloc blocks here.
So I'll fix up the assert in a separate prep patch.

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 4/8] xfs: don't log the inode in xfs_fs_map_blocks if it wasn't modified
  2019-10-29  7:58     ` Christoph Hellwig
@ 2019-10-30 16:12       ` Darrick J. Wong
  2019-10-30 17:56         ` Christoph Hellwig
  0 siblings, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2019-10-30 16:12 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-xfs

On Tue, Oct 29, 2019 at 08:58:43AM +0100, Christoph Hellwig wrote:
> On Mon, Oct 28, 2019 at 09:12:45AM -0700, Darrick J. Wong wrote:
> > On Fri, Oct 25, 2019 at 05:03:32PM +0200, Christoph Hellwig wrote:
> > > Even if we are asked for a write layout there is no point in logging
> > > the inode unless we actually modified it in some way.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  fs/xfs/xfs_pnfs.c | 43 +++++++++++++++++++------------------------
> > >  1 file changed, 19 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
> > > index 9c96493be9e0..fa90c6334c7c 100644
> > > --- a/fs/xfs/xfs_pnfs.c
> > > +++ b/fs/xfs/xfs_pnfs.c
> > > @@ -147,32 +147,27 @@ xfs_fs_map_blocks(
> > >  	if (error)
> > >  		goto out_unlock;
> > >  
> > > -	if (write) {
> > > -		enum xfs_prealloc_flags	flags = 0;
> > > -
> > > +	if (write &&
> > > +	    (!nimaps || imap.br_startblock == HOLESTARTBLOCK)) {
> > >  		ASSERT(imap.br_startblock != DELAYSTARTBLOCK);
> > 
> > The change in code flow makes this assert rather useless, I think, since
> > we only end up in this branch if we have a write and a hole.  If the
> > condition that it checks is important (and it seems to be?) then it
> > ought to be hoisted up a level and turned into:
> > 
> > ASSERT(!write || !nimaps || imap.br_startblock != DELAYSTARTBLOCK);
> > 
> > Right?
> 
> Actually even for !write we should not see delalloc blocks here.
> So I'll fix up the assert in a separate prep patch.

<shrug> I could just fix it, unless you're about to resend the whole series?

--D

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 4/8] xfs: don't log the inode in xfs_fs_map_blocks if it wasn't modified
  2019-10-30 16:12       ` Darrick J. Wong
@ 2019-10-30 17:56         ` Christoph Hellwig
  0 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2019-10-30 17:56 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs

On Wed, Oct 30, 2019 at 09:12:48AM -0700, Darrick J. Wong wrote:
> > Actually even for !write we should not see delalloc blocks here.
> > So I'll fix up the assert in a separate prep patch.
> 
> <shrug> I could just fix it, unless you're about to resend the whole series?

I have the series ready to resend.

^ permalink raw reply	[flat|nested] 21+ messages in thread
end of thread, other threads:[~2019-10-30 17:56 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-25 15:03 a few iomap / bmap cleanups Christoph Hellwig
2019-10-25 15:03 ` [PATCH 1/8] xfs: simplify xfs_iomap_eof_align_last_fsb Christoph Hellwig
2019-10-28 15:55   ` Darrick J. Wong
2019-10-25 15:03 ` [PATCH 2/8] xfs: mark xfs_eof_alignment static Christoph Hellwig
2019-10-28 15:55   ` Darrick J. Wong
2019-10-25 15:03 ` [PATCH 3/8] xfs: remove the extsize argument to xfs_eof_alignment Christoph Hellwig
2019-10-28 16:06   ` Darrick J. Wong
2019-10-29  7:57     ` Christoph Hellwig
2019-10-25 15:03 ` [PATCH 4/8] xfs: don't log the inode in xfs_fs_map_blocks if it wasn't modified Christoph Hellwig
2019-10-28 16:12   ` Darrick J. Wong
2019-10-29  7:58     ` Christoph Hellwig
2019-10-30 16:12       ` Darrick J. Wong
2019-10-30 17:56         ` Christoph Hellwig
2019-10-25 15:03 ` [PATCH 5/8] xfs: simplify the xfs_iomap_write_direct calling conventions Christoph Hellwig
2019-10-28 16:25   ` Darrick J. Wong
2019-10-25 15:03 ` [PATCH 6/8] xfs: refactor xfs_bmapi_allocate Christoph Hellwig
2019-10-28 16:29   ` Darrick J. Wong
2019-10-25 15:03 ` [PATCH 7/8] xfs: move extent zeroing to xfs_bmapi_allocate Christoph Hellwig
2019-10-28 16:31   ` Darrick J. Wong
2019-10-25 15:03 ` [PATCH 8/8] xfs: cleanup use of the XFS_ALLOC_ flags Christoph Hellwig
2019-10-28 16:32   ` Darrick J. Wong

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).