| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5e4e6fdb-03fe-4541-8f09-8300665551a2@oracle.com>
Date: Wed, 7 Aug 2024 14:40:36 +0100
From: John Garry <john.g.garry@...cle.com>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: chandan.babu@...cle.com, dchinner@...hat.com, hch@....de,
viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz,
linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-fsdevel@...r.kernel.org, catherine.hoang@...cle.com,
martin.petersen@...cle.com
Subject: Re: [PATCH v3 12/14] xfs: Unmap blocks according to forcealign
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@...cle.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
Powered by blists - more mailing lists