: reiserfs: D-clear-i_blocks.patch

PATCH In review Nikita Danilov 5 participants 7 messages Aug 2, 2001 – Aug 3, 2001

Subsystems (7)

Memory management VFS tmpfs (shmem filesystem) ext2 file system +3 more subsystems

lore

Share & tools

lore

Nikita Danilov

author

2001-08-02 15:47 UTC #0 lore raw

PATCH 1/1 : reiserfs: D-clear-i_blocks.patch

Hello, Linus,

This patch sets inode.i_blocks to zero on deletion of reiserfs file. This in particular cures hard to believe bug when saving file in EMACS caused top to loose sight of all processes:
. reiserfs didn't properly cleared i_blocks when removing
symlinks. Actually -7 was inserted into unsigned i_blocks field. This didn't usually hurt because file is being deleted;
. inode is reused for procfs and neither get_new_inode() nor
proc_read_inode() cleared i_blocks;
. now procfs inode has huge i_blocks field;
. top calls stat on it and libc wrapper returns EOVERFLOW, as i_blocks
doesn't fit into user-level struct.
. top sees nothing.

[lkml: please CC me, I am not subscribed.]

Nikita.

fs/reiserfs/inode.c +1 -0

diff -rup linux-2.4.8-pre3/fs/reiserfs/inode.c linux-2.4.8-pre3.patched/fs/reiserfs/inode.c
@@ -55,6 +55,7 @@ void reiserfs_delete_inode (struct inode
 	;
     }
     clear_inode (inode); /* note this must go after the journal_end to prevent deadlock */
+    inode->i_blocks = 0;
     unlock_kernel() ;
 }

Andreas Dilger

2001-08-02 17:28 UTC #1 lore raw

Quoted 4 lines · Nikita

This patch sets inode.i_blocks to zero on deletion of reiserfs file. This in particular cures hard to believe bug when saving file in EMACS caused top to loose sight of all processes:
. reiserfs didn't properly cleared i_blocks when removing

Quoted 8 lines

symlinks. Actually -7 was inserted into unsigned i_blocks field. This didn't usually hurt because file is being deleted;
. inode is reused for procfs and neither get_new_inode() nor
proc_read_inode() cleared i_blocks;
. now procfs inode has huge i_blocks field;
. top calls stat on it and libc wrapper returns EOVERFLOW, as i_blocks
doesn't fit into user-level struct.
. top sees nothing.

I wonder if you don't have a different problem hiding in there somewhere. I was thinking that if a non-zero i_blocks field was causing problems for reiserfs, maybe clear_inode() should zero this out for everyone.

Looking through the reiserfs code (which is rather hard to follow), I see that reiserfs_cut_from_item() and prepare_for_delete_or_cut() are changing i_blocks, and this is (eventually) called from reiserfs_delete_item() and reiserfs_delete_object(). Not that I can point to any specific code, but to me it would seem that you are not counting i_blocks correctly somewhere, and eventually decrement i_blocks too much (hence -7 in i_blocks).

So this makes me believe that setting i_blocks=0 is just hiding a block leak somewhere in the reiserfs code, or math errors somewhere.

It is also true that clear_inode() or get_new_inode() (or proc_read_inode()) should initialize all of the used fields to zero so that a problem in one filesystem can't cause problems elsewhere. It appears that i_blocks is set to zero by ext2_new_inode(), so probably proc_read_inode() should do the same (since it uses i_blocks).

Cheers, Andreas

PS - could someone on the reiserfs team (or Linus) run the reiserfs code
through "indent" (or auto format in emacs) per Documentation/CodingStyle? It is really a gross mess, to such a point that you can hardly see what is going on. It's not just that it is a different indent style, it has no coherent indentation or comment formatting at all. Maybe for 2.5?

Signature · 3 lines

Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert

Nikita Danilov

author

2001-08-02 18:32 UTC #2 lore raw

Quoted 18 lines · Andreas Dilger

Quoted 4 lines · Nikita
This patch sets inode.i_blocks to zero on deletion of reiserfs file. This in particular cures hard to believe bug when saving file in EMACS caused top to loose sight of all processes:
. reiserfs didn't properly cleared i_blocks when removing
Quoted 8 lines
symlinks. Actually -7 was inserted into unsigned i_blocks field. This didn't usually hurt because file is being deleted;
. inode is reused for procfs and neither get_new_inode() nor
proc_read_inode() cleared i_blocks;
. now procfs inode has huge i_blocks field;
. top calls stat on it and libc wrapper returns EOVERFLOW, as i_blocks
doesn't fit into user-level struct.
. top sees nothing.
I wonder if you don't have a different problem hiding in there somewhere. I was thinking that if a non-zero i_blocks field was causing problems for reiserfs, maybe clear_inode() should zero this out for everyone.

