linux-kernel - Re: [PATCH v3 10/14] xfs: Do not free EOF blocks 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]

Message-ID: <20240807151254.GG6051@frogsfrogsfrogs>
Date: Wed, 7 Aug 2024 08:12:54 -0700
From: "Darrick J. Wong" <djwong@...nel.org>
To: John Garry <john.g.garry@...cle.com>
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 10/14] xfs: Do not free EOF blocks for forcealign

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
>