Re: [PATCH FOR DISCUSSION] add delalloc debugging

[Dave Chinner <david@fromorbit.com>]

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

> +
> +#ifndef MODULE
> +	spin_lock(&inode_sb_list_lock);
> +	if (!list_empty(&sb->s_bdi->wb.b_dirty)) {

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com