Hi Ted,
I hit the below Oops with the latest patchqueue.
BUG: unable to handle kernel NULL pointer dereference at 00000000
IP: [<c04c0e39>] ext4_new_meta_blocks+0x7c/0xb7
*pdpt = 0000000011945001 *pde = 0000000000000000
....
....
EAX: da5dae60 EBX: e8c233e0 ECX: d4cb2000 EDX: 00000000
ESI: e8c23114 EDI: e8c2302c EBP: d4cb2b70 ESP: d4cb2b28
The problem is due to remove-do_blk_alloc patch.
The patch below should fix the crash.
diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index e950898..2dd1162 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -650,7 +650,7 @@ ext4_fsblk_t ext4_new_meta_blocks(handle_t *handle, struct inode *inode,
*/
if (!(*errp) && EXT4_I(inode)->i_delalloc_reserved_flag) {
spin_lock(&EXT4_I(inode)->i_block_reservation_lock);
- EXT4_I(inode)->i_allocated_meta_blocks += *count;
+ EXT4_I(inode)->i_allocated_meta_blocks += ar.len;
spin_unlock(&EXT4_I(inode)->i_block_reservation_lock);
}
return ret;
I have one question regarding the patch. What about blocks allocated for
directories for the ext3 format. With extent format we are not
setting EXT4_MB_HINT_DATA for non regular files. So i guess we also
need the below patch .
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 1647903..89aa870 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -600,7 +600,9 @@ static int ext4_alloc_blocks(handle_t *handle, struct inode *inode,
ar.goal = goal;
ar.len = target;
ar.logical = iblock;
- ar.flags = EXT4_MB_HINT_DATA;
+ if (S_ISREG(inode->i_mode))
+ /* enable in-core preallocation only for regular files */
+ ar.flags = EXT4_MB_HINT_DATA;
current_block = ext4_mb_new_blocks(handle, &ar, err);
On Tue, Dec 09, 2008 at 04:11:22PM +0530, Aneesh Kumar K.V wrote:
> The problem is due to remove-do_blk_alloc patch.
>
> The patch below should fix the crash.
>
> - EXT4_I(inode)->i_allocated_meta_blocks += *count;
> + EXT4_I(inode)->i_allocated_meta_blocks += ar.len;
Good catch, thanks. I'll add it to the patch queue.
> I have one question regarding the patch. What about blocks allocated for
> directories for the ext3 format. With extent format we are not
> setting EXT4_MB_HINT_DATA for non regular files. So i guess we also
> need the below patch .
One of the good things about getting rid of too many layers of
abstractions is that it makes bugs like this easier to spot. We've
been sending allocating directory and symlinks using EXT4_MB_HINT_DATA
if extents haven't been enabled, and no one noticed before we
simplified out things....
Actually, I wonder if maybe we should set EXT4_MB_HINT_DATA for
directories as well. Making directories contiguous does speed up
certain workloads, and it does speed up fsck. It may be though that
the mballoc algorithms should be tuned specifically for directories,
and what we should do is to define a new flag, EXT4_MB_HINT_DIRECTORY,
and pass it in for that case.
Some experimentation is clearly called for, here....
- Ted
On Fri, Dec 12, 2008 at 09:56:09AM -0500, Theodore Tso wrote:
> On Tue, Dec 09, 2008 at 04:11:22PM +0530, Aneesh Kumar K.V wrote:
> > The problem is due to remove-do_blk_alloc patch.
> >
> > The patch below should fix the crash.
> >
> > - EXT4_I(inode)->i_allocated_meta_blocks += *count;
> > + EXT4_I(inode)->i_allocated_meta_blocks += ar.len;
>
>
> Good catch, thanks. I'll add it to the patch queue.
>
> > I have one question regarding the patch. What about blocks allocated for
> > directories for the ext3 format. With extent format we are not
> > setting EXT4_MB_HINT_DATA for non regular files. So i guess we also
> > need the below patch .
>
> One of the good things about getting rid of too many layers of
> abstractions is that it makes bugs like this easier to spot. We've
> been sending allocating directory and symlinks using EXT4_MB_HINT_DATA
> if extents haven't been enabled, and no one noticed before we
> simplified out things....
We had always sent the directory allocation request with
EXT4_MB_HINT_DATA not set. With Linus kernel I have this
671 static ext4_fsblk_t do_blk_alloc(handle_t *handle, struct inode *inode,
.....
....
686 if (S_ISREG(inode->i_mode) && !(flags & EXT4_META_BLOCK))
687 /* enable in-core preallocation for data block allocation */
688 ar.flags = EXT4_MB_HINT_DATA;
689 else
690 /* disable in-core preallocation for non-regular files */
691 ar.flags = 0;
That means if the request for block allocation is not on regular files
set ar.flags = 0; For regular files if the request is for meta-data
blocks set ar.glags = 0.
>
> Actually, I wonder if maybe we should set EXT4_MB_HINT_DATA for
> directories as well. Making directories contiguous does speed up
> certain workloads, and it does speed up fsck. It may be though that
> the mballoc algorithms should be tuned specifically for directories,
> and what we should do is to define a new flag, EXT4_MB_HINT_DIRECTORY,
> and pass it in for that case.
>
> Some experimentation is clearly called for, here....
>
True. But with the changes to do do_blk_alloc I guess we need to make
sure we request directories with EXT4_MB_HINT_DATA not set.
-aneesh
On Wed, Dec 17, 2008 at 01:26:35PM +0530, Aneesh Kumar K.V wrote:
> > One of the good things about getting rid of too many layers of
> > abstractions is that it makes bugs like this easier to spot. We've
> > been sending allocating directory and symlinks using EXT4_MB_HINT_DATA
> > if extents haven't been enabled, and no one noticed before we
> > simplified out things....
>
> We had always sent the directory allocation request with
> EXT4_MB_HINT_DATA not set.
With extents, yes. With normal indirect block-based files, no. I
agree that for consistency's sake, it should be the same, but at the
moment, it isn't.
> > Actually, I wonder if maybe we should set EXT4_MB_HINT_DATA for
> > directories as well. Making directories contiguous does speed up
> > certain workloads, and it does speed up fsck. It may be though that
> > the mballoc algorithms should be tuned specifically for directories,
> > and what we should do is to define a new flag, EXT4_MB_HINT_DIRECTORY,
> > and pass it in for that case.
> >
>
> True. But with the changes to do do_blk_alloc I guess we need to make
> sure we request directories with EXT4_MB_HINT_DATA not set.
I've played with this a bit, and changing extents.c to pass in
EXT4_MB_HINT_DATA for directories does work, although it's a toss-up
regarding exactly how effective it really is. It does seem to reduce
fragmentation of directories, but I'm concerned that it might impact
the long-term performance of the filesystem as it ages.
My current thinking is that we should consider changing the block
allocation algorithms as follows:
1) Change the inode allocator to strongly avoid (unless no other
inodes are available) block groups where the block group number is a
even multiple of the flex blockgroup size. The reasoning behind this
is these bg's have a fewer number of blocks given that the inode table
blocks are all allocated there, so they are much more likely to
overflow into other bg's when used. So we should try to avoid these
bg's by the inode allocator unless there is no other choice.
2) Directory blocks for inodes in the flex bg metagroup should be
allocated in this first bg of the flexbg metagroup. This keeps the
filesystem metadata together, and keeps directory blocks (which tend
to be much longer-lived that data blocks, especially for source/build
directories) in different block allocation regions, which is a good
thing. It may be that all metadata blocks (i.e., also long symlinks
and extent-tree blocks) should also be located here, although that's
probably less important, simply because there are so few of such
blocks in most ext4 filesystems.
- Ted
On Wed, Dec 17, 2008 at 06:47:11AM -0500, Theodore Tso wrote:
> On Wed, Dec 17, 2008 at 01:26:35PM +0530, Aneesh Kumar K.V wrote:
> > > One of the good things about getting rid of too many layers of
> > > abstractions is that it makes bugs like this easier to spot. We've
> > > been sending allocating directory and symlinks using EXT4_MB_HINT_DATA
> > > if extents haven't been enabled, and no one noticed before we
> > > simplified out things....
> >
> > We had always sent the directory allocation request with
> > EXT4_MB_HINT_DATA not set.
>
> With extents, yes. With normal indirect block-based files, no. I
> agree that for consistency's sake, it should be the same, but at the
> moment, it isn't.
Hmm. May be I am missing something. This is the call chain i followed
with the Linus tree.
ext4_get_block
ext4_get_blocks_wrap
ext4_get_blocks_handle
ext4_alloc_branch
ext4_alloc_blocks
ext4_new_meta_blocks
do_blk_alloc -> which set ar.flags = 0 for meta data.
ext4_new_blocks
do_blk_alloc -> which set ar.flags = 0 for !S_ISREG(inode->i_mode)
So the current Linus kernel using mballoc for non extent format doesn't
set EXT4_MB_HINT_DATA for directories.
-aneesh
On Dec 17, 2008 06:47 -0500, Theodore Ts'o wrote:
> I've played with this a bit, and changing extents.c to pass in
> EXT4_MB_HINT_DATA for directories does work, although it's a toss-up
> regarding exactly how effective it really is. It does seem to reduce
> fragmentation of directories, but I'm concerned that it might impact
> the long-term performance of the filesystem as it ages.
How can reducing fragmentation of the directories hurt long-term performance?
> My current thinking is that we should consider changing the block
> allocation algorithms as follows:
>
> 1) Change the inode allocator to strongly avoid (unless no other
> inodes are available) block groups where the block group number is a
> even multiple of the flex blockgroup size. The reasoning behind this
> is these bg's have a fewer number of blocks given that the inode table
> blocks are all allocated there, so they are much more likely to
> overflow into other bg's when used. So we should try to avoid these
> bg's by the inode allocator unless there is no other choice.
With flex_bg does it really matter at all where the blocks for an inode
are located? There will ALWAYS be a seek from reading the inode until
the first data block is read, so I don't see any significance to whether
the inode's "group" has more free blocks or not.
> 2) Directory blocks for inodes in the flex bg metagroup should be
> allocated in this first bg of the flexbg metagroup. This keeps the
> filesystem metadata together, and keeps directory blocks (which tend
> to be much longer-lived that data blocks, especially for source/build
> directories) in different block allocation regions, which is a good
> thing. It may be that all metadata blocks (i.e., also long symlinks
> and extent-tree blocks) should also be located here, although that's
> probably less important, simply because there are so few of such
> blocks in most ext4 filesystems.
I do agree with this, and if (1) is just a mechanism to ensure that there
is space for (2) then I would tend to agree.
This would also allow implementation of my long-held idea of using LVM
to put some parts of the filesystem on one type of device (e.g. RAID-1
and/or SSD) for metadata, and the rest (data blocks) on RAID-5/6. I had
always thought of doing this with the first N of 128 MB for each group
on the fast storage.
Putting the first of each N whole groups on the fast storage would be
equivalent, and probably less work to configure. Having the allocator
also put other metadata there (index and directory blocks) is a bonus.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
On Tue, Dec 09, 2008 at 04:11:22PM +0530, Aneesh Kumar K.V wrote:
>
> I have one question regarding the patch. What about blocks allocated for
> directories for the ext3 format. With extent format we are not
> setting EXT4_MB_HINT_DATA for non regular files. So i guess we also
> need the below patch .
I've updated the remove-ext4_new_blocks to include your suggested
patch. I would like to try implementing a new, more intelligent
allocation algorithm later, though.
- Ted