Re: [PATCH 5/9] xfs: optimize writes to reflink files

[Brian Foster <bfoster@redhat.com>]

On Thu, Oct 13, 2016 at 08:49:25AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 12, 2016 at 10:12:34AM -0400, Brian Foster wrote:
> > > +		if (xfs_is_reflink_inode(ip)) {
> > > +			bool		shared;
> > > +
> > > +			end_fsb = min(XFS_B_TO_FSB(mp, offset + count),
> > > +					maxbytes_fsb);
> > > +			xfs_trim_extent(&got, offset_fsb, end_fsb - offset_fsb);
> > > +			error = xfs_reflink_reserve_cow(ip, &got, &shared);
> > > +			if (error)
> > > +				goto out_unlock;
> > 
> > All in all this seems fine, but I don't see why we need to get all the
> > way down through xfs_reflink_reserve_cow() ->
> > xfs_reflink_trim_around_shared() to handle the basic delalloc overwrite
> > case on a reflink inode. Could we enhance the is_reflink_inode() helper
> > or create a new one that can consider whether the data fork extent is a
> > hole or delalloc?
> 
> Do you mean delalloc non-overwrite?  We could skip non-overwrite extents
> by factoring out a helper that checks for extent types that don't need to
> be overwritten.  But this would defeat the COW fork speculative
> preallocation logic, which causes additional COW operations even for
> extents we would not nessecarily have to COW.  So we'll always have to
> look at the COW fork first if we already have an allocation to implement
> that scheme (and we should probably document it better).
> 

Either way... delalloc into a hole or overwrite of an existing (data
fork) delalloc, will fall out of xfs_reflink_reserve_cow() so long as
nothing is in the cow fork, right?

But regardless, I see your point now. For whatever reason the comment
update in xfs_reflink_reserve_cow() went right over my head. IIUC, the
idea is that cow delalloc writes can include preallocation and thus have
delalloc for blocks that might not actually be shared in the data fork.
Therefore, we have to query the cow fork first and cannot reliably use
the data fork shared state to determine whether cow fork blocks actually
exist. A clarification of the comment is probably fine.. thanks for the
explanation.

Brian

> xfs_reflink_trim_around_shared does a check for the non-COWable extent
> types as the very first thing, so that's where we are done with the COW
> overhead for a non-overwrite that doesn't have a speculative
> preallocation in the COW fork.
> --
> 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