Date: Mon, 16 Mar 2015 02:08:45 +0100 From: Pawel Jakub Dawidek <pjd@FreeBSD.org> To: "Matthew D. Fuller" <fullermd@over-yonder.net> Cc: freebsd-geom@freebsd.org Subject: Re: RFC: Pass TRIM through GELI Message-ID: <20150316010845.GA1515@garage.freebsd.pl> In-Reply-To: <20150315182444.GB52331@over-yonder.net> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Mar 15, 2015 at 01:24:45PM -0500, Matthew D. Fuller wrote: > > Whoops. Well, that was dumber than usual of me :) > > <http://www.over-yonder.net/~fullermd/dl/geli-delete.3.patch>; > > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150316010845.GA1515>