Message-ID: <20080922084721.GA6691@skywalker>
Date:	Mon, 22 Sep 2008 14:17:21 +0530
From:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
To:	Frédéric Bohé <frederic.bohe@...l.net>
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

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.

-aneesh
--
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