[sheepdog] [PATCH] journal: refactor the journaling subsystem

[Hitoshi Mitake]

At Tue, 25 Jun 2013 20:33:20 +0800,
Liu Yuan wrote:
> 
> On Mon, Jun 24, 2013 at 01:31:41AM -0700, Kai Zhang wrote:
> > Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp> writes:
> > 
> > > Current journaling subsystem has some dirty parts because of its old
> > > journal descriptor types (epoch and config). This patch cleans them.
> > >
> > > Signed-off-by: Hitoshi Mitake <mitake.hitoshi at lab.ntt.co.jp>
> > > ---
> > >  sheep/journal.c |   31 +++++++++++++++----------------
> > >  1 files changed, 15 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/sheep/journal.c b/sheep/journal.c
> > > index 50783f2..f1f94da 100644
> > > --- a/sheep/journal.c
> > > +++ b/sheep/journal.c
> > > @@ -137,21 +137,18 @@ static bool journal_entry_full_write(struct journal_descriptor *jd)
> > >
> > >  static void journal_get_path(struct journal_descriptor *jd, char *path)
> > >  {
> > > -	switch (jd->flag) {
> > > -	case JF_STORE:
> > > -	case JF_REMOVE_OBJ:
> > > -		snprintf(path, PATH_MAX, "%s/%016"PRIx64,
> > > -			 md_get_object_path(jd->oid), jd->oid);
> > > -		if (jd->flag == JF_STORE)
> > > -			sd_iprintf("%s, size %"PRIu64", off %"PRIu64", %d",
> > > -				path, jd->size, jd->offset, jd->create);
> > > -		else		/* JF_REMOVE_OBJ */
> > > -			sd_iprintf("%s (remove)", path);
> > > -		break;
> > > -	default:
> > > -		panic("unknown type of journal flag: %d", jd->flag);
> > > -		break;
> > > -	}
> > > +	snprintf(path, PATH_MAX, "%s/%016"PRIx64,
> > > +		md_get_object_path(jd->oid), jd->oid);
> > > +
> > > +	if (jd->flag == JF_STORE)
> > > +		sd_iprintf("%s, size %"PRIu64", off %"PRIu64", %d",
> > > +			path, jd->size, jd->offset, jd->create);
> > > +	else if (jd->flag == JF_REMOVE_OBJ)
> > > +		sd_iprintf("%s (remove)", path);
> > > +	else
> > > +		panic("unknown type of journal flag,"
> > > +			" the journal flag seems to be corrupted: %d",
> > > +			jd->flag);
> > >  }
> > >
> > >  static int replay_journal_entry(struct journal_descriptor *jd)
> > > @@ -168,6 +165,8 @@ static int replay_journal_entry(struct journal_descriptor *jd)
> > >		return 0;
> > >	}
> > >
> > > +	assert(jd->flag == JF_STORE);
> > > +
> > 
> > Because journal_get_path(jd, patch) is needed whenever jd->flag is
> > JF_REMOVE_OBJ or JF_STORE, I think we can put it to the top to clean the code.
> > And because the value of jd->flag will be checked by both journal_get_path(),
> > and this assert, so it would be better to check it in one place.
> > The modifide code would be like:
> > 
> > journal_get_path(jd, path); // no check
> > if (jd->flag == JF_REMOVE_OBJ) {
> >    unlink(path);
> >    return 0;
> > } else if (jd->flag == JF_RESTORE) {
> >    /* restore logic here */
> > } else {
> >    panic(...);
> > }
> > 
> > A switch case is also a good choice.
> > 
> 
> Put journal_get_path() at top looks better to me, Mitake, could you cook a v2 patch 
> against this comment? We can still fast return if it is JF_REMOVE_OBJ as old code did.

Ah, sorry, I've misunderstood the intention of the Kyle's
comment. I'll extract the code from journal_get_path() and remove it.

I'll send v2 later.

Thanks,
Hitoshi