[Ocfs2-tools-devel] [PATCH] Make tunefs.ocfs2 understand all journal options.

[Goldwyn Rodrigues]

On Thu, Dec 22, 2011 at 5:18 PM, Sunil Mushran <sunil.mushran at oracle.com> wrote:
> On 12/20/2011 06:51 PM, Goldwyn Rodrigues wrote:
>>
>> On Tue, Nov 22, 2011 at 1:49 PM, Joel Becker<jlbec at evilplan.org>  wrote:
>>>
>>> On Thu, Nov 03, 2011 at 06:16:28PM +0100, Michal Srb wrote:
>>>>
>>>> The manual page of tunefs.ocfs2 says that tunefs can understand the same
>>>> -J
>>>> options as mkfs.ocfs2 does. That is: size,[no]block32,[no]block64.
>>>> However
>>>> tunefs.ocfs2 understands only the size. This adds the ability to change
>>>> the
>>>> block option too.
>>>> ---
>>>>  tunefs.ocfs2/Makefile               |    1 +
>>>>  tunefs.ocfs2/libocfs2ne.c           |   18 +++++-
>>>>  tunefs.ocfs2/libocfs2ne.h           |    2 +-
>>>>  tunefs.ocfs2/ocfs2ne.c              |   30 ++++++++++
>>>>  tunefs.ocfs2/op_set_journal_block.c |  109
>>>> +++++++++++++++++++++++++++++++++++
>>>>  tunefs.ocfs2/op_set_journal_size.c  |    6 +-
>>>>  tunefs.ocfs2/op_set_slot_count.c    |    5 +-
>>>>  7 files changed, 164 insertions(+), 7 deletions(-)
>>>>  create mode 100644 tunefs.ocfs2/op_set_journal_block.c
>>>>
>>>> diff --git a/tunefs.ocfs2/Makefile b/tunefs.ocfs2/Makefile
>>>> index 3847d0f..995d717 100644
>>>> --- a/tunefs.ocfs2/Makefile
>>>> +++ b/tunefs.ocfs2/Makefile
>>>> @@ -40,6 +40,7 @@ OCFS2NE_OPERATIONS =                        \
>>>>       op_resize_volume                \
>>>>       op_set_label                    \
>>>>       op_set_journal_size             \
>>>> +     op_set_journal_block            \
>>>>       op_set_slot_count               \
>>>>       op_update_cluster_stack         \
>>>>       op_set_quota_sync_interval      \
>>>> diff --git a/tunefs.ocfs2/libocfs2ne.c b/tunefs.ocfs2/libocfs2ne.c
>>>> index 8e814e7..7965727 100644
>>>> --- a/tunefs.ocfs2/libocfs2ne.c
>>>> +++ b/tunefs.ocfs2/libocfs2ne.c
>>>> @@ -331,7 +331,7 @@ errcode_t tunefs_clear_in_progress(ocfs2_filesys
>>>> *fs, int flag)
>>>>       return ocfs2_write_primary_super(fs);
>>>>  }
>>>>
>>>> -errcode_t tunefs_set_journal_size(ocfs2_filesys *fs, uint64_t new_size)
>>>> +errcode_t tunefs_set_journal_size(ocfs2_filesys *fs, uint64_t new_size,
>>>> ocfs2_fs_options mask, ocfs2_fs_options options)
>>>>  {
>>>>       errcode_t ret = 0;
>>>>       char jrnl_file[OCFS2_MAX_FILENAME_LEN];
>>>> @@ -343,6 +343,8 @@ errcode_t tunefs_set_journal_size(ocfs2_filesys *fs,
>>>> uint64_t new_size)
>>>>       struct ocfs2_dinode *di;
>>>>       struct tunefs_filesystem_state *state = tunefs_get_state(fs);
>>>>       struct tools_progress *prog;
>>>> +     ocfs2_fs_options new_features;
>>>> +     int features_change;
>>>>
>>>>       num_clusters =
>>>>               ocfs2_clusters_in_blocks(fs,
>>>> @@ -379,6 +381,16 @@ errcode_t tunefs_set_journal_size(ocfs2_filesys
>>>> *fs, uint64_t new_size)
>>>>               return ret;
>>>>       }
>>>>
>>>> +     new_features.opt_compat = (state->ts_journal_features.opt_compat&
>>>>  ~mask.opt_compat)
>>>> +                               | (options.opt_compat&
>>>>  mask.opt_compat);
>>>>
>>>> +     new_features.opt_incompat =
>>>> (state->ts_journal_features.opt_incompat&  ~mask.opt_incompat)
>>>> +                                 | (options.opt_incompat&
>>>>  mask.opt_incompat);
>>>>
>>>> +     new_features.opt_ro_compat =
>>>> (state->ts_journal_features.opt_ro_compat&  ~mask.opt_ro_compat)
>>>> +                                 | (options.opt_ro_compat&
>>>>  mask.opt_ro_compat);
>>>>
>>>> +     features_change = (new_features.opt_compat ^
>>>> state->ts_journal_features.opt_compat) ||
>>>> +                       (new_features.opt_incompat ^
>>>> state->ts_journal_features.opt_incompat) ||
>>>> +                       (new_features.opt_ro_compat ^
>>>> state->ts_journal_features.opt_ro_compat);
>>>> +
>>>>       for (i = 0; i<  max_slots; ++i) {
>>>>               ocfs2_sprintf_system_inode_name(jrnl_file,
>>>>                                               OCFS2_MAX_FILENAME_LEN,
>>>> @@ -404,7 +416,7 @@ errcode_t tunefs_set_journal_size(ocfs2_filesys *fs,
>>>> uint64_t new_size)
>>>>               }
>>>>
>>>>               di = (struct ocfs2_dinode *)buf;
>>>> -             if (num_clusters == di->i_clusters) {
>>>> +             if (num_clusters == di->i_clusters&&  !features_change) {
>>>>
>>>>                       tools_progress_step(prog, 1);
>>>>                       continue;
>>>>               }
>>>> @@ -413,7 +425,7 @@ errcode_t tunefs_set_journal_size(ocfs2_filesys *fs,
>>>> uint64_t new_size)
>>>>                        "Resizing journal \"%s\" to %"PRIu32"
>>>> clusters\n",
>>>>                        jrnl_file, num_clusters);
>>>>               ret = ocfs2_make_journal(fs, blkno, num_clusters,
>>>> -&state->ts_journal_features);
>>>> +&new_features);
>>>
>>>        Nowhere in any of this do you check the filesystem you are
>>> changing.  While ocfs2_make_journal() will reformat your journal, it
>>> doesn't handle any of the special case checking that we have in mkfs.
>>> Things like:
>>>
>>> - If your filesystem is larger than can be described by 32bits, you
>>>  can't go to block32, at least without shrinking your filesystem below
>>>  the 32bit boundary.
>>
>> Is it possible to shrink an ocfs2 filesystem? Is it in development?
>>
>> [I was on vacation so sorry if I have missed on something]
>
>
> No. But you are missing his point. The feature as you've implemented allows
> users to modify the journal feature without any checks. While it is alright
> to allow users to change to block64, it is not alright to allow the change
> to
> block32 without first checking the size of the volume.
>

Like the patches I have just posted ;)

> I am wondering whether the correct fix would be to just enable this feature
> during resize.
>

I have put errors in case it does not work with options to let users
know what they are doing. But yes, enabling during resize could be a
good option.

> Or, maybe just enable it by default during mkfs because users won't be able
> to online resize across the boundary safely.
>

This is for new users. What about existing users?

-- 
Goldwyn