[PATCH 0/3] write_super clean up

[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

[PATCH 2/3] nilfs2: clean up nilfs_write_super

[Jiro SEKIBA]

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

---
 fs/nilfs2/super.c     |    8 ++------
 fs/nilfs2/the_nilfs.h |   10 ++++++++++
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index ba69601..55a4359 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -368,17 +368,13 @@ 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_update_super(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_update_alt_super(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..ee448ea 100644
--- a/fs/nilfs2/the_nilfs.h
+++ b/fs/nilfs2/the_nilfs.h
@@ -201,6 +201,16 @@ THE_NILFS_FNS(DISCONTINUED, discontinued)
 /* Minimum interval of periodical update of superblocks (in seconds) */
 #define NILFS_SB_FREQ		10
 #define NILFS_ALTSB_FREQ	60  /* spare superblock */
+static inline int nilfs_update_super(struct the_nilfs *nilfs, u64 t)
+{
+  return t < nilfs->ns_sbwtime[0] || t > nilfs->ns_sbwtime[0] + NILFS_SB_FREQ; 
+}
+
+static inline int nilfs_update_alt_super(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 *);
-- 
1.5.6.5

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

[Jiro SEKIBA]

instead of periodic write_super callback,
commit super block in internal thread.

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..fe25424 100644
--- a/fs/nilfs2/segment.c
+++ b/fs/nilfs2/segment.c
@@ -2486,8 +2486,9 @@ 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_update_alt_super(nilfs,t));
 			up_write(&nilfs->ns_sem);
 		}
 	}
@@ -2675,6 +2676,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 +2699,12 @@ 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(test_bit(NILFS_SC_SUPER_ROOT, &sci->sc_flags) && 
+	   	   nilfs_update_super(nilfs,t))
+			set_nilfs_discontinued(nilfs);
 	}
 	goto loop;
 
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 55a4359..00a2173 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -533,7 +533,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 0/3] write_super clean up

[Ryusuke Konishi]

Hi,
On Fri, 17 Jul 2009 18:12:45 +0900, Jiro SEKIBA wrote:
> 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(-)

First, thank you very much for your help!

Recently, I'm pushed for time and I feel a desperate need for help
just even to maintain this project.

I'll look into each of them a few moments later.

Thanks,
Ryusuke Konishi

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

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

Re: [PATCH 2/3] nilfs2: clean up nilfs_write_super

[Ryusuke Konishi]

On Fri, 17 Jul 2009 18:12:47 +0900, Jiro SEKIBA wrote:
> Signed-off-by: Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>

By the way, you should add changelog to every patch even if they are
in a series.
 
