diff mbox

[v2,3/5] xfs: reuse iomap delalloc code for COW fork reservation

Message ID 1484157249-464-4-git-send-email-bfoster@redhat.com
State New, archived
Headers show

Commit Message

Brian Foster Jan. 11, 2017, 5:54 p.m. UTC
COW fork reservation (delayed allocation) is implemented in
xfs_reflink_reserve_cow() and is generally based on the traditional data
fork delalloc logic in xfs_file_iomap_begin_delay(). In preparation for
further changes to implement more aggressive COW fork preallocation,
refactor the COW reservation code to reuse xfs_file_iomap_begin_delay()
for data fork allocation as well as COW fork reservation. This patch
does not change behavior.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_iomap.c | 74 ++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 55 insertions(+), 19 deletions(-)

Comments

Christoph Hellwig Jan. 13, 2017, 5:13 p.m. UTC | #1
On Wed, Jan 11, 2017 at 12:54:07PM -0500, Brian Foster wrote:
> COW fork reservation (delayed allocation) is implemented in
> xfs_reflink_reserve_cow() and is generally based on the traditional data
> fork delalloc logic in xfs_file_iomap_begin_delay(). In preparation for
> further changes to implement more aggressive COW fork preallocation,
> refactor the COW reservation code to reuse xfs_file_iomap_begin_delay()
> for data fork allocation as well as COW fork reservation. This patch
> does not change behavior.

I'm still trying to understand the point of patches 1-3:  there is no
functionality, but a lot of new code is added to reuse some other code.

How would patch 5 look like without that "reuse"?
--
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
Brian Foster Jan. 16, 2017, 4:26 p.m. UTC | #2
On Fri, Jan 13, 2017 at 09:13:12AM -0800, Christoph Hellwig wrote:
> On Wed, Jan 11, 2017 at 12:54:07PM -0500, Brian Foster wrote:
> > COW fork reservation (delayed allocation) is implemented in
> > xfs_reflink_reserve_cow() and is generally based on the traditional data
> > fork delalloc logic in xfs_file_iomap_begin_delay(). In preparation for
> > further changes to implement more aggressive COW fork preallocation,
> > refactor the COW reservation code to reuse xfs_file_iomap_begin_delay()
> > for data fork allocation as well as COW fork reservation. This patch
> > does not change behavior.
> 
> I'm still trying to understand the point of patches 1-3:  there is no
> functionality, but a lot of new code is added to reuse some other code.
> 

IMO, patches 1-3 stand on their own as cleanup/refactor patches,
regardless of whether we want the actual speculative preallocation patch
(in current form or at all). xfs_reflink_reserve_cow() is mostly a
copy&paste of _iomap_begin_delay() operating on the cow fork rather than
the data fork, so technically we really shouldn't have a need for a
feature specific helper. Duplication aside, I also find the code a bit
confusing to follow in that we have to traverse through several
functions in "do nothing" cases such as non-shared blocks of a reflinked
file.

I would have preferred that code get factored into _begin_delay() from
the start, but it was too late for that by the time I grokked it and
there are also other callers from which prealloc may or may not be
appropriate (and that is why this patch by itself is not sufficient to
kill off _reserve_cow()). That said, we might be able to refactor the
allocation part of both functions such that we can replace the feature
specific helper with a common generic one. IOWs, these patches make it
so I don't have to further duplicate the prealloc stuff between
_write_begin() and _reserve_cow() and in the long term, I think this
helps facilitate further consolidation of duplicate code.

> How would patch 5 look like without that "reuse"?

I suppose we'd copy&paste more of begin_delay() into the reflink
specific helper (e.g., the prealloc bits). Then perhaps add a param to
control whether to perform preallocation.

Brian

> --
> 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
--
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
Christoph Hellwig Jan. 16, 2017, 5:37 p.m. UTC | #3
On Mon, Jan 16, 2017 at 11:26:01AM -0500, Brian Foster wrote:
> IMO, patches 1-3 stand on their own as cleanup/refactor patches,
> regardless of whether we want the actual speculative preallocation patch
> (in current form or at all). xfs_reflink_reserve_cow() is mostly a
> copy&paste of _iomap_begin_delay() operating on the cow fork rather than
> the data fork, so technically we really shouldn't have a need for a
> feature specific helper. Duplication aside, I also find the code a bit
> confusing to follow in that we have to traverse through several
> functions in "do nothing" cases such as non-shared blocks of a reflinked
> file.