Quoted 6 lines

Looking through the reiserfs code (which is rather hard to follow), I see that reiserfs_cut_from_item() and prepare_for_delete_or_cut() are changing i_blocks, and this is (eventually) called from reiserfs_delete_item() and reiserfs_delete_object(). Not that I can point to any specific code, but to me it would seem that you are not counting i_blocks correctly somewhere, and eventually decrement i_blocks too much (hence -7 in i_blocks).

The code path in which symlink gets i_blocks == -7 is precisely known and only occurs when its being deleted, so it's not a problem.

Quoted 7 lines

So this makes me believe that setting i_blocks=0 is just hiding a block leak somewhere in the reiserfs code, or math errors somewhere.
It is also true that clear_inode() or get_new_inode() (or proc_read_inode()) should initialize all of the used fields to zero so that a problem in one filesystem can't cause problems elsewhere. It appears that i_blocks is

Yes, I agree with you and Alexander on this. We just wanted this glaring bug to fixed immediately and so sent out as small patch as possible.

Quoted 8 lines

set to zero by ext2_new_inode(), so probably proc_read_inode() should do the same (since it uses i_blocks).
Cheers, Andreas
PS - could someone on the reiserfs team (or Linus) run the reiserfs code
through "indent" (or auto format in emacs) per Documentation/CodingStyle? It is really a gross mess, to such a point that you can hardly see what

Oh, yes.

Quoted 2 lines

is going on. It's not just that it is a different indent style, it has no coherent indentation or comment formatting at all. Maybe for 2.5?

I hope so. We already have 200k of cleanup patches in 2.4.7-ac3.

Quoted 4 lines

Signature · 3 lines

Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert

Nikita.

Alexander Viro

2001-08-02 17:45 UTC #3 lore raw

In reply to Nikita Danilov

Quoted 15 lines · Nikita Danilov

Hello, Linus,
This patch sets inode.i_blocks to zero on deletion of reiserfs file. This in particular cures hard to believe bug when saving file in EMACS caused top to loose sight of all processes:
. reiserfs didn't properly cleared i_blocks when removing
symlinks. Actually -7 was inserted into unsigned i_blocks field. This didn't usually hurt because file is being deleted;
. inode is reused for procfs and neither get_new_inode() nor
proc_read_inode() cleared i_blocks;
. now procfs inode has huge i_blocks field;
. top calls stat on it and libc wrapper returns EOVERFLOW, as i_blocks
doesn't fit into user-level struct.
. top sees nothing.

Quoted 1 line

[lkml: please CC me, I am not subscribed.]

Thanks for spotting, but I have to disagree with analysis.

	a) it's a procfs bug. We leave ->i_blocks uninitialized and inode

constructor leaves it in undefined state.
b) I'd rather fix it once in fs/inode.c::clean_inode() instead of
hunting similar bugs down again and again.

See if the patch below works for you. It makes sure that inodes passed to ->read_inode() or returned by new_inode() have zero in i_blocks and removes the redundant assignments in filesystems. Warning: it's completely untested.

drivers/isdn/avmb1/capifs.c +0 -1

diff -urN S8-pre3/drivers/isdn/avmb1/capifs.c S8-pre3-i_blocks/drivers/isdn/avmb1/capifs.c
@@ -389,7 +389,6 @@
 	struct inode *inode = new_inode(sb);
 	if (inode) {
 		inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
-		inode->i_blocks = 0;
 		inode->i_blksize = 1024;
 		inode->i_uid = inode->i_gid = 0;
 	}

fs/autofs/inode.c +0 -1

diff -urN S8-pre3/fs/autofs/inode.c S8-pre3-i_blocks/fs/autofs/inode.c
@@ -212,7 +212,6 @@
 	inode->i_mode = S_IFDIR | S_IRUGO | S_IXUGO;
 	inode->i_nlink = 2;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
-	inode->i_blocks = 0;
 	inode->i_blksize = 1024;
 
 	if ( ino == AUTOFS_ROOT_INO ) {

fs/autofs4/inode.c +0 -1

diff -urN S8-pre3/fs/autofs4/inode.c S8-pre3-i_blocks/fs/autofs4/inode.c
@@ -307,7 +307,6 @@
 		inode->i_gid = 0;
 	}
 	inode->i_blksize = PAGE_CACHE_SIZE;
