Hi,
Any idea on this?
Thanks,
wengang
> On May 20, 2024, at 3:42 PM, Wengang Wang <wen.gang.wang@xxxxxxxxxx> wrote:
>
> Thanks Darrick for review, pls see inlines:
>
>> On May 20, 2024, at 11:08 AM, Darrick J. Wong <djwong@xxxxxxxxxx> 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). The patch
> also makes unshare use delayed allocation for bigger extent.
>
>
>>> Signed-off-by: Wengang Wang <wen.gang.wang@xxxxxxxxxx>
>>> ---
>>> fs/xfs/xfs_inode.c | 17 ++++++++++++++++
>>> fs/xfs/xfs_inode.h | 48 +++++++++++++++++++++++---------------------
>>> fs/xfs/xfs_reflink.c | 7 +++++--
>>> 3 files changed, 47 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>>> index d55b42b2480d..ade945c8d783 100644
>>> --- a/fs/xfs/xfs_inode.c
>>> +++ b/fs/xfs/xfs_inode.c
>>> @@ -58,6 +58,15 @@ xfs_get_extsz_hint(
>>> */
>>> if (xfs_is_always_cow_inode(ip))
>>> return 0;
>>> +
>>> + /*
>>> + * let xfs_buffered_write_iomap_begin() do delayed allocation
>>> + * in unshare path so that the new blocks have more chance to
>>> + * be contigurous
>>> + */
>>> + if (xfs_iflags_test(ip, XFS_IUNSHARE))
>>> + return 0;
>>
>> What if the inode is a realtime file? Will this work with the rt
>> delalloc support coming online in 6.10?
>
> This XFS_IUNSHARE is not set in xfs_reflink_unshare() for rt inodes.
> So rt inodes will keep current behavior.
>
>>
>>> +
>>> if ((ip->i_diflags & XFS_DIFLAG_EXTSIZE) && ip->i_extsize)
>>> return ip->i_extsize;
>>> if (XFS_IS_REALTIME_INODE(ip))
>>> @@ -77,6 +86,14 @@ xfs_get_cowextsz_hint(
>>> {
>>> xfs_extlen_t a, b;
>>>
>>> + /*
>>> + * in unshare path, allocate exactly the number of the blocks to be
>>> + * unshared so that no new blocks caused the unshare operation remain
>>> + * in Cow fork after the unshare is done
>>> + */
>>> + if (xfs_iflags_test(ip, XFS_IUNSHARE))
>>> + return 1;
>>
>> Aha, so this is also about turning off speculative preallocations
>> outside the range that's being unshared?
>
> Yes.
>
>>
>>> +
>>> a = 0;
>>> if (ip->i_diflags2 & XFS_DIFLAG2_COWEXTSIZE)
>>> a = ip->i_cowextsize;
>>> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
>>> index ab46ffb3ac19..6a8ad68dac1e 100644
>>> --- a/fs/xfs/xfs_inode.h
>>> +++ b/fs/xfs/xfs_inode.h
>>> @@ -207,13 +207,13 @@ xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size)
>>> * i_flags helper functions
>>> */
>>> static inline void
>>> -__xfs_iflags_set(xfs_inode_t *ip, unsigned short flags)
>>> +__xfs_iflags_set(xfs_inode_t *ip, unsigned long flags)
>>
>> I think this is already queued for 6.10.
>
> Good to know.
>
> Thanks,
> Wengang
