linux-kernel - Re: [PATCH v2 10/13] xfs: Unmap blocks according to forcealign

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240706075858.GC15212@lst.de>
Date: Sat, 6 Jul 2024 09:58:58 +0200
From: Christoph Hellwig <hch@....de>
To: John Garry <john.g.garry@...cle.com>
Cc: chandan.babu@...cle.com, djwong@...nel.org, 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 v2 10/13] xfs: Unmap blocks according to forcealign

> +static xfs_extlen_t
> +xfs_bunmapi_align(
> +	struct xfs_inode	*ip,
> +	xfs_fsblock_t		bno)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	xfs_agblock_t		agbno;
> +
> +	if (xfs_inode_has_forcealign(ip)) {
> +		if (is_power_of_2(ip->i_extsize))
> +			return bno & (ip->i_extsize - 1);
> +
> +		agbno = XFS_FSB_TO_AGBNO(mp, bno);
> +		return agbno % ip->i_extsize;
> +	}
> +	ASSERT(XFS_IS_REALTIME_INODE(ip));
> +	return xfs_rtb_to_rtxoff(ip->i_mount, bno);

This helper isn't really bunmapi sepcific, is it?

> @@ -5425,6 +5444,7 @@ __xfs_bunmapi(
>  	struct xfs_bmbt_irec	got;		/* current extent record */
>  	struct xfs_ifork	*ifp;		/* inode fork pointer */
>  	int			isrt;		/* freeing in rt area */
> +	int			isforcealign;	/* freeing for inode with forcealign */

This is really a bool.  And while it matches the code around it the
code feels a bit too verbose..
> 
> +		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);

Overly long line.

We've been long wanting to split the whole align / convert unwritten /
etc code into a helper outside the main bumapi flow.  And when adding
new logic to it this might indeed be a good time.

> +			if (isforcealign) {
> +				off = ip->i_extsize - mod;
> +			} else {
> +				ASSERT(isrt);
> +				off = mp->m_sb.sb_rextsize - mod;
> +			}

And we'll really need proper helpers so that we don't have to
open code the i_extsize vs sb_rextsize logic all over.