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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux