[PATCH 0/3] v2: write_super clean up

[PATCH 0/3] v2: write_super clean up

[Jiro SEKIBA]

Hi, this is revised version of write_super clean up patch.

It reflects most of Konishi-san's comment, except the nilfs_discontined()
inline functions to be renamed.  That inline funcion is one of inline
functions defined by single macro.  So as Konishi-san said, I'd rather
like to rename it as separate patch.

Any comments and suggestions are appreciate.

Thanks,

regards,

 fs/nilfs2/segment.c   |   12 +++++++++++-
 fs/nilfs2/super.c     |   16 +++++++---------
 fs/nilfs2/the_nilfs.h |   12 ++++++++++++
 3 files changed, 30 insertions(+), 10 deletions(-)
-- 
Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>

[PATCH 1/3] nilfs2: fix disorder of nilfs_write_super in nilfs_sync_fs

[Jiro SEKIBA]

fix disorder of nilfs_write_super in nilfs_sync_fs.
commiting super block must be the end of the function
so that every chanages are reflected.

Signed-off-by: Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>
---
 fs/nilfs2/super.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 8e2ec43..ba69601 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -388,11 +388,12 @@ static int nilfs_sync_fs(struct super_block *sb, int wait)
 {
 	int err = 0;
 
-	nilfs_write_super(sb);
-
 	/* This function is called when super block should be written back */
 	if (wait)
 		err = nilfs_construct_segment(sb);
+
+	nilfs_write_super(sb);
+
 	return err;
 }
 
-- 
1.5.6.5

Re: [PATCH 1/3] nilfs2: fix disorder of nilfs_write_super in nilfs_sync_fs

[Ryusuke Konishi]

Hi,
On Sun, 19 Jul 2009 14:26:20 +0900, Jiro SEKIBA wrote:
> fix disorder of nilfs_write_super in nilfs_sync_fs.
> commiting super block must be the end of the function
> so that every chanages are reflected.
> 
> Signed-off-by: Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>
> ---
>  fs/nilfs2/super.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> index 8e2ec43..ba69601 100644
> --- a/fs/nilfs2/super.c
> +++ b/fs/nilfs2/super.c
> @@ -388,11 +388,12 @@ static int nilfs_sync_fs(struct super_block *sb, int wait)
>  {
>  	int err = 0;
>  
> -	nilfs_write_super(sb);
> -
>  	/* This function is called when super block should be written back */
>  	if (wait)
>  		err = nilfs_construct_segment(sb);
> +
> +	nilfs_write_super(sb);
> +

I read callers of the sync_fs callback, and I felt that
nilfs_sync_fs() would rather call nilfs_commit_super() directly.

nilfs_write_super is written to handle periodic callback,
and it moderates frequency of actual write back.

Is sync_fs called periodically or frequently?

If neither was, I think it's only necessary to do:

    down_write(&nilfs->ns_sem);
    if (sb->s_dirt)
        nilfs_commit_super(NILFS_SB(sb), 1);
    up_write(&nilfs->ns_sem);

Note that ->sync_fs() is not called for read-only filesystems.

The recent migration of write_super into sync_fs made this
transformation possible.

Thanks,
Ryusuke Konishi

>  	return err;
>  }
>  
> -- 
> 1.5.6.5
> 
> _______________________________________________
> users mailing list
> users-JrjvKiOkagjYtjvyW6yDsg@public.gmane.org
> https://www.nilfs.org/mailman/listinfo/users

Re: [PATCH 1/3] nilfs2: fix disorder of nilfs_write_super in nilfs_sync_fs

[Jiro SEKIBA]

Hi,

