Per-open file private data for the cdevs

[Kostik Belousov]

On Tue, May 13, 2008 at 12:37:34PM +0300, Kostik Belousov wrote:
> On Mon, May 12, 2008 at 10:03:14AM -1000, Jeff Roberson wrote:
> > 
> > On Mon, 12 May 2008, Kostik Belousov wrote:
> > 
> > >On Sun, May 11, 2008 at 03:40:14PM -1000, Jeff Roberson wrote:
> > >>
> > >>On Sun, 11 May 2008, Kostik Belousov wrote:
> > >>
> > >>>On Sun, May 11, 2008 at 02:50:30PM +0300, Kostik Belousov wrote:
> > >>>>On Sat, May 10, 2008 at 09:53:12PM -1000, Jeff Roberson wrote:
> > >>>>>On Sun, 4 May 2008, 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.
> > >>>>>>
> > >>>>>
> > >>>>>Hi Kostik,
> > >>>>>
> > >>>>>Are these per-instances structures intended to be used by anything 
> > >>>>>other
> > >>>>>than devices?  If not can we make them a union with the DTYPE_VNODE
> > >>>>>fields to save space?
> > >>>>>
> > >>>>>Thanks,
> > >>>>>Jeff
> > >>>>
> > >>>>The current version of the patch is at
> > >>>>http://people.freebsd.org/~kib/misc/fdpriv.3.patch
> > >>>>
> > >>>>Per insistence of John Baldwin and request of Eric Anholt, the 
> > >>>>destructors
> > >>>>are called now when either file is last closed, or the device is
> > >>>>destroyed.
> > >>>>This versions adds only one pointer to the struct file.
> > >>>>
> > >>>>Jeff, would you, please, explicitely specify what field you propose to
> > >>>>union with the f_cdevpriv ?
> > >>
> > >>f_nextoff and f_seqcount are only used if vn_read() and vn_write() are
> > >>used.  They do not apply to any other descriptors.
> > >I use the f_cdevpriv != NULL as an indicator for the necessity to enter
> > >the cdevpriv code, in particular, locking the cdevpriv_mtx, that would
> > >otherwise needed to be entered at each last close. I think that one
> > >pointer for the struct file is not too big cost, do you agree ?
> > 
> > No, it's not a big cost, however if it is possible to avoid that is best.
> > 
> > Can you not check the type before checking f_cdevpriv?  Should we not only 
> > be checking cdevpriv in contexts where we know that it is not a vnode?
> 
> I am sorry, my english may be not enough, so I may interpret your
> proposal mistakenly. I read it as a suggestion to check the file type
> before accessing the f_cdevpriv.
> 
> The problem with the f_cdevpriv exists only at the _fdrop(). There, we
> have a file of f_type == DTYPE_VNODE both for devfs and normal files.
> I cannot check the f_vnode since the vnode may be reclaimed. The only
> differentiator is the f_ops, that is devfs_ops_f for devfs file, and
> vnops for the normal file during the file lifetime. Unfortunately,
> f_ops is reset to the badfileops by vn_closefile before the _fdrop() is
> getting called.
> 
> Reserving the flag in the f_flag looks not good due to interaction with
> the userspace.
> 
> I do not want the callback to be called before the d_close() driver method
> gets a chance to clean the data.

So, I cannot implement overlay of the cdevpriv over the seqaccess data 
without some additional flag. On the other hand, use of 2 bytes for the
f_type is overkill when the range of the types is [1,8]. I divided it
to two one-byte fields, and new field is used for filetype-specific
flags.

http://people.freebsd.org/~kib/misc/fdpriv.4.patch
shall give the same size of the struct file while not causing unconditional
acquire of the cdevpriv_mtx on the last file close.

Jeff, do you agree with the proposed vivisection of the f_type ? I will
ask Peter to retest the patch then.

-------------- 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/20080514/24120e03/attachment.pgp