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);
+}
+
/* 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.1On 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
>
>
>> +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
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
>
© 2016 - 2024 Red Hat, Inc.