Re: [PATCH 1/3] xfs: simplify extent allocation alignment

John Garry
2024-04-03T11:30:43Z

On 03/04/2024 00:44, Dave Chinner wrote:
>> I seem to have at least 2x problems:
>> - unexpected -ENOSPC in some case
>> - sometimes misaligned extents from ordered combo of punch, truncate, write
> Punch and truncate do not enforce extent alignment and never have.
> They will trim extents to whatever the new EOF block is supposed to
> be. This is by design - they are intended to be able to remove
> extents beyond EOF that may have been done due to extent size hints
> and/or speculative delayed allocation to minimise wasted space.
>
> Again, forced alignment is introducing new constraints, so anything
> that is truncating EOF blocks (punch, truncate, eof block freeing
> during inactivation or blockgc) is going to need to take forced
> extent alignment constraints into account.

Sure

>
> This is likely something that needs to be done in
> xfs_itruncate_extents_flags() for the truncate/inactivation/blockgc
> cases (i.e. correct calculation of first_unmap_block). Punching and
> insert/collapse are a bit more complex - they'll need their
> start/end offsets to be aligned, the chunk sizes they work in to be
> aligned, and the rounding in xfs_flush_unmap_range() to be forced
> alignment aware.

Ack

>
>> I don't know if it is a good use of time for me to try to debug, as I guess
>> you could spot problems in the changes almost immediately.
>>
>> Next steps:
>> I would like to send out a new series for XFS support for atomic writes
>> soon, which so far included forcealign support.
>>
>> Please advise on your preference for what I should do, like wait for your
>> forcealign update and then combine into the XFS support for atomic write
>> series. Or just post the doubtful patches now, as above, and go from there?
> I just sent out the forced allocation alignment series for review.

cheers, I'll give them a spin.

> Forced truncate/punch extent alignment will need to be implemented
> and reviewed as a separate patch set...

Below are my changes.

I think that the xfs_is_falloc_aligned() change is sound. As for the
other two, I'm really not sure.

There is also
https://lore.kernel.org/linux-xfs/ZeeaKrmVEkcXYjbK@dread.disaster.area/T/#m73430d56d96e60f2908bab9ce3e6a0d56d27929c,
which still is still a candidate change.

Please check them, below.

Thanks,
John

------>8-------

From ec86dd3add7153062a612cb1141f36544f34e0cd Mon Sep 17 00:00:00 2001
From: John Garry <john.g.garry@oracle.com>
Date: Wed, 13 Mar 2024 18:14:37 +0000
Subject: [PATCH 1/3] fs: xfs: Update xfs_is_falloc_aligned() mask forcealign

For when forcealign is enabled, we want the alignment mask to cover an
aligned extent, similar to rtvol.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/xfs_file.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 632653e00906..e81e01e6b22b 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -61,7 +61,10 @@ xfs_is_falloc_aligned(
}
mask = XFS_FSB_TO_B(mp, mp->m_sb.sb_rextsize) - 1;
} else {
- mask = mp->m_sb.sb_blocksize - 1;
+ if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1)
+ mask = (mp->m_sb.sb_blocksize * ip->i_extsize) - 1;
+ else
+ mask = mp->m_sb.sb_blocksize - 1;
}

return !((pos | len) & mask);

------8<--------

From c0866d2a5753f1c487872ef3add4e08c03c22d00 Mon Sep 17 00:00:00 2001
From: John Garry <john.g.garry@oracle.com>
Date: Fri, 22 Mar 2024 11:24:45 +0000
Subject: [PATCH 2/3] fs: xfs: Unmap blocks according to forcealign

For when forcealign is enabled, blocks in an inode need to be unmapped
according to extent alignment, like what is already done for rtvol.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/libxfs/xfs_bmap.c | 41 ++++++++++++++++++++++++++++++++++------
fs/xfs/xfs_inode.h | 5 +++++
2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 7a0ef0900097..5dd7a62625db 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5322,6 +5322,15 @@ xfs_bmap_del_extent_real(
return 0;
}

