Sleeping function called in invalid context

DISCUSSION Nikolay Borisov 5 participants 7 messages Aug 3, 2016 – Aug 11, 2016 ext4 file system

lore

Share & tools

lore

Jan Kara

reviewer

2016-08-04 16:05 UTC #1 lore raw

Quoted 7 lines · Nikolay Borisov

While doing some testing on today's checkout of Linus' master branch I got the following:

[    9.302725] BUG: sleeping function called from invalid context at ./include/linux/buffer_head.h:358
[    9.304403] in_atomic(): 1, irqs_disabled(): 0, pid: 1718, name: mount
[    9.305633] 8 locks held by mount/1718:

Yeah, this looks like a regression cause by commit 4743f83990614af "ext4:
Fix WARN_ON_ONCE in ext4_commit_super()". Arguably that cure is worse than the disease but OTOH calling ext4_commit_super() from an atomic context (like __ext4_grp_locked_error() does) sucks as well.

I'm not sure what the right fix is here. The cleanest would probably be to always drop group lock in __ext4_grp_locked_error() and make sure we always properly bail out of mballoc code on such error. But that's a non-trivial amount of work. Not sure if other ext4 people have opinion on this?

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Theodore Ts'o

maintainer

2016-08-04 20:59 UTC #2 lore raw

Quoted 14 lines · Jan Kara

Quoted 7 lines · Nikolay Borisov
While doing some testing on today's checkout of Linus' master branch I got the following:
[    9.302725] BUG: sleeping function called from invalid context at ./include/linux/buffer_head.h:358
[    9.304403] in_atomic(): 1, irqs_disabled(): 0, pid: 1718, name: mount
[    9.305633] 8 locks held by mount/1718:
Yeah, this looks like a regression cause by commit 4743f83990614af "ext4:
Fix WARN_ON_ONCE in ext4_commit_super()". Arguably that cure is worse than the disease but OTOH calling ext4_commit_super() from an atomic context (like __ext4_grp_locked_error() does) sucks as well.

Quoted 4 lines

I'm not sure what the right fix is here. The cleanest would probably be to always drop group lock in __ext4_grp_locked_error() and make sure we always properly bail out of mballoc code on such error. But that's a non-trivial amount of work. Not sure if other ext4 people have opinion on this?

The easist way to fix this is defer the ext4_commit_super() to a workqueue. We only need this in the errors=continue case, and in that scenario we're not in a hurry when the superblock gets written out.

In fact, we probably want to be doing this for all of the errors=continue cases when we want to save the error state to the superblock, so we can do the update properly using the journal, instead of calling ext4_commit_super() which just force writes the block.

