Calling finish_unfinished() too often?

Calling finish_unfinished() too often?

[Jan Kara]

  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...
  Well, I wouldn't mind the performance impact to remount that much but it
would make my life with journaled quota a bit easier ;). Thanks for an
answer in advance.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: Calling finish_unfinished() too often?

[Edward Shishkin]

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.
>
>								Honza
>  
>

Re: Calling finish_unfinished() too often?

[Jan Kara]

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?

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
---

From 1eb1dbd8ebb518b1299bd2c45908db988b56a287 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Tue, 6 May 2008 19:13:38 +0200
Subject: [PATCH] reiserfs: Call finish_unfinished() only during first remount RW

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.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/reiserfs/inode.c            |    2 +-
 fs/reiserfs/super.c            |    7 ++++---
 include/linux/reiserfs_fs_sb.h |    9 +++++----
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
index 5791793..c2fdeb4 100644
--- a/fs/reiserfs/inode.c
+++ b/fs/reiserfs/inode.c
@@ -1454,7 +1454,7 @@ void reiserfs_read_locked_inode(struct inode *inode,
 	   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_unlinked_cleanup != 1) {
 		reiserfs_warning(inode->i_sb,
 				 "vs-13075: reiserfs_read_locked_inode: "
 				 "dead inode read from disk %K. "
diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
index 2c3aa5f..7faed9c 100644
--- a/fs/reiserfs/super.c
+++ b/fs/reiserfs/super.c
@@ -189,7 +189,7 @@ static int finish_unfinished(struct super_block *s)
 #endif
 
 	done = 0;
-	REISERFS_SB(s)->s_is_unlinked_ok = 1;
+	REISERFS_SB(s)->s_unlinked_cleanup = 1;
 	while (!retval) {
 		retval = search_item(s, &max_cpu_key, &path);
 		if (retval != ITEM_NOT_FOUND) {
@@ -298,7 +298,7 @@ static int finish_unfinished(struct super_block *s)
 		printk("done\n");
 		done++;
 	}
-	REISERFS_SB(s)->s_is_unlinked_ok = 0;
+	REISERFS_SB(s)->s_unlinked_cleanup = 2;
 
 #ifdef CONFIG_QUOTA
 	/* Turn quotas off */
@@ -1290,7 +1290,8 @@ static int reiserfs_remount(struct super_block *s, int *mount_flags, char *arg)
 	s->s_dirt = 0;
 
 	if (!(*mount_flags & MS_RDONLY)) {
-		finish_unfinished(s);
+		if (!REISERFS_SB(s)->s_unlinked_cleanup)
+			finish_unfinished(s);
 		reiserfs_xattr_init(s, *mount_flags);
 	}
 
diff --git a/include/linux/reiserfs_fs_sb.h b/include/linux/reiserfs_fs_sb.h
index db5ef9b..5c703e6 100644
--- a/include/linux/reiserfs_fs_sb.h
+++ b/include/linux/reiserfs_fs_sb.h
@@ -390,10 +390,11 @@ struct reiserfs_sb_info {
 	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;
+	/* set up to 0, initially, to 1 when orphan cleanup is in progress
+	   and to 2 when it is done. Used by reiserfs_read_inode2() to check
+	   when read of inode with nlink==0 is OK and to detect first remount
+	   read-write during which finish_unfinished() needs to be called. */
+	int s_unlinked_cleanup;
 	reiserfs_proc_info_data_t s_proc_info_data;
 	struct proc_dir_entry *procdir;
 	int reserved_blocks;	/* amount of blocks reserved for further allocations */
-- 
1.5.2.4

Re: Calling finish_unfinished() too often?

[Edward Shishkin]

[-- 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 */

Re: Calling finish_unfinished() too often?

[Jan Kara]

On Thu 08-05-08 15:44:27, Edward Shishkin wrote:
> 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.
  Great. Thanks. The patch looks fine, only I think you've added some
whitespace noise before reiserfs_xattr_init() call...

								Honza

> 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 */

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR