Negative caching of failed mount attempts

Negative caching of failed mount attempts

[Leonardo Chiquitto]

Hello,

Three years ago, commit 2d753e62b8 removed kernel-side caching
of mount attempts that returns -ENOENT. Apparently it wasn't
working as it should.
I see that this feature is now implemented in the user land AutoFS 5
daemon, but it wasn't back ported to AutoFS 4. I understand that
version 4 is not maintained anymore, but I'd like to ask if someone
has a patch around that implements this feature.

Thanks,
Leonardo

Re: Negative caching of failed mount attempts

[Ian Kent]

Leonardo Chiquitto wrote:
> Hello,
> 
> Three years ago, commit 2d753e62b8 removed kernel-side caching
> of mount attempts that returns -ENOENT. Apparently it wasn't
> working as it should.

It stopped working when 2.6 was released due to changes in the VFS.
These changes made it extremely hard to do in the kernel module. Efforts
to do it in the kernel module have been abandoned at this stage.

> I see that this feature is now implemented in the user land AutoFS 5
> daemon, but it wasn't back ported to AutoFS 4. I understand that
> version 4 is not maintained anymore, but I'd like to ask if someone
> has a patch around that implements this feature.

I guess that's what it amounts to, since all the effort is going into v5.

I have a bunch of patches for v4 from Jeff Moyer that I've not got
around to looking closely at and applying. We didn't get as far as the
negative lookup caching patch though. In fact I don't think it had been
written then.

What version of autofs?
Do you have any other patches applied to it?

Ian

Re: Negative caching of failed mount attempts

[Leonardo Chiquitto]

>> Three years ago, commit 2d753e62b8 removed kernel-side caching
>> of mount attempts that returns -ENOENT. Apparently it wasn't
>> working as it should.
>
> It stopped working when 2.6 was released due to changes in the VFS.
> These changes made it extremely hard to do in the kernel module. Efforts
> to do it in the kernel module have been abandoned at this stage.

Thanks for the details.

> I guess that's what it amounts to, since all the effort is going into v5.
>
> I have a bunch of patches for v4 from Jeff Moyer that I've not got
> around to looking closely at and applying. We didn't get as far as the
> negative lookup caching patch though. In fact I don't think it had been
> written then.

I just checked that Jeff implemented it for RHEL 4. I'll take a look.

> What version of autofs?

autofs-4.1.4

> Do you have any other patches applied to it?

Yes, I have all patches released after 4.1.4 (SLES 10 autofs package).

Thanks,
Leonardo

Re: Negative caching of failed mount attempts

[Ian Kent]

Leonardo Chiquitto wrote:
>>> Three years ago, commit 2d753e62b8 removed kernel-side caching
>>> of mount attempts that returns -ENOENT. Apparently it wasn't
>>> working as it should.
>> It stopped working when 2.6 was released due to changes in the VFS.
>> These changes made it extremely hard to do in the kernel module. Efforts
>> to do it in the kernel module have been abandoned at this stage.
> 
> Thanks for the details.
> 
>> I guess that's what it amounts to, since all the effort is going into v5.
>>
>> I have a bunch of patches for v4 from Jeff Moyer that I've not got
>> around to looking closely at and applying. We didn't get as far as the
>> negative lookup caching patch though. In fact I don't think it had been
>> written then.
> 
> I just checked that Jeff implemented it for RHEL 4. I'll take a look.
> 
>> What version of autofs?
> 
> autofs-4.1.4
> 
>> Do you have any other patches applied to it?
> 
> Yes, I have all patches released after 4.1.4 (SLES 10 autofs package).

So what do you have, 4.1.4 will all the patches from kernel.org or the
SLES 10 package, or an SLES 10 update of some sort?

> 
> Thanks,
> Leonardo

Re: Negative caching of failed mount attempts

[Leonardo Chiquitto]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

> So what do you have, 4.1.4 will all the patches from kernel.org or the
> SLES 10 package, or an SLES 10 update of some sort?

Sorry for not being specific here.

I'm using the latest SLES10 update, which basically is 4.1.4 + some
patches from kernel.org + distribution specific configuration and build
changes + a couple of other bug fixes that are not upstream.

