Re: [PATCH V2] xfs: be honest about used inodes in statfs

From: Eric Sandeen <sandeen@sandeen.net>
To: Dave Chinner <david@fromorbit.com>
Cc: Eric Sandeen <sandeen@redhat.com>, xfs-oss <xfs@oss.sgi.com>
Subject: Re: [PATCH V2] xfs: be honest about used inodes in statfs
Date: Mon, 24 Feb 2014 18:02:40 -0600	[thread overview]
Message-ID: <530BDDA0.7080208@sandeen.net> (raw)
In-Reply-To: <20140224235614.GU13647@dastard>

On 2/24/14, 5:56 PM, Dave Chinner wrote:
> On Mon, Feb 24, 2014 at 05:10:31PM -0600, Eric Sandeen wrote:
>> Because we have lazy counters, it's possible that we over-allocate
>> inodes past the maxicount (imaxpct) limit.  
>>
>> A previous commit,
>>
>>  2fe3366 xfs: ensure f_ffree returned by statfs() is non-negative
>>
>> stopped statfs from underflowing f_ffree in this case, but that
>> only happened when we mis-reported f_files, capped at maxicount.
>>
>> Change statfs to report the actual number of inodes allocated,
>> even if it is greater than maxicount.  It's reality.
>> Deal with it.  
>>
>> (New clearer code flow thanks to Brian!)
>>
>> Logic-made-readable-by: Brian Foster <bfoster@redhat.com>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> V2: Use Brian's suggested logic for working out the numbers
>>
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index f317488..0dbcc17 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -1083,7 +1083,6 @@ xfs_fs_statfs(
>>  	struct xfs_inode	*ip = XFS_I(dentry->d_inode);
>>  	__uint64_t		fakeinos, id;
>>  	xfs_extlen_t		lsize;
>> -	__int64_t		ffree;
>>  
>>  	statp->f_type = XFS_SB_MAGIC;
>>  	statp->f_namelen = MAXNAMELEN - 1;
>> @@ -1100,17 +1099,19 @@ xfs_fs_statfs(
>>  	statp->f_blocks = sbp->sb_dblocks - lsize;
>>  	statp->f_bfree = statp->f_bavail =
>>  				sbp->sb_fdblocks - XFS_ALLOC_SET_ASIDE(mp);
>> +	/*
>> +	 * Potential number of new inodes in free blocks, limited by maxicount.
>> +	 */
>>  	fakeinos = statp->f_bfree << sbp->sb_inopblog;
> 
> Can we rename "fakeinos" to something like "free_inodes" so that
> the code reads a little bit better while we are touching this
> code?

yeah, I thought about that too.

>> -	statp->f_files =
>> -	    MIN(sbp->sb_icount + fakeinos, (__uint64_t)XFS_MAXINUMBER);
>>  	if (mp->m_maxicount)
>> -		statp->f_files = min_t(typeof(statp->f_files),
>> -					statp->f_files,
>> -					mp->m_maxicount);
>> +		fakeinos = mp->m_maxicount > sbp->sb_icount ?
>> +			   MIN(mp->m_maxicount - sbp->sb_icount, fakeinos) : 0;
> 
> Get rid of MIN - it should be min() or min_t().

they are the same types, but yeah, that's better.

> Also the mix of if() and ternary operations makes this difficult to
> follow the logic. Better, IMO, is this:
> 
> 	free_inodes = statp->f_bfree << sbp->sb_inopblog;
> 	if (mp->m_maxicount > sbp->sb_icount)
> 		free_inodes = min(mp->m_maxicount - sbp->sb_icount,
> 				  free_inodes);
> 	else if (mp->m_maxicount)
> 		free_inodes = 0;

ok good point.

> 
>> +
>> +	/* Total possible files is current inodes + potential new inodes */
>> +	statp->f_files = MIN(sbp->sb_icount + fakeinos,
>> +			     (__uint64_t) XFS_MAXINUMBER);
> 
> 	statp->f_files = min_t(u64, sbp->sb_icount + free_inodes,
> 				    XFS_MAXINUMBER);
> 
> And for bonus points: while we are looking at maxicount, the setting
> on maxicount in the growfs code should call xfs_set_maxicount()
> rather than open coding it, and xfs_set_maxicount() needs to be
> reworked to prevent overflow when sbp->sb_dblocks * sbp->sb_imax_pct
> is greater than 64 bits....

Ok.  Well, that's a different patch I think.

I'll also have to invent another tag for you, Dave.

Nitpicked-by: perhaps.  ;)  (I kid! I kid!)

-Eric

> Cheers,
> 
> Dave.
> 

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