linux-ext4 - Re: [PATCH FOR DISCUSSION] add delalloc debugging

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130626131823.GA20140@dastard>
Date:	Wed, 26 Jun 2013 23:18:23 +1000
From:	Dave Chinner <david@...morbit.com>
To:	Theodore Ts'o <tytso@....edu>
Cc:	linux-ext4@...r.kernel.org, Zheng Liu <gnehzuil.liu@...il.com>
Subject: Re: [PATCH FOR DISCUSSION] add delalloc debugging

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@....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@....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@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html