I'm usually not a fan of refactor patches that adds lots of new code
without adding functionality.  In terms of readability I'm obviously
biasses having written a lot of the code, but I find the new code
much harder to read.
--
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
Brian Foster Jan. 16, 2017, 7:04 p.m. UTC | #4
On Mon, Jan 16, 2017 at 09:37:30AM -0800, Christoph Hellwig wrote:
> On Mon, Jan 16, 2017 at 11:26:01AM -0500, Brian Foster wrote:
> > IMO, patches 1-3 stand on their own as cleanup/refactor patches,
> > regardless of whether we want the actual speculative preallocation patch
> > (in current form or at all). xfs_reflink_reserve_cow() is mostly a
> > copy&paste of _iomap_begin_delay() operating on the cow fork rather than
> > the data fork, so technically we really shouldn't have a need for a
> > feature specific helper. Duplication aside, I also find the code a bit
> > confusing to follow in that we have to traverse through several
> > functions in "do nothing" cases such as non-shared blocks of a reflinked
> > file.
> 
> I'm usually not a fan of refactor patches that adds lots of new code
> without adding functionality.  In terms of readability I'm obviously
> biasses having written a lot of the code, but I find the new code
> much harder to read.

Ok, we disagree on whether the refactoring stands on its own. I'm not
quite sure which part you dislike (note that the iomap_search_extents()
part was based on your suggestion from the rfc). Either way, this series
does add functionality too. :)

I'm not that concerned with getting the refactor in by itself if there
is disagreement. I just didn't want to continue to have two delalloc
write paths that differ only by the target fork (and with this series,
initial prealloc size). So, any thoughts on the mechanism itself, or how
you'd envision it look if not as implemented here?

Brian

> --
> 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
--
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
diff mbox

