Re: BUG: warning at fs/ext3/inode.c:1016/ext3_getblk()

[Mingming Cao <cmm@us.ibm.com>]

Dave Kleikamp wrote:
> On Tue, 2006-09-05 at 13:14 -0700, Badari Pulavarty wrote:
> 
>>Dave Kleikamp wrote:
> 
> 
>>>I'm having a hard time figuring out exactly what ext3_get_blocks_handle
>>>is trying to return, but it looks to me like if it is allocating one
>>>data block, and needs to allocate an indirect block as well, then it
>>>will return 2 rather than 1.  Is this expected, or am I just confused?
>>>
>>>  
>>
>>It would return "1" in that case.. (data block)
>>
>> > 0 means get_block() suceeded and indicates the number of blocks mapped
>>= 0 lookup failed
>>< 0 mean error case
> 
> 
> Okay, I got confused looking through the code.  I still don't see how
> ext3_get_blocks_handle() should be returning a number greater than
> maxblocks.
> 

yes ext3_get_blocks_handle() will return the number of data blocks 
allocated(not including the indirect/double indirecto blocks), and that 
number should not than maxblocks. In this case, it should return 1 instead.

The ext3_get_blocks_handle() behavior was changed when multiple blocks 
map/allocation was added to ext3 via this function. Previously, the 
behavior of ext3_get_blokc_handle() returns 0 for success case, and 
returns non-zero(nagive) for error case. While with new behavior, the 
success case is the thre returned value should > 0.

How many blocks is being mapped in this case? > 1? or 0? If it failed to 
map the block (ext3_get_blocks_handle() returns 0), ext3_getblk() will 
also WARN_ON().

> 
>>>>I did search for callers of ext3_get_blocks_handle() and found that
>>>>ext3_readdir() seems to do wrong thing all the time with error check :(
>>>>Need to take a closer look..
>>>>
>>>>	err = ext3_get_blocks_handle(NULL, inode, blk, 1,
>>>>                                                &map_bh, 0, 0);
>>>>        if (err > 0) {  <<<< BAD
>>>>                  page_cache_readahead(sb->s_bdev->bd_inode->i_mapping,
>>>>                                &filp->f_ra,
>>>>                                filp,
>>>>                                map_bh.b_blocknr >>
>>>>                                (PAGE_CACHE_SHIFT - inode->i_blkbits),
>>>>                                1);
>>>>                        bh = ext3_bread(NULL, inode, blk, 0, &err);
>>>>       }
>>>>    
>>>
>>>Bad to do what it's doing, or bad to call name the variable "err"?
>>>I think if it looked like this:
>>>
>>>	count = ext3_get_blocks_handle(NULL, inode, blk, 1,
>>>                                                &map_bh, 0, 0);
>>>        if (count > 0) { 
>>>
>>>it would be a lot less confusing.
>>>  
>>
>>I am sorry !! it is doing the right thing :(
> 
> 
> Not your fault.  The variable is very badly named.