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 <[email protected]>
> > ---
> >  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 <[email protected]>
_______________________________________________
users mailing list
[email protected]
https://www.nilfs.org/mailman/listinfo/users

Reply via email to