Per-open file private data for the cdevs

[John Baldwin]

On Wednesday 14 May 2008 05:32:23 am Kostik Belousov wrote:
> 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.

I would rather add the new pointer to struct file and avoid breaking the ABI 
of fstat.  That is, I would like this patch to be MFCable, but you can't MFC 
this w/o breaking the ABI of struct file since fstat(1) will be reading the 
full short to get the DTYPE.

-- 
John Baldwin