* [PATCH] fix cramfs making duplicate entries in inode cache
@ 2005-08-19 20:17 Dave Johnson
2005-08-19 21:25 ` Phillip Lougher
0 siblings, 1 reply; 4+ messages in thread
From: Dave Johnson @ 2005-08-19 20:17 UTC (permalink / raw)
To: linux-kernel, linux-fsdevel
Every time cramfs_lookup() is called to lookup and inode for a dentry,
get_cramfs_inode() will allocate a new inode without checking to see
if that inode already exists in the inode cache.
This is fine the first time, but if the dentry cache entry(ies)
associated with that inode are aged out, but the inode entry is not
aged out (which can be quite common if the inode has buffer cache
linked to it), cramfs_lookup() will be called again and another inode
will be allocated and added to the inode cache creating a duplicate in
the inode cache.
The big issue here is that the buffers associated with each inode
cache entry are not shared between the duplicates!
The older inode entries are now orphaned as no dentry points to it and
won't be freed until the buffer cache assoicated with them are first
freed. The newest entry will have to create all new buffer cache for
each part of its file as the old buffer cache is now orphaned as well.
Patch below fixes this by making get_cramfs_inode() use the inode
cache before blindly creating a new entry every time. This eliminates
the duplicate inodes and duplicate buffer cache.
--
Dave Johnson
Starent Networks
===== fs/cramfs/inode.c 1.42 vs edited =====
--- 1.42/fs/cramfs/inode.c 2005-07-14 12:24:48 -04:00
+++ edited/fs/cramfs/inode.c 2005-08-19 15:39:05 -04:00
@@ -44,10 +44,10 @@
static struct inode *get_cramfs_inode(struct super_block *sb, struct cramfs_inode * cramfs_inode)
{
- struct inode * inode = new_inode(sb);
+ struct inode * inode = iget_locked(sb, CRAMINO(cramfs_inode));
static struct timespec zerotime;
- if (inode) {
+ if (inode && (inode->i_state & I_NEW)) {
inode->i_mode = cramfs_inode->mode;
inode->i_uid = cramfs_inode->uid;
inode->i_size = cramfs_inode->size;
@@ -57,11 +57,6 @@
/* Struct copy intentional */
inode->i_mtime = inode->i_atime = inode->i_ctime = zerotime;
inode->i_ino = CRAMINO(cramfs_inode);
- /* inode->i_nlink is left 1 - arguably wrong for directories,
- but it's the best we can do without reading the directory
- contents. 1 yields the right result in GNU find, even
- without -noleaf option. */
- insert_inode_hash(inode);
if (S_ISREG(inode->i_mode)) {
inode->i_fop = &generic_ro_fops;
inode->i_data.a_ops = &cramfs_aops;
@@ -76,6 +72,7 @@
init_special_inode(inode, inode->i_mode,
old_decode_dev(cramfs_inode->size));
}
+ unlock_new_inode(inode);
}
return inode;
}
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] fix cramfs making duplicate entries in inode cache
2005-08-19 20:17 [PATCH] fix cramfs making duplicate entries in inode cache Dave Johnson
@ 2005-08-19 21:25 ` Phillip Lougher
2005-08-19 23:17 ` Dave Johnson
0 siblings, 1 reply; 4+ messages in thread
From: Phillip Lougher @ 2005-08-19 21:25 UTC (permalink / raw)
To: Dave Johnson; +Cc: linux-kernel, linux-fsdevel
Dave Johnson wrote:
>
> Patch below fixes this by making get_cramfs_inode() use the inode
> cache before blindly creating a new entry every time. This eliminates
> the duplicate inodes and duplicate buffer cache.
>
> + struct inode * inode = iget_locked(sb, CRAMINO(cramfs_inode));
Doesn't iget_locked() assume inode numbers are unique?
In Cramfs inode numbers are set to 1 for non-data inodes (fifos,
sockets, devices, empty directories), i.e
%stat device namedpipe
File: `device'
Size: 0 Blocks: 0 IO Block: 4096 character
special file
Device: 700h/1792d Inode: 1 Links: 1 Device type: 1,1
Access: (0644/crw-r--r--) Uid: ( 0/ root) Gid: ( 0/ root)
Access: 1970-01-01 01:00:00.000000000 +0100
Modify: 1970-01-01 01:00:00.000000000 +0100
Change: 1970-01-01 01:00:00.000000000 +0100
File: `namedpipe'
Size: 0 Blocks: 0 IO Block: 4096 fifo
Device: 700h/1792d Inode: 1 Links: 1
Access: (0644/prw-r--r--) Uid: ( 0/ root) Gid: ( 0/ root)
Access: 1970-01-01 01:00:00.000000000 +0100
Modify: 1970-01-01 01:00:00.000000000 +0100
Change: 1970-01-01 01:00:00.000000000 +0100
Should iget5_locked() be used here?
Phillip
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fix cramfs making duplicate entries in inode cache
2005-08-19 21:25 ` Phillip Lougher
@ 2005-08-19 23:17 ` Dave Johnson
2005-08-20 2:14 ` Dave Johnson
0 siblings, 1 reply; 4+ messages in thread
From: Dave Johnson @ 2005-08-19 23:17 UTC (permalink / raw)
To: Phillip Lougher; +Cc: linux-kernel, linux-fsdevel
Phillip Lougher writes:
> Doesn't iget_locked() assume inode numbers are unique?
>
> In Cramfs inode numbers are set to 1 for non-data inodes (fifos,
> sockets, devices, empty directories), i.e
>
> %stat device namedpipe
> File: `device'
> Size: 0 Blocks: 0 IO Block: 4096 character
> special file
> Device: 700h/1792d Inode: 1 Links: 1 Device type: 1,1
> Access: (0644/crw-r--r--) Uid: ( 0/ root) Gid: ( 0/ root)
> Access: 1970-01-01 01:00:00.000000000 +0100
> Modify: 1970-01-01 01:00:00.000000000 +0100
> Change: 1970-01-01 01:00:00.000000000 +0100
> File: `namedpipe'
> Size: 0 Blocks: 0 IO Block: 4096 fifo
> Device: 700h/1792d Inode: 1 Links: 1
> Access: (0644/prw-r--r--) Uid: ( 0/ root) Gid: ( 0/ root)
> Access: 1970-01-01 01:00:00.000000000 +0100
> Modify: 1970-01-01 01:00:00.000000000 +0100
> Change: 1970-01-01 01:00:00.000000000 +0100
>
> Should iget5_locked() be used here?
Yep, that was busted. Below patch should be better.
--
Dave Johnson
Starent Networks
===== fs/cramfs/inode.c 1.42 vs edited =====
--- 1.42/fs/cramfs/inode.c 2005-07-14 12:24:48 -04:00
+++ edited/fs/cramfs/inode.c 2005-08-19 18:47:28 -04:00
@@ -42,12 +42,43 @@
#define CRAMINO(x) ((x)->offset?(x)->offset<<2:1)
#define OFFSET(x) ((x)->i_ino)
+
+static int cramfs_iget5_test(struct inode *inode, void *opaque)
+{
+ struct cramfs_inode * cramfs_inode = (struct cramfs_inode *)opaque;
+
+ if (inode->i_ino != 1)
+ return 1;
+
+ /* all empty directories, char, block, pipe, and sock, share inode #1 */
+
+ if ((inode->i_mode != cramfs_inode->mode) ||
+ (inode->i_gid != cramfs_inode->gid) ||
+ (inode->i_uid != cramfs_inode->uid))
+ return 0; /* does not match */
+
+ if ((S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) &&
+ (inode->i_rdev != old_decode_dev(cramfs_inode->size)))
+ return 0; /* does not match */
+
+ return 1; /* matches */
+}
+
+static int cramfs_iget5_set(struct inode *inode, void *opaque)
+{
+ struct cramfs_inode * cramfs_inode = (struct cramfs_inode *)opaque;
+ inode->i_ino = CRAMINO(cramfs_inode);
+ return 0;
+}
+
static struct inode *get_cramfs_inode(struct super_block *sb, struct cramfs_inode * cramfs_inode)
{
- struct inode * inode = new_inode(sb);
+ struct inode * inode = iget5_locked(sb, CRAMINO(cramfs_inode),
+ cramfs_iget5_test, cramfs_iget5_set,
+ cramfs_inode);
static struct timespec zerotime;
- if (inode) {
+ if (inode && (inode->i_state & I_NEW)) {
inode->i_mode = cramfs_inode->mode;
inode->i_uid = cramfs_inode->uid;
inode->i_size = cramfs_inode->size;
@@ -59,11 +92,6 @@
/* Struct copy intentional */
inode->i_mtime = inode->i_atime = inode->i_ctime = zerotime;
inode->i_ino = CRAMINO(cramfs_inode);
- /* inode->i_nlink is left 1 - arguably wrong for directories,
- but it's the best we can do without reading the directory
- contents. 1 yields the right result in GNU find, even
- without -noleaf option. */
- insert_inode_hash(inode);
if (S_ISREG(inode->i_mode)) {
inode->i_fop = &generic_ro_fops;
inode->i_data.a_ops = &cramfs_aops;
@@ -75,6 +104,7 @@
init_special_inode(inode, inode->i_mode,
old_decode_dev(cramfs_inode->size));
}
+ unlock_new_inode(inode);
}
return inode;
}
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] fix cramfs making duplicate entries in inode cache
2005-08-19 23:17 ` Dave Johnson
@ 2005-08-20 2:14 ` Dave Johnson
0 siblings, 0 replies; 4+ messages in thread
From: Dave Johnson @ 2005-08-20 2:14 UTC (permalink / raw)
To: Phillip Lougher; +Cc: linux-kernel, linux-fsdevel
Dave Johnson writes:
> Phillip Lougher writes:
> > Doesn't iget_locked() assume inode numbers are unique?
> >
> > In Cramfs inode numbers are set to 1 for non-data inodes (fifos,
> > sockets, devices, empty directories), i.e
> >
> > %stat device namedpipe
> > File: `device'
> > Size: 0 Blocks: 0 IO Block: 4096 character
> > special file
> > Device: 700h/1792d Inode: 1 Links: 1 Device type: 1,1
> > Access: (0644/crw-r--r--) Uid: ( 0/ root) Gid: ( 0/ root)
> > Access: 1970-01-01 01:00:00.000000000 +0100
> > Modify: 1970-01-01 01:00:00.000000000 +0100
> > Change: 1970-01-01 01:00:00.000000000 +0100
> > File: `namedpipe'
> > Size: 0 Blocks: 0 IO Block: 4096 fifo
> > Device: 700h/1792d Inode: 1 Links: 1
> > Access: (0644/prw-r--r--) Uid: ( 0/ root) Gid: ( 0/ root)
> > Access: 1970-01-01 01:00:00.000000000 +0100
> > Modify: 1970-01-01 01:00:00.000000000 +0100
> > Change: 1970-01-01 01:00:00.000000000 +0100
> >
> > Should iget5_locked() be used here?
>
> Yep, that was busted. Below patch should be better.
Doh, 2nd attempt wasn't verifying the inode number in the test
function to protect against hash collisions (iget_locked does this but
iget5_locked doesn't)
It should be good now.
--
Dave Johnson
Starent Networks
===== fs/cramfs/inode.c 1.42 vs edited =====
--- 1.42/fs/cramfs/inode.c 2005-07-14 12:24:48 -04:00
+++ edited/fs/cramfs/inode.c 2005-08-19 22:06:44 -04:00
@@ -42,12 +42,46 @@
#define CRAMINO(x) ((x)->offset?(x)->offset<<2:1)
#define OFFSET(x) ((x)->i_ino)
+
+static int cramfs_iget5_test(struct inode *inode, void *opaque)
+{
+ struct cramfs_inode * cramfs_inode = (struct cramfs_inode *)opaque;
+
+ if (inode->i_ino != CRAMINO(cramfs_inode))
+ return 0; /* does not match */
+
+ if (inode->i_ino != 1)
+ return 1;
+
+ /* all empty directories, char, block, pipe, and sock, share inode #1 */
+
+ if ((inode->i_mode != cramfs_inode->mode) ||
+ (inode->i_gid != cramfs_inode->gid) ||
+ (inode->i_uid != cramfs_inode->uid))
+ return 0; /* does not match */
+
+ if ((S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) &&
+ (inode->i_rdev != old_decode_dev(cramfs_inode->size)))
+ return 0; /* does not match */
+
+ return 1; /* matches */
+}
+
+static int cramfs_iget5_set(struct inode *inode, void *opaque)
+{
+ struct cramfs_inode * cramfs_inode = (struct cramfs_inode *)opaque;
+ inode->i_ino = CRAMINO(cramfs_inode);
+ return 0;
+}
+
static struct inode *get_cramfs_inode(struct super_block *sb, struct cramfs_inode * cramfs_inode)
{
- struct inode * inode = new_inode(sb);
+ struct inode * inode = iget5_locked(sb, CRAMINO(cramfs_inode),
+ cramfs_iget5_test, cramfs_iget5_set,
+ cramfs_inode);
static struct timespec zerotime;
- if (inode) {
+ if (inode && (inode->i_state & I_NEW)) {
inode->i_mode = cramfs_inode->mode;
inode->i_uid = cramfs_inode->uid;
inode->i_size = cramfs_inode->size;
@@ -63,7 +99,6 @@
but it's the best we can do without reading the directory
contents. 1 yields the right result in GNU find, even
without -noleaf option. */
- insert_inode_hash(inode);
if (S_ISREG(inode->i_mode)) {
inode->i_fop = &generic_ro_fops;
inode->i_data.a_ops = &cramfs_aops;
@@ -75,6 +110,7 @@
init_special_inode(inode, inode->i_mode,
old_decode_dev(cramfs_inode->size));
}
+ unlock_new_inode(inode);
}
return inode;
}
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2005-08-20 2:14 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2005-08-19 20:17 [PATCH] fix cramfs making duplicate entries in inode cache Dave Johnson
2005-08-19 21:25 ` Phillip Lougher
2005-08-19 23:17 ` Dave Johnson
2005-08-20 2:14 ` Dave Johnson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®