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

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

[Eric Sandeen]

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.  ;)

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

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index f317488..7c7a810 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,24 @@ 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 */
 	fakeinos = statp->f_bfree << sbp->sb_inopblog;
+	/* Total possible files is current inodes + potential new inodes */
 	statp->f_files =
 	    MIN(sbp->sb_icount + fakeinos, (__uint64_t)XFS_MAXINUMBER);
+	/* Unless we have maxicount!  Then cap it at that */
 	if (mp->m_maxicount)
 		statp->f_files = min_t(typeof(statp->f_files),
 					statp->f_files,
 					mp->m_maxicount);
 
-	/* make sure statp->f_ffree does not underflow */
-	ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);
-	statp->f_ffree = max_t(__int64_t, ffree, 0);
+	/* But if we already managed to allocate more, let's be honest */
+	statp->f_files = max_t(typeof(statp->f_files),
+				sbp->sb_icount,
+				statp->f_files);
+
+	statp->f_ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);
 
 	spin_unlock(&mp->m_sb_lock);
 

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

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

[Brian Foster]

On Thu, Feb 20, 2014 at 04:12:16PM -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.  ;)
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index f317488..7c7a810 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,24 @@ 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 */
>  	fakeinos = statp->f_bfree << sbp->sb_inopblog;
> +	/* Total possible files is current inodes + potential new inodes */
>  	statp->f_files =
>  	    MIN(sbp->sb_icount + fakeinos, (__uint64_t)XFS_MAXINUMBER);
> +	/* Unless we have maxicount!  Then cap it at that */
>  	if (mp->m_maxicount)
>  		statp->f_files = min_t(typeof(statp->f_files),
>  					statp->f_files,
>  					mp->m_maxicount);
>  
> -	/* make sure statp->f_ffree does not underflow */
> -	ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);
> -	statp->f_ffree = max_t(__int64_t, ffree, 0);
> +	/* But if we already managed to allocate more, let's be honest */
> +	statp->f_files = max_t(typeof(statp->f_files),
> +				sbp->sb_icount,
> +				statp->f_files);
> +
> +	statp->f_ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);

Hi Eric,

Would something like the following be equivalent (and accurate?):

        /*
         * Potential number of new inodes in free blocks, limited by maxicount.
         */
        fakeinos = statp->f_bfree << sbp->sb_inopblog;
        if (mp->m_maxicount)
                fakeinos = mp->m_maxicount > sbp->sb_icount ?
                        MIN(mp->m_maxicount - sbp->sb_icount, fakeinos) : 0;

        /* Total possible files is current inodes + potential new inodes */
        statp->f_files = MIN(sbp->sb_icount + fakeinos,
                                (__uint64_t) XFS_MAXINUMBER);

        statp->f_ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);

Brian

>  
>  	spin_unlock(&mp->m_sb_lock);
>  
> 
> _______________________________________________
> 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: be honest about used inodes in statfs

[Eric Sandeen]

On 2/24/14, 10:01 AM, Brian Foster wrote:
> On Thu, Feb 20, 2014 at 04:12:16PM -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.  ;)
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index f317488..7c7a810 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,24 @@ 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 */
>>  	fakeinos = statp->f_bfree << sbp->sb_inopblog;
>> +	/* Total possible files is current inodes + potential new inodes */
>>  	statp->f_files =
>>  	    MIN(sbp->sb_icount + fakeinos, (__uint64_t)XFS_MAXINUMBER);
>> +	/* Unless we have maxicount!  Then cap it at that */
>>  	if (mp->m_maxicount)
>>  		statp->f_files = min_t(typeof(statp->f_files),
>>  					statp->f_files,
>>  					mp->m_maxicount);
>>  
>> -	/* make sure statp->f_ffree does not underflow */
>> -	ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);
>> -	statp->f_ffree = max_t(__int64_t, ffree, 0);
>> +	/* But if we already managed to allocate more, let's be honest */
>> +	statp->f_files = max_t(typeof(statp->f_files),
>> +				sbp->sb_icount,
>> +				statp->f_files);
>> +
>> +	statp->f_ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);
> 
> Hi Eric,
> 
> Would something like the following be equivalent (and accurate?):
> 
>         /*
>          * Potential number of new inodes in free blocks, limited by maxicount.
>          */
>         fakeinos = statp->f_bfree << sbp->sb_inopblog;
>         if (mp->m_maxicount)
>                 fakeinos = mp->m_maxicount > sbp->sb_icount ?
>                         MIN(mp->m_maxicount - sbp->sb_icount, fakeinos) : 0;
>         /* Total possible files is current inodes + potential new inodes */
>         statp->f_files = MIN(sbp->sb_icount + fakeinos,
>                                 (__uint64_t) XFS_MAXINUMBER);
> 
>         statp->f_ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);

Yeah, I think that's better.  Want to send that patch?  You did the hard work of
making it look sane.  ;)

-Eric

> Brian
> 
>>  
>>  	spin_unlock(&mp->m_sb_lock);
>>  
>>
>> _______________________________________________
>> 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
> 
	

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

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

[Eric Sandeen]

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;
-	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;
+
+	/* Total possible files is current inodes + potential new inodes */
+	statp->f_files = MIN(sbp->sb_icount + fakeinos,
+			     (__uint64_t) XFS_MAXINUMBER);
 
