xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH V2] xfs: be honest about used inodes in statfs
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Mon, 24 Feb 2014 18:02:40 -0600
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140224235614.GU13647@dastard>
References: <53067DC0.9040800@xxxxxxxxxx> <530BD167.2020600@xxxxxxxxxxx> <20140224235614.GU13647@dastard>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.3.0
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@xxxxxxxxxx>
>> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
>> ---
>>
>> 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.
> 

<Prev in Thread] Current Thread [Next in Thread>