Re: Patch for inode SLIST conversion

[Michael Neumann <mneumann@xxxxxxxx>]

Simon 'corecode' Schubert wrote:
> Michael Neumann wrote:
> > Matthias Schmidt wrote:
> > > Hi,
> > >
> > > * Michael Neumann wrote:
> > > > Hi,
> > > >
> > > > Appended my first patch. And it wasn't that hard at all. The 
> > > > Projects page of the DF wiki mentioned to convert the inode i_next 
> > > > field to use a SLIST. This is what the patch does. I'm not sure if 
> > > > it is that usable :
> > >
> > > Just for the record: This issue is also covered by
> > > http://bugs.dragonflybsd.org/issue694
> >
> > Thanks for pointing this out. Next time I'll take a look at the issue 
> > tracker first.
>
> I think we should commit one of them.  The submission by Michael seems 
> more thorough, so I'd go for that one.

Please wait because I'm doing some further cleanup on it.
But before I have a question according to serialization tokens...

In ufs_ihash.c there are only two functions that modify
the inode hash table, ufs_ihashins and ufs_ihashrem given below:

int
ufs_ihashins(struct inode *ip)
{
        struct inode *iq;
        struct inode *prev;
        struct ihashtbl_bucket *head;
        lwkt_tokref ilock;

        KKASSERT((ip->i_flag & IN_HASHED) == 0);
        lwkt_gettoken(&ilock, &ufs_ihash_token);

        head = INOHASH(ip->i_dev, ip->i_number);
        SLIST_FOREACH_WITH_PREV(iq, prev, head, i_next) {
                if (INOEQL(ip, iq->i_dev, iq->i_number)) {
                        lwkt_reltoken(&ilock);
                        return(EBUSY);
                }
        }
        if (prev) SLIST_NEXT(prev, i_next) = ip;
        else      SLIST_FIRST(head) = ip;
        SLIST_NEXT(ip, i_next) = NULL;
        ip->i_flag |= IN_HASHED;
        lwkt_reltoken(&ilock);
        return(0);
}

void
ufs_ihashrem(struct inode *ip)
{
        lwkt_tokref ilock;

        lwkt_gettoken(&ilock, &ufs_ihash_token);
        if (ip->i_flag & IN_HASHED) {
                SLIST_REMOVE(INOHASH(ip->i_dev, ip->i_number), ip,
                    inode, i_next);
                SLIST_NEXT(ip, i_next) = NULL;
                ip->i_flag &= ~IN_HASHED;
        }
        lwkt_reltoken(&ilock);
}

If I understand serialization tokens correctly, they lead in case of the 
two functions above to mutual exclusive execution, because none of them 
voluntarily blocks or calls a blocking method. Is this assumption correct?

Then there is ufs_ihashget:

struct vnode *
ufs_ihashget(cdev_t dev, ino_t inum)
{
        lwkt_tokref ilock;
        struct inode *ip;
        struct vnode *vp;

        lwkt_gettoken(&ilock, &ufs_ihash_token);

loop:
        INOFIND(ip, dev, inum);
        vp = NULL;
        if (ip)
        {
                vp = ITOV(ip);
                if (vget(vp, LK_EXCLUSIVE))
                        goto loop;
                /*
                 * We must check to see if the inode has been ripped
                 * out from under us after blocking.
                 */
                INOFIND(ip, dev, inum);
                if (ip == NULL || ITOV(ip) != vp) {
       			vput(vp);
                        goto loop;
                }
        }

        lwkt_reltoken(&ilock);
        return (vp);
}


See the comment on "ripped out from under us". This can only happen 
because vget might block, right?

Now, ufs_ihashget doesn't modify the hashtable at all. So why not store 
a "transaction id" in each hash table bucket, which gets increased by 
ufs_ihashins and ufs_ihashrem. Then in ufs_ihashget I read in this id 
*before* calling vget and check afterwards if it has changed. If it is 
unchanged, I can omit a second pass over the linked list.

It's actually quite easy to implement and if all my assumptions apply, 
the correct way of doing it. Please correct me if I'm wrong.


Regards,

  Michael