| Message ID | 1484157249-464-4-git-send-email-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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 --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;
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(-)