From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Kleikamp Subject: Re: BUG: warning at fs/ext3/inode.c:1016/ext3_getblk() Date: Tue, 05 Sep 2006 16:19:39 -0500 Message-ID: <1157491179.19432.11.camel@kleikamp.austin.ibm.com> References: <20060905171049.GB27433@ele.uri.edu> <1157479756.23501.18.camel@dyn9047017100.beaverton.ibm.com> <1157482632.19432.6.camel@kleikamp.austin.ibm.com> <44FDDA89.7080207@us.ibm.com> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Cc: Badari Pulavarty , Will Simoneau , lkml , ext4 Return-path: Received: from e3.ny.us.ibm.com ([32.97.182.143]:56212 "EHLO e3.ny.us.ibm.com") by vger.kernel.org with ESMTP id S1750792AbWIEVTm (ORCPT ); Tue, 5 Sep 2006 17:19:42 -0400 To: Badari Pulavarty In-Reply-To: <44FDDA89.7080207@us.ibm.com> Sender: linux-ext4-owner@vger.kernel.org List-Id: linux-ext4.vger.kernel.org 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. > >> 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. -- David Kleikamp IBM Linux Technology Center