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

Mingming Cao
2006-09-05T21:52:25Z

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.