> ---
>  fs/nilfs2/super.c     |    8 ++------
>  fs/nilfs2/the_nilfs.h |   10 ++++++++++
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> index ba69601..55a4359 100644
> --- a/fs/nilfs2/super.c
> +++ b/fs/nilfs2/super.c
> @@ -368,17 +368,13 @@ 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_update_super(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_update_alt_super(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..ee448ea 100644
> --- a/fs/nilfs2/the_nilfs.h
> +++ b/fs/nilfs2/the_nilfs.h
> @@ -201,6 +201,16 @@ THE_NILFS_FNS(DISCONTINUED, discontinued)
>  /* Minimum interval of periodical update of superblocks (in seconds) */
>  #define NILFS_SB_FREQ		10
>  #define NILFS_ALTSB_FREQ	60  /* spare superblock */

A null line should be inserted here.

> +static inline int nilfs_update_super(struct the_nilfs *nilfs, u64 t)
> +{
> +  return t < nilfs->ns_sbwtime[0] || t > nilfs->ns_sbwtime[0] + NILFS_SB_FREQ; 
> +}

The nilfs_update_super() sounds like a function to do update operation
on the super block.

How about nilfs_sb_need_update() or so?

The body of the function looks not to be indented properly.
Please refer to the Chapter 1 of Documentation/CodingStyle.

> +
> +static inline int nilfs_update_alt_super(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;
> +}

ditto.

>  void nilfs_set_last_segment(struct the_nilfs *, sector_t, u64, __u64);
>  struct the_nilfs *find_or_create_nilfs(struct block_device *);
> -- 
> 1.5.6.5
> 

And then, this patch has some style problems.  Applying checkpatch.pl
can save a bit of time.

$ ~/git/linux-2.6/scripts/checkpatch.pl nilfs2-clean-up-nilfs_write_super.patch
WARNING: line over 80 characters
#22: FILE: fs/nilfs2/super.c:373:
+               if (!nilfs_discontinued(nilfs) && !nilfs_update_super(nilfs,t)) {

ERROR: space required after that ',' (ctx:VxV)
#22: FILE: fs/nilfs2/super.c:373:
+               if (!nilfs_discontinued(nilfs) && !nilfs_update_super(nilfs,t)) {
                                                                           ^

ERROR: space required after that ',' (ctx:VxV)
#28: FILE: fs/nilfs2/super.c:377:
+               nilfs_commit_super(sbi, nilfs_update_alt_super(nilfs,t));
                                                                    ^

ERROR: trailing whitespace
#42: FILE: fs/nilfs2/the_nilfs.h:206:
+  return t < nilfs->ns_sbwtime[0] || t > nilfs->ns_sbwtime[0] + NILFS_SB_FREQ; $

total: 3 errors, 1 warnings, 35 lines checked

nilfs2-clean-up-nilfs_write_super.patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Regards,
Ryusuke Konishi

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

[Ryusuke Konishi]

On Fri, 17 Jul 2009 18:12:48 +0900, Jiro SEKIBA wrote:
> instead of periodic write_super callback,
> commit super block in internal thread.

I appreciate if you could clearly describe the problem what this patch
solves because the changelog becomes the only clue after merged into
the mainline; I know it by the first mail, but the cover letter will
be dropped when it's queued in a git tree.

> 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..fe25424 100644
> --- a/fs/nilfs2/segment.c
> +++ b/fs/nilfs2/segment.c
> @@ -2486,8 +2486,9 @@ 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)) {

Well, maybe we should change the name of nilfs_discontinued before
applying this patch.

It originally meant that the latest log got untraceable from
the log which the super block points to.

After applying this patch, the meaning of the flag will change to a
state "the superblock needs to be written back".

> +			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_update_alt_super(nilfs,t));

Yes, this looks needed when stopping use of the callback.

>  			up_write(&nilfs->ns_sem);
>  		}
>  	}
> @@ -2675,6 +2676,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 +2699,12 @@ 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(test_bit(NILFS_SC_SUPER_ROOT, &sci->sc_flags) && 
> +	   	   nilfs_update_super(nilfs,t))
> +			set_nilfs_discontinued(nilfs);

We need to discuss for this.

At least, the NILFS_SC_SUPER_ROOT bit may be cleared by other tasks
outside the transaction lock.


>  	}
>  	goto loop;
>  
> diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> index 55a4359..00a2173 100644
> --- a/fs/nilfs2/super.c
> +++ b/fs/nilfs2/super.c
> @@ -533,7 +533,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
> 

Regards,
Ryusuke Konishi

Re: [PATCH 2/3] nilfs2: clean up nilfs_write_super

[Jiro SEKIBA]

Hi,

Thank you for the comments!

At Sat, 18 Jul 2009 01:24:14 +0900 (JST),
Ryusuke Konishi wrote:

> The nilfs_update_super() sounds like a function to do update operation
> on the super block.
> 
> How about nilfs_sb_need_update() or so?

ok, looks better.

> And then, this patch has some style problems.  Applying checkpatch.pl
> can save a bit of time.

Grr.  I might send the wrong one.
I'll revise the patch and resend it again.

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

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

[Jiro SEKIBA]

At Sat, 18 Jul 2009 03:04:21 +0900 (JST),
Ryusuke Konishi wrote:

> On Fri, 17 Jul 2009 18:12:48 +0900, Jiro SEKIBA wrote:
> > instead of periodic write_super callback,
> > commit super block in internal thread.
> 
> I appreciate if you could clearly describe the problem what this patch
> solves because the changelog becomes the only clue after merged into
> the mainline; I know it by the first mail, but the cover letter will
> be dropped when it's queued in a git tree.

Sure, I will.

> > 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..fe25424 100644
> > --- a/fs/nilfs2/segment.c
> > +++ b/fs/nilfs2/segment.c
> > @@ -2486,8 +2486,9 @@ 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)) {
> 
> Well, maybe we should change the name of nilfs_discontinued before
> applying this patch.
> 
> It originally meant that the latest log got untraceable from
> the log which the super block points to.
> 
> After applying this patch, the meaning of the flag will change to a
> state "the superblock needs to be written back".

I haven't checked yet other place deeply, but if the meaning of the
nilfs_dicontinued() is still used in the other place, how about 
defining a macro like following:

#define nilfs_sb_needed_to_written_back(n) nilfs_discontined((n))

> > +			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_update_alt_super(nilfs,t));
> 
> Yes, this looks needed when stopping use of the callback.
>
> >  			up_write(&nilfs->ns_sem);
> >  		}
> >  	}
> > @@ -2675,6 +2676,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 +2699,12 @@ 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(test_bit(NILFS_SC_SUPER_ROOT, &sci->sc_flags) && 
> > +	   	   nilfs_update_super(nilfs,t))
> > +			set_nilfs_discontinued(nilfs);
> 
> We need to discuss for this.
> 
> At least, the NILFS_SC_SUPER_ROOT bit may be cleared by other tasks
> outside the transaction lock.

