Re: [PATCH] xfs: dont remain new blocks in cowfork for unshare

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Jun 06, 2024 at 06:16:23PM +0000, Wengang Wang wrote:
> 
> 
> > On May 31, 2024, at 10:53 AM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
> > 
> > 
> > 
> >> On May 31, 2024, at 9:00 AM, Darrick J. Wong <djwong@kernel.org> wrote:
> >> 
> >> On Mon, May 20, 2024 at 10:42:02PM +0000, Wengang Wang wrote:
> >>> Thanks Darrick for review, pls see inlines:
> >>> 
> >>>> On May 20, 2024, at 11:08 AM, Darrick J. Wong <djwong@kernel.org> wrote:
> >>>> 
> >>>> On Fri, May 17, 2024 at 02:26:21PM -0700, Wengang Wang wrote:
> >>>>> Unsharing blocks is implemented by doing CoW to those blocks. That has a side
> >>>>> effect that some new allocatd blocks remain in inode Cow fork. As unsharing blocks
> >>>> 
> >>>>                     allocated
> >>>> 
> >>>>> has no hint that future writes would like come to the blocks that follow the
> >>>>> unshared ones, the extra blocks in Cow fork is meaningless.
> >>>>> 
> >>>>> This patch makes that no new blocks caused by unshare remain in Cow fork.
> >>>>> The change in xfs_get_extsz_hint() makes the new blocks have more change to be
> >>>>> contigurous in unshare path when there are multiple extents to unshare.
> >>>> 
> >>>> contiguous
> >>>> 
> >>> Sorry for typos.
> >>> 
> >>>> Aha, so you're trying to combat fragmentation by making unshare use
> >>>> delayed allocation so that we try to allocate one big extent all at once
> >>>> instead of doing this piece by piece.  Or maybe you also don't want
> >>>> unshare to preallocate cow extents beyond the range requested?
> >>>> 
> >>> 
> >>> Yes, The main purpose is for the later (avoid preallocating beyond).
> >> 
> >> But the user set an extent size hint, so presumably they want us to (try
> >> to) obey that even for unshare operations, right?
> > 
> > Yeah, user might set extsize for better IO performance. But they don’t really know
> > much details. Consider this case: 
> > writing to those over/beyond preallocated blocks would cause Cow. Cow includes
> > extra meta changes: releasing old blocks, inserting new extents to data fork and removing
> > staging extents from refcount tree.  That’s a lot, as I think, a Cow is slower than block over-write.
> > In above case, the Cow is caused by unshare, rather than by shared blocks. That might be
> > not what user expected by setting extsize.
> > 
> > 
> May I know if this is a good reason to skip extsize setting, or we
> anyways honor extsize?

I'm not sure -- if someone set (say) a 256k cowextsize and later wants
to unshare a single 4k block of that, they might think it's useful for
the fs to try to push surrounding writes to the same 256k region to
combat fragmentation.

OTOH if the cowextsize is (say) 2M (or 1G) then that might be excessive?
Particularly if the cow reservation shadows an already unshared data
fork block, in which case (as you point out) this turns a cheap
overwrite into an expensive cow.  IOWs, we're trading less fragmentation
for higher individual write times.

Hmm, maybe there's another aspect to think about -- for a directio cow
write, I think we skip the cowextsize thing and only allocate the exact
range the caller asked for.  If that's correct, then perhaps unshare
should follow the directio write allocation pattern if the struct file
has O_DIRECT set?

--D

> Thanks,
> Wengang
>