Hi,


* On Wed, Sep 12, 2012 at 09:21:44AM +1000, Dave Chinner <david@fromorbit.com> wrote:
>On Wed, Sep 12, 2012 at 03:43:24AM +0530, raghu.prabhu13@gmail.com wrote:
>> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>
>> When xfs_dialloc is unable to allocate required number of inodes or there are no
>> AGs with free inodes, printk the error in ratelimited manner.
>>
>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>> ---
>>  fs/xfs/xfs_ialloc.c | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
>> index e75a39d..034131b 100644
>> --- a/fs/xfs/xfs_ialloc.c
>> +++ b/fs/xfs/xfs_ialloc.c
>> @@ -990,8 +990,11 @@ xfs_dialloc(
>>  				goto out_error;
>>
>>  			xfs_perag_put(pag);
>> -			*inop = NULLFSINO;
>> -			return 0;
>> +
>> +			xfs_err_ratelimited(mp,
>> +				"Unable to allocate inodes in AG %d: Required %d, Current %llu, Maximum %llu",
>> +				agno, XFS_IALLOC_INODES(mp), mp->m_sb.sb_icount, mp->m_maxicount);
>> +			goto out_spc;
>
>This changes the error to be returned from 0 to ENOSPC. Adding error
>messages shouldn't change the logic of the code.

1) Yes, there is a confusion regarding that.  

>
>Also, you might want tolook at how ENOSPC is returned from
>xfs_ialloc_ag_alloc(). it only occurs when:
>
>	if (mp->m_maxicount &&
>            mp->m_sb.sb_icount + newlen > mp->m_maxicount) {
>
>i.e. it is exactly the same error case as the "noroom" error below.
>It has nothing to do with being unable to allocate inodes in the
>specific AG - the global inode count is too high. IOWs, the error
>message is not correct.

Now, in xfs_dialloc 


	if (mp->m_maxicount &&
	    mp->m_sb.sb_icount + XFS_IALLOC_INODES(mp) > mp->m_maxicount) {
		noroom = 1;
		okalloc = 0;
	}

why does it not make sense to bail out with ENOSPC then itself? I 
mean, what is the point of the loop when there is no room 
(noroom=1) and no allocations are allowed (okalloc = 0), also 
since the xfs_ialloc_ag_alloc in the loop uses same global logic 
to return.



>
>Also, 80 columns.
>
>>  		}
>>
>>  		if (ialloced) {
>> @@ -1016,11 +1019,19 @@ nextag:
>>  		if (++agno == mp->m_sb.sb_agcount)
>>  			agno = 0;
>>  		if (agno == start_agno) {
>> -			*inop = NULLFSINO;
>> -			return noroom ? ENOSPC : 0;
>> +			if (noroom) {
>> +				xfs_err_ratelimited(mp,
>> +					"Out of AGs with free inodes: Required %d, Current %llu, Maximum %llu",
>> +					 XFS_IALLOC_INODES(mp), mp->m_sb.sb_icount, mp->m_maxicount);
>
>The error message here is misleading - the error is that we've
>exceeded the maximum inode count for the filesystem (same as the
>above error message case), so no further allocations are allowed.
>
>What about the !noroom case? Isn't that a real ENOSPC condition?
>i.e. we've tried to allocate inodes in every AG and yet we've failed
>in all of them because there is no aligned, contiguous free space in
>any of the AGs. Shouldn't that emit an appropriate warning?

The warning is already emitted in call to xfs_ialloc_ag_select. 

Now, what does inop = NULLFSINO, noroom = 0 and return value of 0 mean, 

from the call chain of xfs_dir_ialloc -> xfs_ialloc -> 
xfs_dialloc 

I see that, it is a true ENOSPC only if both the buffer pointed 
by ialloc_context and the inode are NULL or there is an error 
returned, in former case (noroom=0)  xfs_dir_ialloc retries the 
allocation (ie.  when AGI buffer is non-NULL).

Now, in case of global inode exhaustion, it is hard error which 
can be fixed only by remounting with inode64 and nothing else 
will do, hence, I think ENOSPC must be returned as error instead 
of 0. (also in case of point #1 above)

So, are the assumptions made above correct?

>
>> +				goto out_spc;
>> +			}
>> +			return 0;
>>  		}
>>  	}
>>
>> +out_spc:
>> +	*inop = NULLFSINO;
>> +	return ENOSPC;
>>  out_alloc:
>>  	*IO_agbp = NULL;
>>  	return xfs_dialloc_ag(tp, agbp, parent, inop);
>
>Default behaviour on a loop break is to allocate inodes, not return
>ENOSPC.
>
>BTW, there's no need to cc LKML for XFS specific patches. LKML is
>noisy enough as it is without unnecessary cross-posts....
>
>Cheers,
>
>Dave.
>-- 
>Dave Chinner
>david@fromorbit.com
>




Regards,
-- 
Raghavendra Prabhu
GPG Id : 0xD72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
www: wnohang.net