To make things easier to everyone, I grabbed the latest tarball and
patches from kernel.org and applied autofs-4.1.4-negative-timeout.patch
from RHEL 4 (credits to Jeff Moyer). Just one chunk was rejected (the
one from lookup_hesiod.c). Please find the refreshed patch attached,
minimally tested so far.

I'd appreciate if it could be merged upstream (after proper testing, of
course).

Thanks
Leonardo

[-- Attachment #2: autofs-4.1.4-negative-timeout.patch --]
[-- Type: text/x-patch, Size: 14007 bytes --]

Index: autofs-4.1.4/daemon/automount.c
===================================================================
--- autofs-4.1.4.orig/daemon/automount.c
+++ autofs-4.1.4/daemon/automount.c
@@ -603,6 +603,54 @@ static int send_fail(unsigned int wait_q
 	return 0;
 }
 
+/*
+ *  This function updates the negative lookup cache.  The negative
+ *  lookup cache keeps track of failed mounts in order to reduce
+ *  mountd request load on the server.  This cache has a configurable
+ *  timeout, via the daemon option --negative-timeout.
+ */
+void update_negative_cache(char *key)
+{
+	int ret;
+	struct mapent_cache *me;
+
+	debug("update_negative_cache: key: %s", key);
+	if (key == NULL)
+		return;
+
+	/*
+	 * This function should only be called for keys that are not in
+	 * the cache.
+	 */
+	me = cache_lookup(key);
+	if (me && strcmp(me->key, key) == 0) {
+		if (me->negative) {
+			debug("Error: found negative cache entry for key %s\n",
+			      key);
+			return;
+		}
+	} else /* don't match the wild-card key */
+		me = NULL;
+
+	/* Add a negative entry to the cache */
+	debug("Adding negative cache entry for key %s\n", key);
+	if (!me) {
+		ret = cache_add(ap.path, key, NULL, time(NULL));
+		if (ret != CHE_OK) {
+			info("unable to cache failed lookup for %s", key);
+			return;
+		}
+		me = cache_lookup(key);
+		if (!me || strcmp(me->key, key) != 0) {
+			debug("added an entry and it doesn't exist!?");
+			return;
+		}
+	}
+	me->negative = 1;
+	me->last_lookup = time(NULL);
+	debug("Key %s added to negative cache\n", key);
+}
+
 /* Handle exiting children (either from SIGCHLD or synchronous wait at
    shutdown), and return the next state the system should enter as a
    result.  */
@@ -696,9 +744,16 @@ static enum states handle_child(int hang
 				pid, WIFSIGNALED(status),
 				WTERMSIG(status), WEXITSTATUS(status));
 
-			if (WIFSIGNALED(status) || WEXITSTATUS(status) != 0)
+			if (WIFSIGNALED(status))
 				send_fail(mt->wait_queue_token);
-			else
+			else if (WEXITSTATUS(status) != 0) {
+				int error_code = WEXITSTATUS(status);
+
+				if (error_code == EXIT_CODE_MOUNT_FAILED)
+					update_negative_cache(mt->key);
+
+				send_fail(mt->wait_queue_token);
+			} else
 				send_ready(mt->wait_queue_token);
 
 			/* Delete from list and add to freelist,
@@ -1077,6 +1132,7 @@ static int handle_packet_missing(const s
 	sigset_t oldsig;
 	pid_t f;
 	struct pending_mount *mt = NULL;
+	struct mapent_cache *me;
 
 	debug("handle_packet_missing: token %ld, name %s\n",
 		(unsigned long) pkt->wait_queue_token, pkt->name);
@@ -1087,6 +1143,32 @@ static int handle_packet_missing(const s
 		return 0;
 	}
 
+	/* Check to see if there is a negative cache entry */
+	me = cache_lookup(pkt->name);
+	if (me != NULL && strcmp(me->key, pkt->name) == 0 &&
+	    me->negative) {
+		if (time(NULL) - me->last_lookup < ap.negative_timeout) {
+			debug("%s: found negative cache entry for key.\n",
+			      __FUNCTION__);
+			send_fail(pkt->wait_queue_token);
+			return 0;
+		}
+		/*
+		 * If the negative cache entry expired, then remove it.
+		 * This means 1 of 2 things:
+		 * 1) if an entry actually exists in the map, then clear
+		 *    the negative flag to trigger a new lookup
+		 * 2) if this key was actually matched to the wildcard,
+		 *    then remove the cache entry altogether.
+		 */
+		debug("%s: expired negative cache entry for key.\n",
+		      __FUNCTION__);
+		if (me->mapent == NULL)
+			cache_delete(ap.path, pkt->name, 0);
+		else
+			me->negative = 0;
+	}
+
 	chdir(ap.path);
 	if (lstat(pkt->name, &st) == -1 ||
 	   (S_ISDIR(st.st_mode) && st.st_dev == ap.dev)) {
@@ -1162,7 +1244,7 @@ static int handle_packet_missing(const s
 					rm_unwanted(buf, 1, 0); */
 			}
 
-			_exit(err ? 1 : 0);
+			_exit(err);
 		} else {
 			/*
 			 * Important: set up data structures while signals
@@ -1170,6 +1252,8 @@ static int handle_packet_missing(const s
 			 */
 			mt->pid = f;
 			mt->wait_queue_token = pkt->wait_queue_token;
+			mt->key = strdup(pkt->name); /* ok if this fails */
+			debug("mt->key set to %s\n", mt->key);
 			mt->next = ap.mounts;
 			ap.mounts = mt;
 
@@ -1733,6 +1817,7 @@ int main(int argc, char *argv[])
 		{"version", 0, 0, 'V'},
 		{"ghost", 0, 0, 'g'},
 		{"submount", 0, &submount, 1},
+		{"negative-timeout", 1, 0, 'n'},
 		{0, 0, 0, 0}
 	};
 
@@ -1743,9 +1828,10 @@ int main(int argc, char *argv[])
 	ap.ghost = DEFAULT_GHOST_MODE;
 	ap.type = LKP_INDIRECT;
 	ap.dir_created = 0; /* We haven't created the main directory yet */
+	ap.negative_timeout = 60;
 
 	opterr = 0;
-	while ((opt = getopt_long(argc, argv, "+hp:t:vdVg", long_options, NULL)) != EOF) {
+	while ((opt = getopt_long(argc, argv, "+hp:t:vdVgn:", long_options, NULL)) != EOF) {
 		switch (opt) {
 		case 'h':
 			usage();
@@ -1774,7 +1860,9 @@ int main(int argc, char *argv[])
 		case 'g':
 			ap.ghost = LKP_GHOST;
 			break;
-
+		case 'n':
+			ap.negative_timeout = getnumopt(optarg, opt);
+			break;
 		case '?':
 		case ':':
 			printf("%s: Ambiguous or unknown options\n", program);
Index: autofs-4.1.4/include/automount.h
===================================================================
--- autofs-4.1.4.orig/include/automount.h
+++ autofs-4.1.4/include/automount.h
@@ -96,9 +96,13 @@ enum states {
 struct pending_mount {
 	pid_t pid;		/* Which process is mounting for us */
 	unsigned long wait_queue_token;	/* Associated kernel wait token */
+	char *key;		/* Store key for negative caching */
 	volatile struct pending_mount *next;
 };
 
+#define EXIT_CODE_GENERIC_FAILURE	1
+#define EXIT_CODE_MOUNT_FAILED		2
+
 struct autofs_point {
 	char *path;			/* Mount point name */
 	int pipefd;			/* File descriptor for pipe */
@@ -116,6 +120,7 @@ struct autofs_point {
 	int state_pipe[2];
 	unsigned dir_created;		/* Was a directory created for this
 					   mount? */
+	time_t negative_timeout;	/* Timeout in secs for failed mounts */
 };
 
 extern struct autofs_point ap; 
@@ -238,6 +243,9 @@ struct mapent_cache {
 	char *key;
 	char *mapent;
 	time_t age;
+	int negative;	    /* boolean, set to indicate a failed lookup */
+	time_t last_lookup; /* time of last lookup, used for negative cache
+			     * expiration */
 };
 
 void cache_init(void);
Index: autofs-4.1.4/modules/lookup_file.c
===================================================================
--- autofs-4.1.4.orig/modules/lookup_file.c
+++ autofs-4.1.4/modules/lookup_file.c
@@ -252,7 +252,7 @@ static int read_map(const char *root, ti
 	while(1) {
 		entry = read_one(f, key, mapent);
 		if (entry)
-			cache_add(root, key, mapent, age);
+			cache_update(root, key, mapent, age);
 
 		if (feof(f))
 			break;
@@ -383,7 +383,7 @@ int lookup_mount(const char *root, const
 	char key[KEY_MAX_LEN + 1];
 	int key_len;
 	char mapent[MAPENT_MAX_LEN + 1];
-	struct mapent_cache *me;
+	struct mapent_cache *me, *exists;
 	time_t now = time(NULL);
 	time_t t_last_read;
 	int need_hup = 0;
@@ -405,10 +405,15 @@ int lookup_mount(const char *root, const
 	me = cache_lookup_first();
 	t_last_read = me ? now - me->age : ap.exp_runfreq + 1;
 
-	/* only if it has been modified */
+	/* only perform a real lookup if the map was modified */
 	if (st.st_mtime > ctxt->mtime) {
+		/* cache_lookup may match wild-card */
+		exists = cache_lookup(key);
+		if (exists && strcmp(exists->key, key) != 0)
+			exists = NULL;
+
 		ret = lookup_one(root, key, key_len, ctxt);
-		if (!ret)
+		if (ret == CHE_FAIL)
 			return 1;
 
 		debug("ret = %d", ret);
@@ -417,7 +422,7 @@ int lookup_mount(const char *root, const
 			if (ret & (CHE_UPDATED | CHE_MISSING))
 				need_hup = 1;
 
-		if (ret == CHE_MISSING) {
+		if ((ret & CHE_MISSING) && exists) {
 			int wild = CHE_MISSING;
 
 			/* Maybe update wild card map entry */
@@ -445,13 +450,22 @@ int lookup_mount(const char *root, const
 	if (me) {
 		debug(MODPREFIX "%s -> %s", key, mapent);
 		ret = ctxt->parse->parse_mount(root, name, name_len,
-						  mapent, ctxt->parse->context);
-	}
+					       mapent, ctxt->parse->context);
+	} else {
+		ret = 1;
+		error(MODPREFIX "key \"%s\" not found in map.", name);
+        }
 
 	/* Have parent update its map ? */
 	if (need_hup)
 		kill(getppid(), SIGHUP);
 
+	/* EXIT_CODE_MOUNT_FAILED is caught by the upper layers
+	 * and used to populate the negative lookup cache */
+	if (ret)
+		ret = EXIT_CODE_MOUNT_FAILED;
+
+	debug(MODPREFIX "lookup_mount returning %d\n", ret);
 	return ret;
 }
 
Index: autofs-4.1.4/modules/lookup_hesiod.c
===================================================================
--- autofs-4.1.4.orig/modules/lookup_hesiod.c
+++ autofs-4.1.4/modules/lookup_hesiod.c
@@ -125,6 +125,10 @@ int lookup_mount(const char *root, const
 #else
 	free(hes_result);
 #endif
+	/* EXIT_CODE_MOUNT_FAILED is caught by the upper layers
+	 * and used to populate the negative lookup cache */
+	if (rv)
+		rv = EXIT_CODE_MOUNT_FAILED;
 	return rv;
 }
 
Index: autofs-4.1.4/modules/lookup_ldap.c
===================================================================
--- autofs-4.1.4.orig/modules/lookup_ldap.c
+++ autofs-4.1.4/modules/lookup_ldap.c
@@ -657,6 +657,15 @@ int lookup_mount(const char *root, const
 			debug(MODPREFIX "%s -> %s", key, mapent);
 			ret = ctxt->parse->parse_mount(root, name, name_len,
 						  mapent, ctxt->parse->context);
+			/*
+			 *  If the mount fails, it is most likely due to
+			 *  the directory not existing on the server.  In
+			 *  such a case, we want to remember that the mount
+			 *  failed so that we don't trigger successive
+			 *  mounts.
+			 */
+			if (ret)
+				ret = EXIT_CODE_MOUNT_FAILED;
 			me = cache_lookup_next(me);
 		}
 	} else {
Index: autofs-4.1.4/modules/lookup_multi.c
===================================================================
--- autofs-4.1.4.orig/modules/lookup_multi.c
+++ autofs-4.1.4/modules/lookup_multi.c
@@ -134,14 +134,14 @@ int lookup_ghost(const char *root, int g
 int lookup_mount(const char *root, const char *name, int name_len, void *context)
 {
 	struct lookup_context *ctxt = (struct lookup_context *) context;
-	int i;
+	int i, ret;
 
 	for (i = 0; i < ctxt->n; i++) {
-		if (ctxt->m[i].mod->lookup_mount(root, name, name_len,
-						 ctxt->m[i].mod->context) == 0)
+		if ((ret = ctxt->m[i].mod->lookup_mount(root, name, name_len,
+						ctxt->m[i].mod->context)) == 0)
 			return 0;
 	}
-	return 1;		/* No module succeeded */
+	return ret;		/* No module succeeded */
 }
 
 int lookup_done(void *context)
Index: autofs-4.1.4/modules/lookup_nisplus.c
===================================================================
--- autofs-4.1.4.orig/modules/lookup_nisplus.c
+++ autofs-4.1.4/modules/lookup_nisplus.c
@@ -99,6 +99,10 @@ int lookup_mount(const char *root, const
 				      NIS_RES_OBJECT(result)->EN_data.en_cols.
 				      en_cols_val[1].ec_value.ec_value_val,
 				      ctxt->parse->context);
+	/* EXIT_CODE_MOUNT_FAILED is caught by the upper layers
+	 * and used to populate the negative lookup cache */
+	if (rv)
+		rv = EXIT_CODE_MOUNT_FAILED;
 	return rv;
 }
 
Index: autofs-4.1.4/lib/cache.c
===================================================================
--- autofs-4.1.4.orig/lib/cache.c
+++ autofs-4.1.4/lib/cache.c
@@ -179,16 +179,22 @@ int cache_add(const char *root, const ch
 		return CHE_FAIL;
 	}
 
-	pent = malloc(strlen(mapent) + 1);
-	if (!pent) {
-		free(me);
-		free(pkey);
-		return CHE_FAIL;
+	if (mapent) {
+		pent = malloc(strlen(mapent) + 1);
+		if (!pent) {
+			free(me);
+			free(pkey);
+			return CHE_FAIL;
+		}
 	}
-
 	me->key = strcpy(pkey, key);
-	me->mapent = strcpy(pent, mapent);
+	if (mapent)
+		me->mapent = strcpy(pent, mapent);
+	else
+		me->mapent = NULL;
 	me->age = age;
+	me->negative = 0;
+	me->last_lookup = 0;
 
 	/* 
 	 * We need to add to the end if values exist in order to
Index: autofs-4.1.4/modules/lookup_program.c
===================================================================
--- autofs-4.1.4.orig/modules/lookup_program.c
+++ autofs-4.1.4/modules/lookup_program.c
@@ -277,6 +277,10 @@ int lookup_mount(const char *root, const
 
 	ret = ctxt->parse->parse_mount(root, name, name_len,
 				       mapent, ctxt->parse->context);
+	/* EXIT_CODE_MOUNT_FAILED is caught by the upper layers
+	 * and used to populate the negative lookup cache */
+	if (ret)
+		ret = EXIT_CODE_MOUNT_FAILED;
 out_free:
 	free(mapent);
 	return ret;
Index: autofs-4.1.4/modules/lookup_userhome.c
===================================================================
--- autofs-4.1.4.orig/modules/lookup_userhome.c
+++ autofs-4.1.4/modules/lookup_userhome.c
@@ -62,7 +62,9 @@ int lookup_mount(const char *root, const
 
 	if (symlink(pw->pw_dir, name) && errno != EEXIST) {
 		error(MODPREFIX "symlink failed: %m");
-		return 1;
+		/* EXIT_CODE_MOUNT_FAILED is caught by the upper layers
+		 * and used to populate the negative lookup cache */
+		return EXIT_CODE_MOUNT_FAILED;
 	}
 
 	return 0;
Index: autofs-4.1.4/modules/lookup_yp.c
===================================================================
--- autofs-4.1.4.orig/modules/lookup_yp.c
+++ autofs-4.1.4/modules/lookup_yp.c
@@ -220,7 +220,7 @@ int lookup_mount(const char *root, const
 	int key_len;
 	char *mapent;
 	int mapent_len;
-	struct mapent_cache *me;
+	struct mapent_cache *me, *exists;
 	time_t now = time(NULL);
 	time_t t_last_read;
 	int need_hup = 0;
@@ -236,7 +236,15 @@ int lookup_mount(const char *root, const
 	if (key_len > KEY_MAX_LEN)
 		return 1;
 
-	/* check map and if change is detected re-read map */
+	/*
+	 * Check map and if change is detected re-read map
+	 */
+
+	/* First check to see if this entry exists in the cache */
+	exists = cache_lookup(key);
+	if (exists && strcmp(exists->key, key) != 0)
+		exists = NULL;
+
 	ret = lookup_one(root, key, key_len, ctxt);
 	if (!ret)
 		return 1;
@@ -297,6 +305,11 @@ int lookup_mount(const char *root, const
 	if (need_hup)
 		kill(getppid(), SIGHUP);
 
+	/* EXIT_CODE_MOUNT_FAILED is caught by the upper layers
+	 * and used to populate the negative lookup cache */
+	if (ret)
+		ret = EXIT_CODE_MOUNT_FAILED;
+
 	return ret;
 }
 

[-- Attachment #3: Type: text/plain, Size: 140 bytes --]

_______________________________________________
autofs mailing list
autofs@linux.kernel.org
http://linux.kernel.org/mailman/listinfo/autofs

Re: Negative caching of failed mount attempts

[Jeff Moyer]

Leonardo Chiquitto <leonardo.lists@gmail.com> writes:

>> So what do you have, 4.1.4 will all the patches from kernel.org or the
>> SLES 10 package, or an SLES 10 update of some sort?
>
> Sorry for not being specific here.
>
> I'm using the latest SLES10 update, which basically is 4.1.4 + some
> patches from kernel.org + distribution specific configuration and build
> changes + a couple of other bug fixes that are not upstream.
>
> To make things easier to everyone, I grabbed the latest tarball and
> patches from kernel.org and applied autofs-4.1.4-negative-timeout.patch
> from RHEL 4 (credits to Jeff Moyer). Just one chunk was rejected (the
> one from lookup_hesiod.c). Please find the refreshed patch attached,
> minimally tested so far.
>
> I'd appreciate if it could be merged upstream (after proper testing, of
> course).

I think you need at least two more patches:

autofs-4.1.3-negative-timeout-doc-update.patch
autofs-4.1.3-teach-cache-about-negative-mapents.patch

There may be more, though.

Cheers,
Jeff

Re: Negative caching of failed mount attempts

[Jeff Moyer]

Ian Kent <raven@themaw.net> writes:

> Leonardo Chiquitto wrote:
>> Hello,
>> 
>> Three years ago, commit 2d753e62b8 removed kernel-side caching
>> of mount attempts that returns -ENOENT. Apparently it wasn't
>> working as it should.
>
> It stopped working when 2.6 was released due to changes in the VFS.
> These changes made it extremely hard to do in the kernel module. Efforts
> to do it in the kernel module have been abandoned at this stage.
>
>> I see that this feature is now implemented in the user land AutoFS 5
>> daemon, but it wasn't back ported to AutoFS 4. I understand that
>> version 4 is not maintained anymore, but I'd like to ask if someone
>> has a patch around that implements this feature.
>
> I guess that's what it amounts to, since all the effort is going into v5.
>
> I have a bunch of patches for v4 from Jeff Moyer that I've not got
> around to looking closely at and applying. We didn't get as far as the
> negative lookup caching patch though. In fact I don't think it had been
> written then.

Tough call on what to do here.  I don't like that we have all of these
patches in RHEL only.  At the same time, new distribution versions
should be using autofs5.  So, I'm having a hard time justifying the work
of merging all of the work we've done in the RHEL package.

Are people still looking for new features in autofs 4?  If so, why?

Cheers,
Jeff

Re: Negative caching of failed mount attempts

[Leonardo Chiquitto]

>> I have a bunch of patches for v4 from Jeff Moyer that I've not got
>> around to looking closely at and applying. We didn't get as far as the
>> negative lookup caching patch though. In fact I don't think it had been
>> written then.
>
> Tough call on what to do here.  I don't like that we have all of these
> patches in RHEL only.  At the same time, new distribution versions
> should be using autofs5.  So, I'm having a hard time justifying the work
> of merging all of the work we've done in the RHEL package.

I was impressed by the amount of patches too. You are doing a
great job maintaining it.

> Are people still looking for new features in autofs 4?  If so, why?

In this specific case it's not really a new feature, because -- even if
with bugs -- it was there before.