diff mbox series

[v3,10/14] xfs: Do not free EOF blocks for forcealign

Message ID 20240801163057.3981192-11-john.g.garry@oracle.com (mailing list archive)
State New
Headers show
Series forcealign for xfs | expand

Commit Message

John Garry Aug. 1, 2024, 4:30 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> #earlier version
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     |  2 ++
 3 files changed, 21 insertions(+), 2 deletions(-)

Comments

Darrick J. Wong Aug. 6, 2024, 7:24 p.m. UTC | #1 
On Thu, Aug 01, 2024 at 04:30:53PM +0000, John Garry wrote:
> 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> #earlier version
> 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     |  2 ++
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index fe2e2c930975..60389ac8bd45 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -496,6 +496,7 @@ xfs_can_free_eofblocks(
>  	struct xfs_mount	*mp = ip->i_mount;
>  	xfs_fileoff_t		end_fsb;
>  	xfs_fileoff_t		last_fsb;
> +	xfs_fileoff_t		dummy_fsb;
>  	int			nimaps = 1;
>  	int			error;
>  
> @@ -537,8 +538,10 @@ xfs_can_free_eofblocks(
>  	 * forever.
>  	 */
>  	end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip));
> -	if (xfs_inode_has_bigrtalloc(ip))
> -		end_fsb = xfs_rtb_roundup_rtx(mp, end_fsb);
> +
> +	/* Only try to free beyond the allocation unit that crosses EOF */
> +	xfs_roundout_to_alloc_fsbsize(ip, &dummy_fsb, &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 5af12f35062d..d765dedebc15 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3129,6 +3129,20 @@ xfs_inode_alloc_unitsize(
>  	return XFS_FSB_TO_B(ip->i_mount, xfs_inode_alloc_fsbsize(ip));
>  }
>  
> +void
> +xfs_roundout_to_alloc_fsbsize(
> +	struct xfs_inode	*ip,
> +	xfs_fileoff_t		*start,
> +	xfs_fileoff_t		*end)
> +{
> +	unsigned int		blocks = xfs_inode_alloc_fsbsize(ip);
> +
> +	if (blocks == 1)
> +		return;
> +	*start = rounddown_64(*start, blocks);
> +	*end = roundup_64(*end, blocks);
> +}

This is probably going to start another round of shouting, but I think
it's silly to do two rounding operations when you only care about one
value.  In patch 12 it results in a bunch more dummy variables that you
then ignore.

Can't this be:

static inline xfs_fileoff_t
xfs_inode_rounddown_alloc_unit(
	struct xfs_inode	*ip,
	xfs_fileoff		off)
{
	unsigned int		rounding = xfs_inode_alloc_fsbsize(ip);

	if (rounding == 1)
		return off;
	return rounddown_64(off, rounding);
}

static inline xfs_fileoff_t
xfs_inode_roundup_alloc_unit(
	struct xfs_inode	*ip,
	xfs_fileoff		off)
{
	unsigned int		rounding = xfs_inode_alloc_fsbsize(ip);

	if (rounding == 1)
		return off;
	return roundup_64(off, rounding);
}

Then that callsite can be:

	end_fsb = xfs_inode_roundup_alloc_unit(ip,
			XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip)));

--D

> +
>  /* Should we always be using copy on write for file writes? */
>  bool
>  xfs_is_always_cow_inode(
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 158afad8c7a4..7f86c4781bd8 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -643,6 +643,8 @@ void xfs_inode_count_blocks(struct xfs_trans *tp, struct xfs_inode *ip,
>  		xfs_filblks_t *dblocks, xfs_filblks_t *rblocks);
>  unsigned int xfs_inode_alloc_fsbsize(struct xfs_inode *ip);
>  unsigned int xfs_inode_alloc_unitsize(struct xfs_inode *ip);
> +void xfs_roundout_to_alloc_fsbsize(struct xfs_inode *ip,
> +		xfs_fileoff_t *start, xfs_fileoff_t *end);
>  
>  int xfs_icreate_dqalloc(const struct xfs_icreate_args *args,
>  		struct xfs_dquot **udqpp, struct xfs_dquot **gdqpp,
> -- 
> 2.31.1
> 
>
John Garry Aug. 7, 2024, 12:33 p.m. UTC | #2 
>> +void
>> +xfs_roundout_to_alloc_fsbsize(
>> +	struct xfs_inode	*ip,
>> +	xfs_fileoff_t		*start,
>> +	xfs_fileoff_t		*end)
>> +{
>> +	unsigned int		blocks = xfs_inode_alloc_fsbsize(ip);
>> +
>> +	if (blocks == 1)
>> +		return;
>> +	*start = rounddown_64(*start, blocks);
>> +	*end = roundup_64(*end, blocks);
>> +}
> 
> This is probably going to start another round of shouting, but I think
> it's silly to do two rounding operations when you only care about one
> value. 

Sure, but the "in" version does use the 2x values and I wanted to be 
consistent. Anyway, I really don't feel strongly about this.

> In patch 12 it results in a bunch more dummy variables that you
> then ignore.
> 
> Can't this be:
> 
> static inline xfs_fileoff_t
> xfs_inode_rounddown_alloc_unit(

Just a question about the naming:
xfs_inode_alloc_unitsize() returns bytes, so I would expect 
xfs_inode_rounddown_alloc_unit() to deal in bytes. Would you be 
satisfied with xfs_rounddown_alloc_fsbsize()? Or any other suggestion?

> 	struct xfs_inode	*ip,
> 	xfs_fileoff		off)
> {
> 	unsigned int		rounding = xfs_inode_alloc_fsbsize(ip);
> 
> 	if (rounding == 1)
> 		return off;
> 	return rounddown_64(off, rounding);
> }
> 
> static inline xfs_fileoff_t
> xfs_inode_roundup_alloc_unit(
> 	struct xfs_inode	*ip,
> 	xfs_fileoff		off)
> {
> 	unsigned int		rounding = xfs_inode_alloc_fsbsize(ip);
> 
> 	if (rounding == 1)
> 		return off;
> 	return roundup_64(off, rounding);
> }
> 
> Then that callsite can be:
> 
> 	end_fsb = xfs_inode_roundup_alloc_unit(ip,
> 			XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip)));


