Re: SUJ Changes

On 5/27/10, Garrett Cooper <yanefbsd_at_gmail.com> wrote:
> On Thu, May 27, 2010 at 3:44 PM, Marcelo/Porks <marcelorossi_at_gmail.com>
> wrote:
>> On 5/27/10, John Baldwin <jhb_at_freebsd.org> wrote:
>>> On Thursday 27 May 2010 10:13:38 am Marcelo/Porks wrote:
>>>> On Thu, May 27, 2010 at 10:33 AM, John Baldwin <jhb_at_freebsd.org> wrote:
>>>> > On Wednesday 26 May 2010 7:56:24 pm Garrett Cooper wrote:
>>>> >> On Wed, May 26, 2010 at 3:52 PM, Marcelo/Porks
>>>> >> <marcelorossi_at_gmail.com>
>>>> >>
>>> wrote:
>>>> >> >
>>>> >> > Hi guys. I'm not sure if I could call this a problem but I can
>>>> >> > disable
>>>> >> > SU when SUJ is enabled, so SUJ will remain enabled and SU will be
>>>> >> > disabled.
>>>> >> >
>>>> >> > #tunefs -j enable /dev/device
>>>> >> > #tunefs -n disable /dev/device
>>>> >> >
>>>> >> > I did a patch for sbin/tunefs/tunefs.c that disable SUJ when the
>>>> >> > user
>>>> >> > disable SU. Maybe this will be useful for some of you.
>>>> >> >
>>>> >> > Thanks.
>>>> >> >
>>>> >> >
>>>> >> > Index: sbin/tunefs/tunefs.c
>>>> >> > ===================================================================
>>>> >> > --- sbin/tunefs/tunefs.c        (revision 208580)
>>>> >> > +++ sbin/tunefs/tunefs.c        (working copy)
>>>> >> > _at__at_ -460,6 +460,14 _at__at_
>>>> >> >                        if ((~sblock.fs_flags & FS_DOSOFTDEP) ==
>>>> > FS_DOSOFTDEP)
>>>> >> >                                warnx("%s remains unchanged as
>>> disabled",
>>>> > name);
>>>> >> >                        else {
>>>> >> > +                               /* also disable SUJ */
>>>> >> > +                               if ((sblock.fs_flags & FS_SUJ) ==
>>> FS_SUJ)
>>>> > {
>>>> >> > +                                       warnx("soft updates
>>>> >> > journaling
>>>> >> > will be disabled too");
>>>> >> > +                                       journal_clear();
>>>> >> > +                                       sblock.fs_flags &= ~FS_SUJ;
>>>> >> > +                                       sblock.fs_sujfree = 0;
>>>> >> > +                                       warnx("remove .sujournal to
>>>> >> > reclaim space");
>>>> >> > +                               }
>>>> >> >                                sblock.fs_flags &= ~FS_DOSOFTDEP;
>>>> >> >                                warnx("%s cleared", name);
>>>> >> >                        }
>>>> >>
>>>> > I think that attempting to disable SU if SUJ
>>>> > is enabled should just fail with an error message.  The sysadmin can
>>>> > then
>>>> > choose to disable both SUJ and SU if desired.
>>>>
>>>> If SU is disabled and One tries to enable SUJ then SU will be
>>>> automatically enabled.
>>>> So Why not automatically disable SUJ when One tries to disable SU?
>>>
>>> I'm probably not a big fan of either really. :)  For something as rarely
>>> done
>>> as tunefs I would prefer to err on the side of caution and require the
>>> admin
>>> to explicitly specify everything.
>>
>> Hi John. I agree with you, I do prefer a fail of tunefs in this
>> situation. Before of I have posted the first patch I have done another
>> one that fails to enable SUJ when SU is disabled and fails to disable
>> SU when SUJ is enabled. Now this patch is bellow and attached.
>>
>> *All the stuff that I will say bellow is based on the attached patch*
>>
>> The attached patch fails to disable SU when SUJ is enabled.
>> And it fails to enable SUJ when SU is disabled.
>>
>> Earlier I decided to not post the patch because it will cause
>> confusion when SU and SUJ is disabled and you try to do something such
>> as:
>> # tunefs -j enable -n enable /dev/ad2s1d
>>
>> The flags's processing order is alphabetically, so It will try to
>> enable SUJ first (and it will fail) and after it will enable SU. We
>> could resolve this by processing '-n' (SU) first and after '-j' (SUJ)
>> but this schema will make to fail the following command:
>> # tunefs -j disable -n disable /dev/ad2s1d
>>
>> So I think we could create a function for each flag to be processed
>> and the main() calls this functions as necessary (and main() disabled
>> the relative flag so the function will not be called again).
>>
>> But I'm just a curious doing trivial patches, You can talk about this
>> with more experience and knowlege.
>>
>> =========patch==========
>>
>> Index: sbin/tunefs/tunefs.c
>> ===================================================================
>> --- sbin/tunefs/tunefs.c    (revision 208584)
>> +++ sbin/tunefs/tunefs.c    (working copy)
>> _at__at_ -341,16 +341,19 _at__at_
>>    if (jflag) {
>>        name = "soft updates journaling";
>>        if (strcmp(jvalue, "enable") == 0) {
>> -           if ((sblock.fs_flags & (FS_DOSOFTDEP | FS_SUJ)) ==
>> -               (FS_DOSOFTDEP | FS_SUJ)) {
>> +           if ((sblock.fs_flags & FS_SUJ) == FS_SUJ) {
>>                warnx("%s remains unchanged as enabled", name);
>> +           } else if ((sblock.fs_flags & FS_DOSOFTDEP) !=
>> +               FS_DOSOFTDEP) {
>> +               warnx("%s cannot be enabled until soft updates is
>> enabled",
>> +                   name);
>>            } else if (sblock.fs_clean == 0) {
>>                warnx("%s cannot be enabled until fsck is run",
>>                    name);
>>            } else if (journal_alloc(Svalue) != 0) {
>>                warnx("%s can not be enabled", name);
>>            } else {
>> -               sblock.fs_flags |= FS_DOSOFTDEP | FS_SUJ;
>> +               sblock.fs_flags |= FS_SUJ;
>
> I assume FS_DOSOFTDEP is being set elsewhere in the code?
>
>>                warnx("%s set", name);
>>            }
>>        } else if (strcmp(jvalue, "disable") == 0) {
>> _at__at_ -459,6 +462,9 _at__at_
>>        } else if (strcmp(nvalue, "disable") == 0) {
>>            if ((~sblock.fs_flags & FS_DOSOFTDEP) == FS_DOSOFTDEP)
>>                warnx("%s remains unchanged as disabled", name);
>> +           else if ((sblock.fs_flags & FS_SUJ) == FS_SUJ)
>> +               warnx("%s cannot be enabled until soft updates \
>> +journaling is disabled", name);
>
> This message doesn't make sense (enable vs disable).

Oh for god shake, I'm sorry. The correct message was supposed to be:
warnx("%s cannot be disabled until soft updates journaling is disabled", name);

Sorry...

-- 
Marcelo Rossi
"This e-mail is provided "AS IS" with no warranties, and confers no rights."