Date: Thu, 25 Aug 2011 19:02:41 -0400 (EDT) From: Benjamin Kaduk <kaduk@MIT.EDU> To: Rick Macklem <rmacklem@uoguelph.ca> Cc: current@FreeBSD.org Subject: Re: fsid change of ZFS? Message-ID: <alpine.GSO.1.10.1108251901520.1411@multics.mit.edu> In-Reply-To: <1252487773.337126.1314285501432.JavaMail.root@erie.cs.uoguelph.ca> References: <1252487773.337126.1314285501432.JavaMail.root@erie.cs.uoguelph.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 25 Aug 2011, Rick Macklem wrote:
> Benjamin Kaduk wrote:
>>
>> If we're confident that we won't ever fully fill the hash table, I
>> would
>> think that this should wrap around back to zero (or one?) instead of
>> overflowing.
>>
> Here's my updated patch (it will wrap to 1 the first time and then
> exceed 255 if 1<->255 are all in use).
> --- kern/vfs_init.c.sav 2011-06-11 18:58:33.000000000 -0400
> +++ kern/vfs_init.c 2011-08-25 11:09:14.000000000 -0400
> @@ -39,6 +39,7 @@ __FBSDID("$FreeBSD: head/sys/kern/vfs_in
>
> #include <sys/param.h>
> #include <sys/systm.h>
> +#include <sys/fnv_hash.h>
> #include <sys/kernel.h>
> #include <sys/linker.h>
> #include <sys/mount.h>
> @@ -138,6 +139,9 @@ vfs_register(struct vfsconf *vfc)
> struct sysctl_oid *oidp;
> struct vfsops *vfsops;
> static int once;
> + struct vfsconf *tvfc;
> + uint32_t hashval;
> + int secondpass;
>
> if (!once) {
> vattr_null(&va_null);
> @@ -152,7 +156,31 @@ vfs_register(struct vfsconf *vfc)
> if (vfs_byname(vfc->vfc_name) != NULL)
> return EEXIST;
>
> - vfc->vfc_typenum = maxvfsconf++;
> + /*
> + * Calculate a hash on vfc_name to use for vfc_typenum. Unless
> + * all of 1<->255 are assigned, it is limited to 8bits since that is
> + * what ZFS uses from vfc_typenum and is also the preferred range
> + * for vfs_getnewfsid().
> + */
> + hashval = fnv_32_str(vfc->vfc_name, FNV1_32_INIT);
> + hashval &= 0xff;
> + secondpass = 0;
> + do {
> + /* Look for and fix any collision. */
> + TAILQ_FOREACH(tvfc, &vfsconf, vfc_list) {
> + if (hashval == tvfc->vfc_typenum) {
> + if (hashval == 255 && secondpass == 0) {
> + hashval = 1;
> + secondpass = 1;
> + } else
> + hashval++;
> + break;
> + }
> + }
> + } while (tvfc != NULL);
> + vfc->vfc_typenum = hashval;
> + if (vfc->vfc_typenum >= maxvfsconf)
> + maxvfsconf = vfc->vfc_typenum + 1;
> TAILQ_INSERT_TAIL(&vfsconf, vfc, vfc_list);
>
> /*
>
>> Do we need to care about something attempting to add the same vfc_name
>> twice? This code will happily add a second entry at the next available
>> index.
>>
> If file systems use VFS_SET(), I don't think this can happen, since the
> same vfc_name would imply "same module name" and the 2nd one wouldn't load.
> (Been there, w.r.t. nfs.)
Ah. I guess I should get my act together and use VFS_SET, then.
*hangs head sheepishly*
>
>>> + break;
>>> + }
>>> + }
>>> + } while (tvfc != NULL);
>>> + vfc->vfc_typenum = hashval;
>>> + if (vfc->vfc_typenum >= maxvfsconf)
>>> + maxvfsconf = vfc->vfc_typenum + 1;
>>
>> I guess we're holding off on killing maxvfsconf until after 9.0 is
>> out?
>
> Well, I still don't know if anything has a use for vfs_sysctl(), so
> I'm not volunteering to take it out. (If others feel it should come
> out for 9.0, maybe... But I would still consider that a separate patch.)
I don't particularly have an axe to grind, Danish or otherwise.
Thanks for the update,
Ben
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.GSO.1.10.1108251901520.1411>