At Sun, 19 Jul 2009 19:53:58 +0900 (JST),
Ryusuke Konishi wrote:
> 
> Hi,
> On Sun, 19 Jul 2009 14:26:20 +0900, Jiro SEKIBA wrote:
> > fix disorder of nilfs_write_super in nilfs_sync_fs.
> > commiting super block must be the end of the function
> > so that every chanages are reflected.
> > 
> > Signed-off-by: Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>
> > ---
> >  fs/nilfs2/super.c |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> > index 8e2ec43..ba69601 100644
> > --- a/fs/nilfs2/super.c
> > +++ b/fs/nilfs2/super.c
> > @@ -388,11 +388,12 @@ static int nilfs_sync_fs(struct super_block *sb, int wait)
> >  {
> >  	int err = 0;
> >  
> > -	nilfs_write_super(sb);
> > -
> >  	/* This function is called when super block should be written back */
> >  	if (wait)
> >  		err = nilfs_construct_segment(sb);
> > +
> > +	nilfs_write_super(sb);
> > +
> 
> I read callers of the sync_fs callback, and I felt that
> nilfs_sync_fs() would rather call nilfs_commit_super() directly.
> 
> nilfs_write_super is written to handle periodic callback,
> and it moderates frequency of actual write back.
> 
> Is sync_fs called periodically or frequently?

I added printk in nilfs_sync_fs and nilfs_write_super.
As you mentioned, nilfs_sync_fs is not called periodically
nor frequently like nilfs_write_super.

> If neither was, I think it's only necessary to do:
> 
>     down_write(&nilfs->ns_sem);
>     if (sb->s_dirt)
>         nilfs_commit_super(NILFS_SB(sb), 1);
>     up_write(&nilfs->ns_sem);
> 
> Note that ->sync_fs() is not called for read-only filesystems.

So this change be applicable.

thank you for the comments! I'll revise patches and resent it.

> The recent migration of write_super into sync_fs made this
> transformation possible.
>
> Thanks,
> Ryusuke Konishi
> 
> >  	return err;
> >  }
> >  
> > -- 
> > 1.5.6.5
> > 
> > _______________________________________________
> > users mailing list
> > users-JrjvKiOkagjYtjvyW6yDsg@public.gmane.org
> > https://www.nilfs.org/mailman/listinfo/users
> _______________________________________________
> users mailing list
> users-JrjvKiOkagjYtjvyW6yDsg@public.gmane.org
> https://www.nilfs.org/mailman/listinfo/users
> 
> 
> 

-- 
Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>

[PATCH 2/3] nilfs2: clean up nilfs_write_super

[Jiro SEKIBA]

Separated conditions to check if syncing super block and alternative
super block are required as inline functions to reuse the conditions.

