Re: Vnode API change: add global vnode cache

["J. Hannken-Illjes" <hannken%eis.cs.tu-bs.de@localhost>]

On 09 Apr 2014, at 19:09, Chuck Silvers <chuq%chuq.com@localhost> wrote:

> On Tue, Apr 08, 2014 at 11:23:11AM +0200, J. Hannken-Illjes wrote:
>> 
>> On 07 Apr 2014, at 19:28, Chuck Silvers <chuq%chuq.com@localhost> wrote:
>> 
>>> On Sun, Apr 06, 2014 at 12:14:24PM +0200, J. Hannken-Illjes wrote:
>>>> Currently all file systems have to implement their own cache of
>>>> vnode / fs node pairs.  Most file systems use a copy and pasted
>>>> version of ufs_ihash.
>>>> 
>>>> So add a global vnode cache with lookup and remove:
>>> [...]
>>> 
>>> hi,
>>> 
>>> the principle of this is good, but I have a couple concerns about the 
>>> details:
>>> 
>>> - why use an rb tree instead of a hash table?
>>>  other people are saying that the lock contention is the same,
>>>  but there are two differences:
>>>   - updating an rb tree touches a lot more memory, so the lock is held for
>>>     longer.
>>>   - different parts of a hash table can easily be protected by
>>>     different locks, but that doesn't work for a tree.
>> 
>> The underlying data structure may change.  Once at least ufs, layerfs and nfs
>> use this cache it will be easier to do some measurements.
> 
> ok, no point in arguing this further without some actual data.
> but we should have that data before this is checked in.
> 
> 
>>> - it looks like this increases the effective size of a vnode by a net 40 
>>> bytes
>>>  (7 pointers added, 2 removed).  it would be better to do this without using
>>>  more memory than we do now.  is there any real reason to have a separate
>>>  structure for this rather than just putting the fields directly in
>>>  struct vnode?
>> 
>> Not all file systems will use the cache (tmpfs for example doesn't need it)
>> so to me it looks better to use an extra struct when needed.
> 
> making every other file system use more memory just so that tmpfs can use less
> doesn't seem like a good trade-off to me.  the only reason that tmpfs wouldn't
> need it right now is because tmpfs currently can't be NFS-exported, is that 
> right?

No, tmpfs stores a back pointer to the vnode inside the tmpfs node and
therefore doesn't need a cache.

> if we fixed tmpfs to be NFS-exportable then it would need this?

No.

> 
> 
>>> to avoid needing fields for the "key" pointer and key length,
>>>  each fs could supply its own comparison function.
>> 
>> Going this route (more or less making the current ufs_ihash general)
>> makes it impossible to serialise node creation.
> 
> I don't follow, how does this make serialising node creation impossible?
> (I assume that by "serialise node creation" you mean ensure preventing the
> creation of multiple vnodes for the same file.)

If the key is part of the file system node we cannot prevent different
threads to load the same inode before the inode is at least partially
initialised.

--
J. Hannken-Illjes - hannken%eis.cs.tu-bs.de@localhost - TU Braunschweig 
(Germany)