[PATCH] xfs: clean up xfs_set_maxicount & use in growfs

[PATCH] xfs: clean up xfs_set_maxicount & use in growfs

[Eric Sandeen]

xfs_set_maxicount() overflowed fairly easily for large filesystems
and large maxicount; we started out by multiplying dblocks by
the percentage, *then* dividing by 100, and never checked for
an overflow.  The calculations were also, IMHO, a little hard
to follow.

I rewrote it using mult_frac (so handy!) and rounddown, to
do a nice no-overflow, no-precision-loss multiplication by
(%/100), then round down to the inode allocation unit.
Then check to see if we've overflowed the u64, and set to
XFS_MAXINUMBER if so.

Also, growfs open-coded the setting of maxicount; call the
handy helper instead.

This slightly changes growfs behavior, because we weren't
rounding to the allocation multiple in that code.  I checked
that we get the same answers for non-growfs cases by running
100 mkfs.xfs's with imaxpct from 1 to 100, and it gives the
same inode count in all cases.

Thanks to dchinner for pointing out that this could use fixing.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Ok Brian, have at it.  ;)


diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 02fb943..867e476 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -471,12 +471,7 @@ xfs_growfs_data_private(
 	/* New allocation groups fully initialized, so update mount struct */
 	if (nagimax)
 		mp->m_maxagi = nagimax;
-	if (mp->m_sb.sb_imax_pct) {
-		__uint64_t icount = mp->m_sb.sb_dblocks * mp->m_sb.sb_imax_pct;
-		do_div(icount, 100);
-		mp->m_maxicount = icount << mp->m_sb.sb_inopblog;
-	} else
-		mp->m_maxicount = 0;
+	xfs_set_maxicount(mp);
 	xfs_set_low_space_thresholds(mp);
 
 	/* update secondary superblocks. */
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 02df7b4..0ec2b19 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -419,24 +419,24 @@ xfs_update_alignment(xfs_mount_t *mp)
 }
 
 /*
- * Set the maximum inode count for this filesystem
+ * Set the maximum inode count for this filesystem based on sb_imax_pct
  */
