linux-kernel - Re: [PATCH RFC v3 12/21] xfs: Only free full extents for 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]

Date: Wed, 1 May 2024 10:53:28 +1000
From: Dave Chinner <david@...morbit.com>
To: John Garry <john.g.garry@...cle.com>
Cc: djwong@...nel.org, hch@....de, viro@...iv.linux.org.uk,
	brauner@...nel.org, jack@...e.cz, chandan.babu@...cle.com,
	willy@...radead.org, axboe@...nel.dk, martin.petersen@...cle.com,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	tytso@....edu, jbongio@...gle.com, ojaswin@...ux.ibm.com,
	ritesh.list@...il.com, mcgrof@...nel.org, p.raghav@...sung.com,
	linux-xfs@...r.kernel.org, catherine.hoang@...cle.com
Subject: Re: [PATCH RFC v3 12/21] xfs: Only free full extents for forcealign

On Mon, Apr 29, 2024 at 05:47:37PM +0000, John Garry wrote:
> Like we already do for rtvol, only free full extents for forcealign in
> xfs_free_file_space().
> 
> Signed-off-by: John Garry <john.g.garry@...cle.com>
> ---
>  fs/xfs/xfs_bmap_util.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index f26d1570b9bd..1dd45dfb2811 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -847,8 +847,11 @@ xfs_free_file_space(
>  	startoffset_fsb = XFS_B_TO_FSB(mp, offset);
>  	endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len);
>  
> -	/* We can only free complete realtime extents. */
> -	if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) {
> +	/* Free only complete extents. */
> +	if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1) {
> +		startoffset_fsb = roundup_64(startoffset_fsb, ip->i_extsize);
> +		endoffset_fsb = rounddown_64(endoffset_fsb, ip->i_extsize);
> +	} else if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) {
>  		startoffset_fsb = xfs_rtb_roundup_rtx(mp, startoffset_fsb);
>  		endoffset_fsb = xfs_rtb_rounddown_rtx(mp, endoffset_fsb);
>  	}

When you look at xfs_rtb_roundup_rtx() you'll find it's just a one
line wrapper around roundup_64().

So lets get rid of the obfuscation that the one line RT wrapper
introduces, and it turns into this:

	rounding = 1;
	if (xfs_inode_has_forcealign(ip)
		rounding = ip->i_extsize;
	else if (XFS_IS_REALTIME_INODE(ip))
		rounding = mp->m_sb.sb_rextsize;

	if (rounding > 1) {
		startoffset_fsb = roundup_64(startoffset_fsb, rounding);
		endoffset_fsb = rounddown_64(endoffset_fsb, rounding);
	}

What this points out is that the prep steps for fallocate operations
also need to handle both forced alignment and rtextsize rounding,
and it does neither right now.  xfs_flush_unmap_range() is the main
offender here, but xfs_prepare_shift() also needs fixing.

Hence:

static inline xfs_extlen_t
xfs_extent_alignment(
	struct xfs_inode	*ip)
{
	if (xfs_inode_has_forcealign(ip))
		return ip->i_extsize;
	if (XFS_IS_REALTIME_INODE(ip))
		return mp->m_sb.sb_rextsize;
	return 1;
}


In xfs_flush_unmap_range():

	/*
	 * Make sure we extend the flush out to extent alignment
	 * boundaries so any extent range overlapping the start/end
	 * of the modification we are about to do is clean and idle.
	 */
	rounding = XFS_FSB_TO_B(mp, xfs_extent_alignment(ip));
	rounding = max(rounding, PAGE_SIZE);
	...

in xfs_free_file_space()

	/*
	 * Round the range we are going to free inwards to extent
	 * alignment boundaries so we don't free blocks outside the
	 * range requested.
	 */
	rounding = xfs_extent_alignment(ip);
	if (rounding > 1 ) {
		startoffset_fsb = roundup_64(startoffset_fsb, rounding);
		endoffset_fsb = rounddown_64(endoffset_fsb, rounding);
	}

and in xfs_prepare_shift()

	/*
	 * Shift operations must stabilize the start block offset boundary along
	 * with the full range of the operation. If we don't, a COW writeback
	 * completion could race with an insert, front merge with the start
	 * extent (after split) during the shift and corrupt the file. Start
	 * with the aligned block just prior to the start to stabilize the boundary.
	 */
	rounding = XFS_FSB_TO_B(mp, xfs_extent_alignment(ip));
	offset = round_down(offset, rounding);
	if (offset)
		offset -= rounding;

Also, I think that the changes I suggested earlier to 
xfs_is_falloc_aligned() could use this xfs_extent_alignment()
helper...

Overall this makes the code a whole lot easier to read and it also
allows forced alignment to work correctly on RT devices...

-Dave.
-- 
Dave Chinner
david@...morbit.com