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

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

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>