From: John Garry <john.g.garry@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org, ojaswin@linux.ibm.com, ritesh.list@gmail.com
Subject: Re: [PATCH 1/3] xfs: simplify extent allocation alignment
Date: Wed, 3 Apr 2024 12:30:26 +0100 [thread overview]
Message-ID: <a7de9521-d101-402d-a59b-f7ce936ba383@oracle.com> (raw)
In-Reply-To: <ZgyYcu7pzsGJzxGX@dread.disaster.area>
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
next prev parent reply other threads:[~2024-04-03 11:30 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-04 13:04 [PATCH v2 00/14] block atomic writes for XFS John Garry
2024-03-04 13:04 ` [PATCH v2 01/14] block: Add blk_validate_atomic_write_op_size() John Garry
2024-03-04 13:04 ` [PATCH v2 02/14] fs: xfs: Don't use low-space allocator for alignment > 1 John Garry
2024-03-04 22:15 ` Dave Chinner
2024-03-05 13:36 ` John Garry
2024-03-04 13:04 ` [PATCH v2 03/14] fs: xfs: Introduce FORCEALIGN inode flag John Garry
2024-03-04 13:04 ` [PATCH v2 04/14] fs: xfs: Make file data allocations observe the 'forcealign' flag John Garry
2024-03-05 0:44 ` Dave Chinner
2024-03-05 15:22 ` John Garry
2024-03-05 22:18 ` Dave Chinner
2024-03-06 5:20 ` [RFC PATCH 0/3] xfs: forced extent alignment Dave Chinner
2024-03-06 5:20 ` [PATCH 1/3] xfs: simplify extent allocation alignment Dave Chinner
2024-03-13 11:03 ` John Garry
2024-03-20 4:35 ` Dave Chinner
2024-03-26 16:08 ` John Garry
2024-04-02 5:58 ` Dave Chinner
2024-04-02 7:49 ` John Garry
2024-04-02 15:11 ` John Garry
2024-04-02 21:26 ` Dave Chinner
2024-04-03 8:49 ` John Garry
2024-04-02 23:44 ` Dave Chinner
2024-04-03 11:30 ` John Garry [this message]
2024-03-06 5:20 ` [PATCH 2/3] xfs: make EOF allocation simpler Dave Chinner
2024-03-06 5:20 ` [PATCH 3/3] xfs: introduce forced allocation alignment Dave Chinner
2024-03-06 11:46 ` [RFC PATCH 0/3] xfs: forced extent alignment John Garry
2024-03-06 17:52 ` John Garry
2024-03-06 20:54 ` Dave Chinner
2024-03-13 18:32 ` John Garry
2024-03-06 9:41 ` [PATCH v2 04/14] fs: xfs: Make file data allocations observe the 'forcealign' flag John Garry
2024-03-04 13:04 ` [PATCH v2 05/14] fs: xfs: Enable file data forcealign feature John Garry
2024-03-04 13:04 ` [PATCH v2 06/14] fs: xfs: Do not free EOF blocks for forcealign John Garry
2024-03-06 21:07 ` Dave Chinner
2024-03-07 11:38 ` John Garry
2024-03-04 13:04 ` [PATCH v2 07/14] fs: iomap: Sub-extent zeroing John Garry
2024-03-06 21:14 ` Dave Chinner
2024-03-07 11:51 ` John Garry
2024-03-04 13:04 ` [PATCH v2 08/14] fs: xfs: " John Garry
2024-03-06 22:00 ` Dave Chinner
2024-03-07 12:57 ` John Garry
2024-03-04 13:04 ` [PATCH v2 09/14] fs: Add FS_XFLAG_ATOMICWRITES flag John Garry
2024-03-04 13:04 ` [PATCH v2 10/14] fs: iomap: Atomic write support John Garry
2024-03-04 13:04 ` [PATCH v2 11/14] fs: xfs: Support FS_XFLAG_ATOMICWRITES for forcealign John Garry
2024-03-06 21:43 ` Dave Chinner
2024-03-07 12:42 ` John Garry
2024-03-04 13:04 ` [PATCH v2 12/14] fs: xfs: Support atomic write for statx John Garry
2024-03-06 21:31 ` Dave Chinner
2024-03-07 10:35 ` John Garry
2024-03-04 13:04 ` [PATCH v2 13/14] fs: xfs: Validate atomic writes John Garry
2024-03-06 21:22 ` Dave Chinner
2024-03-07 10:19 ` John Garry
2024-03-04 13:04 ` [PATCH v2 14/14] fs: xfs: Support setting FMODE_CAN_ATOMIC_WRITE John Garry
2024-03-06 21:33 ` Dave Chinner
2024-03-07 11:55 ` John Garry
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a7de9521-d101-402d-a59b-f7ce936ba383@oracle.com \
--to=john.g.garry@oracle.com \
--cc=david@fromorbit.com \
--cc=linux-xfs@vger.kernel.org \
--cc=ojaswin@linux.ibm.com \
--cc=ritesh.list@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).