[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

Steven Whitehouse wrote:

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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]