(Of course, if the journal is aborted we'll need to fall back to using ext4_commit_super, of course.)

- Ted

Nikolay Borisov

author

2016-08-05 06:30 UTC #3 lore raw

Quoted 15 lines · Theodore Ts'o

Quoted 14 lines · Jan Kara
Quoted 7 lines · Nikolay Borisov
While doing some testing on today's checkout of Linus' master branch I got the following:
[    9.302725] BUG: sleeping function called from invalid context at ./include/linux/buffer_head.h:358
[    9.304403] in_atomic(): 1, irqs_disabled(): 0, pid: 1718, name: mount
[    9.305633] 8 locks held by mount/1718:
Yeah, this looks like a regression cause by commit 4743f83990614af "ext4:
Fix WARN_ON_ONCE in ext4_commit_super()". Arguably that cure is worse than the disease but OTOH calling ext4_commit_super() from an atomic context (like __ext4_grp_locked_error() does) sucks as well.

Quoted 8 lines

Quoted 4 lines
I'm not sure what the right fix is here. The cleanest would probably be to always drop group lock in __ext4_grp_locked_error() and make sure we always properly bail out of mballoc code on such error. But that's a non-trivial amount of work. Not sure if other ext4 people have opinion on this?
The easist way to fix this is defer the ext4_commit_super() to a workqueue. We only need this in the errors=continue case, and in that scenario we're not in a hurry when the superblock gets written out.

Is errors=continue the default option if nothing specifically is specified at mount time, since I don't have this set explicitly:

/dev/vda / ext4 rw,relatime,data=ordered 0 0

Quoted 16 lines

In fact, we probably want to be doing this for all of the errors=continue cases when we want to save the error state to the superblock, so we can do the update properly using the journal, instead of calling ext4_commit_super() which just force writes the block.
(Of course, if the journal is aborted we'll need to fall back to using ext4_commit_super, of course.)
- Ted
--
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

Theodore Ts'o

maintainer

2016-08-05 14:56 UTC #4 lore raw

Quoted 8 lines · Nikolay Borisov

Quoted 3 lines
The easist way to fix this is defer the ext4_commit_super() to a workqueue. We only need this in the errors=continue case, and in that scenario we're not in a hurry when the superblock gets written out.
Is errors=continue the default option if nothing specifically is specified at mount time, since I don't have this set explicitly:
/dev/vda / ext4 rw,relatime,data=ordered 0 0

Yes, it's the default. I keep wondering whether we should change the default to remount-ro or even panic, since people sometimes don't notice that the "file system has been corrupted" messages, and then they can end up losing a lot more detail if we forced them to address the issue right away.

- Ted

Darrick J. Wong

2016-08-05 17:07 UTC #5 lore raw

Quoted 8 lines · Theodore Ts'o

Quoted 7 lines · Nikolay Borisov
Quoted 3 lines
The easist way to fix this is defer the ext4_commit_super() to a workqueue. We only need this in the errors=continue case, and in that scenario we're not in a hurry when the superblock gets written out.
Is errors=continue the default option if nothing specifically is specified at mount time, since I don't have this set explicitly:

Quoted 7 lines

Quoted 1 line
/dev/vda / ext4 rw,relatime,data=ordered 0 0
Yes, it's the default. I keep wondering whether we should change the default to remount-ro or even panic, since people sometimes don't notice that the "file system has been corrupted" messages, and then they can end up losing a lot more detail if we forced them to address the issue right away.

Yes, please. :)

(At the moment I'm also trying to sort out some ext3 bug where jbd flushes some dirty pages to disk ahead of writing out a transaction but the dirty page writeback fails. Apparently the userspace program is long gone by the time this happens (I haven't been able to verify that they're actually calling fsync) and the FS doesn't shut down, but a later umount/mount/reread shows old file contents. Hence I now know about data_err=abort.)

--D

Quoted 6 lines

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

Andreas Dilger

2016-08-11 19:52 UTC #6 lore raw

In reply to Theodore Ts'o

Quoted 11 lines · Theodore Ts'o

Quoted 7 lines · Nikolay Borisov
Quoted 3 lines
The easist way to fix this is defer the ext4_commit_super() to a workqueue. We only need this in the errors=continue case, and in that scenario we're not in a hurry when the superblock gets written out.
Is errors=continue the default option if nothing specifically is specified at mount time, since I don't have this set explicitly:

Quoted 7 lines

Quoted 1 line
/dev/vda / ext4 rw,relatime,data=ordered 0 0
Yes, it's the default. I keep wondering whether we should change the default to remount-ro or even panic, since people sometimes don't notice that the "file system has been corrupted" messages, and then they can end up losing a lot more detail if we forced them to address the issue right away.

I'd definitely be in favour of making the default "errors=remount-ro". We've been setting that explicitly for years, since otherwise people may not notice their ongoing problems until the filesystem completely explodes.

Related to that, there is a Lustre patch to handle inconsistencies between group descriptors and block/inode bitmaps by marking only the group as unusable for new allocations, instead of marking the whole filesystem in error. Is that something that is of interest to a wider audience?

Patch against RHEL7 is attached, but could be updated for newer kernels if there is interest.

Cheers, Andreas

Since we could skip corrupt block groups, this patch use ext4_warning() intead of ext4_error() to make FS not emount RO in default, also fix a leftover from upstream commit 163a203ddb36c36d4a1c942
---

fs/ext4/balloc.c +13 ~~-28~~

index e069155..692b5e4 100644
@@ -185,25 +185,17 @@ static int ext4_init_block_bitmap(struct super_block *sb,
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	ext4_fsblk_t start, tmp;
 	int flex_bg = 0;
-	struct ext4_group_info *grp;
 
 	J_ASSERT_BH(bh, buffer_locked(bh));
 
 	/* If checksum is bad mark all blocks used to prevent allocation
 	 * essentially implementing a per-group read-only flag. */
 	if (!ext4_group_desc_csum_verify(sb, block_group, gdp)) {
-		grp = ext4_get_group_info(sb, block_group);
-		if (!EXT4_MB_GRP_BBITMAP_CORRUPT(grp))
-			percpu_counter_sub(&sbi->s_freeclusters_counter,
-					   grp->bb_free);
-		set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, &grp->bb_state);
-		if (!EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) {
-			int count;
-			count = ext4_free_inodes_count(sb, gdp);
-			percpu_counter_sub(&sbi->s_freeinodes_counter,
-					   count);
-		}
-		set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, &grp->bb_state);
+		ext4_corrupted_block_group(sb, block_group,
+				EXT4_GROUP_INFO_BBITMAP_CORRUPT |
+				EXT4_GROUP_INFO_IBITMAP_CORRUPT,
+				"Checksum bad for group %u",
+				block_group);
 		return -EIO;
 	}
 	memset(bh->b_data, 0, sb->s_blocksize);