-	inode->i_blocks = 0;
 	inode->i_rdev = 0;
 	inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;

fs/bfs/dir.c +1 -1

diff -urN S8-pre3/fs/bfs/dir.c S8-pre3-i_blocks/fs/bfs/dir.c
@@ -93,7 +93,7 @@
 	inode->i_uid = current->fsuid;
 	inode->i_gid = (dir->i_mode & S_ISGID) ? dir->i_gid : current->fsgid;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
-	inode->i_blocks = inode->i_blksize = 0;
+	inode->i_blksize = 0;
 	inode->i_op = &bfs_file_inops;
 	inode->i_fop = &bfs_file_operations;
 	inode->i_mapping->a_ops = &bfs_aops;

fs/devfs/base.c +0 -1

diff -urN S8-pre3/fs/devfs/base.c S8-pre3-i_blocks/fs/devfs/base.c
@@ -2263,7 +2263,6 @@
 	printk ("%s: read_inode(%d): VFS inode: %p  devfs_entry: %p\n",
 		DEVFS_NAME, (int) inode->i_ino, inode, de);
 #endif
-    inode->i_blocks = 0;
     inode->i_blksize = 1024;
     inode->i_op = &devfs_iops;
     inode->i_fop = &devfs_fops;

fs/devpts/inode.c +0 -2

diff -urN S8-pre3/fs/devpts/inode.c S8-pre3-i_blocks/fs/devpts/inode.c
@@ -145,7 +145,6 @@
 		goto fail_free;
 	inode->i_ino = 1;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
-	inode->i_blocks = 0;
 	inode->i_blksize = 1024;
 	inode->i_uid = inode->i_gid = 0;
 	inode->i_mode = S_IFDIR | S_IRUGO | S_IXUGO | S_IWUSR;

@@ -193,7 +192,6 @@
 	if (!inode)
 		return;
 	inode->i_ino = number+2;
-	inode->i_blocks = 0;
 	inode->i_blksize = 1024;
 	inode->i_uid = sbi->setuid ? sbi->uid : current->fsuid;
 	inode->i_gid = sbi->setgid ? sbi->gid : current->fsgid;

fs/efs/inode.c +1 -4

diff -urN S8-pre3/fs/efs/inode.c S8-pre3-i_blocks/fs/efs/inode.c
@@ -93,11 +93,8 @@
 	inode->i_ctime = be32_to_cpu(efs_inode->di_ctime);
 
 	/* this is the number of blocks in the file */
-	if (inode->i_size == 0) {
-		inode->i_blocks = 0;
-	} else {
+	if (inode->i_size)
 		inode->i_blocks = ((inode->i_size - 1) >> EFS_BLOCKSIZE_BITS) + 1;
-	}
 
 	/*
 	 * BUG: irix dev_t is 32-bits. linux dev_t is only 16-bits.

fs/ext2/ialloc.c +0 -1

diff -urN S8-pre3/fs/ext2/ialloc.c S8-pre3-i_blocks/fs/ext2/ialloc.c
@@ -431,7 +431,6 @@
 
 	inode->i_ino = j;
 	inode->i_blksize = PAGE_SIZE;	/* This is the optimal IO size (for stat), not the fs block size */
-	inode->i_blocks = 0;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	inode->u.ext2_i.i_new_inode = 1;
 	inode->u.ext2_i.i_flags = dir->u.ext2_i.i_flags;

fs/inode.c +1 -0

diff -urN S8-pre3/fs/inode.c S8-pre3-i_blocks/fs/inode.c
@@ -750,6 +750,7 @@
 	inode->i_nlink = 1;
 	atomic_set(&inode->i_writecount, 0);
 	inode->i_size = 0;
+	inode->i_blocks = 0;
 	inode->i_generation = 0;
 	memset(&inode->i_dquot, 0, sizeof(inode->i_dquot));
 	inode->i_pipe = NULL;

fs/isofs/inode.c +1 -1

diff -urN S8-pre3/fs/isofs/inode.c S8-pre3-i_blocks/fs/isofs/inode.c
@@ -1173,7 +1173,7 @@
 	}
 	inode->i_uid = inode->i_sb->u.isofs_sb.s_uid;
 	inode->i_gid = inode->i_sb->u.isofs_sb.s_gid;
