For when forcealign is enabled, we want the EOF to be aligned as well, so
do not free EOF blocks.
Signed-off-by: John Garry <[email protected]>
---
fs/xfs/xfs_bmap_util.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 19e11d1da660..f26d1570b9bd 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -542,8 +542,13 @@ xfs_can_free_eofblocks(
* forever.
*/
end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip));
- if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1)
+
+ /* Do not free blocks when forcing extent sizes */
+ if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1)
+ end_fsb = roundup_64(end_fsb, ip->i_extsize);
+ else if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1)
end_fsb = xfs_rtb_roundup_rtx(mp, end_fsb);
+
last_fsb = XFS_B_TO_FSB(mp, mp->m_super->s_maxbytes);
if (last_fsb <= end_fsb)
return false;
--
2.31.1
On 30/04/2024 23:54, Dave Chinner wrote:
> On Mon, Apr 29, 2024 at 05:47:34PM +0000, John Garry wrote:
>> For when forcealign is enabled, we want the EOF to be aligned as well, so
>> do not free EOF blocks.
>
> This is doesn't match what the code does. The code is correct - it
> rounds the range to be trimmed up to the aligned offset beyond EOF
> and then frees them. The description needs to be updated to reflect
> this.
ok, fine
>
>>
>> Signed-off-by: John Garry <[email protected]>
>> ---
>> fs/xfs/xfs_bmap_util.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index 19e11d1da660..f26d1570b9bd 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -542,8 +542,13 @@ xfs_can_free_eofblocks(
>> * forever.
>> */
>> end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip));
>> - if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1)
>> +
>> + /* Do not free blocks when forcing extent sizes */
>> + if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1)
>
> I see this sort of check all through the remaining patches.
>
> Given there are significant restrictions on forced alignment,
> shouldn't this all the details be pushed inside the helper function?
> e.g.
>
> /*
> * Forced extent alignment is dependent on extent size hints being
> * set to define the alignment. Alignment is only necessary when the
> * extent size hint is larger than a single block.
> *
> * If reflink is enabled on the file or we are in always_cow mode,
> * we can't easily do forced alignment.
> *
> * We don't support forced alignment on realtime files.
> * XXX(dgc): why not?
There is no technical reason to not be able to support forcealign on RT,
AFAIK. My idea is to support RT after non-RT is supported.
> */
> static inline bool
> xfs_inode_has_forcealign(struct xfs_inode *ip)
> {
> if (!(ip->di_flags & XFS_DIFLAG_EXTSIZE))
> return false;
> if (ip->i_extsize <= 1)
> return false;
>
> if (xfs_is_cow_inode(ip))
> return false;
Could we just include this in the forcealign validate checks? Currently
we just check CoW extsize is zero there.
> if (ip->di_flags & XFS_DIFLAG_REALTIME)
> return false;
We check this in xfs_inode_validate_forcealign()
>
> return ip->di_flags2 & XFS_DIFLAG2_FORCEALIGN;
> }
>
So can we simply have:
static inline bool
xfs_inode_has_forcealign(struct xfs_inode *ip)
{
if (!(ip->di_flags & XFS_DIFLAG_EXTSIZE))
return false;
if (ip->i_extsize <= 1)
return false;
return ip->di_flags2 & XFS_DIFLAG2_FORCEALIGN;
}
Thanks,
John
On 02/05/2024 02:11, Dave Chinner wrote:
>>> static inline bool
>>> xfs_inode_has_forcealign(struct xfs_inode *ip)
>>> {
>>> if (!(ip->di_flags & XFS_DIFLAG_EXTSIZE))
>>> return false;
>>> if (ip->i_extsize <= 1)
>>> return false;
>>>
>>> if (xfs_is_cow_inode(ip))
>>> return false;
>> Could we just include this in the forcealign validate checks? Currently we
>> just check CoW extsize is zero there.
> Checking COW extsize is zero doesn't tell us anything useful about
> whether the inode might have shared extents, or that the filesystem
> has had the sysfs "always cow" debug knob turned on. That changes
> filesystem behaviour at mount time and has nothing to do with the
> on-disk format constraints.
>
> And now that I think about it, checking for COW extsize is
> completely the wrong thing to do because it doesn't get used until
> an extent is shared and a COW trigger is hit. So the presence of COW
> extsize has zero impact on whether we can use forced alignment or
> not.
ok
>
> IOWs, we have to check for shared extents or always cow here,
> because even a file with correctly set up forced alignment needs to
> have forced alignment disabled when always_cow is enabled. Every
> write is going to use the COW path and AFAICT we don't support
> forced alignment through that path yet.
ok
>
>>> if (ip->di_flags & XFS_DIFLAG_REALTIME)
>>> return false;
>> We check this in xfs_inode_validate_forcealign()
> That's kinda my point - we have a random smattering of different
> checks at different layers and in different contexts. i.e. There's
> no one function that performs -all- the "can we do forced alignment"
> checks that allow forced alignment to be used. This simply adds all
> those checks in the one place and ensures that even if other code
> gets checks wrong, we won't use forcealign inappropriately.
Fine, I can do that if you think it is the best strategy.
Thanks,
John