-STATIC void
+void
 xfs_set_maxicount(xfs_mount_t *mp)
 {
 	xfs_sb_t	*sbp = &(mp->m_sb);
-	__uint64_t	icount;
+	__uint64_t	iblocks;
 
 	if (sbp->sb_imax_pct) {
 		/*
 		 * Make sure the maximum inode count is a multiple
 		 * of the units we allocate inodes in.
 		 */
-		icount = sbp->sb_dblocks * sbp->sb_imax_pct;
-		do_div(icount, 100);
-		do_div(icount, mp->m_ialloc_blks);
-		mp->m_maxicount = (icount * mp->m_ialloc_blks)  <<
-				   sbp->sb_inopblog;
+		iblocks = mult_frac(sbp->sb_dblocks, sbp->sb_imax_pct, 100);
+		iblocks = rounddown(iblocks, mp->m_ialloc_blks);
+		mp->m_maxicount = iblocks << sbp->sb_inopblog;
+		if (mp->m_maxicount < iblocks)
+			mp->m_maxicount = XFS_MAXINUMBER;
 	} else {
 		mp->m_maxicount = 0;
 	}
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index a466c5e..6288a56 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -388,6 +388,7 @@ extern int	xfs_mod_incore_sb_batch(xfs_mount_t *, xfs_mod_sb_t *,
 extern int	xfs_mount_log_sb(xfs_mount_t *, __int64_t);
 extern struct xfs_buf *xfs_getsb(xfs_mount_t *, int);
 extern int	xfs_readsb(xfs_mount_t *, int);
+void		xfs_set_maxicount(xfs_mount_t *mp);
 extern void	xfs_freesb(xfs_mount_t *);
 extern int	xfs_fs_writable(xfs_mount_t *);
 extern int	xfs_sb_validate_fsb_count(struct xfs_sb *, __uint64_t);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: clean up xfs_set_maxicount & use in growfs

[Christoph Hellwig]

On Mon, Feb 24, 2014 at 11:27:35PM -0600, Eric Sandeen wrote:
> xfs_set_maxicount() overflowed fairly easily for large filesystems
> and large maxicount; we started out by multiplying dblocks by
> the percentage, *then* dividing by 100, and never checked for
> an overflow.  The calculations were also, IMHO, a little hard
> to follow.

Would be useful to get this test case into xfstests..

> -STATIC void
> +void
>  xfs_set_maxicount(xfs_mount_t *mp)
>  {
>  	xfs_sb_t	*sbp = &(mp->m_sb);
> -	__uint64_t	icount;
> +	__uint64_t	iblocks;

Seems like this could move into the if clause below.

> @@ -388,6 +388,7 @@ extern int	xfs_mod_incore_sb_batch(xfs_mount_t *, xfs_mod_sb_t *,
>  extern int	xfs_mount_log_sb(xfs_mount_t *, __int64_t);
>  extern struct xfs_buf *xfs_getsb(xfs_mount_t *, int);
>  extern int	xfs_readsb(xfs_mount_t *, int);
> +void		xfs_set_maxicount(xfs_mount_t *mp);
>  extern void	xfs_freesb(xfs_mount_t *);
>  extern int	xfs_fs_writable(xfs_mount_t *);
>  extern int	xfs_sb_validate_fsb_count(struct xfs_sb *, __uint64_t);

A missing extern while all other prototypes around it have one seems
rather odd.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: clean up xfs_set_maxicount & use in growfs

[Eric Sandeen]

On 2/25/14, 8:11 PM, Christoph Hellwig wrote:
> On Mon, Feb 24, 2014 at 11:27:35PM -0600, Eric Sandeen wrote:
>> xfs_set_maxicount() overflowed fairly easily for large filesystems
>> and large maxicount; we started out by multiplying dblocks by
>> the percentage, *then* dividing by 100, and never checked for
>> an overflow.  The calculations were also, IMHO, a little hard
>> to follow.
> 
> Would be useful to get this test case into xfstests..

Your'e bot, aren't you.  ;)

yeah you're right.  I went down this path by starting a testcase
for 3-patches-ago, and got here.  That one can be sent soon, and
sure, it'd be worth doing this too.

>> -STATIC void
>> +void
>>  xfs_set_maxicount(xfs_mount_t *mp)
>>  {
>>  	xfs_sb_t	*sbp = &(mp->m_sb);
>> -	__uint64_t	icount;
>> +	__uint64_t	iblocks;
> 
> Seems like this could move into the if clause below.
> 
>> @@ -388,6 +388,7 @@ extern int	xfs_mod_incore_sb_batch(xfs_mount_t *, xfs_mod_sb_t *,
>>  extern int	xfs_mount_log_sb(xfs_mount_t *, __int64_t);
>>  extern struct xfs_buf *xfs_getsb(xfs_mount_t *, int);
>>  extern int	xfs_readsb(xfs_mount_t *, int);
>> +void		xfs_set_maxicount(xfs_mount_t *mp);
>>  extern void	xfs_freesb(xfs_mount_t *);
>>  extern int	xfs_fs_writable(xfs_mount_t *);
>>  extern int	xfs_sb_validate_fsb_count(struct xfs_sb *, __uint64_t);
> 
> A missing extern while all other prototypes around it have one seems
> rather odd.

Sure does!

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: clean up xfs_set_maxicount & use in growfs

[Eric Sandeen]

On 2/25/14, 8:11 PM, Christoph Hellwig wrote:
> On Mon, Feb 24, 2014 at 11:27:35PM -0600, Eric Sandeen wrote:
>> xfs_set_maxicount() overflowed fairly easily for large filesystems
>> and large maxicount; we started out by multiplying dblocks by
>> the percentage, *then* dividing by 100, and never checked for
>> an overflow.  The calculations were also, IMHO, a little hard
>> to follow.
> 
> Would be useful to get this test case into xfstests..

Ok so I was going on Dave's assertion about that.  ;)

To overflow, we'd need dblocks * 100 to be > 2^64-1:

so dblocks would need to be > (2^64-1)/100

for 4k blocks that's 655 exabytes.  Maybe not so possible after all ;)

Dave, maybe just removing the open-code is enough here.

-Eric

>> -STATIC void
>> +void
>>  xfs_set_maxicount(xfs_mount_t *mp)
>>  {
>>  	xfs_sb_t	*sbp = &(mp->m_sb);
>> -	__uint64_t	icount;
>> +	__uint64_t	iblocks;
> 
> Seems like this could move into the if clause below.
> 
>> @@ -388,6 +388,7 @@ extern int	xfs_mod_incore_sb_batch(xfs_mount_t *, xfs_mod_sb_t *,
>>  extern int	xfs_mount_log_sb(xfs_mount_t *, __int64_t);
>>  extern struct xfs_buf *xfs_getsb(xfs_mount_t *, int);
>>  extern int	xfs_readsb(xfs_mount_t *, int);
>> +void		xfs_set_maxicount(xfs_mount_t *mp);
>>  extern void	xfs_freesb(xfs_mount_t *);
>>  extern int	xfs_fs_writable(xfs_mount_t *);
>>  extern int	xfs_sb_validate_fsb_count(struct xfs_sb *, __uint64_t);
> 
> A missing extern while all other prototypes around it have one seems
> rather odd.
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: clean up xfs_set_maxicount & use in growfs

[Dave Chinner]

On Wed, Feb 26, 2014 at 12:29:22PM -0600, Eric Sandeen wrote:
> On 2/25/14, 8:11 PM, Christoph Hellwig wrote:
> > On Mon, Feb 24, 2014 at 11:27:35PM -0600, Eric Sandeen wrote:
> >> xfs_set_maxicount() overflowed fairly easily for large filesystems
> >> and large maxicount; we started out by multiplying dblocks by
> >> the percentage, *then* dividing by 100, and never checked for
> >> an overflow.  The calculations were also, IMHO, a little hard
> >> to follow.
> > 
> > Would be useful to get this test case into xfstests..
> 
> Ok so I was going on Dave's assertion about that.  ;)
> 
> To overflow, we'd need dblocks * 100 to be > 2^64-1:
> 
> so dblocks would need to be > (2^64-1)/100
> 
> for 4k blocks that's 655 exabytes.  Maybe not so possible after all ;)

Until the block count is corrupted by fsfuzzer? ;)

> Dave, maybe just removing the open-code is enough here.

Sure, but I still like the conversion to use mult_frac....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: clean up xfs_set_maxicount & use in growfs

[Eric Sandeen]

On 2/27/14, 1:11 AM, Dave Chinner wrote:
> On Wed, Feb 26, 2014 at 12:29:22PM -0600, Eric Sandeen wrote:
>> On 2/25/14, 8:11 PM, Christoph Hellwig wrote:
>>> On Mon, Feb 24, 2014 at 11:27:35PM -0600, Eric Sandeen wrote:
>>>> xfs_set_maxicount() overflowed fairly easily for large filesystems
>>>> and large maxicount; we started out by multiplying dblocks by
>>>> the percentage, *then* dividing by 100, and never checked for
>>>> an overflow.  The calculations were also, IMHO, a little hard
>>>> to follow.
>>>
>>> Would be useful to get this test case into xfstests..
>>
>> Ok so I was going on Dave's assertion about that.  ;)
>>
>> To overflow, we'd need dblocks * 100 to be > 2^64-1:
>>
>> so dblocks would need to be > (2^64-1)/100
>>
>> for 4k blocks that's 655 exabytes.  Maybe not so possible after all ;)
> 
> Until the block count is corrupted by fsfuzzer? ;)
> 
>> Dave, maybe just removing the open-code is enough here.
> 
> Sure, but I still like the conversion to use mult_frac....

Ok, I do too.  Just a bit resistant to fixing what ain't
broke.  I'll see if I can write up a test to be sure we're
doing sane things statfs/growfs with the change.

-Eric

> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs