Steven Whitehouse wrote:
From aa974896eb5c1a70d4be6df1e3f9f5e12b8887f9 Mon Sep 17 00:00:00 2001
From: Steven Whitehouse <[EMAIL PROTECTED]>
Date: Mon, 15 Oct 2007 16:29:05 +0100
Subject: [PATCH] [GFS2] Remove useless i_cache from inodes
The i_cache was designed to keep references to the indirect blocks
used during block mapping so that they didn't have to be looked
up continually. The idea failed because there are too many places
where the i_cache needs to be freed, and this has in the past been
the cause of many bugs.
There are not many places where this cache needs to be flushed. In GFS1
and earlier versions of GFS2, it is done in glock transition and/or page
releasing logic. These are places where the meta buffer could get
altered by another nodes so the flush is required.
It is not clear to me why i_cache needs to be flushed in
gfs2_remove_from_ail(). The added logic started from this patch:
http://lkml.org/lkml/2007/10/4/103 . I have doubts what this patch could
fix. This is the place where gfs2 completes its journal entry
processing. There is no reason to flush meta buffer cache since journal
transaction doesn't seem to have anything to do with other node altering
the meta buffer. If we want to remove anything, this is the logic that I
would agree to get removed.
In addition there was no performance benefit being gained since the
disk blocks in question were cached anyway. So this patch removes
it in order to simplify the code to prepare for other changes which
would otherwise have had to add further support for this feature.
I don't have performance data at this moment to back up my objection to
this patch (but you don't have that either). However, intuitively,
i_cache is part of the gfs2 inode struct. Accessing it is quick from
latency point of view since we already have gfs2 inode on hand when this
logic is involved. Be aware that all we care here is buffer head, not
the page itself. But this new patch needs to parse thru VFS global page
cache radix tree, including read_lock_irq() with the tree_lock, then get
the page, then find the buffer head. It is hard to be convinced that it
will help performance. And remember GFS2_MAX_META_HEIGHT is 10, way too
small comparing to the page cache radix tree.
In reality, this is probably a trivial issue and would not worth our
time to have heated debates on this. However, my gut feeling is that I
woud prefer to keeping the i_cache. On the other hand, I do think the
i_cache flushing needs to get removed from gfs2_remove_from_ail() - i.e.
the original patch needs to get reverted (but need some testing time on
this).
So the important question here is why and how we need to mess with
i_cache to "prepare for other changes" ?
-- Wendy