Thanks,
John
Darrick J. Wong Aug. 7, 2024, 3:12 p.m. UTC | #3 
On Wed, Aug 07, 2024 at 01:33:49PM +0100, John Garry wrote:
> > > +void
> > > +xfs_roundout_to_alloc_fsbsize(
> > > +	struct xfs_inode	*ip,
> > > +	xfs_fileoff_t		*start,
> > > +	xfs_fileoff_t		*end)
> > > +{
> > > +	unsigned int		blocks = xfs_inode_alloc_fsbsize(ip);
> > > +
> > > +	if (blocks == 1)
> > > +		return;
> > > +	*start = rounddown_64(*start, blocks);
> > > +	*end = roundup_64(*end, blocks);
> > > +}
> > 
> > This is probably going to start another round of shouting, but I think
> > it's silly to do two rounding operations when you only care about one
> > value.
> 
> Sure, but the "in" version does use the 2x values and I wanted to be
> consistent. Anyway, I really don't feel strongly about this.
> 
> > In patch 12 it results in a bunch more dummy variables that you
> > then ignore.
> > 
> > Can't this be:
> > 
> > static inline xfs_fileoff_t
> > xfs_inode_rounddown_alloc_unit(
> 
> Just a question about the naming:
> xfs_inode_alloc_unitsize() returns bytes, so I would expect
> xfs_inode_rounddown_alloc_unit() to deal in bytes. Would you be satisfied
> with xfs_rounddown_alloc_fsbsize()? Or any other suggestion?

xfs_fileoff_t is the unit for file logical blocks, no need to append
stuff to the name.  It's clear that this function takes a file block
offset and returns another one.  If we need a second function for file
byte offsets then we can add another function and maybe wrap the whole
mess in _Generic.

--D

> > 	struct xfs_inode	*ip,
> > 	xfs_fileoff		off)
> > {
> > 	unsigned int		rounding = xfs_inode_alloc_fsbsize(ip);
> > 
> > 	if (rounding == 1)
> > 		return off;
> > 	return rounddown_64(off, rounding);
> > }
> > 
> > static inline xfs_fileoff_t
> > xfs_inode_roundup_alloc_unit(
> > 	struct xfs_inode	*ip,
> > 	xfs_fileoff		off)
> > {
> > 	unsigned int		rounding = xfs_inode_alloc_fsbsize(ip);
> > 
> > 	if (rounding == 1)
> > 		return off;
> > 	return roundup_64(off, rounding);
> > }
> > 
> > Then that callsite can be:
> > 
> > 	end_fsb = xfs_inode_roundup_alloc_unit(ip,
> > 			XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip)));
> 
> 
> Thanks,
> John
>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index fe2e2c930975..60389ac8bd45 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -496,6 +496,7 @@  xfs_can_free_eofblocks(
 	struct xfs_mount	*mp = ip->i_mount;
 	xfs_fileoff_t		end_fsb;
 	xfs_fileoff_t		last_fsb;
+	xfs_fileoff_t		dummy_fsb;
 	int			nimaps = 1;
 	int			error;
 
@@ -537,8 +538,10 @@  xfs_can_free_eofblocks(
 	 * forever.
 	 */
 	end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip));
-	if (xfs_inode_has_bigrtalloc(ip))
-		end_fsb = xfs_rtb_roundup_rtx(mp, end_fsb);
+
+	/* Only try to free beyond the allocation unit that crosses EOF */
+	xfs_roundout_to_alloc_fsbsize(ip, &dummy_fsb, &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 5af12f35062d..d765dedebc15 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3129,6 +3129,20 @@  xfs_inode_alloc_unitsize(
 	return XFS_FSB_TO_B(ip->i_mount, xfs_inode_alloc_fsbsize(ip));
 }
 
+void
+xfs_roundout_to_alloc_fsbsize(
+	struct xfs_inode	*ip,
+	xfs_fileoff_t		*start,
+	xfs_fileoff_t		*end)
+{
+	unsigned int		blocks = xfs_inode_alloc_fsbsize(ip);
+
+	if (blocks == 1)
+		return;
+	*start = rounddown_64(*start, blocks);
+	*end = roundup_64(*end, blocks);
+}
+
 /* Should we always be using copy on write for file writes? */
 bool
 xfs_is_always_cow_inode(
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 158afad8c7a4..7f86c4781bd8 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -643,6 +643,8 @@  void xfs_inode_count_blocks(struct xfs_trans *tp, struct xfs_inode *ip,
 		xfs_filblks_t *dblocks, xfs_filblks_t *rblocks);
 unsigned int xfs_inode_alloc_fsbsize(struct xfs_inode *ip);
 unsigned int xfs_inode_alloc_unitsize(struct xfs_inode *ip);
+void xfs_roundout_to_alloc_fsbsize(struct xfs_inode *ip,
+		xfs_fileoff_t *start, xfs_fileoff_t *end);
 
 int xfs_icreate_dqalloc(const struct xfs_icreate_args *args,
 		struct xfs_dquot **udqpp, struct xfs_dquot **gdqpp,