Re: Calling finish_unfinished() too often?

[Edward Shishkin <edward.shishkin@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

Jan Kara wrote:
> On Wed 30-04-08 22:13:35, Edward Shishkin wrote:
>   
>> Jan Kara wrote:
>>
>>     
>>>  Hi,
>>>
>>>  I was just looking into reiserfs remount code and it seems it calls
>>> finish_unfinished() whenever filesystem is remounted and isn't in 
>>> read-only
>>> mode. Isn't this unnecessary? I thought finish_unfinished() is needed only
>>> when we remount from read-only to read-write state for the first time...
>>>  
>>>       
>> Perhaps, you are right (if we don't miss some points). But we need to keep 
>> a track
>> of first successful remounts to rw state, and it requires a special flag in 
>> in-memory
>> superblock (I don't see another way).
>>
>>     
>>>  Well, I wouldn't mind the performance impact to remount that much
>>>
>>>       
>> yeah,  definitely benchmarks lack "1000 remounts"statistics ;)
>>
>>     
>>> but it
>>> would make my life with journaled quota a bit easier ;). Thanks for an
>>> answer in advance.
>>>       
>   OK, so would you accept the patch below?
>
>   

I think it should work properly, however Linus doesn't like mysterious
values.
The attached patch might look better: it does the same things, but uses
status flags.

Thanks,
Edward.

[-- Attachment #2: reiserfs-avoid-unneeded-finish_unfinished-calls.patch --]
[-- Type: text/x-patch, Size: 3350 bytes --]

We need to call finish_unfinished() only when the filesystem is remounted
read-write for the first time. This not only saves a few cycles on remount
(rather irrelevant) but also makes reiserfs handle journaled quota on remount
better - quota is the correctly reenabled on remount read-write.
So:
. add a field to the in-memory superblock for mount status flags;
. keep a track of the first completed finish_unfinished().

Signed-off-by: Edward Shishkin <edward.shishkin@gmail.com>
---
 linux-2.6.25-mm1/fs/reiserfs/inode.c            |    2 +-
 linux-2.6.25-mm1/fs/reiserfs/super.c            |   10 ++++++----
 linux-2.6.25-mm1/include/linux/reiserfs_fs_sb.h |   13 +++++++++----
 3 files changed, 16 insertions(+), 9 deletions(-)

--- linux-2.6.25-mm1/fs/reiserfs/inode.c.orig
+++ linux-2.6.25-mm1/fs/reiserfs/inode.c
@@ -1454,7 +1454,7 @@
 	   nlink==0: processing of open-unlinked and half-truncated files
 	   during mount (fs/reiserfs/super.c:finish_unfinished()). */
 	if ((inode->i_nlink == 0) &&
-	    !REISERFS_SB(inode->i_sb)->s_is_unlinked_ok) {
+	    !(REISERFS_SB(inode->i_sb)->s_mnt_status_flags & UNLINKED_OK)) {
 		reiserfs_warning(inode->i_sb,
 				 "vs-13075: reiserfs_read_locked_inode: "
 				 "dead inode read from disk %K. "
--- linux-2.6.25-mm1/fs/reiserfs/super.c.orig
+++ linux-2.6.25-mm1/fs/reiserfs/super.c
@@ -189,7 +189,7 @@
 #endif
 
 	done = 0;
-	REISERFS_SB(s)->s_is_unlinked_ok = 1;
+	REISERFS_SB(s)->s_mnt_status_flags |= UNLINKED_OK;
 	while (!retval) {
 		retval = search_item(s, &max_cpu_key, &path);
 		if (retval != ITEM_NOT_FOUND) {
@@ -298,7 +298,8 @@
 		printk("done\n");
 		done++;
 	}
-	REISERFS_SB(s)->s_is_unlinked_ok = 0;
+	REISERFS_SB(s)->s_mnt_status_flags &= ~UNLINKED_OK;
+	REISERFS_SB(s)->s_mnt_status_flags |= UNFINISHED_OK;
 
 #ifdef CONFIG_QUOTA
 	/* Turn quotas off */
@@ -1256,8 +1257,9 @@
 	s->s_dirt = 0;
 
 	if (!(*mount_flags & MS_RDONLY)) {
-		finish_unfinished(s);
-		reiserfs_xattr_init(s, *mount_flags);
+	        if (!(REISERFS_SB(s)->s_mnt_status_flags & UNFINISHED_OK))
+			finish_unfinished(s);
+	        reiserfs_xattr_init(s, *mount_flags);
 	}
 
 out_ok:
--- linux-2.6.25-mm1/include/linux/reiserfs_fs_sb.h.orig
+++ linux-2.6.25-mm1/include/linux/reiserfs_fs_sb.h
@@ -341,6 +341,14 @@
 } reiserfs_proc_info_data_t;
 #endif
 
+/* (re)mount status flags */
+#define UNLINKED_OK 1 /* set up when it's ok for reiserfs_read_inode2()
+		       * to read from disk inode with nlink==0.
+		       * Currently this is only used during
+		       * finish_unfinished() processing at mount time */
+#define UNFINISHED_OK 2 /* indicates that finish_unfinished() was
+			 * completed successfully */
+
 /* reiserfs union of in-core super block data */
 struct reiserfs_sb_info {
 	struct buffer_head *s_sbh;	/* Buffer containing the super block */
@@ -389,10 +397,7 @@
 	int s_bmaps_without_search;
 	int s_direct2indirect;
 	int s_indirect2direct;
-	/* set up when it's ok for reiserfs_read_inode2() to read from
-	   disk inode with nlink==0. Currently this is only used during
-	   finish_unfinished() processing at mount time */
-	int s_is_unlinked_ok;
+	unsigned s_mnt_status_flags; /* these flags are set in (re)mount time */
 	reiserfs_proc_info_data_t s_proc_info_data;
 	struct proc_dir_entry *procdir;
 	int reserved_blocks;	/* amount of blocks reserved for further allocations */