cramfs and named-pipe

cramfs and named-pipe

[Atsushi Nemoto]

After commit a97c9bf33f4612e2aed6f000f6b1d268b6814f3c (fix cramfs
making duplicate entries in inode cache) in kernel 2.6.14, named-pipe
on cramfs does not work properly.

It seems the commit make all named-pipe on cramfs share their inode
(and named-pipe buffer).  Is this fix correct?

diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 0c3b618..87754ff 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -61,6 +61,9 @@ static int cramfs_iget5_test(struct inode *inode, void *opaque)
 	    (inode->i_rdev != old_decode_dev(cramfs_inode->size)))
 		return 0; /* does not match */
 
+	if (S_ISFIFO(inode->i_mode))
+		return 0; /* does not match */
+
 	return 1; /* matches */
 }
 

Re: cramfs and named-pipe

[Al Viro]

On Mon, Aug 04, 2008 at 06:21:43PM +0900, Atsushi Nemoto wrote:
> After commit a97c9bf33f4612e2aed6f000f6b1d268b6814f3c (fix cramfs
> making duplicate entries in inode cache) in kernel 2.6.14, named-pipe
> on cramfs does not work properly.
> 
> It seems the commit make all named-pipe on cramfs share their inode
> (and named-pipe buffer).  Is this fix correct?

Eeek...  I'd rather not play these games with directories and devices nodes
as well.  Rationale for the original patch simply doesn't apply for those.

IOW, I think it would be much saner if we did the following: make ..._test()
refuse to merge inodes with ->i_ino == 1, take inode setup back to
get_cramfs_inode() and make ->drop_inode() evict ones with ->i_ino == 1
immediately.  Comments?

Patch below is completely untested; it builds, but that's it.

diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 0c3b618..c7722cc 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -43,58 +43,13 @@ static DEFINE_MUTEX(read_mutex);
 static int cramfs_iget5_test(struct inode *inode, void *opaque)
 {
 	struct cramfs_inode *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 */
+	return inode->i_ino == CRAMINO(cramfs_inode) && inode->i_ino != 1;
 }
 
 static int cramfs_iget5_set(struct inode *inode, void *opaque)
 {
-	static struct timespec zerotime;
 	struct cramfs_inode *cramfs_inode = opaque;
-	inode->i_mode = cramfs_inode->mode;
-	inode->i_uid = cramfs_inode->uid;
-	inode->i_size = cramfs_inode->size;
-	inode->i_blocks = (cramfs_inode->size - 1) / 512 + 1;
-	inode->i_gid = cramfs_inode->gid;
-	/* 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. */
-	if (S_ISREG(inode->i_mode)) {
-		inode->i_fop = &generic_ro_fops;
-		inode->i_data.a_ops = &cramfs_aops;
-	} else if (S_ISDIR(inode->i_mode)) {
-		inode->i_op = &cramfs_dir_inode_operations;
-		inode->i_fop = &cramfs_directory_operations;
-	} else if (S_ISLNK(inode->i_mode)) {
-		inode->i_op = &page_symlink_inode_operations;
-		inode->i_data.a_ops = &cramfs_aops;
-	} else {
-		inode->i_size = 0;
-		inode->i_blocks = 0;
-		init_special_inode(inode, inode->i_mode,
-			old_decode_dev(cramfs_inode->size));
-	}
 	return 0;
 }
 
@@ -104,12 +59,48 @@ static struct inode *get_cramfs_inode(struct super_block *sb,
 	struct inode *inode = iget5_locked(sb, CRAMINO(cramfs_inode),
 					    cramfs_iget5_test, cramfs_iget5_set,
 					    cramfs_inode);
+	static struct timespec zerotime;
+
 	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;
+		inode->i_blocks = (cramfs_inode->size - 1) / 512 + 1;
+		inode->i_gid = cramfs_inode->gid;
+		/* Struct copy intentional */
+		inode->i_mtime = inode->i_atime = inode->i_ctime = zerotime;
+		/* 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. */
+		if (S_ISREG(inode->i_mode)) {
+			inode->i_fop = &generic_ro_fops;
+			inode->i_data.a_ops = &cramfs_aops;
+		} else if (S_ISDIR(inode->i_mode)) {
+			inode->i_op = &cramfs_dir_inode_operations;
+			inode->i_fop = &cramfs_directory_operations;
+		} else if (S_ISLNK(inode->i_mode)) {
+			inode->i_op = &page_symlink_inode_operations;
+			inode->i_data.a_ops = &cramfs_aops;
+		} else {
+			inode->i_size = 0;
+			inode->i_blocks = 0;
+			init_special_inode(inode, inode->i_mode,
+				old_decode_dev(cramfs_inode->size));
+		}
 		unlock_new_inode(inode);
 	}
 	return inode;
 }
 
