Message-Id: <1222075960.3581.33.camel@frecb007923.frec.bull.fr>
Date:	Mon, 22 Sep 2008 11:32:40 +0200
From:	Frédéric Bohé <frederic.bohe@...l.net>
To:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
Cc:	Theodore Tso <tytso@....edu>,
	"linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH v2] ext4: fix initialization of UNINIT bitmap blocks

Le lundi 22 septembre 2008 à 14:17 +0530, Aneesh Kumar K.V a écrit :
> On Mon, Sep 22, 2008 at 10:09:57AM +0200, Frédéric Bohé wrote:
> > Le samedi 20 septembre 2008 à 20:44 -0400, Theodore Tso a écrit :
> > > On Thu, Sep 18, 2008 at 03:45:14PM +0200, Frédéric Bohé wrote:
> > > > The issue here is that you can't use all inode of the second group of
> > > > the fs.
> > > > 
> > > > This happens because resize2fs make a call to ext2fs_read_bitmaps. This
> > > > function reads all bitmaps while paying attention not to read the
> > > > uninited bitmap. This works well as long as the fs block size is equal
> > > > to the page size. But in the above test case, the fs use 1k blocks and
> > > > we have an issue. 
> > > > 
> > > > That's because the "read" function issued by ext2fs_read_bitmaps is a
> > > > call to kernel's block_read_full_page function. So when a single bitmap
> > > > block is asked for, 4 blocks (for 1k blocks fs on x86) are actually read
> > > > (including the uninited ones) and their respective buffer set to
> > > > uptodate. 
> > > > 
> > > > As we rely on the buffer's uptodate flags to initialize or not this
> > > > buffer, it may happen that certain bitmap blocks are not initialized at
> > > > all. So their buffer contains the random garbage that was present on the
> > > > disk prior to the mkfs ( In the above test case, the inode bitmap of the
> > > > second group is full a random bits so I can't use all of its inodes ).
> > > 
> > > Actually that's the problem.  We shouldn't be relying on the buffer's
> > > uptodate flags as a hint to tell mballoc to reload the buddy bitmaps.
> > > Unfortunately I didn't notice this problem by not carefully auditing
> > > commit 5f21b0e6 before it went in, but it's seriously buggy by trying
> > > to overload the use of the buffer's uptodate flag for anything other
> > > than error handling.
> > > 
> > 
> > Maybe I missed something, but I thought the bug I am talking about here,
> > is neither related to buddy nor directly to mballoc. Sorry, I was not
> > clear enough. In fact, it happens even without using mballoc. It is
> > related to uninit feature with filesystems using blocks which are
> > smaller than page size. If any userland process call ext2fs_read_bitmaps
> > function (or try to read a bitmap block directly), you may end up with
> > those buffers full of garbage. It concerns either block bitmap buffers
> > or inode bitmap buffers.
> > 
> > 
> > 
> > > > I am a bit lost on how to fix this. Aneesh was right, I think it's an
> > > > ext2fs_read_bitmaps bug, not a kernel bug. I guess we need a userland
> > > > function to read a single block whatever the block size and page size
> > > > are. I've made a try using O_DIRECT flag but I was unsuccessful. Any
> > > > ideas/suggestions ?
> > > 
> > > No!!!!  Think about it.  It's always fair for userspace to read from
> > > the block device.  If this causes the kernel to blow up, then it's a
> > > kernel bug, not a userspace bug.  And it is a *perfect* demonstration
> > > why overloading the uptodate flag by using it for *anything* other
> > > than error signalling from the buffer I/O layer is wrong and horribly
> > > fragile.
> > 
> > You are probably right, so maybe the patch I sent at the beginning of
> > this thread makes sense ?
> > 
> 
> What you can do is make ext4_group_info generic for both mballoc and
> oldalloc. We can then add bg_flag to the in memory ext4_group_info
> that would indicate whether the group is initialized or not. Here
> initialized for an UNINIT_GROUP indicate we have done
> ext4_init_block_bitmap on the buffer_head. Then 
> instead of depending on the buffer_head uptodate flag we can check
> for the ext4_group_info bg_flags and decided whether the block/inode
> bitmap need to be initialized.
> 

That makes sense ! I agree with you, we need an additional in-memory
flag to know whether buffers are initialized or not. Anyway, making
ext4_group_info generic will lead to unneeded memory consumption for
oldalloc. Maybe a simple independent bits array could do the trick. Is
there any advantage to re-use ext4_group_info ?




--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists