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

[Kai Zhang]

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,
Kyle
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.wpkg.org/pipermail/sheepdog/attachments/20130625/72d952f1/attachment-0004.html>