Re: [PATCH v5 10/11] xfs: make the calculation generic in xfs_sb_validate_fsb_count()

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, May 07, 2024 at 09:40:58AM +0100, John Garry wrote:
> On 03/05/2024 10:53, Luis Chamberlain wrote:
> > From: Pankaj Raghav <p.raghav@samsung.com>
> > 
> > Instead of assuming that PAGE_SHIFT is always higher than the blocklog,
> > make the calculation generic so that page cache count can be calculated
> > correctly for LBS.
> > 
> > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >   fs/xfs/xfs_mount.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index df370eb5dc15..56d71282972a 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -133,9 +133,16 @@ xfs_sb_validate_fsb_count(
> >   {
> >   	ASSERT(PAGE_SHIFT >= sbp->sb_blocklog);
> >   	ASSERT(sbp->sb_blocklog >= BBSHIFT);
> > +	uint64_t max_index;
> > +	uint64_t max_bytes;

Extra nit: the  ^ indentation of the names should have tabs, like the
other xfs functions.

--D

> nit: any other XFS code which I have seen puts the declarations before any
> ASSERT() calls
> 
> > +
> > +	if (check_shl_overflow(nblocks, sbp->sb_blocklog, &max_bytes))
> > +		return -EFBIG;
> >   	/* Limited by ULONG_MAX of page cache index */
> > -	if (nblocks >> (PAGE_SHIFT - sbp->sb_blocklog) > ULONG_MAX)
> > +	max_index = max_bytes >> PAGE_SHIFT;
> > +
> > +	if (max_index > ULONG_MAX)
> >   		return -EFBIG;
> >   	return 0;
> >   }
> 
>