[Ocfs2-tools-devel] [PATCH 2/4] defrag.ocfs2: Pass 1: Defrag individual files and directories

[Goldwyn Rodrigues]

Hi Joel,

On Sun, Jul 18, 2010 at 11:46 AM, Joel Becker <Joel.Becker at oracle.com> wrote:
> On Tue, May 11, 2010 at 11:02:34PM -0500, Goldwyn Rodrigues wrote:
>> Defragging file -
>> Allocate the maximum possible extent and copy the file into the
>> newly allocated extent. If the allocated extent is smaller than
>> the extent read from the file, write the collected data, and
>> re-use the file extent. At any point, if the number of extents
>> in the new inode increases than in the existing inode, abort.
>>
>> Defragging directory -
>> Allocate an extent, and copy dirents to the new extent, skipping
>> holes and empty dirents. For each dirent, the dirent length
>> is recalculated to optimize on space.
>>
>> TODO: Find the optimum extent size, perhaps by reading a
>> group_desc
>>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn at suse.de>
>
>        Can you break this up into three patches?  The first adds
> defrag.c, which has a main function that parses options but doesn't
> actually do anything.  The next patch adds file defrag, and the third
> does dir defrag.

Yes, sure. I will be travelling so won't be able to do this immediately.

>        It's just that this is a lot of code to review in one email.
> It's easier if broken up, when there is a logical boundary to break it
> up.  I'm going to review in multiple emails to take each piece of
> functionality one at a time.
>
>> diff --git a/defrag.ocfs2/defrag.c b/defrag.ocfs2/defrag.c
>> new file mode 100644
>> index 0000000..65c1a13
>> --- /dev/null
>> +++ b/defrag.ocfs2/defrag.c
>> @@ -0,0 +1,401 @@
>> +/* -*- mode: c; c-basic-offset: 8; -*-
>> +   * vim: noexpandtab sw=8 ts=8 sts=0:
>> +   *
>> +   * defrag.c
>> +   *
>> +   * Copyright (C) 2010 Oracle. All rights reserved.
>
>        Make sure you set the copyright to the company that employs you.
> Oracle can't claim ownership on code not written by an Oracle employee.
> Well, at least not without copyright assignment, and we don't really
> need to involve the lawyers, do we? ;-)


Yes, sure :)

>
>> +static errcode_t check_superblock(struct defrag_state *dst)
>> +{
>> +     struct ocfs2_dinode *di = dst->dst_fs->fs_super;
>> +     struct ocfs2_super_block *sb = OCFS2_RAW_SB(di);
>> +     errcode_t ret = 0;
>> +
>> +     if (sb->s_max_slots == 0) {
>> +             printf("The superblock max_slots field is set to 0.\n");
>> +             ret = OCFS2_ET_CORRUPT_SUPERBLOCK;
>> +     }
>> +     dst->fs_generation = di->i_fs_generation;
>> +
>> +     return ret;
>> +}
>
>        I don't get this.  This should never happen, so why are you
> checking for it?

Hmm, I don't remember. Seems like this was left of what I copied from fsck.

>
>> +static int fs_clean(struct ocfs2_super_block *sb, char *filename)
>> +{
>> +     time_t now = time(NULL);
>> +     time_t next = sb->s_lastcheck + sb->s_checkinterval;
>> +
>> +     if (sb->s_checkinterval > 0 && now >= next) {
>> +             unsigned scaled_time;
>> +             char *scaled_units;
>> +
>> +             scale_time(now - sb->s_lastcheck, &scaled_time, &scaled_units);
>> +             fprintf(stderr, "Filesystem has gone %u %s without"
>> +                             "being checked\n", scaled_time, scaled_units);
>> +             return 0;
>> +     }
>> +
>> +     if (sb->s_mnt_count > 0) {
>> +             fprintf(stderr, "Filesystem mounted for %u mounts "
>> +                             "without fsck. ", sb->s_mnt_count);
>> +             return 0;
>> +     }
>> +
>> +     return 1;
>> +}
>
>        And why are you detecting and printing the fsck counts?

Because I want to assume that the filesystem is perfect before we
perform defrag.fsck. However, now I feel that this number is not
incremented.

>
>> +static errcode_t recover_cluster_info(struct defrag_state *dst)
>> +{
>> +     errcode_t ret = 0;
>> +     struct o2cb_cluster_desc disk = {NULL, NULL}, running = {NULL, NULL};
>> +
>> +     ret = o2cb_running_cluster_desc(&running);
>> +     if (ret)
>> +             goto bail;
>> +
>> +     ret = ocfs2_fill_cluster_desc(dst->dst_fs, &disk);
>> +     if (ret)
>> +             goto bail;
>> +
>> +     /*
>> +      * If the disk matches the running cluster, there is nothing we
>> +      * can fix.
>> +      */
>> +     if ((!running.c_stack && !disk.c_stack) ||
>> +                     (running.c_stack && running.c_cluster &&
>> +                      disk.c_stack && disk.c_cluster &&
>> +                      !strcmp(running.c_stack, disk.c_stack) &&
>> +                      !strcmp(running.c_cluster, disk.c_cluster)))
>> +             goto bail;
>> +
>> +bail:
>> +     o2cb_free_cluster_desc(&running);
>> +     o2cb_free_cluster_desc(&disk);
>> +
>> +     return ret;
>> +}
>
>        I don't get this function either.  It doesn't do anything.
> But...
>
>> +     if (!dst->skip_o2cb && !ocfs2_mount_local(dst->dst_fs)) {
>> +             ret = o2cb_init();
>> +             if (ret) {
>> +                     com_err(whoami, ret, "while initializing the cluster");
>> +                     goto close;
>> +             }
>> +
>> +             block_signals(SIG_BLOCK);
>> +             ret = ocfs2_initialize_dlm(dst->dst_fs, whoami);
>> +             if (ret == O2CB_ET_INVALID_STACK_NAME) {
>> +                     block_signals(SIG_UNBLOCK);
>> +                     ret = recover_cluster_info(dst);
>
>        No, no, no, no, no.  If the cluster stack is invalid, you just
> error.  They need to run 'tunfes.ocfs2 --update-cluster-stack' if they
> want to adjust the disk's stack information.  defrag.ocfs2 shouldn't be
> touching this stuff.

Yes, I will clean this up as well.

Thanks.


-- 
Goldwyn