diff mbox series

[v2,06/14] fs: xfs: Do not free EOF blocks for forcealign

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

Commit Message

John Garry March 4, 2024, 1:04 p.m. UTC
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(-)

Comments

Dave Chinner March 6, 2024, 9:07 p.m. UTC | #1
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.
John Garry March 7, 2024, 11:38 a.m. UTC | #2
>>   	 */
>>   	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 mbox series

Patch

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,