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.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/