+/* Return the offset of an block number within an extent for forcealign. */
+static xfs_extlen_t
+xfs_forcealign_extent_offset(
+ struct xfs_inode *ip,
+ xfs_fsblock_t bno)
+{
+ return bno & (ip->i_extsize - 1);
+}
+
/*
* Unmap (remove) blocks from a file.
* If nexts is nonzero then the number of extents to remove is limited to
@@ -5344,6 +5353,7 @@ __xfs_bunmapi(
struct xfs_bmbt_irec got; /* current extent record */
struct xfs_ifork *ifp; /* inode fork pointer */
int isrt; /* freeing in rt area */
+ int isforcealign; /* freeing for file inode with forcealign */
int logflags; /* transaction logging flags */
xfs_extlen_t mod; /* rt extent offset */
struct xfs_mount *mp = ip->i_mount;
@@ -5380,7 +5390,10 @@ __xfs_bunmapi(
return 0;
}
XFS_STATS_INC(mp, xs_blk_unmap);
- isrt = xfs_ifork_is_realtime(ip, whichfork);
+ isrt = (whichfork == XFS_DATA_FORK) && XFS_IS_REALTIME_INODE(ip);
+ isforcealign = (whichfork == XFS_DATA_FORK) &&
+ xfs_inode_has_forcealign(ip) &&
+ xfs_inode_has_extsize(ip) && ip->i_extsize > 1;
end = start + len;

if (!xfs_iext_lookup_extent_before(ip, ifp, &end, &icur, &got)) {
@@ -5442,11 +5455,17 @@ __xfs_bunmapi(
if (del.br_startoff + del.br_blockcount > end + 1)
del.br_blockcount = end + 1 - del.br_startoff;

- if (!isrt || (flags & XFS_BMAPI_REMAP))
+ if ((!isrt && !isforcealign) || (flags & XFS_BMAPI_REMAP))
goto delete;

- mod = xfs_rtb_to_rtxoff(mp,
- del.br_startblock + del.br_blockcount);
+ if (isrt)
+ mod = xfs_rtb_to_rtxoff(mp,
+ del.br_startblock + del.br_blockcount);
+ else if (isforcealign)
+ mod = xfs_forcealign_extent_offset(ip,
+ del.br_startblock + del.br_blockcount);
if (mod) {
/*
* Realtime extent not lined up at the end.
@@ -5494,9 +5513,19 @@ __xfs_bunmapi(
goto nodelete;
}

- mod = xfs_rtb_to_rtxoff(mp, del.br_startblock);
+ if (isrt)
+ mod = xfs_rtb_to_rtxoff(mp, del.br_startblock);
+ else if (isforcealign)
+ mod = xfs_forcealign_extent_offset(ip,
+ del.br_startblock);
+
if (mod) {
- xfs_extlen_t off = mp->m_sb.sb_rextsize - mod;
+ xfs_extlen_t off;
+ if (isrt)
+ off = mp->m_sb.sb_rextsize - mod;
+ else if (isforcealign) {
+ off = ip->i_extsize - mod;
+ }

/*
* Realtime extent is lined up at the end but not
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 065028789473..3f13943ab3a3 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -316,6 +316,11 @@ static inline bool xfs_inode_has_forcealign(struct
xfs_inode *ip)
return ip->i_diflags2 & XFS_DIFLAG2_FORCEALIGN;
}

+static inline bool xfs_inode_has_extsize(struct xfs_inode *ip)
+{
+ return ip->i_diflags & XFS_DIFLAG_EXTSIZE;
+}
+
/*
* Return the buftarg used for data allocations on a given inode.

------>8-------

From 8cb2b61fa419b961d22609e12f2d941ce976d0f0 Mon Sep 17 00:00:00 2001
From: John Garry <john.g.garry@oracle.com>
Date: Wed, 20 Mar 2024 13:05:39 +0000
Subject: [PATCH 3/3] fs: xfs: Only free full extents for forcealign

Like we already do for rtvol, only free full extents for forcealign in
xfs_free_file_space().

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/xfs_bmap_util.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 178a4865d1ed..e3e6e27a33bf 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -848,8 +848,11 @@ xfs_free_file_space(
startoffset_fsb = XFS_B_TO_FSB(mp, offset);
endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len);

- /* We can only free complete realtime extents. */
- if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) {
+ /* Free only complete extents. */
+ if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1) {
+ startoffset_fsb = roundup_64(startoffset_fsb, ip->i_extsize);
+ endoffset_fsb = rounddown_64(endoffset_fsb, ip->i_extsize);
+ } else if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) {
startoffset_fsb = xfs_rtb_roundup_rtx(mp, startoffset_fsb);
endoffset_fsb = xfs_rtb_rounddown_rtx(mp, endoffset_fsb);
}

------8<--------
EOM