From owner-freebsd-current@FreeBSD.ORG Fri May 28 02:04:18 2010 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 715631065675 for ; Fri, 28 May 2010 02:04:18 +0000 (UTC) (envelope-from marcelorossi@gmail.com) Received: from mail-qy0-f181.google.com (mail-qy0-f181.google.com [209.85.221.181]) by mx1.freebsd.org (Postfix) with ESMTP id 297D88FC14 for ; Fri, 28 May 2010 02:04:17 +0000 (UTC) Received: by qyk11 with SMTP id 11so1043633qyk.13 for ; Thu, 27 May 2010 19:04:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:received:in-reply-to :references:date:message-id:subject:from:to:content-type; bh=jI3q6rqO4n7CTPiHWIOwUqPuQK1YnTQS2gGmxuDezr4=; b=BOZE2AmheRrXFZewcByvLmKvAdolFn6IReCNwSFEawhH5PGUhYRg4ZR8Xme47iaCOo GVo5HohjYLVVUzaJgtL3FxaaZL+ghkwFEXZLyzWW1h5EVcGi2HfuFk6INxDKdGiScHXj JvuyXjuXAJTJQ1TI04yU6EouyfyNyqpix1TBQ= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; b=lA6iJ0CXxT0+n0qltHE2klqJ2sfXdGkD5xPMyj73kMlMYBZCQVeuuRcqb+jCW+mE1o hae8BPrY5rKGOZaP7sdHE7+ykEB0DkI8CXp8RyQ8HHyhz0BEPSKlLmaRMd32HRSoS7BK pI8qDFMUtHrwV0dCus2X+DFtb7z+HPmSXN+2w= MIME-Version: 1.0 Received: by 10.229.182.1 with SMTP id ca1mr2493665qcb.158.1275012257281; Thu, 27 May 2010 19:04:17 -0700 (PDT) Received: by 10.229.80.6 with HTTP; Thu, 27 May 2010 19:04:16 -0700 (PDT) In-Reply-To: References: <201005270933.42760.jhb@freebsd.org> <201005271147.20155.jhb@freebsd.org> Date: Thu, 27 May 2010 23:04:16 -0300 Message-ID: From: "Marcelo/Porks" To: freebsd-current@freebsd.org Content-Type: text/plain; charset=ISO-8859-1 Subject: Re: SUJ Changes X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 May 2010 02:04:18 -0000 On 5/27/10, Garrett Cooper wrote: > On Thu, May 27, 2010 at 3:44 PM, Marcelo/Porks > wrote: >> On 5/27/10, John Baldwin wrote: >>> On Thursday 27 May 2010 10:13:38 am Marcelo/Porks wrote: >>>> On Thu, May 27, 2010 at 10:33 AM, John Baldwin wrote: >>>> > On Wednesday 26 May 2010 7:56:24 pm Garrett Cooper wrote: >>>> >> On Wed, May 26, 2010 at 3:52 PM, Marcelo/Porks >>>> >> >>>> >> >>> 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) >>>> >> > @@ -460,6 +460,14 @@ >>>> >> > 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) >> @@ -341,16 +341,19 @@ >> 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) { >> @@ -459,6 +462,9 @@ >> } 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."