-	inode->i_blocks = inode->i_blksize = 0;
+	inode->i_blksize = 0;
 
 
 	inode->u.isofs_i.i_section_size = isonum_733 (de->size);

fs/minix/bitmap.c +1 -1

diff -urN S8-pre3/fs/minix/bitmap.c S8-pre3-i_blocks/fs/minix/bitmap.c
@@ -261,7 +261,7 @@
 	inode->i_gid = (dir->i_mode & S_ISGID) ? dir->i_gid : current->fsgid;
 	inode->i_ino = j;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
-	inode->i_blocks = inode->i_blksize = 0;
+	inode->i_blksize = 0;
 	insert_inode_hash(inode);
 	mark_inode_dirty(inode);

fs/minix/inode.c +2 -2

diff -urN S8-pre3/fs/minix/inode.c S8-pre3-i_blocks/fs/minix/inode.c
@@ -456,7 +456,7 @@
 	inode->i_nlink = raw_inode->i_nlinks;
 	inode->i_size = raw_inode->i_size;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = raw_inode->i_time;
-	inode->i_blocks = inode->i_blksize = 0;
+	inode->i_blksize = 0;
 	for (block = 0; block < 9; block++)
 		inode->u.minix_i.u.i1_data[block] = raw_inode->i_zone[block];
 	if (S_ISREG(inode->i_mode)) {

@@ -509,7 +509,7 @@
 	inode->i_mtime = raw_inode->i_mtime;
 	inode->i_atime = raw_inode->i_atime;
 	inode->i_ctime = raw_inode->i_ctime;
-	inode->i_blocks = inode->i_blksize = 0;
+	inode->i_blksize = 0;
 	for (block = 0; block < 10; block++)
 		inode->u.minix_i.u.i2_data[block] = raw_inode->i_zone[block];
 	if (S_ISREG(inode->i_mode)) {

fs/ramfs/inode.c +0 -1

diff -urN S8-pre3/fs/ramfs/inode.c S8-pre3-i_blocks/fs/ramfs/inode.c
@@ -117,7 +117,6 @@
 		inode->i_uid = current->fsuid;
 		inode->i_gid = current->fsgid;
 		inode->i_blksize = PAGE_CACHE_SIZE;
-		inode->i_blocks = 0;
 		inode->i_rdev = NODEV;
 		inode->i_mapping->a_ops = &ramfs_aops;
 		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;

fs/reiserfs/inode.c +0 -1

diff -urN S8-pre3/fs/reiserfs/inode.c S8-pre3-i_blocks/fs/reiserfs/inode.c
@@ -940,7 +940,6 @@
 	inode->i_op = &page_symlink_inode_operations;
 	inode->i_mapping->a_ops = &reiserfs_address_space_operations;
     } else {
-	inode->i_blocks = 0;
 	init_special_inode(inode, inode->i_mode, rdev) ;
     }
 }

fs/sysv/ialloc.c +1 -1

diff -urN S8-pre3/fs/sysv/ialloc.c S8-pre3-i_blocks/fs/sysv/ialloc.c
@@ -164,7 +164,7 @@
 	inode->i_uid = current->fsuid;
 	inode->i_ino = fs16_to_cpu(sb, ino);
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
-	inode->i_blocks = inode->i_blksize = 0;
+	inode->i_blksize = 0;
 	insert_inode_hash(inode);
 	mark_inode_dirty(inode);

fs/sysv/inode.c +1 -1

diff -urN S8-pre3/fs/sysv/inode.c S8-pre3-i_blocks/fs/sysv/inode.c
@@ -171,7 +171,7 @@
 	inode->i_atime = fs32_to_cpu(sb, raw_inode->i_atime);
 	inode->i_mtime = fs32_to_cpu(sb, raw_inode->i_mtime);
 	inode->i_ctime = fs32_to_cpu(sb, raw_inode->i_ctime);
-	inode->i_blocks = inode->i_blksize = 0;
+	inode->i_blksize = 0;
 	for (block = 0; block < 10+1+1+1; block++)
 		read3byte(sb, &raw_inode->i_a.i_addb[3*block],
 			(unsigned char*)&inode->u.sysv_i.i_data[block]);

fs/udf/ialloc.c +0 -1

