[sheepdog] [PATCH] journal: refactor the journaling subsystem
Kai Zhang
kyle at zelin.io
Mon Jun 24 11:20:59 CEST 2013
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.
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.
Thanks,
Kyle
More information about the sheepdog
mailing list