+static void cramfs_drop_inode(struct inode *inode)
+{
+	if (inode->i_ino == 1)
+		generic_delete_inode(inode);
+	else
+		generic_drop_inode(inode);
+}
+
 /*
  * We have our own block cache: don't fill up the buffer cache
  * with the rom-image, because the way the filesystem is set
@@ -534,6 +525,7 @@ static const struct super_operations cramfs_ops = {
 	.put_super	= cramfs_put_super,
 	.remount_fs	= cramfs_remount,
 	.statfs		= cramfs_statfs,
+	.drop_inode	= cramfs_drop_inode,
 };
 
 static int cramfs_get_sb(struct file_system_type *fs_type,

Re: cramfs and named-pipe

[Atsushi Nemoto]

On Mon, 4 Aug 2008 11:03:47 +0100, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> Eeek...  I'd rather not play these games with directories and devices nodes
> as well.  Rationale for the original patch simply doesn't apply for those.
> 
> IOW, I think it would be much saner if we did the following: make ..._test()
> refuse to merge inodes with ->i_ino == 1, take inode setup back to
> get_cramfs_inode() and make ->drop_inode() evict ones with ->i_ino == 1
> immediately.  Comments?
> 
> Patch below is completely untested; it builds, but that's it.

Thanks, your patch works well for me.  But it looks a bit large for
stable tree (100 line rule).

With current code, I think no problem on empty directories and device
nodes.  So how about fixing only FIFO case first (and send it to
stable tree) and then go to your patch?

---
Atsushi Nemoto

Re: cramfs and named-pipe

[Andrew Morton]

On Tue, 05 Aug 2008 12:09:09 +0900 (JST)
Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:

> On Mon, 4 Aug 2008 11:03:47 +0100, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> > Eeek...  I'd rather not play these games with directories and devices nodes
> > as well.  Rationale for the original patch simply doesn't apply for those.
> > 
> > IOW, I think it would be much saner if we did the following: make ..._test()
> > refuse to merge inodes with ->i_ino == 1, take inode setup back to
> > get_cramfs_inode() and make ->drop_inode() evict ones with ->i_ino == 1
> > immediately.  Comments?
> > 
> > Patch below is completely untested; it builds, but that's it.
> 
> Thanks, your patch works well for me.  But it looks a bit large for
> stable tree (100 line rule).
> 
> With current code, I think no problem on empty directories and device
> nodes.  So how about fixing only FIFO case first (and send it to
> stable tree) and then go to your patch?
> 

Nothing seems to have happened.  Al, do you think your (now tested) patch
is good for 2.6.27 and 2.6.26.x?  And, it seems, 2.6.25.x.  (All the way
down to 2.6.14.x!)

Re: cramfs and named-pipe

[Andrew Morton]

On Fri, 15 Aug 2008 15:52:20 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 05 Aug 2008 12:09:09 +0900 (JST)
> Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> 
> > On Mon, 4 Aug 2008 11:03:47 +0100, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> > > Eeek...  I'd rather not play these games with directories and devices nodes
> > > as well.  Rationale for the original patch simply doesn't apply for those.
> > > 
> > > IOW, I think it would be much saner if we did the following: make ..._test()
> > > refuse to merge inodes with ->i_ino == 1, take inode setup back to
> > > get_cramfs_inode() and make ->drop_inode() evict ones with ->i_ino == 1
> > > immediately.  Comments?
> > > 
> > > Patch below is completely untested; it builds, but that's it.
> > 
> > Thanks, your patch works well for me.  But it looks a bit large for
> > stable tree (100 line rule).
> > 
> > With current code, I think no problem on empty directories and device
> > nodes.  So how about fixing only FIFO case first (and send it to
> > stable tree) and then go to your patch?
> > 
> 
> Nothing seems to have happened.  Al, do you think your (now tested) patch
> is good for 2.6.27 and 2.6.26.x?  And, it seems, 2.6.25.x.  (All the way
> down to 2.6.14.x!)

<crickets chirping>

Oh well, I'll send it in.