On Thu, Oct 13, 2016 at 09:26:05AM -0400, Brian Foster wrote:
> 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.
Yep, that's correct. We can promote non-CoW writes to CoW as a strategy
to try to reduce fragmentation. I'll clarify that aspect in the docs.
--D
>
> 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@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
