add delalloc debugging

From f6417debc1c96a9dfa6b9f19da14eff35bf0f504 Mon Sep 17 00:00:00 2001

I've been carrying a patch in the unstable portion of the patch series for a while now to debug problems with delayed allocation. This allows us to observe the state of which inodes have inodes subject for delayed allocation, and how many data/metadata blocks have been reserved.

I've finally cleaned it up enough that it's something where I wouldn't feel terrible dropping it into the mainline kernel. (It's still a little gross, but it's not truly horrifying any more.)

What do people think? Is this something that's worth having in the kernel sources? Or shall I continue carrying it as an out-of-tree debugging patch?

(Note: we can use similar technique to gain visibility into the status the extent status LRU list.)

- Ted

From f6417debc1c96a9dfa6b9f19da14eff35bf0f504 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <[email protected]>
Date: Thu, 20 Jun 2013 12:35:39 -0400
Subject: [PATCH] ext4: add delalloc debugging

This adds a file in /proc/fs/ext4/<dev> which when opened for reading, will trigger debugging code that dumps a lot of information about inodes subject to delayed allocation to the console.

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 fs/ext4/super.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

fs/ext4/super.c +0 -0

index 85b3dd6..ecb8256 100644

fs/ext4/super.c +53 -1

+
+	printk(KERN_DEBUG "EXT4-fs debug delalloc of %s\n", sb->s_id);
+	printk(KERN_DEBUG "EXT4-fs: dirty clusters %lld free clusters %lld\n",
+	       percpu_counter_sum(&sbi->s_dirtyclusters_counter),
+	       percpu_counter_sum(&sbi->s_freeclusters_counter));
+
+#ifndef MODULE
+	spin_lock(&inode_sb_list_lock);
+	if (!list_empty(&sb->s_bdi->wb.b_dirty)) {
+		printk(KERN_DEBUG "s_bdi->wb.b_dirty list:\n");
+		list_for_each_entry(inode, &sb->s_bdi->wb.b_dirty,
+				    i_wb_list) {
+			print_inode_delalloc_info(inode);
+		}
+	}
+	if (!list_empty(&sb->s_bdi->wb.b_io)) {
+		printk(KERN_DEBUG "s_bdi->wb.b_io list:\n");
+		list_for_each_entry(inode, &sb->s_bdi->wb.b_io,
+				    i_wb_list) {
+			print_inode_delalloc_info(inode);
+		}
+	}
+	if (!list_empty(&sb->s_bdi->wb.b_more_io)) {
+		printk(KERN_DEBUG "s_bdi->wb.b_more_io list:\n");
+		list_for_each_entry(inode, &sb->s_bdi->wb.b_more_io,
+				    i_wb_list) {
+			print_inode_delalloc_info(inode);
+		}
+	}
+	spin_unlock(&inode_sb_list_lock);
+	printk(KERN_DEBUG "ext4 debug delalloc done\n");
+#endif
+	return single_open(file, debug_delalloc_show, sb);
+}
+
+static const struct file_operations ext4_seq_delalloc_debug_fops = {
+	.owner = THIS_MODULE,
+	.open = options_delalloc_debug_open_fs,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+#endif
+
 static int ext4_setup_super(struct super_block *sb, struct ext4_super_block *es,
 			    int read_only)
 {

@@ -3764,9 +3832,14 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	if (ext4_proc_root)
 		sbi->s_proc = proc_mkdir(sb->s_id, ext4_proc_root);
 
-	if (sbi->s_proc)
+	if (sbi->s_proc) {
 		proc_create_data("options", S_IRUGO, sbi->s_proc,
 				 &ext4_seq_options_fops, sb);
+#ifdef CONFIG_EXT4_DEBUG
+		proc_create_data("delalloc_debug", S_IRUSR, sbi->s_proc,
+				 &ext4_seq_delalloc_debug_fops, sb);
+#endif
+	}
 
 	bgl_lock_init(sbi->s_blockgroup_lock);
 

@@ -4149,6 +4222,9 @@ failed_mount:
 		crypto_free_shash(sbi->s_chksum_driver);
 	if (sbi->s_proc) {
 		remove_proc_entry("options", sbi->s_proc);
+#ifdef CONFIG_EXT4_DEBUG
+		remove_proc_entry("delalloc_debug", sbi->s_proc);
+#endif
 		remove_proc_entry(sb->s_id, ext4_proc_root);
 	}
 #ifdef CONFIG_QUOTA

Signature · 1 line

1.7.12.rc0.22.gcdd159b

Date: Tue, 25 Jun 2013 11:50:03 +0800
From: Zheng Liu <[email protected]>
To: Theodore Ts'o <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH FOR DISCUSSION] add delalloc debugging

Hi Ted,

Quoted 10 lines · Theodore Ts'o

I've been carrying a patch in the unstable portion of the patch series for a while now to debug problems with delayed allocation. This allows us to observe the state of which inodes have inodes subject for delayed allocation, and how many data/metadata blocks have been reserved.

I've finally cleaned it up enough that it's something where I wouldn't feel terrible dropping it into the mainline kernel. (It's still a little gross, but it's not truly horrifying any more.)

Quoted 3 lines

What do people think? Is this something that's worth having in the kernel sources? Or shall I continue carrying it as an out-of-tree debugging patch?

I think it is worth having it in the kernel source. But before we apply this patch, it seems that we need to solve some problems.

1. Now when we read /proc/fs/ext4/{$DEV}/delalloc_debug, it will print the result in console. IMHO, I don't think it is a good choice. I prefer to print this result in debugfs or in sysfs.

2. If we want to gain this feature, we will enable EXT4_DEBUG option. But in a product system, we never enable it because of performance degradation. So I think that maybe we can compile it without EXT4_DEBUG option and dynamically enable/disalbe it.

3. Maybe we can provide a interface to let the user indicate which inode they want to observe.

Finally, the patch itself still has two minor problems. We forget to call remove_proc_entry() in ext4_put_super(). Another problem is compile warnings.

Quoted 3 lines

(Note: we can use similar technique to gain visibility into the status the extent status LRU list.)

I am happy to generate a patch for extent status LRU list.

Quoted 22 lines

- Ted

From f6417debc1c96a9dfa6b9f19da14eff35bf0f504 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <[email protected]>
Date: Thu, 20 Jun 2013 12:35:39 -0400
Subject: [PATCH] ext4: add delalloc debugging

This adds a file in /proc/fs/ext4/<dev> which when opened for reading, will trigger debugging code that dumps a lot of information about inodes subject to delayed allocation to the console.

Signed-off-by "Theodore Ts'o" <[email protected]>

---
 fs/ext4/super.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

fs/ext4/super.c +0 -0

index 85b3dd6..ecb8256 100644

fs/ext4/super.c static const struct file_operations ext4_seq_options_fops = { · line 1835

Quoted 4 lines

fs/ext4/super.c +1 -0

@@ -1832,6 +1832,74 @@ static const struct file_operations ext4_seq_options_fops = {
 	.release = single_release,
 };
 
+#ifdef CONFIG_EXT4_DEBUG

#ifndef MODULE

fs/ext4/super.c static const struct file_operations ext4_seq_options_fops = { · line 1845

Quoted 10 lines

fs/ext4/super.c +10 -0

+static void print_inode_delalloc_info(struct inode *inode)
+{
+	if (!EXT4_I(inode)->i_reserved_data_blocks ||
+	    !EXT4_I(inode)->i_reserved_meta_blocks)
+		return;
+
+	printk(KERN_DEBUG "ino %lu: %u %u\n", inode->i_ino,
+	       EXT4_I(inode)->i_reserved_data_blocks,
+	       EXT4_I(inode)->i_reserved_meta_blocks);
+}

#endif

fs/ext4/super.c static const struct file_operations ext4_seq_options_fops = { · line 1856

Quoted 11 lines

fs/ext4/super.c +11 -0

+
+static int debug_delalloc_show(struct seq_file *seq, void *offset)
+{
+	return 0;
+}
+
+static int options_delalloc_debug_open_fs(struct inode *proc_inode,
+					  struct file *file)
+{
+	struct super_block *sb = PDE_DATA(proc_inode);
+	struct ext4_sb_info *sbi = EXT4_SB(sb);

#ifndef MODULE

fs/ext4/super.c static const struct file_operations ext4_seq_options_fops = { · line 1858

Quoted 2 lines

fs/ext4/super.c +2 -0

+	struct inode *inode;
+	extern spinlock_t inode_sb_list_lock;

#endif

Regards,
- Zheng

Quoted 76 lines

fs/ext4/super.c +53 -1

+
+	printk(KERN_DEBUG "EXT4-fs debug delalloc of %s\n", sb->s_id);
+	printk(KERN_DEBUG "EXT4-fs: dirty clusters %lld free clusters %lld\n",
+	       percpu_counter_sum(&sbi->s_dirtyclusters_counter),
+	       percpu_counter_sum(&sbi->s_freeclusters_counter));
+
+#ifndef MODULE
+	spin_lock(&inode_sb_list_lock);
+	if (!list_empty(&sb->s_bdi->wb.b_dirty)) {
+		printk(KERN_DEBUG "s_bdi->wb.b_dirty list:\n");
+		list_for_each_entry(inode, &sb->s_bdi->wb.b_dirty,
+				    i_wb_list) {
+			print_inode_delalloc_info(inode);
+		}
+	}
+	if (!list_empty(&sb->s_bdi->wb.b_io)) {
+		printk(KERN_DEBUG "s_bdi->wb.b_io list:\n");
+		list_for_each_entry(inode, &sb->s_bdi->wb.b_io,
+				    i_wb_list) {
+			print_inode_delalloc_info(inode);
+		}
+	}
+	if (!list_empty(&sb->s_bdi->wb.b_more_io)) {
+		printk(KERN_DEBUG "s_bdi->wb.b_more_io list:\n");
+		list_for_each_entry(inode, &sb->s_bdi->wb.b_more_io,
+				    i_wb_list) {
+			print_inode_delalloc_info(inode);
+		}
+	}
+	spin_unlock(&inode_sb_list_lock);
+	printk(KERN_DEBUG "ext4 debug delalloc done\n");
+#endif
+	return single_open(file, debug_delalloc_show, sb);
+}
+
+static const struct file_operations ext4_seq_delalloc_debug_fops = {
+	.owner = THIS_MODULE,
+	.open = options_delalloc_debug_open_fs,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+#endif
+
 static int ext4_setup_super(struct super_block *sb, struct ext4_super_block *es,
 			    int read_only)
 {

@@ -3764,9 +3832,14 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	if (ext4_proc_root)
 		sbi->s_proc = proc_mkdir(sb->s_id, ext4_proc_root);
 
-	if (sbi->s_proc)
+	if (sbi->s_proc) {
 		proc_create_data("options", S_IRUGO, sbi->s_proc,
 				 &ext4_seq_options_fops, sb);
+#ifdef CONFIG_EXT4_DEBUG
+		proc_create_data("delalloc_debug", S_IRUSR, sbi->s_proc,
+				 &ext4_seq_delalloc_debug_fops, sb);
+#endif
+	}
 
 	bgl_lock_init(sbi->s_blockgroup_lock);
 

@@ -4149,6 +4222,9 @@ failed_mount:
 		crypto_free_shash(sbi->s_chksum_driver);
 	if (sbi->s_proc) {
 		remove_proc_entry("options", sbi->s_proc);
+#ifdef CONFIG_EXT4_DEBUG
+		remove_proc_entry("delalloc_debug", sbi->s_proc);
+#endif
 		remove_proc_entry(sb->s_id, ext4_proc_root);
 	}
 #ifdef CONFIG_QUOTA

Signature · 1 line

1.7.12.rc0.22.gcdd159b

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

Quoted 3 lines

1. Now when we read /proc/fs/ext4/{$DEV}/delalloc_debug, it will print the result in console. IMHO, I don't think it is a good choice. I prefer to print this result in debugfs or in sysfs.

I would prefer to print this info in debug_delalloc_show() so we can simply read the file instead of read the file + read the log.

I think that having this enabled only is EXT4_DEBUG is set is just fine. However if you want to be able to use it even without EXT4_DEBUG option we could just set proper permissions on the /proc/fs/ext4/{$DEV}/delalloc_debug so that only privileged user can open it.

Quoted 2 lines

3. Maybe we can provide a interface to let the user indicate which inode they want to observe.

you can grep for it, I do not think it's a big deal.

Quoted 3 lines

Finally, the patch itself still has two minor problems. We forget to call remove_proc_entry() in ext4_put_super(). Another problem is compile warnings.

I've been carrying a patch in the unstable portion of the patch series for a while now to debug problems with delayed allocation. This allows us to observe the state of which inodes have inodes subject for delayed allocation, and how many data/metadata blocks have been reserved.

I've finally cleaned it up enough that it's something where I wouldn't feel terrible dropping it into the mainline kernel. (It's still a little gross, but it's not truly horrifying any more.)

What do people think? Is this something that's worth having in the kernel sources? Or shall I continue carrying it as an out-of-tree debugging patch?

(Note: we can use similar technique to gain visibility into the status the extent status LRU list.)

- Ted

From f6417debc1c96a9dfa6b9f19da14eff35bf0f504 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <[email protected]>
Date: Thu, 20 Jun 2013 12:35:39 -0400
Subject: [PATCH] ext4: add delalloc debugging

This adds a file in /proc/fs/ext4/<dev> which when opened for reading, will trigger debugging code that dumps a lot of information about inodes subject to delayed allocation to the console.

Signed-off-by "Theodore Ts'o" <[email protected]>

Do not use VFS level locks and list walks in filesystem code, not even in debugging code. It's ok for you to do this in private patches and maintain such abuses yourself, but it's not acceptible for merging into the mainline tree...

Besides, you haven't even used the correct lock - these bdi writeback lists are protected by the bdi writeback list lock, not the superblock inode list lock....