Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 5 May 2008 23:40:14 +0300
From:      Kostik Belousov <kostikbel@gmail.com>
To:        John Baldwin <jhb@freebsd.org>
Cc:        freebsd-arch@freebsd.org
Subject:   Re: Per-open file private data for the cdevs
Message-ID:  <20080505204013.GW18958@deviant.kiev.zoral.com.ua>
In-Reply-To: <200805051156.14706.jhb@freebsd.org>
References:  <20080504171002.GN18958@deviant.kiev.zoral.com.ua> <200805050939.42425.jhb@freebsd.org> <20080505142051.GS18958@deviant.kiev.zoral.com.ua> <200805051156.14706.jhb@freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help

--0kX0f002JLNe82mQ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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=20
> ago
> > > > mostly says that it would be better to implement per-file private d=
ata
> > > > instead, I produced the patch along this line,
> > > >
> > > > The patch does not change the cdevsw ABI, instead, three new functi=
ons
> > > > 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=20
> 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 th=
at
> > > > 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.
> > >=20
> > > I like this very much and will use it instead of devfs cloning in ipm=
i(4)=20
> so=20
> > > we can use ipmievd when it is committed.  IWBN if there was a more=20
> automated=20
> > > way of handling the unload race, for example if destroy_dev() could=
=20
> somehow=20
> > > clear all the per-open fd data.  That may not be easily feasible, how=
ever. =20
> > > (In theory each cdev could have a list of "attached" 'struct file' ob=
jects=20
> > > that it updates in VOP_CLOSE() and for a destroy dev it detaches all =
the=20
> > > private data after marking the cdev with a bad/null cdevsw, however, =
that=20
> > > would bloat 'struct file' with at least one more pointer (if not two)=
.)
> >=20
> > 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.
> >=20
> > 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().
>=20
> The disconnect as I see it is that right now destroy_dev() "orphans" devi=
ces=20
> in that the files still exist but the backing cdev's now have dead=20
> operations.  However, now you can have a partially orphaned cdev in that =
not=20
> all of the cdev state is torn down in destroy_dev().  In that sense it wo=
uld=20
> be nice to have the behavior described above, and yes you would have to h=
ave=20
> the cdev's keep track of 'file's as I suggested.
>=20
> 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.


--0kX0f002JLNe82mQ
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (FreeBSD)

iEYEARECAAYFAkgfcK0ACgkQC3+MBN1Mb4gq7QCdFdTDH9mMfrCXzttLcfu8ukwM
mSMAoJwp4hyu2Fw0yxtfiR7RufO8Rzl9
=SD3v
-----END PGP SIGNATURE-----

--0kX0f002JLNe82mQ--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080505204013.GW18958>