fix mballoc oopses on mkdir

PATCH Mingming Cao 2 participants 2 messages Aug 29, 2007 – Aug 30, 2007 5 messages not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Mingming Cao

author

2007-08-29 21:01 UTC #3 lore raw

Quoted 8 lines · Eric Sandeen

Ugh. brain clearly not fully engaged... here we go:
------------------
Tried out Ted's git tree + all pending patches today, and immediately oopsed on a mkdir thanks to this in ext4_mb_new_group_pa()

Quoted 6 lines

	BUG_ON(!S_ISREG(ac->ac_inode->i_mode));
(there are 54 BUGs and BUG_ONs in this file...!)
I think something like this patch is needed?

Thanks for catching this and the fix. I saw the same kernel oops today when rum mballoc on fsstress (mkdir). I just added this fix to the patch queue with some comments.

Quoted 8 lines

Signed-off-by Eric Sandeen <[email protected]>
Index: ext4.git/fs/ext4/extents.c
===================================================================

fs/ext4/extents.c int ext4_ext_get_blocks(handle_t *handle · line 2541

Quoted 8 lines

fs/ext4/extents.c +4 -1

@@ -2535,7 +2535,10 @@ int ext4_ext_get_blocks(handle_t *handle
 	ar.goal = ext4_ext_find_goal(inode, path, iblock);
 	ar.logical = iblock;
 	ar.len = allocated;
-	ar.flags = EXT4_MB_HINT_DATA;
+	if (S_ISREG(inode->i_mode))
+		ar.flags = EXT4_MB_HINT_DATA;
+	else
+		ar.flags = 0;

I see. We set the flags to zero here to avoid doing in-core preallocation for directory files later.

Well, I am not sure whether it's worth the effort to support in-core preallocation for directory files. But I don't see hard reason we cannot do this. My understanding is: this is the new in-core preallocation which does a should not depend on extents or any on-disk format changes.

Alex, any comments?

Mingming

Quoted 8 lines

fs/ext4/extents.c +0 -1
 	newblock = ext4_mb_new_blocks(handle, &ar, &err);
 	if (!newblock)
 		goto out2;
 
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Andreas Dilger

2007-08-30 14:31 UTC #6 lore raw

Quoted 8 lines · Eric Sandeen

Quoted 4 lines · Andreas Dilger
Quoted 1 line
(there are 54 BUGs and BUG_ONs in this file...!)
Yes, we like lots of assertions in our code, because it is exceedingly complex to debug otherwise.
But wouldn't error recovery be a better choice? You could macro-ize it to BUG for development purposes if you wanted, I suppose...

No, the goal is if something "impossible" happens you want to stop dead at that point instead of continuing to work in an unknown environment. I believe that all of the extents/mballoc code will NOT BUG on bad data from disk.

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.