On Mon, Nov 29, 2010 at 5:43 AM, Arjan van de Ven <arjan@xxxxxxxxxxxxx> wrote:
> On Sun, 28 Nov 2010 12:52:56 -0500
> Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
>
>> Looks generally good to me.
>>
>> Two sugestions:
>>
>> - also trace the flags value and decode it using __print_flags in the
>> output
>> - use a dev_t instead of the split major/minor to follow the way the
>> block layer, writeback and xfs tracepoints track the block device.
>> Seems like this was copied from the recent conversion of ext4 away
>> from it's braindead string printing, which didn't follow the
>> existing way either.
>>
>
> ok how about this?
>
> only question left is if we should trave every dirty, or only those that
> change the flags.
>
> Right now the patch only traces the non-trivial (eg changing) ones... but opinions welcome
> (otherwise you get MANY duplicate, basically no-op trace events)
>
>
> From 3950d3c04a6bf8ccf9ff912a49bdd242a2fe9e47 Mon Sep 17 00:00:00 2001
> From: Arjan van de Ven <arjan@xxxxxxxxxxxxxxx>
> Date: Fri, 26 Nov 2010 12:18:03 -0800
> Subject: [PATCH] vfs: Add a trace point in the mark_inode_dirty function
>
> PowerTOP would like to be able to show who is keeping the disk
> busy by dirtying data. The most logical spot for this is in the vfs
> in the mark_inode_dirty() function, doing this on the block level
> is not possible because by the time the IO hits the block layer the
> guilty party can no longer be found ("kjournald" and "pdflush" are not
> useful answers to "who caused this file to be dirty).
>
> The trace point follows the same logic/style as the block_dump code
> and pretty much dumps the same data, just not to dmesg (and thus to
> /var/log/messages) but via the trace events streams.
>
> Eventually we should be able to phase out the block dump code, but that's
> for later on after a transition time.
>
> Signed-of-by: Arjan van de Ven <arjan@xxxxxxxxxxxxxxx>
> ---
> fs/fs-writeback.c | 3 +++
> include/linux/fs.h | 12 ++++++++++++
> include/trace/events/writeback.h | 28 ++++++++++++++++++++++++++++
> 3 files changed, 43 insertions(+), 0 deletions(-)
>
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 3d06ccc..62e33cc 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -952,6 +952,9 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> if ((inode->i_state & flags) == flags)
> return;
>
> + if (flags & (I_DIRTY_SYNC | I_DIRTY_DATASYNC | I_DIRTY_PAGES))
> + trace_writeback_inode_dirty(inode, flags);
> +
> if (unlikely(block_dump))
> block_dump___mark_inode_dirty(inode);
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c9e06cc..25935e1 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1676,6 +1676,18 @@ struct super_operations {
>
> #define I_DIRTY (I_DIRTY_SYNC | I_DIRTY_DATASYNC | I_DIRTY_PAGES)
>
> +#define INODE_DIRTY_FLAGS \
> + { I_DIRTY_SYNC, "DIRTY-SYNC" }, \
> + { I_DIRTY_DATASYNC, "DIRTY-DATASYNC" }, \
> + { I_DIRTY_PAGES, "DIRTY-PAGES" }, \
> + { I_NEW, "NEW" }, \
> + { I_WILL_FREE, "WILL-FREE" }, \
> + { I_FREEING, "FREEING" }, \
> + { I_CLEAR, "CLEAR" }, \
> + { I_SYNC, "SYNC" }, \
> + { I_REFERENCED, "REFERENCED" }
Can you change these to exactly the same name as the flags ("I_DIRTY_SYNC",..).
Arbitrarily changing them a little bit is unnecessary.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
