RFC: Pass TRIM through GELI

[Pawel Jakub Dawidek]

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