I see, then it doesn't make sence to check this bit in the thread.
Reason I've tested the bit is that nilfs_segctor_construct is checking
that bit when commiting super block.

I would need to take time to see inside more deeply maybe.

thank you for the comments anyway.

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

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

[Ryusuke Konishi]

On Sat, 18 Jul 2009 16:24:09 +0900, Jiro SEKIBA wrote:
> At Sat, 18 Jul 2009 03:04:21 +0900 (JST),
> Ryusuke Konishi wrote:
> > >  		if (test_bit(NILFS_SC_SUPER_ROOT, &sci->sc_flags) &&
> > >  		    nilfs_discontinued(nilfs)) {
> > 
> > Well, maybe we should change the name of nilfs_discontinued before
> > applying this patch.
> > 
> > It originally meant that the latest log got untraceable from
> > the log which the super block points to.
> > 
> > After applying this patch, the meaning of the flag will change to a
> > state "the superblock needs to be written back".
> 
> I haven't checked yet other place deeply, but if the meaning of the
> nilfs_dicontinued() is still used in the other place, how about 
> defining a macro like following:
> 
> #define nilfs_sb_needed_to_written_back(n) nilfs_discontined((n))

I don't think we have to define the intermediate step because the
related definitions are never used outside nilfs.

But, okay, let's leave renaming the `discontinued' until later. It's
not a priority.

> > > +		t = get_seconds();
> > > +		sbi = sci->sc_sbi;
> > > +	        nilfs = sbi->s_nilfs;
> > > +		if(test_bit(NILFS_SC_SUPER_ROOT, &sci->sc_flags) && 
> > > +	   	   nilfs_update_super(nilfs,t))
> > > +			set_nilfs_discontinued(nilfs);
> > 
> > We need to discuss for this.
> > 
> > At least, the NILFS_SC_SUPER_ROOT bit may be cleared by other tasks
> > outside the transaction lock.
> 
> I see, then it doesn't make sence to check this bit in the thread.
> Reason I've tested the bit is that nilfs_segctor_construct is checking
> that bit when commiting super block.
 
This bit means that the latest log has a super root block, so it's
cleared when the next log is written out.

It makes sense in nilfs_segctor_construct(), but it doesn't in the
thread function.

How about testing sci->sc_super->s_dirt instead ?

Regards,
Ryusuke Konishi

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

[Jiro SEKIBA]

At Sat, 18 Jul 2009 18:25:32 +0900 (JST),
Ryusuke Konishi wrote:
> 
> On Sat, 18 Jul 2009 16:24:09 +0900, Jiro SEKIBA wrote:
> > At Sat, 18 Jul 2009 03:04:21 +0900 (JST),
> > Ryusuke Konishi wrote:
> > > >  		if (test_bit(NILFS_SC_SUPER_ROOT, &sci->sc_flags) &&
> > > >  		    nilfs_discontinued(nilfs)) {
> > > 
> > > Well, maybe we should change the name of nilfs_discontinued before
> > > applying this patch.
> > > 
> > > It originally meant that the latest log got untraceable from
> > > the log which the super block points to.
> > > 
> > > After applying this patch, the meaning of the flag will change to a
> > > state "the superblock needs to be written back".
> > 
> > I haven't checked yet other place deeply, but if the meaning of the
> > nilfs_dicontinued() is still used in the other place, how about 
> > defining a macro like following:
> > 
> > #define nilfs_sb_needed_to_written_back(n) nilfs_discontined((n))
> 
> I don't think we have to define the intermediate step because the
> related definitions are never used outside nilfs.
> 
> But, okay, let's leave renaming the `discontinued' until later. It's
> not a priority.
> 
> > > > +		t = get_seconds();
> > > > +		sbi = sci->sc_sbi;
> > > > +	        nilfs = sbi->s_nilfs;
> > > > +		if(test_bit(NILFS_SC_SUPER_ROOT, &sci->sc_flags) && 
> > > > +	   	   nilfs_update_super(nilfs,t))
> > > > +			set_nilfs_discontinued(nilfs);
> > > 
> > > We need to discuss for this.
> > > 
> > > At least, the NILFS_SC_SUPER_ROOT bit may be cleared by other tasks
> > > outside the transaction lock.
> > 
> > I see, then it doesn't make sence to check this bit in the thread.
> > Reason I've tested the bit is that nilfs_segctor_construct is checking
> > that bit when commiting super block.
>  
> This bit means that the latest log has a super root block, so it's
> cleared when the next log is written out.
> 
> It makes sense in nilfs_segctor_construct(), but it doesn't in the
> thread function.
> 
> How about testing sci->sc_super->s_dirt instead ?

Thank you for the suggestions!
It looks much more sense to me as well.

I'll resend revised patches.

thanks,

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

[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 3/3] nilfs2: stop using periodic write_super callback

[Jiro SEKIBA]

remove nilfs_write_super and commit 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 |    7 ++++++-
 fs/nilfs2/super.c   |   46 +---------------------------------------------
 2 files changed, 7 insertions(+), 46 deletions(-)

diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
index 8b5e477..0f110a1 100644
--- a/fs/nilfs2/segment.c
+++ b/fs/nilfs2/segment.c
@@ -2487,7 +2487,8 @@ static int nilfs_segctor_construct(struct nilfs_sc_info *sci,
 		if (test_bit(NILFS_SC_SUPER_ROOT, &sci->sc_flags) &&
 		    nilfs_discontinued(nilfs)) {
 			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));
 			up_write(&nilfs->ns_sem);
 		}
 	}
