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

[Hitoshi Mitake]

At Mon, 24 Jun 2013 02:20:59 -0700,
Kai Zhang wrote:
> 
> Hitoshi Mitake <mitake.hitoshi at gmail.com> writes:
> 
> > At Mon, 24 Jun 2013 01:31:41 -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 */
> >
> > Do you mean STORE?
> >
> 
> Sorry, my mistake. It should be STORE.
> 
> >> } else {
> >>    panic(...);
> >> }
> >>
> >> A switch case is also a good choice.
> >
> > Because current journal entry has only two types, the above
> > indentation of /* restore logic here */ or switch statements would be
> > too flowery.
> >
> 
> How about a helper function to wrap them up?
> Anyway, maybe this is not a big issue.

Do you mean implementing functions for each types of journal entry? I
feel it is an over abstraction.

> But what I stressed is that journal_get_path() and the logic of checking
> jd->flag were duplicated. And there is a way to clean them.
> 

IIUC, you think that the assert(jd->flag == JF_STORE); is
redundant. Of course we can remove the assert practically. But I added
the assert mainly for documentation. And it produces no runtime
overhead. I believe this would be helpful for understanding of other
developers.

Thanks,
Hitoshi