Per-open file private data for the cdevs

[Kostik Belousov]

On Mon, May 05, 2008 at 11:56:14AM -0400, John Baldwin wrote:
> On Monday 05 May 2008 10:20:51 am Kostik Belousov wrote:
> > On Mon, May 05, 2008 at 09:39:42AM -0400, John Baldwin wrote:
> > > On Sunday 04 May 2008 01:10:02 pm Kostik Belousov wrote:
> > > > Since the review for the clone-at-open patch (fdclone) posted some time 
> ago
> > > > mostly says that it would be better to implement per-file private data
> > > > instead, I produced the patch along this line,
> > > >
> > > > The patch does not change the cdevsw ABI, instead, three new functions
> > > > nt	devfs_get_cdevpriv(void **datap);
> > > > int	devfs_set_cdevpriv(void *priv, cdevpriv_dtr_t dtr);
> > > > void	devfs_clear_cdevpriv(void);
> > > > are provided for manipulation of the per-file private data.
> > > >
> > > > devfs_set_cdevpriv assigns the priv as private data for the file 
> descriptor
> > > > which is used to initiate currently performed driver operation. dtr
> > > > is the function that will be called when either the last refernce to
> > > > the file goes away or devfs_clear_cdevpriv is called.
> > > >
> > > > devfs_get_cdevpriv is the obvious accessor.
> > > >
> > > > devfs_clear_cdevpriv allows to clear the private data for the still
> > > > open file.
> > > >
> > > > The synchronization of the cdev data and file private data is left
> > > > to the driver code, I did not found any generic helper mechanism that
> > > > could be useful there.
> > > >
> > > > Patch:
> > > > http://people.freebsd.org/~kib/misc/fdpriv.1.patch
> > > >
> > > > Dumb driver that shows the basic usage of the proposed KPI:
> > > > http://people.freebsd.org/~kib/misc/fpclone.c
> > > >
> > > > Previous version of the patch was tested by Peter Holm.
> > > 
> > > I like this very much and will use it instead of devfs cloning in ipmi(4) 
> so 
> > > we can use ipmievd when it is committed.  IWBN if there was a more 
> automated 
> > > way of handling the unload race, for example if destroy_dev() could 
> somehow 
> > > clear all the per-open fd data.  That may not be easily feasible, however.  
> > > (In theory each cdev could have a list of "attached" 'struct file' objects 
> > > that it updates in VOP_CLOSE() and for a destroy dev it detaches all the 
> > > private data after marking the cdev with a bad/null cdevsw, however, that 
> > > would bloat 'struct file' with at least one more pointer (if not two).)
> > 
> > Probably, I shall clarify that the dtr is called when the last reference
> > to the struct file going away, unless the priv data is already cleared
> > by the call to the devfs_clear_cdevpriv.
> > 
> > The problem with VOP_CLOSE() is that some actions (like forced unmount
> > or revoke) may cause less calls to the devfs_close then the files
> > pointing to the cdev. Your proposal basically means that we need,
> > besides the normal pointers file->vnode->cdev, have the reverse path
> > cdev->file. I think this is ugly and better be handled by the fdrop().
> 
> The disconnect as I see it is that right now destroy_dev() "orphans" devices 
> in that the files still exist but the backing cdev's now have dead 
> operations.  However, now you can have a partially orphaned cdev in that not 
> all of the cdev state is torn down in destroy_dev().  In that sense it would 
> be nice to have the behavior described above, and yes you would have to have 
> the cdev's keep track of 'file's as I suggested.
> 
> Could you handle the revoke(2) and forced umount cases either via
> VOP_REVOKE() or by adding a VOP_INACTIVE() to devfs?

I do not see how VOP_INACTIVE would be helpful there. Use of it still
means the vnode->file walker. And, I quite dislike the backreferences
like cdev->files or vnodes->files.

My previous attempt of the cloning at open() resolved the lifecycle issues
by making the per-fd data a cdev with the usual cdev management.

I thought about either incrementing si_threadcount, or adding another
refcounter to the cdev to track presence of the priv data. But
destroy_dev() then have to block, and driver author still required to
keep track the list of the allocated priv references to be able to
actively unblock, like the d_purge. I then decided to leave this to the
driver.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-arch/attachments/20080505/15679d4d/attachment.pgp