diff -urN S8-pre3/fs/udf/ialloc.c S8-pre3-i_blocks/fs/udf/ialloc.c
@@ -128,7 +128,6 @@
 	UDF_I_LOCATION(inode).partitionReferenceNum = UDF_I_LOCATION(dir).partitionReferenceNum;
 	inode->i_ino = udf_get_lb_pblock(sb, UDF_I_LOCATION(inode), 0);
 	inode->i_blksize = PAGE_SIZE;
-	inode->i_blocks = 0;
 	UDF_I_LENEATTR(inode) = 0;
 	UDF_I_LENALLOC(inode) = 0;
 	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_USE_EXTENDED_FE))

fs/ufs/ialloc.c +0 -1

diff -urN S8-pre3/fs/ufs/ialloc.c S8-pre3-i_blocks/fs/ufs/ialloc.c
@@ -268,7 +268,6 @@
 
 	inode->i_ino = cg * uspi->s_ipg + bit;
 	inode->i_blksize = PAGE_SIZE;	/* This is the optimal IO size (for stat), not the fs block size */
-	inode->i_blocks = 0;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	inode->u.ufs_i.i_flags = dir->u.ufs_i.i_flags;
 	inode->u.ufs_i.i_lastfrag = 0;

mm/shmem.c +0 -1

diff -urN S8-pre3/mm/shmem.c S8-pre3-i_blocks/mm/shmem.c
@@ -508,7 +508,6 @@
 		inode->i_uid = current->fsuid;
 		inode->i_gid = current->fsgid;
 		inode->i_blksize = PAGE_CACHE_SIZE;
-		inode->i_blocks = 0;
 		inode->i_rdev = NODEV;
 		inode->i_mapping->a_ops = &shmem_aops;
 		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;

Hans Reiser

2001-08-02 19:22 UTC #4 lore raw

Quoted 9 lines · Alexander Viro

Thanks for spotting, but I have to disagree with analysis.
        a) it's a procfs bug. We leave ->i_blocks uninitialized and inode
constructor leaves it in undefined state.
b) I'd rather fix it once in fs/inode.c::clean_inode() instead of
hunting similar bugs down again and again.
See if the patch below works for you. It makes sure that inodes passed to ->read_inode() or returned by new_inode() have zero in i_blocks and removes the redundant assignments in filesystems. Warning: it's completely untested.

Nikita sent you an email saying that he though you should fix it in the manner you fixed it above, and then posted a patch which was localized in its impact to just ReiserFS because he was sure it was correct.

Nikita should be more aggressive in patching VFS rather than asking you to do it, but I don't think you can claim to be differing with his analysis.

Hans

[email protected]

2001-08-03 12:53 UTC #5 lore raw

In reply to Nikita Danilov

Quoted 8 lines · Nikita Danilov

Quoted 4 lines · Andreas Dilger
PS - could someone on the reiserfs team (or Linus) run the reiserfs code
through "indent" (or auto format in emacs) per Documentation/CodingStyle? It is really a gross mess, to such a point that you can hardly see what
Oh, yes.

Quoted 7 lines

Quoted 3 lines
is going on. It's not just that it is a different indent style, it has no coherent indentation or comment formatting at all. Maybe for 2.5?
I hope so. We already have 200k of cleanup patches in 2.4.7-ac3.
From past experience, if you want to run the code through indent, I would

recomend that you submit patches for that which contain no other changes. If you mix indent changes with code changes, it gets very difficult to see what was actually changed if bugs crop up.

Thanks,

Jim

Nikita Danilov

author

2001-08-03 16:18 UTC #6 lore raw

Quoted 13 lines · [email protected] writes:

Quoted 12 lines · Nikita Danilov
Quoted 4 lines · Andreas Dilger
PS - could someone on the reiserfs team (or Linus) run the reiserfs code
through "indent" (or auto format in emacs) per Documentation/CodingStyle? It is really a gross mess, to such a point that you can hardly see what
Oh, yes.
Quoted 3 lines
is going on. It's not just that it is a different indent style, it has no coherent indentation or comment formatting at all. Maybe for 2.5?

Quoted 6 lines

Quoted 3 lines
I hope so. We already have 200k of cleanup patches in 2.4.7-ac3.
From past experience, if you want to run the code through indent, I would
recomend that you submit patches for that which contain no other changes. If you mix indent changes with code changes, it gets very difficult to see what was actually changed if bugs crop up.

Thank you. Do you have an experience in or opinion on asking Linus to perform such conversion himself, because this will probably save some trouble?

Quoted 4 lines

Thanks,
Jim

[lkml: please CC me, I am not subscribed.]

Nikita.