| Message ID | 20240429174746.2132161-13-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | block atomic writes for XFS | expand |
On Mon, Apr 29, 2024 at 05:47:37PM +0000, John Garry wrote: > 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 f26d1570b9bd..1dd45dfb2811 100644 > --- a/fs/xfs/xfs_bmap_util.c > +++ b/fs/xfs/xfs_bmap_util.c > @@ -847,8 +847,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); > } When you look at xfs_rtb_roundup_rtx() you'll find it's just a one line wrapper around roundup_64(). So lets get rid of the obfuscation that the one line RT wrapper introduces, and it turns into this: rounding = 1; if (xfs_inode_has_forcealign(ip) rounding = ip->i_extsize; else if (XFS_IS_REALTIME_INODE(ip)) rounding = mp->m_sb.sb_rextsize; if (rounding > 1) { startoffset_fsb = roundup_64(startoffset_fsb, rounding); endoffset_fsb = rounddown_64(endoffset_fsb, rounding); } What this points out is that the prep steps for fallocate operations also need to handle both forced alignment and rtextsize rounding, and it does neither right now. xfs_flush_unmap_range() is the main offender here, but xfs_prepare_shift() also needs fixing. Hence: static inline xfs_extlen_t xfs_extent_alignment( struct xfs_inode *ip) { if (xfs_inode_has_forcealign(ip)) return ip->i_extsize; if (XFS_IS_REALTIME_INODE(ip)) return mp->m_sb.sb_rextsize; return 1; } In xfs_flush_unmap_range(): /* * Make sure we extend the flush out to extent alignment * boundaries so any extent range overlapping the start/end * of the modification we are about to do is clean and idle. */ rounding = XFS_FSB_TO_B(mp, xfs_extent_alignment(ip)); rounding = max(rounding, PAGE_SIZE); ... in xfs_free_file_space() /* * Round the range we are going to free inwards to extent * alignment boundaries so we don't free blocks outside the * range requested. */ rounding = xfs_extent_alignment(ip); if (rounding > 1 ) { startoffset_fsb = roundup_64(startoffset_fsb, rounding); endoffset_fsb = rounddown_64(endoffset_fsb, rounding); } and in xfs_prepare_shift() /* * Shift operations must stabilize the start block offset boundary along * with the full range of the operation. If we don't, a COW writeback * completion could race with an insert, front merge with the start * extent (after split) during the shift and corrupt the file. Start * with the aligned block just prior to the start to stabilize the boundary. */ rounding = XFS_FSB_TO_B(mp, xfs_extent_alignment(ip)); offset = round_down(offset, rounding); if (offset) offset -= rounding; Also, I think that the changes I suggested earlier to xfs_is_falloc_aligned() could use this xfs_extent_alignment() helper... Overall this makes the code a whole lot easier to read and it also allows forced alignment to work correctly on RT devices... -Dave.
On 01/05/2024 01:53, Dave Chinner wrote: > On Mon, Apr 29, 2024 at 05:47:37PM +0000, John Garry wrote: >> 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 f26d1570b9bd..1dd45dfb2811 100644 >> --- a/fs/xfs/xfs_bmap_util.c >> +++ b/fs/xfs/xfs_bmap_util.c >> @@ -847,8 +847,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); >> } > > When you look at xfs_rtb_roundup_rtx() you'll find it's just a one > line wrapper around roundup_64(). > > So lets get rid of the obfuscation that the one line RT wrapper > introduces, and it turns into this: > > rounding = 1; > if (xfs_inode_has_forcealign(ip) > rounding = ip->i_extsize; > else if (XFS_IS_REALTIME_INODE(ip)) > rounding = mp->m_sb.sb_rextsize; > > if (rounding > 1) { > startoffset_fsb = roundup_64(startoffset_fsb, rounding); > endoffset_fsb = rounddown_64(endoffset_fsb, rounding); > } ok, and the same idea for xfs_can_free_eofblocks() with xfs_rtb_roundup_rtx(), right? > > What this points out is that the prep steps for fallocate operations > also need to handle both forced alignment and rtextsize rounding, > and it does neither right now. xfs_flush_unmap_range() is the main > offender here, but xfs_prepare_shift() also needs fixing. When you say fix, is this something to spin off separately for RT? This series is big enough already... > > Hence: > > static inline xfs_extlen_t > xfs_extent_alignment( > struct xfs_inode *ip) > { > if (xfs_inode_has_forcealign(ip)) > return ip->i_extsize; > if (XFS_IS_REALTIME_INODE(ip)) > return mp->m_sb.sb_rextsize; > return 1; > } > > > In xfs_flush_unmap_range(): > > /* > * Make sure we extend the flush out to extent alignment > * boundaries so any extent range overlapping the start/end > * of the modification we are about to do is clean and idle. > */ > rounding = XFS_FSB_TO_B(mp, xfs_extent_alignment(ip)); > rounding = max(rounding, PAGE_SIZE); > ... > > in xfs_free_file_space() > > /* > * Round the range we are going to free inwards to extent > * alignment boundaries so we don't free blocks outside the > * range requested. > */ > rounding = xfs_extent_alignment(ip); > if (rounding > 1 ) { > startoffset_fsb = roundup_64(startoffset_fsb, rounding); > endoffset_fsb = rounddown_64(endoffset_fsb, rounding); > } > > and in xfs_prepare_shift() > > /* > * Shift operations must stabilize the start block offset boundary along > * with the full range of the operation. If we don't, a COW writeback > * completion could race with an insert, front merge with the start > * extent (after split) during the shift and corrupt the file. Start > * with the aligned block just prior to the start to stabilize the boundary. > */ > rounding = XFS_FSB_TO_B(mp, xfs_extent_alignment(ip)); > offset = round_down(offset, rounding); > if (offset) > offset -= rounding; > > Also, I think that the changes I suggested earlier to > xfs_is_falloc_aligned() could use this xfs_extent_alignment() > helper... > > Overall this makes the code a whole lot easier to read and it also > allows forced alignment to work correctly on RT devices... > ok, fine Thanks, John
On Wed, May 01, 2024 at 10:53:28AM +1000, Dave Chinner wrote: > On Mon, Apr 29, 2024 at 05:47:37PM +0000, John Garry wrote: > > 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 f26d1570b9bd..1dd45dfb2811 100644 > > --- a/fs/xfs/xfs_bmap_util.c > > +++ b/fs/xfs/xfs_bmap_util.c > > @@ -847,8 +847,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); > > } > > When you look at xfs_rtb_roundup_rtx() you'll find it's just a one > line wrapper around roundup_64(). I added this a couple of cycles ago to get ready for realtime modernization. That will create a bunch *more* churn in my tree just to convert everything *back*. Where the hell were you when that was being reviewed?!!! NO! This is pointless busywork! --D > So lets get rid of the obfuscation that the one line RT wrapper > introduces, and it turns into this: > > rounding = 1; > if (xfs_inode_has_forcealign(ip) > rounding = ip->i_extsize; > else if (XFS_IS_REALTIME_INODE(ip)) > rounding = mp->m_sb.sb_rextsize; > > if (rounding > 1) { > startoffset_fsb = roundup_64(startoffset_fsb, rounding); > endoffset_fsb = rounddown_64(endoffset_fsb, rounding); > } > > What this points out is that the prep steps for fallocate operations > also need to handle both forced alignment and rtextsize rounding, > and it does neither right now. xfs_flush_unmap_range() is the main > offender here, but xfs_prepare_shift() also needs fixing. > > Hence: > > static inline xfs_extlen_t > xfs_extent_alignment( > struct xfs_inode *ip) > { > if (xfs_inode_has_forcealign(ip)) > return ip->i_extsize; > if (XFS_IS_REALTIME_INODE(ip)) > return mp->m_sb.sb_rextsize; > return 1; > } > > > In xfs_flush_unmap_range(): > > /* > * Make sure we extend the flush out to extent alignment > * boundaries so any extent range overlapping the start/end > * of the modification we are about to do is clean and idle. > */ > rounding = XFS_FSB_TO_B(mp, xfs_extent_alignment(ip)); > rounding = max(rounding, PAGE_SIZE); > ... > > in xfs_free_file_space() > > /* > * Round the range we are going to free inwards to extent > * alignment boundaries so we don't free blocks outside the > * range requested. > */ > rounding = xfs_extent_alignment(ip); > if (rounding > 1 ) { > startoffset_fsb = roundup_64(startoffset_fsb, rounding); > endoffset_fsb = rounddown_64(endoffset_fsb, rounding); > } > > and in xfs_prepare_shift() > > /* > * Shift operations must stabilize the start block offset boundary along > * with the full range of the operation. If we don't, a COW writeback > * completion could race with an insert, front merge with the start > * extent (after split) during the shift and corrupt the file. Start > * with the aligned block just prior to the start to stabilize the boundary. > */ > rounding = XFS_FSB_TO_B(mp, xfs_extent_alignment(ip)); > offset = round_down(offset, rounding); > if (offset) > offset -= rounding; > > Also, I think that the changes I suggested earlier to > xfs_is_falloc_aligned() could use this xfs_extent_alignment() > helper... > > Overall this makes the code a whole lot easier to read and it also > allows forced alignment to work correctly on RT devices... > > -Dave. > -- > Dave Chinner > david@fromorbit.com >
On Wed, May 01, 2024 at 04:53:10PM -0700, Darrick J. Wong wrote: > On Wed, May 01, 2024 at 10:53:28AM +1000, Dave Chinner wrote: > > On Mon, Apr 29, 2024 at 05:47:37PM +0000, John Garry wrote: > > > 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 f26d1570b9bd..1dd45dfb2811 100644 > > > --- a/fs/xfs/xfs_bmap_util.c > > > +++ b/fs/xfs/xfs_bmap_util.c > > > @@ -847,8 +847,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); > > > } > > > > When you look at xfs_rtb_roundup_rtx() you'll find it's just a one > > line wrapper around roundup_64(). > > I added this a couple of cycles ago to get ready for realtime > modernization. Yes, I know. I'm not suggesting that there's anything wrong with this code, just pointing out that the RT wrappers are doing the exact same conversion as the force-align code is doing. And from that observation, a common implementation makes a lot of sense because that same logic is repeated in quite a few places.... > That will create a bunch *more* churn in my tree just to > convert everything *back*. This doesn't change anything significant in your tree, nor do you need to "convert everything back". The RT wrappers are unchanged, and the only material difference in your tree vs the upstream xfs_free_file_space() this patchset is based on is this: - if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) { + if (xfs_inode_has_bigrtalloc(ip)) { That's it. All the suggestion I made does is change where you need to make this one line change. It would also remove the need to do this one line change in multiple other places, so it would actually -reduce- your ongoing rebase pain, not make it worse. That's a net win for everyone, and it's most definitely not a reason to shout at people and threaten to revert any changes they might make in this area of the code. > Where the hell were you when that was being reviewed?!!! How is this sort of unhelpful statement in any way relevant to improving the forcealign functionality to the point where we can actually merge it and start making use of it for atomic writes? -Dave.
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c index f26d1570b9bd..1dd45dfb2811 100644 --- a/fs/xfs/xfs_bmap_util.c +++ b/fs/xfs/xfs_bmap_util.c @@ -847,8 +847,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); }
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(-)