| Message ID | 20240705162450.3481169-9-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | forcealign for xfs | expand |
On Fri, Jul 05, 2024 at 04:24:45PM +0000, John Garry wrote: > - if (xfs_inode_has_bigrtalloc(ip)) > + > + /* Only try to free beyond the allocation unit that crosses EOF */ > + if (xfs_inode_has_forcealign(ip)) > + end_fsb = roundup_64(end_fsb, ip->i_extsize); > + else if (xfs_inode_has_bigrtalloc(ip)) > end_fsb = xfs_rtb_roundup_rtx(mp, end_fsb); Shouldn't we have a common helper to align things the right way? But more importantly shouldn't this also cover hole punching if we really want force aligned boundaries?
On Sat, Jul 06, 2024 at 09:56:09AM +0200, Christoph Hellwig wrote: > On Fri, Jul 05, 2024 at 04:24:45PM +0000, John Garry wrote: > > - if (xfs_inode_has_bigrtalloc(ip)) > > + > > + /* Only try to free beyond the allocation unit that crosses EOF */ > > + if (xfs_inode_has_forcealign(ip)) > > + end_fsb = roundup_64(end_fsb, ip->i_extsize); > > + else if (xfs_inode_has_bigrtalloc(ip)) > > end_fsb = xfs_rtb_roundup_rtx(mp, end_fsb); > > Shouldn't we have a common helper to align things the right way? Yes, that's what I keep saying. The common way to do this is: align = xfs_inode_alloc_unitsize(ip); if (align > mp->m_blocksize) end_fsb = roundup_64(end_fsb, align); Wrapping that into a helper might be appropriate, though we'd need wrappers for aligning both the start (down) and end (up). To make this work, the xfs_inode_alloc_unitsize() code needs to grow a forcealign check. That overrides the RT rextsize value (force align on RT should work the same as it does on data devs) and needs to look like this: unsigned int blocks = 1; + if (xfs_inode_has_forcealign(ip) + blocks = ip->i_extsize; - if (XFS_IS_REALTIME_INODE(ip)) + else if (XFS_IS_REALTIME_INODE(ip)) blocks = ip->i_mount->m_sb.sb_rextsize; return XFS_FSB_TO_B(ip->i_mount, blocks); > But more importantly shouldn't this also cover hole punching if we > really want force aligned boundaries? Yes, that's what I keep saying. There is no difference in the alignment behaviour needed for "xfs_inode_has_bigrtalloc" and "xfs_inode_has_forcealign" except for the source of the allocation alignment value. -Dave.
On 08/07/2024 02:44, Dave Chinner wrote: > On Sat, Jul 06, 2024 at 09:56:09AM +0200, Christoph Hellwig wrote: >> On Fri, Jul 05, 2024 at 04:24:45PM +0000, John Garry wrote: >>> - if (xfs_inode_has_bigrtalloc(ip)) >>> + >>> + /* Only try to free beyond the allocation unit that crosses EOF */ >>> + if (xfs_inode_has_forcealign(ip)) >>> + end_fsb = roundup_64(end_fsb, ip->i_extsize); >>> + else if (xfs_inode_has_bigrtalloc(ip)) >>> end_fsb = xfs_rtb_roundup_rtx(mp, end_fsb); >> >> Shouldn't we have a common helper to align things the right way? > > Yes, that's what I keep saying. Such a change was introduced in https://lore.kernel.org/linux-xfs/20240501235310.GP360919@frogsfrogsfrogs/ and, as you can see, Darrick was less than happy with it. That is why I kept this method which removed recently added RT code. Darrick, can we find a better method to factor this code out, like below? > The common way to do this is: > > align = xfs_inode_alloc_unitsize(ip); > if (align > mp->m_blocksize) > end_fsb = roundup_64(end_fsb, align); > > Wrapping that into a helper might be appropriate, though we'd need > wrappers for aligning both the start (down) and end (up). > > To make this work, the xfs_inode_alloc_unitsize() code needs to grow > a forcealign check. That overrides the RT rextsize value (force > align on RT should work the same as it does on data devs) and needs > to look like this: > > unsigned int blocks = 1; > > + if (xfs_inode_has_forcealign(ip) > + blocks = ip->i_extsize; > - if (XFS_IS_REALTIME_INODE(ip)) > + else if (XFS_IS_REALTIME_INODE(ip)) > blocks = ip->i_mount->m_sb.sb_rextsize; That's in 09/13 > > return XFS_FSB_TO_B(ip->i_mount, blocks); > >> But more importantly shouldn't this also cover hole punching if we >> really want force aligned boundaries? so doesn't the xfs_file_fallocate(PUNCH_HOLES) -> xfs_flush_unmap_range() -> rounding with xfs_inode_alloc_unitsize() do the required job? > > Yes, that's what I keep saying. There is no difference in the > alignment behaviour needed for "xfs_inode_has_bigrtalloc" and > "xfs_inode_has_forcealign" except for the source of the allocation > alignment value. >
On Mon, Jul 08, 2024 at 08:36:52AM +0100, John Garry wrote: > On 08/07/2024 02:44, Dave Chinner wrote: > > On Sat, Jul 06, 2024 at 09:56:09AM +0200, Christoph Hellwig wrote: > > > On Fri, Jul 05, 2024 at 04:24:45PM +0000, John Garry wrote: > > > > - if (xfs_inode_has_bigrtalloc(ip)) > > > > + > > > > + /* Only try to free beyond the allocation unit that crosses EOF */ > > > > + if (xfs_inode_has_forcealign(ip)) > > > > + end_fsb = roundup_64(end_fsb, ip->i_extsize); > > > > + else if (xfs_inode_has_bigrtalloc(ip)) > > > > end_fsb = xfs_rtb_roundup_rtx(mp, end_fsb); > > > > > > Shouldn't we have a common helper to align things the right way? > > > > Yes, that's what I keep saying. > > Such a change was introduced in > https://lore.kernel.org/linux-xfs/20240501235310.GP360919@frogsfrogsfrogs/ > > and, as you can see, Darrick was less than happy with it. That is why I kept > this method which removed recently added RT code. I know. However, "This is pointless busywork!" is not a technical argument against the observation that rtbigalloc and forcealign are exactly the same thing from the BMBT management POV and so should be combined. Arguing that "but doing the right thing makes more work for me" doesn't hold any weight. It never has. Shouting and ranting irrationally is a great way to shut down any further conversation, though. Our normal process is to factor the code and add the extra condition for the new feature. That's all I'm asking to be done. It's not technically difficult. It makes the code better. it makes the code easier to test, too, because there are now two entries in the test matrix taht exercise that code path. It's simpler to understand months down the track, makes new alignment features easier to add in future, etc. Put simply: if we just do what we have always done, then we end up with better code. Hence I just don't see why people are trying to make a mountain out of this... > Darrick, can we find a better method to factor this code out, like below? > > > The common way to do this is: > > > > align = xfs_inode_alloc_unitsize(ip); > > if (align > mp->m_blocksize) > > end_fsb = roundup_64(end_fsb, align); > > > > Wrapping that into a helper might be appropriate, though we'd need > > wrappers for aligning both the start (down) and end (up). > > > > To make this work, the xfs_inode_alloc_unitsize() code needs to grow > > a forcealign check. That overrides the RT rextsize value (force > > align on RT should work the same as it does on data devs) and needs > > to look like this: > > > > unsigned int blocks = 1; > > > > + if (xfs_inode_has_forcealign(ip) > > + blocks = ip->i_extsize; > > - if (XFS_IS_REALTIME_INODE(ip)) > > + else if (XFS_IS_REALTIME_INODE(ip)) > > blocks = ip->i_mount->m_sb.sb_rextsize; > > That's in 09/13 Thanks, I thought it was somewhere in this patch series, I just wanted to point out (once again) that rtbigalloc and forcealign are basically the same thing. And, in case it isn't obvious to everyone, setting forcealign on a rt inode is basically the equivalent of turning on "rtbigalloc" for just that inode.... > > return XFS_FSB_TO_B(ip->i_mount, blocks); > > > > > But more importantly shouldn't this also cover hole punching if we > > > really want force aligned boundaries? > > so doesn't the xfs_file_fallocate(PUNCH_HOLES) -> xfs_flush_unmap_range() -> > rounding with xfs_inode_alloc_unitsize() do the required job? No, xfs_flush_unmap_range() should be flushing to *outwards* block/page size boundaries because it is cleaning and invalidating the page cache over the punch range, not manipulating the physical extents underlying the data. It's only once we go to punch out the extents in xfs_free_file_space() that we need to use xfs_inode_alloc_unitsize() to determine the *inwards* rounding for the extent punch vs writing physical zeroes.... -Dave.
On 08/07/2024 12:12, Dave Chinner wrote: > On Mon, Jul 08, 2024 at 08:36:52AM +0100, John Garry wrote: >> On 08/07/2024 02:44, Dave Chinner wrote: >>> On Sat, Jul 06, 2024 at 09:56:09AM +0200, Christoph Hellwig wrote: >>>> On Fri, Jul 05, 2024 at 04:24:45PM +0000, John Garry wrote: >>>>> - if (xfs_inode_has_bigrtalloc(ip)) >>>>> + >>>>> + /* Only try to free beyond the allocation unit that crosses EOF */ >>>>> + if (xfs_inode_has_forcealign(ip)) >>>>> + end_fsb = roundup_64(end_fsb, ip->i_extsize); >>>>> + else if (xfs_inode_has_bigrtalloc(ip)) >>>>> end_fsb = xfs_rtb_roundup_rtx(mp, end_fsb); >>>> >>>> Shouldn't we have a common helper to align things the right way? >>> >>> Yes, that's what I keep saying. >> >> Such a change was introduced in >> https://urldefense.com/v3/__https://lore.kernel.org/linux-xfs/20240501235310.GP360919@frogsfrogsfrogs/__;!!ACWV5N9M2RV99hQ!LU97IJHrwX9otpItjJI_ewbNt-T-Lgyt5ulyz0yGKc5Dmms4jqhwZv5NregBEK_dTEtEDCYCwcA43RxQFnc$ >> >> and, as you can see, Darrick was less than happy with it. That is why I kept >> this method which removed recently added RT code. > > I know. > > However, "This is pointless busywork!" is not a technical argument > against the observation that rtbigalloc and forcealign are exactly > the same thing from the BMBT management POV and so should be > combined. > > Arguing that "but doing the right thing makes more work for me" > doesn't hold any weight. It never has. Shouting and ranting > irrationally is a great way to shut down any further conversation, > though. > > Our normal process is to factor the code and add the extra condition > for the new feature. That's all I'm asking to be done. It's not > technically difficult. It makes the code better. it makes the code > easier to test, too, because there are now two entries in the test > matrix taht exercise that code path. It's simpler to understand > months down the track, makes new alignment features easier to add in > future, etc. > > Put simply: if we just do what we have always done, then we end up > with better code. Hence I just don't see why people are trying to > make a mountain out of this... I tend to agree with what you said; and the conversation was halted, so left me in an awkward position. > >> Darrick, can we find a better method to factor this code out, like below? >> >>> The common way to do this is: >>> >>> align = xfs_inode_alloc_unitsize(ip); >>> if (align > mp->m_blocksize) >>> end_fsb = roundup_64(end_fsb, align); >>> >>> Wrapping that into a helper might be appropriate, though we'd need >>> wrappers for aligning both the start (down) and end (up). >>> >>> To make this work, the xfs_inode_alloc_unitsize() code needs to grow >>> a forcealign check. That overrides the RT rextsize value (force >>> align on RT should work the same as it does on data devs) and needs >>> to look like this: >>> >>> unsigned int blocks = 1; >>> >>> + if (xfs_inode_has_forcealign(ip) >>> + blocks = ip->i_extsize; >>> - if (XFS_IS_REALTIME_INODE(ip)) >>> + else if (XFS_IS_REALTIME_INODE(ip)) >>> blocks = ip->i_mount->m_sb.sb_rextsize; >> >> That's in 09/13 > > Thanks, I thought it was somewhere in this patch series, I just > wanted to point out (once again) that rtbigalloc and forcealign are > basically the same thing. > > And, in case it isn't obvious to everyone, setting forcealign on a > rt inode is basically the equivalent of turning on "rtbigalloc" for > just that inode.... sure > >>> return XFS_FSB_TO_B(ip->i_mount, blocks); >>> >>>> But more importantly shouldn't this also cover hole punching if we >>>> really want force aligned boundaries? >> >> so doesn't the xfs_file_fallocate(PUNCH_HOLES) -> xfs_flush_unmap_range() -> >> rounding with xfs_inode_alloc_unitsize() do the required job? > > No, xfs_flush_unmap_range() should be flushing to *outwards* > block/page size boundaries because it is cleaning and invalidating > the page cache over the punch range, not manipulating the physical > extents underlying the data. > > It's only once we go to punch out the extents in > xfs_free_file_space() that we need to use xfs_inode_alloc_unitsize() > to determine the *inwards* rounding for the extent punch vs writing > physical zeroes.... > ok, well we are covered for forcealign in both xfs_flush_unmap_range() and xfs_free_file_space().
On Mon, Jul 08, 2024 at 11:44:59AM +1000, Dave Chinner wrote: > On Sat, Jul 06, 2024 at 09:56:09AM +0200, Christoph Hellwig wrote: > > On Fri, Jul 05, 2024 at 04:24:45PM +0000, John Garry wrote: > > > - if (xfs_inode_has_bigrtalloc(ip)) > > > + > > > + /* Only try to free beyond the allocation unit that crosses EOF */ > > > + if (xfs_inode_has_forcealign(ip)) > > > + end_fsb = roundup_64(end_fsb, ip->i_extsize); > > > + else if (xfs_inode_has_bigrtalloc(ip)) > > > end_fsb = xfs_rtb_roundup_rtx(mp, end_fsb); > > > > Shouldn't we have a common helper to align things the right way? > > Yes, that's what I keep saying. The common way to do this is: > > align = xfs_inode_alloc_unitsize(ip); > if (align > mp->m_blocksize) > end_fsb = roundup_64(end_fsb, align); > > Wrapping that into a helper might be appropriate, though we'd need > wrappers for aligning both the start (down) and end (up). I think the above is already good enough.
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c index fe2e2c930975..b9f8093ae78c 100644 --- a/fs/xfs/xfs_bmap_util.c +++ b/fs/xfs/xfs_bmap_util.c @@ -537,8 +537,13 @@ xfs_can_free_eofblocks( * forever. */ end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip)); - if (xfs_inode_has_bigrtalloc(ip)) + + /* Only try to free beyond the allocation unit that crosses EOF */ + if (xfs_inode_has_forcealign(ip)) + end_fsb = roundup_64(end_fsb, ip->i_extsize); + else if (xfs_inode_has_bigrtalloc(ip)) 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;