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