[ofa-general] Re: [PATCH 6/11] core: XRC - file descriptors associated with XRC domains

[Jack Morgenstein]

On Tuesday 24 June 2008 00:08, Roland Dreier wrote:
>  > +static struct inode * xrc_fd2inode(unsigned int fd)
>  > +{
>  > +	struct file * f = fget(fd);
>  > +
>  > +	if (!f)
>  > +		return NULL;
>  > +
>  > +	return f->f_dentry->d_inode;
>  > +}
> 
> Am I missing something, or is there no fput() that matches this fget()?

You're right. I'm adding this after I grab the inode and in the error path
in procedure ib_uverbs_open_xrc_domain().

> Is there some reason why we don't need an igrab() of the inode we're
> using here?  (If we do need it then a corresponding iput() when we're
> done with the inode is required of course)

We do need igrab/iput.  The algorithm here is to use the fd to get to the inode,
and then grab the inode (to guarantee that it is not deleted out from under).
Once I have grabbed the inode, I don't need the fd anymore (so that I can fput()).

I am using igrab/iput in the patch (easy to miss):
+static int insert_xrcd(struct ib_device *dev, struct inode *i_n,
+                      struct ib_xrcd *xrcd)
+{
+       int ret;
+
+       ret = xrcd_table_insert(dev, i_n, xrcd);
+       if (!ret)
+               igrab(i_n);
+
+       return ret;
+}

...
+static void xrcd_table_delete(struct ib_device *dev,
+                             struct inode *i_n)
+{
+       struct xrcd_table_entry *entry = xrcd_table_search(dev, i_n);
+
+       if (entry) {
+               iput(i_n);
+               rb_erase(&entry->node, &dev->ib_uverbs_xrcd_table);
+               kfree(entry);
+       }
+}

At second look, its still a bit messy.  I can get rid of insert_xrcd entirely, and just move the igrab to xrcd_table_insert.
(as is done for xrcd_table_delete/iput).

- Jack