diff mbox series

[v2,08/13] xfs: Do not free EOF blocks for forcealign

Message ID 20240705162450.3481169-9-john.g.garry@oracle.com (mailing list archive)
State Superseded, archived
Headers show
Series forcealign for xfs | expand

Commit Message

John Garry July 5, 2024, 4:24 p.m. UTC
For when forcealign is enabled, we want the EOF to be aligned as well, so
do not free EOF blocks.

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 fs/xfs/xfs_bmap_util.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Christoph Hellwig July 6, 2024, 7:56 a.m. UTC | #1
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?
Dave Chinner July 8, 2024, 1:44 a.m. UTC | #2
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.
John Garry July 8, 2024, 7:36 a.m. UTC | #3
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.
>
Dave Chinner July 8, 2024, 11:12 a.m. UTC | #4
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.
John Garry July 8, 2024, 2:41 p.m. UTC | #5
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().
Christoph Hellwig July 9, 2024, 7:41 a.m. UTC | #6
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 mbox series

Patch

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;