Re: [PATCH FOR DISCUSSION] add delalloc debugging

[Zheng Liu <gnehzuil.liu@gmail.com>]

Hi Ted,

On Thu, Jun 20, 2013 at 12:42:49PM -0400, Theodore Ts'o wrote:
> 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?

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.

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

> 
>       	          	       	      	  - Ted
> 
> From f6417debc1c96a9dfa6b9f19da14eff35bf0f504 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <tytso@mit.edu>
> 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" <tytso@mit.edu>
> ---
>  fs/ext4/super.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 77 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 85b3dd6..ecb8256 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1832,6 +1832,74 @@ static const struct file_operations ext4_seq_options_fops = {
>  	.release = single_release,
>  };
>  
> +#ifdef CONFIG_EXT4_DEBUG
#ifndef MODULE
> +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
> +
> +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
> +	struct inode *inode;
> +	extern spinlock_t inode_sb_list_lock;
#endif

Regards,
                                                - Zheng

> +
> +	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
> -- 
> 1.7.12.rc0.22.gcdd159b
>