Re: fsid change of ZFS?

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
> _at__at_ -39,6 +39,7 _at__at_ __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>
> _at__at_ -138,6 +139,9 _at__at_ 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);
> _at__at_ -152,7 +156,31 _at__at_ 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