Re: Per-open file private data for the cdevs

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.

Attachment: pgpsPS7WNZykW.pgp
Description: PGP signature

Relevant Pages

  • Re: Per-open file private data for the cdevs
    ... On Mon, 12 May 2008, Kostik Belousov wrote: ... The patch does not change the cdevsw ABI, instead, three new functions ... are provided for manipulation of the per-file private data. ... which is used to initiate currently performed driver operation. ...
    (freebsd-arch)
  • Re: Per-open file private data for the cdevs
    ... The patch does not change the cdevsw ABI, instead, three new functions ... are provided for manipulation of the per-file private data. ... The synchronization of the cdev data and file private data is left ...
    (freebsd-arch)
  • Re: Per-open file private data for the cdevs
    ... The patch does not change the cdevsw ABI, instead, three new functions ... are provided for manipulation of the per-file private data. ... which is used to initiate currently performed driver operation. ... This versions adds only one pointer to the struct file. ...
    (freebsd-arch)
  • Re: Per-open file private data for the cdevs
    ... The patch does not change the cdevsw ABI, instead, three new functions ... are provided for manipulation of the per-file private data. ... which is used to initiate currently performed driver operation. ...
    (freebsd-arch)
  • Re: Per-open file private data for the cdevs
    ... The patch does not change the cdevsw ABI, instead, three new functions ... are provided for manipulation of the per-file private data. ... which is used to initiate currently performed driver operation. ...
    (freebsd-arch)