@@ -2675,6 +2676,7 @@ static int nilfs_segctor_thread(void *arg)
 	} else {
 		DEFINE_WAIT(wait);
 		int should_sleep = 1;
+		struct the_nilfs *nilfs;
 
 		prepare_to_wait(&sci->sc_wait_daemon, &wait,
 				TASK_INTERRUPTIBLE);
@@ -2695,6 +2697,9 @@ 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));
+		nilfs = sci->sc_sbi->s_nilfs;
+		if (sci->sc_super->s_dirt && nilfs_sb_need_update(nilfs))
+			set_nilfs_discontinued(nilfs);
 	}
 	goto loop;
 
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 02f7a76..bad0ce1 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -65,7 +65,6 @@ MODULE_DESCRIPTION("A New Implementation of the Log-structured Filesystem "
 		   "(NILFS)");
 MODULE_LICENSE("GPL");
 
-static void nilfs_write_super(struct super_block *sb);
 static int nilfs_remount(struct super_block *sb, int *flags, char *data);
 
 /**
@@ -333,49 +332,6 @@ static void nilfs_put_super(struct super_block *sb)
 	unlock_kernel();
 }
 
-/**
- * nilfs_write_super - write super block(s) of NILFS
- * @sb: super_block
- *
- * nilfs_write_super() gets a fs-dependent lock, writes super block(s), and
- * clears s_dirt.  This function is called in the section protected by
- * lock_super().
- *
- * The s_dirt flag is managed by each filesystem and we protect it by ns_sem
- * of the struct the_nilfs.  Lock order must be as follows:
- *
- *   1. lock_super()
- *   2.    down_write(&nilfs->ns_sem)
- *
- * Inside NILFS, locking ns_sem is enough to protect s_dirt and the buffer
- * of the super block (nilfs->ns_sbp[]).
- *
- * In most cases, VFS functions call lock_super() before calling these
- * methods.  So we must be careful not to bring on deadlocks when using
- * lock_super();  see generic_shutdown_super(), write_super(), and so on.
- *
- * Note that order of lock_kernel() and lock_super() depends on contexts
- * of VFS.  We should also note that lock_kernel() can be used in its
- * protective section and only the outermost one has an effect.
- */
-static void nilfs_write_super(struct super_block *sb)
-{
-	struct nilfs_sb_info *sbi = NILFS_SB(sb);
-	struct the_nilfs *nilfs = sbi->s_nilfs;
-
-	down_write(&nilfs->ns_sem);
-	if (!(sb->s_flags & MS_RDONLY)) {
-		if (!nilfs_discontinued(nilfs) &&
-		    !nilfs_sb_need_update(nilfs)) {
-			up_write(&nilfs->ns_sem);
-			return;
-		}
-		nilfs_commit_super(sbi, nilfs_altsb_need_update(nilfs));
-	}
-	sb->s_dirt = 0;
-	up_write(&nilfs->ns_sem);
-}
-
 static int nilfs_sync_fs(struct super_block *sb, int wait)
 {
 	int err = 0;
@@ -534,7 +490,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