-	/* make sure statp->f_ffree does not underflow */
-	ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);
-	statp->f_ffree = max_t(__int64_t, ffree, 0);
+	statp->f_ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);
 
 	spin_unlock(&mp->m_sb_lock);
 

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

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

[Brian Foster]

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>
> ---
> 

Heh, looks good to me. I don't know if the LMRB tag disqualifies me as a
reviewer now ;), but feel free to replace it with:

Reviewed-by: Brian Foster <bfoster@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;
> -	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;
> +
> +	/* Total possible files is current inodes + potential new inodes */
> +	statp->f_files = MIN(sbp->sb_icount + fakeinos,
> +			     (__uint64_t) XFS_MAXINUMBER);
>  
> -	/* make sure statp->f_ffree does not underflow */
> -	ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);
> -	statp->f_ffree = max_t(__int64_t, ffree, 0);
> +	statp->f_ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);
>  
>  	spin_unlock(&mp->m_sb_lock);
>  
> 
> _______________________________________________
> 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 V2] xfs: be honest about used inodes in statfs

[Dave Chinner]

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?

> -	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().

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;


> +
> +	/* 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....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Eric Sandeen]

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

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

[Eric Sandeen]

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.  

Also rework code & rename vars for clarity after input from
dchinner & bfoster.

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

V2: code cleanup thanks to Brian
V3: more cleanup thanks to Dave

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index f317488..2f80923 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1081,9 +1081,8 @@ xfs_fs_statfs(
 	struct xfs_mount	*mp = XFS_M(dentry->d_sb);
 	xfs_sb_t		*sbp = &mp->m_sb;
 	struct xfs_inode	*ip = XFS_I(dentry->d_inode);
-	__uint64_t		fakeinos, id;
+	__uint64_t		potential, id;
 	xfs_extlen_t		lsize;
-	__int64_t		ffree;
 
 	statp->f_type = XFS_SB_MAGIC;
 	statp->f_namelen = MAXNAMELEN - 1;
@@ -1100,17 +1099,17 @@ xfs_fs_statfs(
 	statp->f_blocks = sbp->sb_dblocks - lsize;
 	statp->f_bfree = statp->f_bavail =
 				sbp->sb_fdblocks - XFS_ALLOC_SET_ASIDE(mp);
-	fakeinos = statp->f_bfree << sbp->sb_inopblog;
-	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);
-
-	/* make sure statp->f_ffree does not underflow */
-	ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);
-	statp->f_ffree = max_t(__int64_t, ffree, 0);
+
+	/* Potential new inodes in free blocks, limited by maxicount */
+	potential = statp->f_bfree << sbp->sb_inopblog;
+	if (mp->m_maxicount > sbp->sb_icount)
+		potential = min(mp->m_maxicount - sbp->sb_icount, potential);
+	else
+		potential = 0;
+
+	/* Total possible files is current inodes + potential new inodes */
+	statp->f_files = min_t(u64, sbp->sb_icount + potential, XFS_MAXINUMBER);
+	statp->f_ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);
 
 	spin_unlock(&mp->m_sb_lock);
 

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

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

[Eric Sandeen]

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.  

Also rework code & rename vars for clarity after input from
dchinner & bfoster.

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

V2: code cleanup thanks to Brian
V3: more cleanup thanks to Dave
V4: Oh for crying out loud... (add maxicount test in else)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index f317488..02537f4 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1081,9 +1081,8 @@ xfs_fs_statfs(
 	struct xfs_mount	*mp = XFS_M(dentry->d_sb);
 	xfs_sb_t		*sbp = &mp->m_sb;
 	struct xfs_inode	*ip = XFS_I(dentry->d_inode);
-	__uint64_t		fakeinos, id;
+	__uint64_t		potential, id;
 	xfs_extlen_t		lsize;
-	__int64_t		ffree;
 
 	statp->f_type = XFS_SB_MAGIC;
 	statp->f_namelen = MAXNAMELEN - 1;
@@ -1100,17 +1099,17 @@ xfs_fs_statfs(
 	statp->f_blocks = sbp->sb_dblocks - lsize;
 	statp->f_bfree = statp->f_bavail =
 				sbp->sb_fdblocks - XFS_ALLOC_SET_ASIDE(mp);
-	fakeinos = statp->f_bfree << sbp->sb_inopblog;
-	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);
-
-	/* make sure statp->f_ffree does not underflow */
-	ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);
-	statp->f_ffree = max_t(__int64_t, ffree, 0);
+
+	/* Potential new inodes in free blocks, limited by maxicount */
+	potential = statp->f_bfree << sbp->sb_inopblog;
+	if (mp->m_maxicount > sbp->sb_icount)
+		potential = min(mp->m_maxicount - sbp->sb_icount, potential);
+	else if (mp->m_maxicount)
+		potential = 0;
+
+	/* Total possible files is current inodes + potential new inodes */
+	statp->f_files = min_t(u64, sbp->sb_icount + potential, XFS_MAXINUMBER);
+	statp->f_ffree = statp->f_files - (sbp->sb_icount - sbp->sb_ifree);
 
 	spin_unlock(&mp->m_sb_lock);
 


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

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

[Dave Chinner]

On Mon, Feb 24, 2014 at 06:27:35PM -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.  
> 
> Also rework code & rename vars for clarity after input from
> dchinner & bfoster.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> V2: code cleanup thanks to Brian
> V3: more cleanup thanks to Dave
> V4: Oh for crying out loud... (add maxicount test in else)

Looks good now!

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

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