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

[Brian Foster <bfoster@redhat.com>]

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