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

[Hitoshi Mitake]

At Tue, 25 Jun 2013 09:41:05 +0800,
Kai Zhang wrote:
> 
> [1  <text/plain; us-ascii (quoted-printable)>]
> 
> On Jun 24, 2013, at 11:54 PM, Hitoshi Mitake <mitake.hitoshi at gmail.com> wrote:
> 
> > 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.
> 
> 
> I see.
> Maybe it's just a matter of taste.
> And because I'm not a maintainer currently, please ignore my comments and
> wait for Yuan or Kazutaka to merge this patch.
> 

Thanks for your opinion. Let's wait for their review.

Thanks,
Hitoshi