From owner-freebsd-geom@FreeBSD.ORG Mon Mar 16 01:11:39 2015 Return-Path: Delivered-To: freebsd-geom@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id E9B3CD15 for ; Mon, 16 Mar 2015 01:11:39 +0000 (UTC) Received: from mail.dawidek.net (garage.dawidek.net [91.121.88.72]) by mx1.freebsd.org (Postfix) with ESMTP id B1F471F6 for ; Mon, 16 Mar 2015 01:11:38 +0000 (UTC) Received: from localhost (unknown [91.206.210.241]) by mail.dawidek.net (Postfix) with ESMTPSA id 7ECF2D16; Mon, 16 Mar 2015 02:05:56 +0100 (CET) Date: Mon, 16 Mar 2015 02:08:45 +0100 From: Pawel Jakub Dawidek To: "Matthew D. Fuller" Subject: Re: RFC: Pass TRIM through GELI Message-ID: <20150316010845.GA1515@garage.freebsd.pl> References: <20150308000131.GP1742@over-yonder.net> <20150314151453.GJ24274@over-yonder.net> <501bca86.508d8e26@fabiankeil.de> <20150315145324.GA52331@over-yonder.net> <20150315182444.GB52331@over-yonder.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150315182444.GB52331@over-yonder.net> X-OS: FreeBSD 11.0-CURRENT amd64 User-Agent: Mutt/1.5.23 (2014-03-12) Cc: freebsd-geom@freebsd.org X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Mar 2015 01:11:40 -0000 On Sun, Mar 15, 2015 at 01:24:45PM -0500, Matthew D. Fuller wrote: > > Whoops. Well, that was dumber than usual of me :) > > > > now includes that fix. Overall the patch looks good. The main concern I have is that we do nothing if the underlying provider returns EOPNOTSUPP on BIO_DELETE, we will just keep sending those requests. Few small nits inline. --- sbin/geom/class/eli/geli.8 (revision 279210) +++ sbin/geom/class/eli/geli.8 (working copy) [...] +It will, however, provide some information (how much space you're actually I've been told some time ago that one should use 'you are', etc. in manual pages. --- sbin/geom/class/eli/geom_eli.c (revision 279210) +++ sbin/geom/class/eli/geom_eli.c (working copy) [...] + if(changed) Missing space after 'if'. --- sys/geom/eli/g_eli.c (revision 279210) +++ sys/geom/eli/g_eli.c (working copy) @@ -314,8 +314,11 @@ /* * We could eventually support BIO_DELETE request. * It could be done by overwritting requested sector with - * random data g_eli_overwrites number of times. + * random data g_eli_overwrites number of times. Or if the user + * has set the DELETE flag, we just pass it down the stack. This whole comment needs to be rewritten. In the current form it is not obvious if we support BIO_DELETE in any way or not. I'd start with what we got and add a note at the end what might be the other way to handle BIO_DELETE. --- sys/geom/eli/g_eli.h (revision 279210) +++ sys/geom/eli/g_eli.h (working copy) @@ -94,6 +94,8 @@ #define G_ELI_FLAG_AUTH 0x00000010 /* Provider is read-only, we should deny all write attempts. */ #define G_ELI_FLAG_RO 0x00000020 +/* Pass through BIO_DELETE requests */ Missing period at the end of the sentence. --- sys/geom/eli/g_eli_ctl.c (revision 279210) +++ sys/geom/eli/g_eli_ctl.c (working copy) @@ -377,11 +384,13 @@ char param[16]; const char *prov; u_char *sector; - int *nargs, *boot, *noboot; + int *nargs, *boot, *noboot, *trim, *notrim; int error; + int changed; Feel free to merge it with 'int error'. g_topology_assert(); + changed = 0; Please add an empty line to separate assertion from the initialization. + if (sc->sc_flags & G_ELI_FLAG_ONETIME) { + gctl_error(req, "Cannot change configuration of " + "onetime provider %s.", prov); + continue; + } Actually, nothing stops us from allowing to change trim/notrim for one-time providers. It is just we had no options before that would make sense for those kind of providers. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://mobter.com