Patch

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 8791ed5..19b7eb0 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -527,9 +527,8 @@  xfs_iomap_prealloc_size(
  * inode data and COW forks. If an existing data extent is shared, determine
  * whether COW fork reservation is necessary.
  *
- * The 'found' parameter indicates whether a writable mapping was found. If the
- * mapping is shared, a COW reservation is performed for the corresponding
- * range.
+ * The 'found' parameter indicates whether a writable mapping was found. If a
+ * shared mapping exists, found is set only if COW reservation exists as well.
  */
 static int
 xfs_iomap_search_extents(
@@ -540,12 +539,14 @@  xfs_iomap_search_extents(
 	int			*eof,
 	int			*idx,
 	struct xfs_bmbt_irec	*got,
+	bool			*shared,
 	bool			*found)		/* found usable extent */
 {
 	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
 	int			error = 0;
+	bool			trimmed;
 
-	*found = false;
+	*shared = *found = false;
 
 	/*
 	 * Look up a preexisting extent directly into imap. Set got for the
@@ -556,17 +557,37 @@  xfs_iomap_search_extents(
 		*got = *imap;
 		return 0;
 	}
+	if (!xfs_is_reflink_inode(ip)) {
+		*found = true;
+		return 0;
+	}
 
-	if (xfs_is_reflink_inode(ip)) {
-		bool		shared;
-
-		xfs_trim_extent(imap, offset_fsb, end_fsb - offset_fsb);
-		error = xfs_reflink_reserve_cow(ip, imap, &shared);
-		if (error)
-			return error;
+	/*
+	 * Found a data extent but we don't know if it is shared. If an extent
+	 * exists in the cow fork, assume that it is. Use got for this lookup as
+	 * imap must retain the data mapping.
+	 */
+	xfs_trim_extent(imap, offset_fsb, end_fsb - offset_fsb);
+	ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
+	*eof = !xfs_iext_lookup_extent(ip, ifp, offset_fsb, idx, got);
+	if (!*eof && got->br_startoff <= imap->br_startoff) {
+		trace_xfs_reflink_cow_found(ip, got);
+		xfs_trim_extent(imap, got->br_startoff, got->br_blockcount);
+		*found = true;
+		return 0;
 	}
 
-	*found = true;
+	/*
+	 * There is no existing cow fork extent. We have to explicitly check if
+	 * the data extent is shared to determine whether COW fork reservation
+	 * is required to map the data extent. Trim the mapping to the next
+	 * (un)shared boundary at the same time.
+	 */
+	error = xfs_reflink_trim_around_shared(ip, imap, shared, &trimmed);
+	if (error)
+		return error;
+	if (!*shared)
+		*found = true;
 
 	return error;
 }
@@ -587,11 +608,13 @@  xfs_file_iomap_begin_delay(
 	xfs_fileoff_t		maxbytes_fsb =
 		XFS_B_TO_FSB(mp, mp->m_super->s_maxbytes);
 	int			error = 0, eof = 0;
+	int			fork = XFS_DATA_FORK;
 	struct xfs_bmbt_irec	imap;
 	struct xfs_bmbt_irec	got;
 	xfs_extnum_t		idx;
 	xfs_fsblock_t		prealloc_blocks = 0;
 	bool			found;
+	bool			shared;
 
 	ASSERT(!XFS_IS_REALTIME_INODE(ip));
 	ASSERT(!xfs_get_extsz_hint(ip));
@@ -620,16 +643,20 @@  xfs_file_iomap_begin_delay(
 
 	/*
 	 * Search for preexisting extents. If an existing data extent is shared,
-	 * this will perform COW fork reservation.
+	 * switch to the COW fork for COW reservation.
 	 */
 	error = xfs_iomap_search_extents(ip, offset_fsb, end_fsb, &imap, &eof,
-					 &idx, &got, &found);
+					 &idx, &got, &shared, &found);
 	if (error)
 		goto out_unlock;
 	if (found) {
 		trace_xfs_iomap_found(ip, offset, count, 0, &imap);
 		goto done;
 	}
+	if (shared) {
+		end_fsb = imap.br_startoff + imap.br_blockcount;
+		fork = XFS_COW_FORK;
+	}
 
 	error = xfs_qm_dqattach_locked(ip, 0);
 	if (error)
@@ -645,9 +672,10 @@  xfs_file_iomap_begin_delay(
 	 * the lower level functions are updated.
 	 */
 	count = min_t(loff_t, count, 1024 * PAGE_SIZE);
-	end_fsb = min(XFS_B_TO_FSB(mp, offset + count), maxbytes_fsb);
+	end_fsb = min(end_fsb, XFS_B_TO_FSB(mp, offset + count));
+	xfs_trim_extent(&imap, offset_fsb, end_fsb - offset_fsb);
 
-	if (eof) {
+	if (eof && fork == XFS_DATA_FORK) {
 		prealloc_blocks = xfs_iomap_prealloc_size(ip, offset, count, idx);
 		if (prealloc_blocks) {
 			xfs_extlen_t	align;
@@ -669,7 +697,7 @@  xfs_file_iomap_begin_delay(
 	}
 
 retry:
-	error = xfs_bmapi_reserve_delalloc(ip, XFS_DATA_FORK, offset_fsb,
+	error = xfs_bmapi_reserve_delalloc(ip, fork, offset_fsb,
 			end_fsb - offset_fsb, prealloc_blocks, &got, &idx, eof);
 	switch (error) {
 	case 0:
@@ -687,8 +715,16 @@  xfs_file_iomap_begin_delay(
 		goto out_unlock;
 	}
 
-	trace_xfs_iomap_alloc(ip, offset, count, 0, &got);
-	imap = got;
+	/*
+	 * For the data fork, the iomap mapping is simply the extent that was
+	 * returned from the delalloc request. Otherwise, use imap as it refers
+	 * to the data fork but is trimmed according to the shared state.
+	 */
+	if (fork == XFS_DATA_FORK) {
+		trace_xfs_iomap_alloc(ip, offset, count, 0, &got);
+		imap = got;
+	} else
+		trace_xfs_reflink_cow_alloc(ip, &got);
 done:
 	if (isnullstartblock(imap.br_startblock))
 		imap.br_startblock = DELAYSTARTBLOCK;