Re: SUJ Changes

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;
                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);
            else {
                sblock.fs_flags &= ~FS_DOSOFTDEP;
                warnx("%s cleared", name);



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