Signed-off-by: Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>
---
 fs/nilfs2/super.c     |    9 +++------
 fs/nilfs2/the_nilfs.h |   12 ++++++++++++
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index ba69601..d85b360 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -368,17 +368,14 @@ static void nilfs_write_super(struct super_block *sb)
 
 	down_write(&nilfs->ns_sem);
 	if (!(sb->s_flags & MS_RDONLY)) {
-		struct nilfs_super_block **sbp = nilfs->ns_sbp;
 		u64 t = get_seconds();
-		int dupsb;
 
-		if (!nilfs_discontinued(nilfs) && t >= nilfs->ns_sbwtime[0] &&
-		    t < nilfs->ns_sbwtime[0] + NILFS_SB_FREQ) {
+		if (!nilfs_discontinued(nilfs) &&
+		    !nilfs_sb_need_update(nilfs, t)) {
 			up_write(&nilfs->ns_sem);
 			return;
 		}
-		dupsb = sbp[1] && t > nilfs->ns_sbwtime[1] + NILFS_ALTSB_FREQ;
-		nilfs_commit_super(sbi, dupsb);
+		nilfs_commit_super(sbi, nilfs_altsb_need_update(nilfs, t));
 	}
 	sb->s_dirt = 0;
 	up_write(&nilfs->ns_sem);
diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
index e8adbff..0a77fb5 100644
--- a/fs/nilfs2/the_nilfs.h
+++ b/fs/nilfs2/the_nilfs.h
@@ -202,6 +202,18 @@ THE_NILFS_FNS(DISCONTINUED, discontinued)
 #define NILFS_SB_FREQ		10
 #define NILFS_ALTSB_FREQ	60  /* spare superblock */
 
+static inline int nilfs_sb_need_update(struct the_nilfs *nilfs, u64 t)
+{
+	return t < nilfs->ns_sbwtime[0] ||
+		 t > nilfs->ns_sbwtime[0] + NILFS_SB_FREQ;
+}
+
+static inline int nilfs_altsb_need_update(struct the_nilfs *nilfs, u64 t)
+{
+	struct nilfs_super_block **sbp = nilfs->ns_sbp;
+	return sbp[1] && t > nilfs->ns_sbwtime[1] + NILFS_ALTSB_FREQ;
+}
+
 void nilfs_set_last_segment(struct the_nilfs *, sector_t, u64, __u64);
 struct the_nilfs *find_or_create_nilfs(struct block_device *);
 void put_nilfs(struct the_nilfs *);
-- 
1.5.6.5

[PATCH 3/3] nilfs2: stop using periodic write_super callback

[Jiro SEKIBA]

This patch makes commiting super block in nilfs internal thread,
instead of periodic write_super callback.

VFS layer calls ->write_super callback periodically.  However,
it looks like that calling back is ommited when disk I/O is busy.
And when cleanerd (nilfs GC) is runnig, disk I/O tend to be busy thus
nilfs superblock is not synchronized as nilfs designed.

To avoid it, syncing superblock by nilfs thread instead of pdflush.

Signed-off-by: Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>
---
 fs/nilfs2/segment.c |   12 +++++++++++-
 fs/nilfs2/super.c   |    2 +-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
index 8b5e477..46f7a49 100644
--- a/fs/nilfs2/segment.c
+++ b/fs/nilfs2/segment.c
@@ -2486,8 +2486,10 @@ static int nilfs_segctor_construct(struct nilfs_sc_info *sci,
 			atomic_set(&nilfs->ns_ndirtyblks, 0);
 		if (test_bit(NILFS_SC_SUPER_ROOT, &sci->sc_flags) &&
 		    nilfs_discontinued(nilfs)) {
+			u64 t = get_seconds();
 			down_write(&nilfs->ns_sem);
-			req->sb_err = nilfs_commit_super(sbi, 0);
+			req->sb_err = nilfs_commit_super(sbi,
+					nilfs_altsb_need_update(nilfs, t));
 			up_write(&nilfs->ns_sem);
 		}
 	}
@@ -2675,6 +2677,9 @@ static int nilfs_segctor_thread(void *arg)
 	} else {
 		DEFINE_WAIT(wait);
 		int should_sleep = 1;
+		u64 t;
+		struct nilfs_sb_info *sbi;
+		struct the_nilfs *nilfs;
 
 		prepare_to_wait(&sci->sc_wait_daemon, &wait,
 				TASK_INTERRUPTIBLE);
@@ -2695,6 +2700,11 @@ static int nilfs_segctor_thread(void *arg)
 		finish_wait(&sci->sc_wait_daemon, &wait);
 		timeout = ((sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
 			   time_after_eq(jiffies, sci->sc_timer->expires));
+		t = get_seconds();
+		sbi = sci->sc_sbi;
+		nilfs = sbi->s_nilfs;
+		if (sci->sc_super->s_dirt && nilfs_sb_need_update(nilfs, t))
+			set_nilfs_discontinued(nilfs);
 	}
 	goto loop;
 
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index d85b360..a8c5875 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -534,7 +534,7 @@ static struct super_operations nilfs_sops = {
 	/* .drop_inode	  = nilfs_drop_inode, */
 	.delete_inode   = nilfs_delete_inode,
 	.put_super      = nilfs_put_super,
-	.write_super    = nilfs_write_super,
+	/* .write_super    = nilfs_write_super, */
 	.sync_fs        = nilfs_sync_fs,
 	/* .write_super_lockfs */
 	/* .unlockfs */
-- 
1.5.6.5

Re: [PATCH 3/3] nilfs2: stop using periodic write_super callback

[Ryusuke Konishi]

On Sun, 19 Jul 2009 14:26:22 +0900, Jiro SEKIBA wrote:
> This patch makes commiting super block in nilfs internal thread,
> instead of periodic write_super callback.
> 
> VFS layer calls ->write_super callback periodically.  However,
> it looks like that calling back is ommited when disk I/O is busy.
> And when cleanerd (nilfs GC) is runnig, disk I/O tend to be busy thus
> nilfs superblock is not synchronized as nilfs designed.
> 
> To avoid it, syncing superblock by nilfs thread instead of pdflush.
> 
> Signed-off-by: Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>

Thanks.

If you replaced nilfs_write_super() in nilfs_sync_super(), you can get
rid of nilfs_write_super() in this patch including the comments and
prototype declaration for it.

Here are a few additional comments on that condition.

> ---
>  fs/nilfs2/segment.c |   12 +++++++++++-
>  fs/nilfs2/super.c   |    2 +-
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
> index 8b5e477..46f7a49 100644
> --- a/fs/nilfs2/segment.c
> +++ b/fs/nilfs2/segment.c
> @@ -2486,8 +2486,10 @@ static int nilfs_segctor_construct(struct nilfs_sc_info *sci,
>  			atomic_set(&nilfs->ns_ndirtyblks, 0);
>  		if (test_bit(NILFS_SC_SUPER_ROOT, &sci->sc_flags) &&
>  		    nilfs_discontinued(nilfs)) {
> +			u64 t = get_seconds();
>  			down_write(&nilfs->ns_sem);
> -			req->sb_err = nilfs_commit_super(sbi, 0);
> +			req->sb_err = nilfs_commit_super(sbi,
> +					nilfs_altsb_need_update(nilfs, t));
>  			up_write(&nilfs->ns_sem);

You can hide get_seconds() inside nilfs_sb_need_update() and
nilfs_altsb_need_update() especially if you will get rid of
nilfs_write_super().

>  		}
>  	}
> @@ -2675,6 +2677,9 @@ static int nilfs_segctor_thread(void *arg)
>  	} else {
>  		DEFINE_WAIT(wait);
>  		int should_sleep = 1;
> +		u64 t;
> +		struct nilfs_sb_info *sbi;
> +		struct the_nilfs *nilfs;
>  
>  		prepare_to_wait(&sci->sc_wait_daemon, &wait,
>  				TASK_INTERRUPTIBLE);
> @@ -2695,6 +2700,11 @@ static int nilfs_segctor_thread(void *arg)
>  		finish_wait(&sci->sc_wait_daemon, &wait);
>  		timeout = ((sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
>  			   time_after_eq(jiffies, sci->sc_timer->expires));
> +		t = get_seconds();
> +		sbi = sci->sc_sbi;
> +		nilfs = sbi->s_nilfs;
> +		if (sci->sc_super->s_dirt && nilfs_sb_need_update(nilfs, t))
> +			set_nilfs_discontinued(nilfs);

ditto.

And the sbi local variable seems omissible.

>  	}
>  	goto loop;
>  
> diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> index d85b360..a8c5875 100644
> --- a/fs/nilfs2/super.c
> +++ b/fs/nilfs2/super.c
> @@ -534,7 +534,7 @@ static struct super_operations nilfs_sops = {
>  	/* .drop_inode	  = nilfs_drop_inode, */
>  	.delete_inode   = nilfs_delete_inode,
>  	.put_super      = nilfs_put_super,
> -	.write_super    = nilfs_write_super,
> +	/* .write_super    = nilfs_write_super, */
>  	.sync_fs        = nilfs_sync_fs,
>  	/* .write_super_lockfs */
>  	/* .unlockfs */
> -- 
> 1.5.6.5
> 

Thanks,
Ryusuke Konishi

[PATCH 0/3] v3: write_super clean up

[Jiro SEKIBA]

Hi, this is re-revised version of write_super clean up patch.

This time, 3rd one removed nilfs_write_super out.

Any comments and suggestions are appreciate.

Thanks,

Regards,
--                                                                              
Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>                                                     

 fs/nilfs2/segment.c   |    7 +++++-
 fs/nilfs2/super.c     |   61 +++++++-----------------------------------------
 fs/nilfs2/the_nilfs.h |   14 +++++++++++
 3 files changed, 29 insertions(+), 53 deletions(-)

[PATCH 1/3] nilfs2: fix disorder of nilfs_write_super in nilfs_sync_fs

[Jiro SEKIBA]

fix disorder of nilfs_write_super in nilfs_sync_fs.
commiting super block must be the end of the function
so that every chanages are reflected.

->sync_fs() is not called frequently so call nilfs_commit_super
instead of calling nilfs_write_super.

Signed-off-by: Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>
---
 fs/nilfs2/super.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 63ab2d1..13614d8 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -384,12 +384,18 @@ static void nilfs_write_super(struct super_block *sb)
 static int nilfs_sync_fs(struct super_block *sb, int wait)
 {
 	int err = 0;
-
-	nilfs_write_super(sb);
+	struct nilfs_sb_info *sbi = NILFS_SB(sb);
+	struct the_nilfs *nilfs = sbi->s_nilfs;
 
 	/* This function is called when super block should be written back */
 	if (wait)
 		err = nilfs_construct_segment(sb);
+
+	down_write(&nilfs->ns_sem);
+	if (sb->s_dirt)
+		nilfs_commit_super(NILFS_SB(sb), 1);
+	up_write(&nilfs->ns_sem);
+
 	return err;
 }
 
-- 
1.5.6.5

[PATCH 0/3] write_super clean up

[Jiro SEKIBA]

Hi, this is a candidate patch to cleanup write_super callback.

VFS layer calls ->write_super callback periodically.  However,
it looks like that calling back is ommited when disk I/O is busy.
And when cleanerd is runnig, disk I/O tend to be busy thus
nilfs superblock is not synchronized as nilfs designed.

To avoid it, syncing superblock by nilfs thread instead of VFS pdflush.

These patches show the basic idea.

First one is a just clean up for nilfs_sync_fs.  writing superblock
must be done very end of the function.

Second one will separeate condition for syncing superblock and alternative
superblock as a inline function.

Third one is the actual patch to sync the superblock in nilfs internal thread.
Because the conditions in nilfs_write_super are using seconds, I used
it as well in the patch.  However, it might be good to use jiffies instead
of using get_seconds.

 
 fs/nilfs2/segment.c   |   13 ++++++++++++-
 fs/nilfs2/super.c     |   16 +++++++---------
 fs/nilfs2/the_nilfs.h |   10 ++++++++++
 3 files changed, 29 insertions(+), 10 deletions(-)

-- 
Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>

[PATCH 1/3] nilfs2: fix disorder of nilfs_write_super in nilfs_sync_fs

[Jiro SEKIBA]

Signed-off-by: Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>

---
 fs/nilfs2/super.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 8e2ec43..ba69601 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -388,11 +388,12 @@ static int nilfs_sync_fs(struct super_block *sb, int wait)
 {
 	int err = 0;
 
-	nilfs_write_super(sb);
-
 	/* This function is called when super block should be written back */
 	if (wait)
 		err = nilfs_construct_segment(sb);
+
+	nilfs_write_super(sb);
+
 	return err;
 }
 
-- 
1.5.6.5

Re: [PATCH 1/3] nilfs2: fix disorder of nilfs_write_super in nilfs_sync_fs

[Ryusuke Konishi]

On Fri, 17 Jul 2009 18:12:46 +0900, Jiro SEKIBA wrote:
> Signed-off-by: Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>
> 
> ---
>  fs/nilfs2/super.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> index 8e2ec43..ba69601 100644
> --- a/fs/nilfs2/super.c
> +++ b/fs/nilfs2/super.c
> @@ -388,11 +388,12 @@ static int nilfs_sync_fs(struct super_block *sb, int wait)
>  {
>  	int err = 0;
>  
> -	nilfs_write_super(sb);
> -
>  	/* This function is called when super block should be written back */
>  	if (wait)
>  		err = nilfs_construct_segment(sb);
> +
> +	nilfs_write_super(sb);
> +
>  	return err;
>  }
>  
> -- 
> 1.5.6.5

Yes, right.

Writing back the super-block should be moved after log writing.
A recent vfs cleanup made room for this change.

Ryusuke Konishi