From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wendy Cheng Date: Tue, 16 Oct 2007 16:25:56 -0400 Subject: [Cluster-devel] [GFS2] Remove useless i_cache from inodes In-Reply-To: <1192535578.3323.8.camel@menhir.chygwyn.com> References: <1192535578.3323.8.camel@menhir.chygwyn.com> Message-ID: <47151E54.9040201@redhat.com> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Steven Whitehouse wrote: >>>From aa974896eb5c1a70d4be6df1e3f9f5e12b8887f9 Mon Sep 17 00:00:00 2001 >From: Steven Whitehouse >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