On Tue, 25 Jun 2013, Zheng Liu wrote:
> Date: Tue, 25 Jun 2013 11:50:03 +0800
> From: Zheng Liu <gnehzuil.liu@xxxxxxxxx>
> To: Theodore Ts'o <tytso@xxxxxxx>
> Cc: linux-ext4@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH FOR DISCUSSION] add delalloc debugging
>
> 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.
Yes, I very much like this patch. It's worth having and it can yield
important information.
>
> 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.
>
> 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.
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.
>
> 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.
>
> 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@xxxxxxx>
> > 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@xxxxxxx>
> > ---
> > 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
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
