For when forcealign is enabled, blocks in an inode need to be unmapped
according to extent alignment, like what is already done for rtvol.
Change variable isrt in __xfs_bunmapi() to a bool, as that is really what
it is.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/libxfs/xfs_bmap.c | 48 +++++++++++++++++++++++++++++-----------
fs/xfs/xfs_inode.c | 16 ++++++++++++++
fs/xfs/xfs_inode.h | 2 ++
3 files changed, 53 insertions(+), 13 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 0c3df8c71c6d..d6ae344a17fc 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5409,6 +5409,25 @@ xfs_bmap_del_extent_real(
return 0;
}
+static xfs_extlen_t
+xfs_bunmapi_align(
+ struct xfs_inode *ip,
+ xfs_fsblock_t fsbno,
+ xfs_extlen_t *off)
+{
+ struct xfs_mount *mp = ip->i_mount;
+
+ if (XFS_IS_REALTIME_INODE(ip))
+ return xfs_inode_alloc_fsbsize_align(ip, fsbno, off);
+ /*
+ * The agbno for the fsbno is aligned to extsize, but the fsbno itself
+ * is not necessarily aligned (to extsize), so use agbno to determine
+ * mod+offset to the alloc unit boundary.
+ */
+ return xfs_inode_alloc_fsbsize_align(ip, XFS_FSB_TO_AGBNO(mp, fsbno),
+ off);
+}
+
/*
* Unmap (remove) blocks from a file.
* If nexts is nonzero then the number of extents to remove is limited to
@@ -5430,7 +5449,8 @@ __xfs_bunmapi(
xfs_extnum_t extno; /* extent number in list */
struct xfs_bmbt_irec got; /* current extent record */
struct xfs_ifork *ifp; /* inode fork pointer */
- int isrt; /* freeing in rt area */
+ bool isrt; /* freeing in rt area */
+ bool isforcealign; /* forcealign inode */
int logflags; /* transaction logging flags */
xfs_extlen_t mod; /* rt extent offset */
struct xfs_mount *mp = ip->i_mount;
@@ -5468,6 +5488,8 @@ __xfs_bunmapi(
}
XFS_STATS_INC(mp, xs_blk_unmap);
isrt = xfs_ifork_is_realtime(ip, whichfork);
+ isforcealign = (whichfork != XFS_ATTR_FORK) &&
+ xfs_inode_has_forcealign(ip);
end = start + len;
if (!xfs_iext_lookup_extent_before(ip, ifp, &end, &icur, &got)) {
@@ -5486,6 +5508,8 @@ __xfs_bunmapi(
extno = 0;
while (end != (xfs_fileoff_t)-1 && end >= start &&
(nexts == 0 || extno < nexts)) {
+ xfs_extlen_t off;
+
/*
* Is the found extent after a hole in which end lives?
* Just back up to the previous extent, if so.
@@ -5519,18 +5543,18 @@ __xfs_bunmapi(
if (del.br_startoff + del.br_blockcount > end + 1)
del.br_blockcount = end + 1 - del.br_startoff;
- if (!isrt || (flags & XFS_BMAPI_REMAP))
+ if ((!isrt && !isforcealign) || (flags & XFS_BMAPI_REMAP))
goto delete;
- mod = xfs_rtb_to_rtxoff(mp,
- del.br_startblock + del.br_blockcount);
+ mod = xfs_bunmapi_align(ip,
+ del.br_startblock + del.br_blockcount, &off);
if (mod) {
/*
- * Realtime extent not lined up at the end.
+ * Not aligned to allocation unit on the end.
* The extent could have been split into written
* and unwritten pieces, or we could just be
* unmapping part of it. But we can't really
- * get rid of part of a realtime extent.
+ * get rid of part of an extent.
*/
if (del.br_state == XFS_EXT_UNWRITTEN) {
/*
@@ -5554,7 +5578,7 @@ __xfs_bunmapi(
ASSERT(del.br_state == XFS_EXT_NORM);
ASSERT(tp->t_blk_res > 0);
/*
- * If this spans a realtime extent boundary,
+ * If this spans an extent boundary,
* chop it back to the start of the one we end at.
*/
if (del.br_blockcount > mod) {
@@ -5571,14 +5595,12 @@ __xfs_bunmapi(
goto nodelete;
}
- mod = xfs_rtb_to_rtxoff(mp, del.br_startblock);
+ mod = xfs_bunmapi_align(ip, del.br_startblock, &off);
if (mod) {
- xfs_extlen_t off = mp->m_sb.sb_rextsize - mod;
-
/*
- * Realtime extent is lined up at the end but not
- * at the front. We'll get rid of full extents if
- * we can.
+ * Extent is lined up to the allocation unit at the
+ * end but not at the front. We'll get rid of full
+ * extents if we can.
*/
if (del.br_blockcount > off) {
del.br_blockcount -= off;
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index e7fa155fcbde..bb8abf990186 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -3164,3 +3164,19 @@ xfs_is_always_cow_inode(
{
return ip->i_mount->m_always_cow && xfs_has_reflink(ip->i_mount);
}
+
+/* Return mod+offset for a blkno to an extent boundary */
+xfs_extlen_t
+xfs_inode_alloc_fsbsize_align(
+ struct xfs_inode *ip,
+ xfs_fileoff_t blkno,
+ xfs_extlen_t *off)
+{
+ xfs_fileoff_t blkno_start = blkno;
+ xfs_fileoff_t blkno_end = blkno;
+
+ xfs_roundout_to_alloc_fsbsize(ip, &blkno_start, &blkno_end);
+
+ *off = blkno_end - blkno;
+ return blkno - blkno_start;
+}
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 6dd8055c98b3..7b77797c3943 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -647,6 +647,8 @@ void xfs_roundout_to_alloc_fsbsize(struct xfs_inode *ip,
xfs_fileoff_t *start, xfs_fileoff_t *end);
void xfs_roundin_to_alloc_fsbsize(struct xfs_inode *ip,
xfs_fileoff_t *start, xfs_fileoff_t *end);
+xfs_extlen_t xfs_inode_alloc_fsbsize_align(struct xfs_inode *ip,
+ xfs_fileoff_t blkno, xfs_extlen_t *off);
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:55PM +0000, John Garry wrote:
> For when forcealign is enabled, blocks in an inode need to be unmapped
> according to extent alignment, like what is already done for rtvol.
>
> Change variable isrt in __xfs_bunmapi() to a bool, as that is really what
> it is.
>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 48 +++++++++++++++++++++++++++++-----------
> fs/xfs/xfs_inode.c | 16 ++++++++++++++
> fs/xfs/xfs_inode.h | 2 ++
> 3 files changed, 53 insertions(+), 13 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 0c3df8c71c6d..d6ae344a17fc 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5409,6 +5409,25 @@ xfs_bmap_del_extent_real(
> return 0;
> }
>
> +static xfs_extlen_t
> +xfs_bunmapi_align(
> + struct xfs_inode *ip,
> + xfs_fsblock_t fsbno,
> + xfs_extlen_t *off)
> +{
> + struct xfs_mount *mp = ip->i_mount;
> +
> + if (XFS_IS_REALTIME_INODE(ip))
> + return xfs_inode_alloc_fsbsize_align(ip, fsbno, off);
> + /*
> + * The agbno for the fsbno is aligned to extsize, but the fsbno itself
> + * is not necessarily aligned (to extsize), so use agbno to determine
> + * mod+offset to the alloc unit boundary.
> + */
> + return xfs_inode_alloc_fsbsize_align(ip, XFS_FSB_TO_AGBNO(mp, fsbno),
> + off);
> +}
> +
> /*
> * Unmap (remove) blocks from a file.
> * If nexts is nonzero then the number of extents to remove is limited to
> @@ -5430,7 +5449,8 @@ __xfs_bunmapi(
> xfs_extnum_t extno; /* extent number in list */
> struct xfs_bmbt_irec got; /* current extent record */
> struct xfs_ifork *ifp; /* inode fork pointer */
> - int isrt; /* freeing in rt area */
> + bool isrt; /* freeing in rt area */
> + bool isforcealign; /* forcealign inode */
> int logflags; /* transaction logging flags */
> xfs_extlen_t mod; /* rt extent offset */
> struct xfs_mount *mp = ip->i_mount;
> @@ -5468,6 +5488,8 @@ __xfs_bunmapi(
> }
> XFS_STATS_INC(mp, xs_blk_unmap);
> isrt = xfs_ifork_is_realtime(ip, whichfork);
> + isforcealign = (whichfork != XFS_ATTR_FORK) &&
> + xfs_inode_has_forcealign(ip);
> end = start + len;
>
> if (!xfs_iext_lookup_extent_before(ip, ifp, &end, &icur, &got)) {
> @@ -5486,6 +5508,8 @@ __xfs_bunmapi(
> extno = 0;
> while (end != (xfs_fileoff_t)-1 && end >= start &&
> (nexts == 0 || extno < nexts)) {
> + xfs_extlen_t off;
I got really confused because I thought this was a file block offset and
only after more digging realized that this is a sometimes dummy
adjustment variable.
> +
> /*
> * Is the found extent after a hole in which end lives?
> * Just back up to the previous extent, if so.
> @@ -5519,18 +5543,18 @@ __xfs_bunmapi(
> if (del.br_startoff + del.br_blockcount > end + 1)
> del.br_blockcount = end + 1 - del.br_startoff;
>
> - if (!isrt || (flags & XFS_BMAPI_REMAP))
> + if ((!isrt && !isforcealign) || (flags & XFS_BMAPI_REMAP))
> goto delete;
>
> - mod = xfs_rtb_to_rtxoff(mp,
> - del.br_startblock + del.br_blockcount);
> + mod = xfs_bunmapi_align(ip,
> + del.br_startblock + del.br_blockcount, &off);
> if (mod) {
Oof. I don't like how this loop body has the rtx adjustment code
inlined into it. We only use the isrt flag for the one test above.
I tried hoisting this into something less gross involving separate
adjustment functions but then you have to pass in so many outer
variables that it becomes a mess.
The best I can come up with for now is:
unsigned int alloc_fsb = xfs_inode_alloc_fsbsize(ip);
/* no more isrt/isforcealign bools */
...
if (alloc_fsb == 1 || (flags & XFS_BMAPI_REMAP))
goto delete;
mod = do_div(del.br_startblock + del.br_blockcount,
alloc_fsb);
if (mod) {
> /*
> - * Realtime extent not lined up at the end.
> + * Not aligned to allocation unit on the end.
> * The extent could have been split into written
> * and unwritten pieces, or we could just be
> * unmapping part of it. But we can't really
> - * get rid of part of a realtime extent.
> + * get rid of part of an extent.
> */
> if (del.br_state == XFS_EXT_UNWRITTEN) {
> /*
> @@ -5554,7 +5578,7 @@ __xfs_bunmapi(
> ASSERT(del.br_state == XFS_EXT_NORM);
> ASSERT(tp->t_blk_res > 0);
> /*
> - * If this spans a realtime extent boundary,
> + * If this spans an extent boundary,
> * chop it back to the start of the one we end at.
> */
> if (del.br_blockcount > mod) {
> @@ -5571,14 +5595,12 @@ __xfs_bunmapi(
> goto nodelete;
> }
>
> - mod = xfs_rtb_to_rtxoff(mp, del.br_startblock);
> + mod = xfs_bunmapi_align(ip, del.br_startblock, &off);
> if (mod) {
> - xfs_extlen_t off = mp->m_sb.sb_rextsize - mod;
mod = do_div(del.br_startblock, alloc_fsb);
if (mod) {
xfs_extlen_t off = alloc_fsb - mod;
At least then you don't need this weird xfs_inode_alloc_fsbsize_align
that passes back two xfs_extlen_t arguments.
--D
> -
> /*
> - * Realtime extent is lined up at the end but not
> - * at the front. We'll get rid of full extents if
> - * we can.
> + * Extent is lined up to the allocation unit at the
> + * end but not at the front. We'll get rid of full
> + * extents if we can.
> */
> if (del.br_blockcount > off) {
> del.br_blockcount -= off;
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index e7fa155fcbde..bb8abf990186 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3164,3 +3164,19 @@ xfs_is_always_cow_inode(
> {
> return ip->i_mount->m_always_cow && xfs_has_reflink(ip->i_mount);
> }
> +
> +/* Return mod+offset for a blkno to an extent boundary */
> +xfs_extlen_t
> +xfs_inode_alloc_fsbsize_align(
> + struct xfs_inode *ip,
> + xfs_fileoff_t blkno,
> + xfs_extlen_t *off)
> +{
> + xfs_fileoff_t blkno_start = blkno;
> + xfs_fileoff_t blkno_end = blkno;
> +
> + xfs_roundout_to_alloc_fsbsize(ip, &blkno_start, &blkno_end);
> +
> + *off = blkno_end - blkno;
> + return blkno - blkno_start;
> +}
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 6dd8055c98b3..7b77797c3943 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -647,6 +647,8 @@ void xfs_roundout_to_alloc_fsbsize(struct xfs_inode *ip,
> xfs_fileoff_t *start, xfs_fileoff_t *end);
> void xfs_roundin_to_alloc_fsbsize(struct xfs_inode *ip,
> xfs_fileoff_t *start, xfs_fileoff_t *end);
> +xfs_extlen_t xfs_inode_alloc_fsbsize_align(struct xfs_inode *ip,
> + xfs_fileoff_t blkno, xfs_extlen_t *off);
>
> int xfs_icreate_dqalloc(const struct xfs_icreate_args *args,
> struct xfs_dquot **udqpp, struct xfs_dquot **gdqpp,
> --
> 2.31.1
>
>
On 06/08/2024 21:14, Darrick J. Wong wrote:
> On Thu, Aug 01, 2024 at 04:30:55PM +0000, John Garry wrote:
>> For when forcealign is enabled, blocks in an inode need to be unmapped
>> according to extent alignment, like what is already done for rtvol.
>>
>> Change variable isrt in __xfs_bunmapi() to a bool, as that is really what
>> it is.
>>
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>> fs/xfs/libxfs/xfs_bmap.c | 48 +++++++++++++++++++++++++++++-----------
>> fs/xfs/xfs_inode.c | 16 ++++++++++++++
>> fs/xfs/xfs_inode.h | 2 ++
>> 3 files changed, 53 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
>> index 0c3df8c71c6d..d6ae344a17fc 100644
>> --- a/fs/xfs/libxfs/xfs_bmap.c
>> +++ b/fs/xfs/libxfs/xfs_bmap.c
>> @@ -5409,6 +5409,25 @@ xfs_bmap_del_extent_real(
>> return 0;
>> }
>>
>> +static xfs_extlen_t
>> +xfs_bunmapi_align(
>> + struct xfs_inode *ip,
>> + xfs_fsblock_t fsbno,
>> + xfs_extlen_t *off)
>> +{
>> + struct xfs_mount *mp = ip->i_mount;
>> +
>> + if (XFS_IS_REALTIME_INODE(ip))
>> + return xfs_inode_alloc_fsbsize_align(ip, fsbno, off);
>> + /*
>> + * The agbno for the fsbno is aligned to extsize, but the fsbno itself
>> + * is not necessarily aligned (to extsize), so use agbno to determine
>> + * mod+offset to the alloc unit boundary.
>> + */
>> + return xfs_inode_alloc_fsbsize_align(ip, XFS_FSB_TO_AGBNO(mp, fsbno),
>> + off);
>> +}
>> +
>> /*
>> * Unmap (remove) blocks from a file.
>> * If nexts is nonzero then the number of extents to remove is limited to
>> @@ -5430,7 +5449,8 @@ __xfs_bunmapi(
>> xfs_extnum_t extno; /* extent number in list */
>> struct xfs_bmbt_irec got; /* current extent record */
>> struct xfs_ifork *ifp; /* inode fork pointer */
>> - int isrt; /* freeing in rt area */
>> + bool isrt; /* freeing in rt area */
>> + bool isforcealign; /* forcealign inode */
>> int logflags; /* transaction logging flags */
>> xfs_extlen_t mod; /* rt extent offset */
>> struct xfs_mount *mp = ip->i_mount;
>> @@ -5468,6 +5488,8 @@ __xfs_bunmapi(
>> }
>> XFS_STATS_INC(mp, xs_blk_unmap);
>> isrt = xfs_ifork_is_realtime(ip, whichfork);
>> + isforcealign = (whichfork != XFS_ATTR_FORK) &&
>> + xfs_inode_has_forcealign(ip);
>> end = start + len;
>>
>> if (!xfs_iext_lookup_extent_before(ip, ifp, &end, &icur, &got)) {
>> @@ -5486,6 +5508,8 @@ __xfs_bunmapi(
>> extno = 0;
>> while (end != (xfs_fileoff_t)-1 && end >= start &&
>> (nexts == 0 || extno < nexts)) {
>> + xfs_extlen_t off;
>
> I got really confused because I thought this was a file block offset and
> only after more digging realized that this is a sometimes dummy
> adjustment variable.
yeah, I put it here to use the common helper in both callsites
>
>> +
>> /*
>> * Is the found extent after a hole in which end lives?
>> * Just back up to the previous extent, if so.
>> @@ -5519,18 +5543,18 @@ __xfs_bunmapi(
>> if (del.br_startoff + del.br_blockcount > end + 1)
>> del.br_blockcount = end + 1 - del.br_startoff;
>>
>> - if (!isrt || (flags & XFS_BMAPI_REMAP))
>> + if ((!isrt && !isforcealign) || (flags & XFS_BMAPI_REMAP))
>> goto delete;
>>
>> - mod = xfs_rtb_to_rtxoff(mp,
>> - del.br_startblock + del.br_blockcount);
>> + mod = xfs_bunmapi_align(ip,
>> + del.br_startblock + del.br_blockcount, &off);
>> if (mod) {
>
> Oof. I don't like how this loop body has the rtx adjustment code
> inlined into it. We only use the isrt flag for the one test above.
> I tried hoisting this into something less gross involving separate
> adjustment functions but then you have to pass in so many outer
> variables that it becomes a mess.
>
> The best I can come up with for now is:
>
> unsigned int alloc_fsb = xfs_inode_alloc_fsbsize(ip);
> /* no more isrt/isforcealign bools */
>
> ...
>
> if (alloc_fsb == 1 || (flags & XFS_BMAPI_REMAP))
> goto delete;
ok, good
>
> mod = do_div(del.br_startblock + del.br_blockcount,
> alloc_fsb);
Note that xfs_bunmapi_align() uses agbno for !rt
> if (mod) {
>
>> /*
>> - * Realtime extent not lined up at the end.
>> + * Not aligned to allocation unit on the end.
>> * The extent could have been split into written
>> * and unwritten pieces, or we could just be
>> * unmapping part of it. But we can't really
>> - * get rid of part of a realtime extent.
>> + * get rid of part of an extent.
>> */
>> if (del.br_state == XFS_EXT_UNWRITTEN) {
>> /*
>> @@ -5554,7 +5578,7 @@ __xfs_bunmapi(
>> ASSERT(del.br_state == XFS_EXT_NORM);
>> ASSERT(tp->t_blk_res > 0);
>> /*
>> - * If this spans a realtime extent boundary,
>> + * If this spans an extent boundary,
>> * chop it back to the start of the one we end at.
>> */
>> if (del.br_blockcount > mod) {
>> @@ -5571,14 +5595,12 @@ __xfs_bunmapi(
>> goto nodelete;
>> }
>>
>> - mod = xfs_rtb_to_rtxoff(mp, del.br_startblock);
>> + mod = xfs_bunmapi_align(ip, del.br_startblock, &off);
>> if (mod) {
>> - xfs_extlen_t off = mp->m_sb.sb_rextsize - mod;
>
> mod = do_div(del.br_startblock, alloc_fsb);
> if (mod) {
> xfs_extlen_t off = alloc_fsb - mod;
>
> At least then you don't need this weird xfs_inode_alloc_fsbsize_align
> that passes back two xfs_extlen_t arguments.
Sure, but same point about xfs_bunmapi_align() using agbno. I suppose I
can just make the change to not have xfs_bunmapi_align() be passed the
off address pointer, and do the calcation here for off here (as you
suggest).
Thanks,
John
On Wed, Aug 07, 2024 at 02:40:36PM +0100, John Garry wrote:
> On 06/08/2024 21:14, Darrick J. Wong wrote:
> > On Thu, Aug 01, 2024 at 04:30:55PM +0000, John Garry wrote:
> > > For when forcealign is enabled, blocks in an inode need to be unmapped
> > > according to extent alignment, like what is already done for rtvol.
> > >
> > > Change variable isrt in __xfs_bunmapi() to a bool, as that is really what
> > > it is.
> > >
> > > Signed-off-by: John Garry <john.g.garry@oracle.com>
> > > ---
> > > fs/xfs/libxfs/xfs_bmap.c | 48 +++++++++++++++++++++++++++++-----------
> > > fs/xfs/xfs_inode.c | 16 ++++++++++++++
> > > fs/xfs/xfs_inode.h | 2 ++
> > > 3 files changed, 53 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > > index 0c3df8c71c6d..d6ae344a17fc 100644
> > > --- a/fs/xfs/libxfs/xfs_bmap.c
> > > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > > @@ -5409,6 +5409,25 @@ xfs_bmap_del_extent_real(
> > > return 0;
> > > }
> > > +static xfs_extlen_t
> > > +xfs_bunmapi_align(
> > > + struct xfs_inode *ip,
> > > + xfs_fsblock_t fsbno,
> > > + xfs_extlen_t *off)
> > > +{
> > > + struct xfs_mount *mp = ip->i_mount;
> > > +
> > > + if (XFS_IS_REALTIME_INODE(ip))
> > > + return xfs_inode_alloc_fsbsize_align(ip, fsbno, off);
> > > + /*
> > > + * The agbno for the fsbno is aligned to extsize, but the fsbno itself
> > > + * is not necessarily aligned (to extsize), so use agbno to determine
> > > + * mod+offset to the alloc unit boundary.
> > > + */
> > > + return xfs_inode_alloc_fsbsize_align(ip, XFS_FSB_TO_AGBNO(mp, fsbno),
> > > + off);
> > > +}
> > > +
> > > /*
> > > * Unmap (remove) blocks from a file.
> > > * If nexts is nonzero then the number of extents to remove is limited to
> > > @@ -5430,7 +5449,8 @@ __xfs_bunmapi(
> > > xfs_extnum_t extno; /* extent number in list */
> > > struct xfs_bmbt_irec got; /* current extent record */
> > > struct xfs_ifork *ifp; /* inode fork pointer */
> > > - int isrt; /* freeing in rt area */
> > > + bool isrt; /* freeing in rt area */
> > > + bool isforcealign; /* forcealign inode */
> > > int logflags; /* transaction logging flags */
> > > xfs_extlen_t mod; /* rt extent offset */
> > > struct xfs_mount *mp = ip->i_mount;
> > > @@ -5468,6 +5488,8 @@ __xfs_bunmapi(
> > > }
> > > XFS_STATS_INC(mp, xs_blk_unmap);
> > > isrt = xfs_ifork_is_realtime(ip, whichfork);
> > > + isforcealign = (whichfork != XFS_ATTR_FORK) &&
> > > + xfs_inode_has_forcealign(ip);
> > > end = start + len;
> > > if (!xfs_iext_lookup_extent_before(ip, ifp, &end, &icur, &got)) {
> > > @@ -5486,6 +5508,8 @@ __xfs_bunmapi(
> > > extno = 0;
> > > while (end != (xfs_fileoff_t)-1 && end >= start &&
> > > (nexts == 0 || extno < nexts)) {
> > > + xfs_extlen_t off;
> >
> > I got really confused because I thought this was a file block offset and
> > only after more digging realized that this is a sometimes dummy
> > adjustment variable.
>
> yeah, I put it here to use the common helper in both callsites
>
> >
> > > +
> > > /*
> > > * Is the found extent after a hole in which end lives?
> > > * Just back up to the previous extent, if so.
> > > @@ -5519,18 +5543,18 @@ __xfs_bunmapi(
> > > if (del.br_startoff + del.br_blockcount > end + 1)
> > > del.br_blockcount = end + 1 - del.br_startoff;
> > > - if (!isrt || (flags & XFS_BMAPI_REMAP))
> > > + if ((!isrt && !isforcealign) || (flags & XFS_BMAPI_REMAP))
> > > goto delete;
> > > - mod = xfs_rtb_to_rtxoff(mp,
> > > - del.br_startblock + del.br_blockcount);
> > > + mod = xfs_bunmapi_align(ip,
> > > + del.br_startblock + del.br_blockcount, &off);
> > > if (mod) {
> >
> > Oof. I don't like how this loop body has the rtx adjustment code
> > inlined into it. We only use the isrt flag for the one test above.
> > I tried hoisting this into something less gross involving separate
> > adjustment functions but then you have to pass in so many outer
> > variables that it becomes a mess.
> >
> > The best I can come up with for now is:
> >
> > unsigned int alloc_fsb = xfs_inode_alloc_fsbsize(ip);
> > /* no more isrt/isforcealign bools */
> >
> > ...
> >
> > if (alloc_fsb == 1 || (flags & XFS_BMAPI_REMAP))
> > goto delete;
>
> ok, good
>
> >
> > mod = do_div(del.br_startblock + del.br_blockcount,
> > alloc_fsb);
>
> Note that xfs_bunmapi_align() uses agbno for !rt
Right, I forgot about that. Can we make the name say that we're
computing the offset within an allocunit? It's not actually aligning
any of its parameters.
static inline unsigned int
xfs_bmap_alloc_unit_offset(
struct xfs_inode *ip,
xfs_fsblock_t fsbno,
unsigned int alloc_fsb)
{
xfs_agblock_t agbno;
if (XFS_IS_REALTIME_INODE(ip))
return do_div(fsbno, alloc_fsb);
agbno = XFS_FSB_TO_AGBNO(ip->i_mount, fsbno);
return agbno % alloc_fsb;
}
mod = xfs_bmap_alloc_unit_offset(ip,
del.br_startblock + del.br_blockcount,
alloc_fsb);
and
mod = xfs_bmap_alloc_unit_offset(ip, del.br_startblock,
alloc_fsb);
--D
>
> > if (mod) {
> >
> > > /*
> > > - * Realtime extent not lined up at the end.
> > > + * Not aligned to allocation unit on the end.
> > > * The extent could have been split into written
> > > * and unwritten pieces, or we could just be
> > > * unmapping part of it. But we can't really
> > > - * get rid of part of a realtime extent.
> > > + * get rid of part of an extent.
> > > */
> > > if (del.br_state == XFS_EXT_UNWRITTEN) {
> > > /*
> > > @@ -5554,7 +5578,7 @@ __xfs_bunmapi(
> > > ASSERT(del.br_state == XFS_EXT_NORM);
> > > ASSERT(tp->t_blk_res > 0);
> > > /*
> > > - * If this spans a realtime extent boundary,
> > > + * If this spans an extent boundary,
> > > * chop it back to the start of the one we end at.
> > > */
> > > if (del.br_blockcount > mod) {
> > > @@ -5571,14 +5595,12 @@ __xfs_bunmapi(
> > > goto nodelete;
> > > }
> > > - mod = xfs_rtb_to_rtxoff(mp, del.br_startblock);
> > > + mod = xfs_bunmapi_align(ip, del.br_startblock, &off);
> > > if (mod) {
> > > - xfs_extlen_t off = mp->m_sb.sb_rextsize - mod;
> >
> > mod = do_div(del.br_startblock, alloc_fsb);
> > if (mod) {
> > xfs_extlen_t off = alloc_fsb - mod;
> >
> > At least then you don't need this weird xfs_inode_alloc_fsbsize_align
> > that passes back two xfs_extlen_t arguments.
>
> Sure, but same point about xfs_bunmapi_align() using agbno. I suppose I can
> just make the change to not have xfs_bunmapi_align() be passed the off
> address pointer, and do the calcation here for off here (as you suggest).
>
> Thanks,
> John
>
© 2016 - 2024 Red Hat, Inc.