@@ -367,8 +359,6 @@ static void ext4_validate_block_bitmap(struct super_block *sb,
 				       struct buffer_head *bh)
 {
 	ext4_fsblk_t	blk;
-	struct ext4_group_info *grp = ext4_get_group_info(sb, block_group);
-	struct ext4_sb_info *sbi = EXT4_SB(sb);
 
 	if (buffer_verified(bh))
 		return;

@@ -377,22 +367,19 @@ static void ext4_validate_block_bitmap(struct super_block *sb,
 	blk = ext4_valid_block_bitmap(sb, desc, block_group, bh);
 	if (unlikely(blk != 0)) {
 		ext4_unlock_group(sb, block_group);
-		ext4_error(sb, "bg %u: block %llu: invalid block bitmap",
-			   block_group, blk);
-		if (!EXT4_MB_GRP_BBITMAP_CORRUPT(grp))
-			percpu_counter_sub(&sbi->s_freeclusters_counter,
-					   grp->bb_free);
-		set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, &grp->bb_state);
+		ext4_corrupted_block_group(sb, block_group,
+				EXT4_GROUP_INFO_BBITMAP_CORRUPT,
+				"bg %u: block %llu: invalid block bitmap",
+				block_group, blk);
 		return;
 	}
 	if (unlikely(!ext4_block_bitmap_csum_verify(sb, block_group,
 			desc, bh))) {
 		ext4_unlock_group(sb, block_group);
-		ext4_error(sb, "bg %u: bad block bitmap checksum", block_group);
-		if (!EXT4_MB_GRP_BBITMAP_CORRUPT(grp))
-			percpu_counter_sub(&sbi->s_freeclusters_counter,
-					   grp->bb_free);
-		set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, &grp->bb_state);
+		ext4_corrupted_block_group(sb, block_group,
+				EXT4_GROUP_INFO_BBITMAP_CORRUPT,
+				"bg %u: bad block bitmap checksum",
+				block_group);
 		return;
 	}
 	set_buffer_verified(bh);

@@ -445,8 +432,6 @@ ext4_read_block_bitmap_nowait(struct super_block *sb, ext4_group_t block_group)
 		set_buffer_uptodate(bh);
 		ext4_unlock_group(sb, block_group);
 		unlock_buffer(bh);
-		if (err)
-			ext4_error(sb, "Checksum bad for grp %u", block_group);
 		return bh;
 	}
 	ext4_unlock_group(sb, block_group);

fs/ext4/ext4.h +15 -0

index 3c41773..63a63b6 100644
@@ -91,6 +91,17 @@ typedef __u32 ext4_lblk_t;
 /* data type for block group number */
 typedef unsigned int ext4_group_t;
 
+void __ext4_corrupted_block_group(struct super_block *sb,
+				  ext4_group_t group, unsigned int flags,
+				  const char *function, unsigned int line);
+
+#define ext4_corrupted_block_group(sb, group, flags, fmt, ...)		\
+	do {								\
+		__ext4_warning(sb, __func__, __LINE__, fmt,		\
+				##__VA_ARGS__);				\
+		__ext4_corrupted_block_group(sb, group, flags,		\
+					__func__, __LINE__);		\
+	} while (0)
 /*
  * Flags used in mballoc's allocation_context flags field.
  *

@@ -2673,7 +2684,11 @@ struct ext4_group_info {
 #define EXT4_GROUP_INFO_NEED_INIT_BIT		0
 #define EXT4_GROUP_INFO_WAS_TRIMMED_BIT		1
 #define EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT	2
+#define EXT4_GROUP_INFO_BBITMAP_CORRUPT		\
+	(1 << EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT)
 #define EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT	3
+#define EXT4_GROUP_INFO_IBITMAP_CORRUPT		\
+	(1 << EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT)
 
 #define EXT4_MB_GRP_NEED_INIT(grp)	\
 	(test_bit(EXT4_GROUP_INFO_NEED_INIT_BIT, &((grp)->bb_state)))

fs/ext4/ialloc.c +11 ~~-35~~

75 lines not on this page · raw message

fs/ext4/mballoc.c +44 ~~-51~~

198 lines not on this page · raw message

fs/ext4/super.c +31 -0

index c625960..0de22f2 100644
@@ -633,6 +633,37 @@ void __ext4_warning(struct super_block *sb, const char *function,
 	va_end(args);
 }
 
+void __ext4_corrupted_block_group(struct super_block *sb, ext4_group_t group,
+				  unsigned int flags, const char *function,
+				  unsigned int line)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	struct ext4_group_info *grp = ext4_get_group_info(sb, group);
+	struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL);
+
+	if (flags & EXT4_GROUP_INFO_BBITMAP_CORRUPT &&
+	    !EXT4_MB_GRP_BBITMAP_CORRUPT(grp)) {
+		percpu_counter_sub(&sbi->s_freeclusters_counter,
+					grp->bb_free);
+		set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT,
+			&grp->bb_state);
+	}
+
+	if (flags & EXT4_GROUP_INFO_IBITMAP_CORRUPT &&
+	    !EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) {
+		if (gdp) {
+			int count;
+
+			count = ext4_free_inodes_count(sb, gdp);
+			percpu_counter_sub(&sbi->s_freeinodes_counter,
+					   count);
+		}
+		set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT,
+			&grp->bb_state);
+	}
+	save_error_info(sb, function, line);
+}
+
 void __ext4_grp_locked_error(const char *function, unsigned int line,
 			     struct super_block *sb, ext4_group_t grp,
 			     unsigned long ino, ext4_fsblk_t block,