[Sheepdog] sheepdog Digest, Vol 14, Issue 8

[Narendra Prasad Madanapalli]

Hi Kazutaka,

Thanks for your time. Please see my comments inline below

>> Hi,
>>
>> This patch adds journalling support for sheepdog that can aid in
>> performing atomic operations.
>
> Great!
>
>> +#define JRNL_PATH "/journal/"
>> +
>> +static int  jrnl_init_path(const char *base_path)
>> +{
>> +     int new, ret;
>> +
>> +     /* Create journal directory */
>> +     jrnl_path = zalloc(strlen(base_path) + strlen(JRNL_PATH) + 1);
>> +     sprintf(jrnl_path, "%s" JRNL_PATH, base_path);
>> +
>> +     ret = init_path(jrnl_path, &new);
>> +        /* Error during directory creation */
>> +     if (ret)
>> +             return ret;
>> +     /* If journal is newly created */
>> +     if (new)
>> +             return 0;
>> +
>> +     return 0;
>> +}
>
> Other functions to initialize directories are named
> init_[target]_path(), so this function name should be
> init_jrnl_path()?
I'll change the name to init_jrnl_path()

>> +
>> +int  jrnl_write_end_mark(jrnl_desc_t  *jd)
>> +{
>> +     ssize_t  ret;
>> +     end_mark_t  end_mark = 1;
>
> The value of end_mark should be defined as a macro, and it should be
> more complex value.
Sure, I'll do that.

>> +
>> +     ret = pwrite64(jd->jdf_fd, &end_mark, sizeof(end_mark),
>> +                    sizeof(jd->jd_head) + jd->jdh_size);
>> +
>> +     if (ret != sizeof(end_mark)) {
>> +             if (errno == ENOSPC)
>> +                     ret = SD_RES_NO_SPACE;
>> +             else
>> +                     ret = SD_RES_EIO;
>> +     } else
>> +             ret = SD_RES_SUCCESS;
>> +
>> +     return ret;
>> +}
>> +
>
>> +
>> +int  jrnl_apply_to_targe_vdi_object(jrnl_file_desc_t *jfd)
>> +{
>> +     char path[1024], *buf;
>> +     int buf_len, res = 0, ret;
>> +     ssize_t retsize;
>> +     jrnl_head_t jh;
>> +
>> +     /* FIXME: handle larger size */
>> +     buf_len = (1 << 22);
>> +     buf = zalloc(buf_len);
>> +     if (!buf) {
>> +             eprintf("failed to allocate memory\n");
>> +             res = SD_RES_NO_MEM;
>> +             goto out;
>> +     }
>> +
>> +     /* Open journal */
>> +     snprintf(path, sizeof(path), "%s%08u/%016" PRIx64, jrnl_path,
>> jfd->jf_epoch, jfd->jf_oid);
>> +     ret = jrnl_open(jfd, O_RDONLY);
>> +     if (ret) {
>> +             res = SD_RES_EIO;
>> +             goto freemem;
>> +     }
>> +
>> +     /* Flush out journal to disk (vdi object) */
>> +     retsize = pread64(jfd->jf_fd, &jh, sizeof(jh), 0);
>> +     retsize = pread64(jfd->jf_fd, buf, jh.jh_size, sizeof(jh));
>
> We should check that the journal has an end mark here.
Will add this check here.

>> +     retsize = pwrite64(jfd->jf_vdi_fd, buf, jh.jh_size, jh.jh_offset);
>> +
>> +     /* Clean up */
>> +     close(jfd->jf_fd);
>> +     jfd->jf_fd = 0;
>> +freemem:
>> +     free(buf);
>> +out:
>> +     return res;
>> +}
>> +
>> +int  jrnl_writte_data_to_targe_vdi_object(jrnl_file_desc_t *jfd, void *data)
>> +{
>> +     //
>> +     // FIXME: Need to implement this function
>> +     //
>> +
>> +     return 0;
>> +}
>
> What's the difference between jrnl_apply_to_targe_vdi_object() and
> jrnl_write_data_to_target_vdi_object()?  When do we use
> jrnl_write_data_to_target_vdi_object()?

Which is same as jrnl_apply_to_targe_vdi_object() except it directly
writes data into vdi object rather than reading from journal and
writing into vdi object.
The thought behind this is to replace code snippets that responsible
for writing data to vdi objects with this function.

Any ideas?

>> +
>> +int  jrnl_recover_vdis()
>> +{
>> +     DIR *dir;
>> +     struct dirent *d;
>> +     char jrnl_dir[1024], jrnl_file_path[1024];
>> +     int  epoch;
>> +
>> +     epoch = get_latest_epoch();
>> +     if (epoch < 0) {
>> +             return 1;
>> +     }
>> +     snprintf(jrnl_dir, sizeof(jrnl_dir), "%s%08u", jrnl_path, epoch);
>> +
>> +     dir = opendir(jrnl_dir);
>> +     if (!dir)
>> +             return -1;
>> +
>> +     /* TODO: Not complete as error handling to be added */
>> +     while ((d = readdir(dir))) {
>> +             jrnl_file_desc_t  jfd;
>> +             jfd.jf_oid = atoi(d->d_name);
>> +             snprintf(jrnl_file_path, sizeof(jrnl_file_path), "%s%016" PRIx64,
>> jrnl_dir, jfd.jf_oid);
>> +             jrnl_open(&jfd, O_RDONLY);
>> +             jrnl_apply_to_targe_vdi_object(&jfd);
>> +     }
>> +     closedir(dir);
>> +
>> +     return 0;
>> +}
>> +
>

> I think the followings are not implemented yet, yes?
There is a function wirtten jrnl_remove() that carries out this operations.

>  - remove a journal file when it becomes unnecessary
>  - apply journal data before accessing a vdi object if the journal file
>   exists
>


Thanks,
Narendra.