Re: SUJ Changes

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).

>            else {
>                sblock.fs_flags &= ~FS_DOSOFTDEP;
>                warnx("%s cleared", name);

Thanks,
-Garrett