[Cluster-devel] [GFS2] Remove useless i_cache from inodes

[Wendy Cheng]

Steven Whitehouse wrote:

>>From aa974896eb5c1a70d4be6df1e3f9f5e12b8887f9 Mon Sep 17 00:00:00 2001
>From: Steven Whitehouse <swhiteho at redhat.com>
>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