| Message ID | 20240304130428.13026-7-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block atomic writes for XFS | expand |
On Mon, Mar 04, 2024 at 01:04:20PM +0000, John Garry wrote: > For when forcealign is enabled, we want the EOF to be aligned as well, so > do not free EOF blocks. > > Add helper function xfs_get_extsz() to get the guaranteed extent size > alignment for forcealign enabled. Since this code is only relevant to > forcealign and forcealign is not possible for RT yet, ignore RT. > > Signed-off-by: John Garry <john.g.garry@oracle.com> > --- > fs/xfs/xfs_bmap_util.c | 7 ++++++- > fs/xfs/xfs_inode.c | 14 ++++++++++++++ > fs/xfs/xfs_inode.h | 1 + > 3 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c > index c2531c28905c..07bfb03c671a 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 */ That comment seems wrong - this just rounds up where we start trimming from to be aligned... > + if (xfs_get_extsz(ip) > 1) > + end_fsb = roundup_64(end_fsb, xfs_get_extsz(ip)); > + else if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) > end_fsb = xfs_rtb_roundup_rtx(mp, end_fsb); I think this would be better written as: /* * Forced extent alignment requires us to round up where we * start trimming from so that result is correctly aligned. */ if (xfs_inode_forcealign(ip)) { if (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); } Because we only want end-fsb alignment when forced alignment is required. Which then makes me wonder: truncate needs this, too, doesn't it? And the various fallocate() ops like hole punching and extent shifting? > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 2c439df8c47f..bbb8886f1d32 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -65,6 +65,20 @@ xfs_get_extsz_hint( > return 0; > } > > +/* > + * Helper function to extract extent size. It will return a power-of-2, > + * as forcealign requires this. > + */ > +xfs_extlen_t > +xfs_get_extsz( > + struct xfs_inode *ip) > +{ > + if (xfs_inode_forcealign(ip) && ip->i_extsize) > + return ip->i_extsize; > + > + return 1; > +} This can go away - if it is needed elsewhere, then I think it would be better open coded because it better documents what the code is doing... -Dave.
>> */ >> 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 */ > > That comment seems wrong - this just rounds up where we start > trimming from to be aligned... ok > >> + if (xfs_get_extsz(ip) > 1) >> + end_fsb = roundup_64(end_fsb, xfs_get_extsz(ip)); >> + else if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) >> end_fsb = xfs_rtb_roundup_rtx(mp, end_fsb); > > I think this would be better written as: > > /* > * Forced extent alignment requires us to round up where we > * start trimming from so that result is correctly aligned. > */ > if (xfs_inode_forcealign(ip)) { > if (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); > } > > Because we only want end-fsb alignment when forced alignment is > required. But why change current rtvol behaviour? > > Which then makes me wonder: truncate needs this, too, doesn't it? > And the various fallocate() ops like hole punching and extent > shifting? > Yes, I would think so. I quickly checked rtvol for truncate and it does the round up. I would need to check the relevant code for truncate and fallocate for forcealign now. I do also wonder if we could factor out this rounding up code for truncate, facallocate, and whatever else. >> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c >> index 2c439df8c47f..bbb8886f1d32 100644 >> --- a/fs/xfs/xfs_inode.c >> +++ b/fs/xfs/xfs_inode.c >> @@ -65,6 +65,20 @@ xfs_get_extsz_hint( >> return 0; >> } >> >> +/* >> + * Helper function to extract extent size. It will return a power-of-2, >> + * as forcealign requires this. >> + */ >> +xfs_extlen_t >> +xfs_get_extsz( >> + struct xfs_inode *ip) >> +{ >> + if (xfs_inode_forcealign(ip) && ip->i_extsize) >> + return ip->i_extsize; >> + >> + return 1; >> +} > > This can go away - if it is needed elsewhere, then I think it would > be better open coded because it better documents what the code is > doing... > I would rather get rid of xfs_get_extsz() for sure. Thanks, John
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c index c2531c28905c..07bfb03c671a 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_get_extsz(ip) > 1) + end_fsb = roundup_64(end_fsb, xfs_get_extsz(ip)); + 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; diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 2c439df8c47f..bbb8886f1d32 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -65,6 +65,20 @@ xfs_get_extsz_hint( return 0; } +/* + * Helper function to extract extent size. It will return a power-of-2, + * as forcealign requires this. + */ +xfs_extlen_t +xfs_get_extsz( + struct xfs_inode *ip) +{ + if (xfs_inode_forcealign(ip) && ip->i_extsize) + return ip->i_extsize; + + return 1; +} + /* * Helper function to extract CoW extent size hint from inode. * Between the extent size hint and the CoW extent size hint, we diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index 82e2838f6d64..b6c42c27943e 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -547,6 +547,7 @@ void xfs_lock_two_inodes(struct xfs_inode *ip0, uint ip0_mode, struct xfs_inode *ip1, uint ip1_mode); xfs_extlen_t xfs_get_extsz_hint(struct xfs_inode *ip); +xfs_extlen_t xfs_get_extsz(struct xfs_inode *ip); xfs_extlen_t xfs_get_cowextsz_hint(struct xfs_inode *ip); int xfs_init_new_inode(struct mnt_idmap *idmap, struct xfs_trans *tp,
For when forcealign is enabled, we want the EOF to be aligned as well, so do not free EOF blocks. Add helper function xfs_get_extsz() to get the guaranteed extent size alignment for forcealign enabled. Since this code is only relevant to forcealign and forcealign is not possible for RT yet, ignore RT. Signed-off-by: John Garry <john.g.garry@oracle.com> --- fs/xfs/xfs_bmap_util.c | 7 ++++++- fs/xfs/xfs_inode.c | 14 ++++++++++++++ fs/xfs/xfs_inode.h | 1 + 3 files changed, 21 insertions(+), 1 deletion(-)