Page MenuHomeFreeBSD
Differential D25612 Diff 75654 sys/fs/devfs/devfs_vnops.c

Changeset View

Standalone View

View Options

sys/fs/devfs/devfs_vnops.c

Show First 20 Lines • Show All 216 Lines • ▼ Show 20 Linesdevfs_clear_cdevpriv(void)
struct file *fp; struct file *fp;
fp = curthread->td_fpop; fp = curthread->td_fpop;
if (fp == NULL) if (fp == NULL)
return; return;
devfs_fpdrop(fp); devfs_fpdrop(fp);
} }
static void
devfs_usecount_add(struct vnode *vp)
{
struct devfs_dirent *de;
struct cdev *dev;
mtx_lock(&devfs_de_interlock);
VI_LOCK(vp);
kibUnsubmitted
Not Done
Inline Actions

Why do you need VI_LOCK() in all places covered by devfs_de_interlock ?
May be, declare that device usecount is protected by the de_interlock mtx ?

kib: Why do you need VI_LOCK() in all places covered by devfs_de_interlock ? May be, declare that…
mjgAuthorUnsubmitted
Done
Inline Actions

I need stable ->v_data which right now is either dev lock or vnode interlock.

mjg: I need stable ->v_data which right now is either dev lock or vnode interlock.
kibUnsubmitted
Not Done
Inline Actions

v_data is under both vnode lock and de_interlock, and _not_ protected by the vnode interlock. Look at devfs_reclaim.

kib: v_data is under both vnode lock and de_interlock, and _not_ protected by the vnode interlock.
if (VN_IS_DOOMED(vp)) {
goto out_unlock;
}
VNPASS(vp->v_type == VCHR, vp);
kibUnsubmitted
Not Done
Inline Actions

For dereferencing v_data for devfs vnode, you either need to have the vnode lock, or devfs_de_interlock. Same is true for de->de_vnode.

If you own the vnode lock always there, you should assert it, otherwise interlock help is needed.

kib: For dereferencing v_data for devfs vnode, you either need to have the vnode lock, or…
de = vp->v_data;
dev = vp->v_rdev;
MPASS(de != NULL);
MPASS(dev != NULL);
dev->si_usecount++;
de->de_usecount++;
out_unlock:
VI_UNLOCK(vp);
mtx_unlock(&devfs_de_interlock);
}
static void
devfs_usecount_subl(struct vnode *vp)
{
struct devfs_dirent *de;
struct cdev *dev;
mtx_assert(&devfs_de_interlock, MA_OWNED);
ASSERT_VI_LOCKED(vp, __func__);
VNPASS(vp->v_type == VCHR || vp->v_type == VBAD, vp);
de = vp->v_data;
dev = vp->v_rdev;
if (de == NULL)
return;
if (dev == NULL) {
MPASS(de->de_usecount == 0);
return;
}
if (dev->si_usecount < de->de_usecount)
panic("%s: si_usecount underflow for dev %p "
"(has %ld, dirent has %d)\n",
__func__, dev, dev->si_usecount, de->de_usecount);
if (VN_IS_DOOMED(vp)) {
dev->si_usecount -= de->de_usecount;
de->de_usecount = 0;
} else {
if (de->de_usecount == 0)
panic("%s: de_usecount underflow for dev %p\n",
__func__, dev);
dev->si_usecount--;
de->de_usecount--;
}
}
static void
devfs_usecount_sub(struct vnode *vp)
{
mtx_lock(&devfs_de_interlock);
VI_LOCK(vp);
devfs_usecount_subl(vp);
VI_UNLOCK(vp);
mtx_unlock(&devfs_de_interlock);
}
int
devfs_usecountl(struct vnode *vp)
kibUnsubmitted
Not Done
Inline Actions

static ?

kib: static ?
mjgAuthorUnsubmitted
Done
Inline Actions

will make it static

mjg: will make it static
{
VNPASS(vp->v_type == VCHR, vp);
mtx_assert(&devfs_de_interlock, MA_OWNED);
ASSERT_VI_LOCKED(vp, __func__);
return (vp->v_rdev->si_usecount);
}
int
devfs_usecount(struct vnode *vp)
{
int count;
VNPASS(vp->v_type == VCHR, vp);
mtx_lock(&devfs_de_interlock);
VI_LOCK(vp);
count = devfs_usecountl(vp);
VI_UNLOCK(vp);
mtx_unlock(&devfs_de_interlock);
return (count);
}
void
devfs_ctty_ref(struct vnode *vp)
kibUnsubmitted
Not Done
Inline Actions

Can this become static ?

kib: Can this become static ?
mjgAuthorUnsubmitted
Done
Inline Actions

unref is called from places outside of the file, I think it would be weird to have a mismatch here

mjg: unref is called from places outside of the file, I think it would be weird to have a mismatch…
{
vrefact(vp);
devfs_usecount_add(vp);
}
void
devfs_ctty_unref(struct vnode *vp)
{
devfs_usecount_sub(vp);
vrele(vp);
}
/* /*
* On success devfs_populate_vp() returns with dmp->dm_lock held. * On success devfs_populate_vp() returns with dmp->dm_lock held.
*/ */
static int static int
devfs_populate_vp(struct vnode *vp) devfs_populate_vp(struct vnode *vp)
{ {
struct devfs_dirent *de; struct devfs_dirent *de;
struct devfs_mount *dmp; struct devfs_mount *dmp;
▲ Show 20 Lines • Show All 249 Lines • ▼ Show 20 Linesloop:
if (de->de_dirent->d_type == DT_CHR) { if (de->de_dirent->d_type == DT_CHR) {
vp->v_type = VCHR; vp->v_type = VCHR;
VI_LOCK(vp); VI_LOCK(vp);
dev_lock(); dev_lock();
dev_refl(dev); dev_refl(dev);
/* XXX: v_rdev should be protect by vnode lock */ /* XXX: v_rdev should be protect by vnode lock */
vp->v_rdev = dev; vp->v_rdev = dev;
VNPASS(vp->v_usecount == 1, vp); VNPASS(vp->v_usecount == 1, vp);
dev->si_usecount++;
/* Special casing of ttys for deadfs. Probably redundant. */ /* Special casing of ttys for deadfs. Probably redundant. */
dsw = dev->si_devsw; dsw = dev->si_devsw;
if (dsw != NULL && (dsw->d_flags & D_TTY) != 0) if (dsw != NULL && (dsw->d_flags & D_TTY) != 0)
vp->v_vflag |= VV_ISTTY; vp->v_vflag |= VV_ISTTY;
dev_unlock(); dev_unlock();
VI_UNLOCK(vp); VI_UNLOCK(vp);
if ((dev->si_flags & SI_ETERNAL) != 0) if ((dev->si_flags & SI_ETERNAL) != 0)
vp->v_vflag |= VV_ETERNALDEV; vp->v_vflag |= VV_ETERNALDEV;
▲ Show 20 Lines • Show All 65 Lines • ▼ Show 20 Lines
static int static int
devfs_close(struct vop_close_args *ap) devfs_close(struct vop_close_args *ap)
{ {
struct vnode *vp = ap->a_vp, *oldvp; struct vnode *vp = ap->a_vp, *oldvp;
struct thread *td = ap->a_td; struct thread *td = ap->a_td;
struct proc *p; struct proc *p;
struct cdev *dev = vp->v_rdev; struct cdev *dev = vp->v_rdev;
struct cdevsw *dsw; struct cdevsw *dsw;
struct devfs_dirent *de = vp->v_data;
int dflags, error, ref, vp_locked; int dflags, error, ref, vp_locked;
/* /*
* XXX: Don't call d_close() if we were called because of * XXX: Don't call d_close() if we were called because of
* XXX: insmntque1() failure. * XXX: insmntque1() failure.
*/ */
if (vp->v_data == NULL) if (vp->v_data == NULL)
return (0); return (0);
/* /*
* Hack: a tty device that is a controlling terminal * Hack: a tty device that is a controlling terminal
* has a reference from the session structure. * has a reference from the session structure.
* We cannot easily tell that a character device is * We cannot easily tell that a character device is
* a controlling terminal, unless it is the closing * a controlling terminal, unless it is the closing
* process' controlling terminal. In that case, * process' controlling terminal. In that case,
* if the reference count is 2 (this last descriptor * if the reference count is 2 (this last descriptor
* plus the session), release the reference from the session. * plus the session), release the reference from the session.
*/ */
if (vp->v_usecount == 2 && td != NULL) { if (de->de_usecount == 2 && td != NULL) {
p = td->td_proc; p = td->td_proc;
PROC_LOCK(p); PROC_LOCK(p);
if (vp == p->p_session->s_ttyvp) { if (vp == p->p_session->s_ttyvp) {
PROC_UNLOCK(p); PROC_UNLOCK(p);
oldvp = NULL; oldvp = NULL;
sx_xlock(&proctree_lock); sx_xlock(&proctree_lock);
if (vp == p->p_session->s_ttyvp) { if (vp == p->p_session->s_ttyvp) {
SESS_LOCK(p->p_session); SESS_LOCK(p->p_session);
mtx_lock(&devfs_de_interlock);
VI_LOCK(vp); VI_LOCK(vp);
if (vp->v_usecount == 2 && vcount(vp) == 1 && if (de->de_usecount == 2 && devfs_usecountl(vp) == 2 &&
!VN_IS_DOOMED(vp)) { !VN_IS_DOOMED(vp)) {
p->p_session->s_ttyvp = NULL; p->p_session->s_ttyvp = NULL;
p->p_session->s_ttydp = NULL; p->p_session->s_ttydp = NULL;
oldvp = vp; oldvp = vp;
} }
VI_UNLOCK(vp); VI_UNLOCK(vp);
mtx_unlock(&devfs_de_interlock);
SESS_UNLOCK(p->p_session); SESS_UNLOCK(p->p_session);
} }
sx_xunlock(&proctree_lock); sx_xunlock(&proctree_lock);
if (oldvp != NULL) if (oldvp != NULL)
vrele(oldvp); devfs_ctty_unref(oldvp);
} else } else
PROC_UNLOCK(p); PROC_UNLOCK(p);
} }
/* /*
* We do not want to really close the device if it * We do not want to really close the device if it
* is still in use unless we are trying to close it * is still in use unless we are trying to close it
* forcibly. Since every use (buffer, vnode, swap, cmap) * forcibly. Since every use (buffer, vnode, swap, cmap)
* holds a reference to the vnode, and because we mark * holds a reference to the vnode, and because we mark
* any other vnodes that alias this device, when the * any other vnodes that alias this device, when the
* sum of the reference counts on all the aliased * sum of the reference counts on all the aliased
* vnodes descends to one, we are on last close. * vnodes descends to one, we are on last close.
*/ */
dsw = dev_refthread(dev, &ref); dsw = dev_refthread(dev, &ref);
if (dsw == NULL) if (dsw == NULL)
return (ENXIO); return (ENXIO);
dflags = 0; dflags = 0;
mtx_lock(&devfs_de_interlock);
VI_LOCK(vp); VI_LOCK(vp);
if (vp->v_usecount == 1 && vcount(vp) == 1) if (devfs_usecountl(vp) == 1)
kibUnsubmitted
Not Done
Inline Actions

If devfs_usecountl() == 1, doesn't it imply that de_usecount is also 1 ? Otherwise, how did we get close on this de/vp ?

Or do you think there is a race ? If not, I think it would be an interesting experiment to commit this with assert and see what happens in wild.

kib: If devfs_usecountl() == 1, doesn't it imply that de_usecount is also 1 ? Otherwise, how did we…
mjgAuthorUnsubmitted
Done
Inline Actions

There can be no race here as both counters are always updated together with devfs_de_interlock held. The separate check is a leftover from previous code which had "updated the global count on 0<->1 transitions of local count" policy. I'll remove it. Said policy is beneficial to implement as it would save on global lock acquire in the common case, but runs into lock ordering issues with vnode interlock. Iow it's just some mess which can be changed later.

mjg: There can be no race here as both counters are always updated together with devfs_de_interlock…
dflags |= FLASTCLOSE; dflags |= FLASTCLOSE;
kibUnsubmitted
Not Done
Inline Actions

Why VN_IS_DOOMED() test appeared there ? Wouldn't it cause useless (or panicing, not sure) calls to devfs_usecount_dec() ?

kib: Why VN_IS_DOOMED() test appeared there ? Wouldn't it cause useless (or panicing, not sure)…
devfs_usecount_subl(vp);
mtx_unlock(&devfs_de_interlock);
kibUnsubmitted
Not Done
Inline Actions

The check for v_usecount == 1 was used to try to avoid issues with parallel opens and close sometimes mismatched or loose FLASTCLOSE calls. You patch makes the race wider, by postponing usecount_inc() after d_open() method returns (d_open()/d_close() often sleep).

bde tried to handle the race in a way that would be acceptable for drivers, mostly serial hardware, but it was too complex by requiring inspecting each tty driver. I hesitate to change this aspect of devfs without having some plan how to solve that race.

kib: The check for v_usecount == 1 was used to try to avoid issues with parallel opens and close…
if (VN_IS_DOOMED(vp)) { if (VN_IS_DOOMED(vp)) {
/* Forced close. */ /* Forced close. */
dflags |= FREVOKE | FNONBLOCK; dflags |= FREVOKE | FNONBLOCK;
} else if (dsw->d_flags & D_TRACKCLOSE) { } else if (dsw->d_flags & D_TRACKCLOSE) {
/* Keep device updated on status. */ /* Keep device updated on status. */
} else if ((dflags & FLASTCLOSE) == 0) { } else if ((dflags & FLASTCLOSE) == 0) {
VI_UNLOCK(vp); VI_UNLOCK(vp);
dev_relthread(dev, ref); dev_relthread(dev, ref);
▲ Show 20 Lines • Show All 206 Lines • ▼ Show 20 Lines if (error == 0 && com == TIOCSCTTY) {
*/ */
sx_slock(&proctree_lock); sx_slock(&proctree_lock);
sess = td->td_proc->p_session; sess = td->td_proc->p_session;
if (sess->s_ttyvp == vp || sess->s_ttyp == NULL) { if (sess->s_ttyvp == vp || sess->s_ttyp == NULL) {
sx_sunlock(&proctree_lock); sx_sunlock(&proctree_lock);
return (0); return (0);
} }
vrefact(vp); devfs_ctty_ref(vp);
SESS_LOCK(sess); SESS_LOCK(sess);
vpold = sess->s_ttyvp; vpold = sess->s_ttyvp;
sess->s_ttyvp = vp; sess->s_ttyvp = vp;
sess->s_ttydp = cdev2priv(dev); sess->s_ttydp = cdev2priv(dev);
SESS_UNLOCK(sess); SESS_UNLOCK(sess);
sx_sunlock(&proctree_lock); sx_sunlock(&proctree_lock);
▲ Show 20 Lines • Show All 292 Lines • ▼ Show 20 Linesdevfs_open(struct vop_open_args *ap)
dsw = dev_refthread(dev, &ref); dsw = dev_refthread(dev, &ref);
if (dsw == NULL) if (dsw == NULL)
return (ENXIO); return (ENXIO);
if (fp == NULL && dsw->d_fdopen != NULL) { if (fp == NULL && dsw->d_fdopen != NULL) {
dev_relthread(dev, ref); dev_relthread(dev, ref);
return (ENXIO); return (ENXIO);
} }
if (vp->v_type == VCHR)
devfs_usecount_add(vp);
vlocked = VOP_ISLOCKED(vp); vlocked = VOP_ISLOCKED(vp);
VOP_UNLOCK(vp); VOP_UNLOCK(vp);
fpop = td->td_fpop; fpop = td->td_fpop;
td->td_fpop = fp; td->td_fpop = fp;
if (fp != NULL) { if (fp != NULL) {
fp->f_data = dev; fp->f_data = dev;
fp->f_vnode = vp; fp->f_vnode = vp;
} }
if (dsw->d_fdopen != NULL) if (dsw->d_fdopen != NULL)
error = dsw->d_fdopen(dev, ap->a_mode, td, fp); error = dsw->d_fdopen(dev, ap->a_mode, td, fp);
else else
error = dsw->d_open(dev, ap->a_mode, S_IFCHR, td); error = dsw->d_open(dev, ap->a_mode, S_IFCHR, td);
/* Clean up any cdevpriv upon error. */ /* Clean up any cdevpriv upon error. */
if (error != 0) if (error != 0)
devfs_clear_cdevpriv(); devfs_clear_cdevpriv();
td->td_fpop = fpop; td->td_fpop = fpop;
vn_lock(vp, vlocked | LK_RETRY); vn_lock(vp, vlocked | LK_RETRY);
if (error != 0 && vp->v_type == VCHR)
devfs_usecount_sub(vp);
dev_relthread(dev, ref); dev_relthread(dev, ref);
if (error != 0) { if (error != 0) {
if (error == ERESTART) if (error == ERESTART)
error = EINTR; error = EINTR;
return (error); return (error);
} }
#if 0 /* /dev/console */ #if 0 /* /dev/console */
▲ Show 20 Lines • Show All 212 Lines • ▼ Show 20 Lines
devfs_readlink(struct vop_readlink_args *ap) devfs_readlink(struct vop_readlink_args *ap)
{ {
struct devfs_dirent *de; struct devfs_dirent *de;
de = ap->a_vp->v_data; de = ap->a_vp->v_data;
return (uiomove(de->de_symlink, strlen(de->de_symlink), ap->a_uio)); return (uiomove(de->de_symlink, strlen(de->de_symlink), ap->a_uio));
} }
static int static void
devfs_reclaim(struct vop_reclaim_args *ap) devfs_reclaiml(struct vnode *vp)
{ {
struct vnode *vp;
struct devfs_dirent *de; struct devfs_dirent *de;
vp = ap->a_vp; mtx_assert(&devfs_de_interlock, MA_OWNED);
kibUnsubmitted
Not Done
Inline Actions

Why is this call needed ? Wouldn't you mechanism only needed for VCHR vnodes ?

BTW, I think that you should assert the vnode type in devfs_usecount_*().

kib: Why is this call needed ? Wouldn't you mechanism only needed for VCHR vnodes ? BTW, I think…
mtx_lock(&devfs_de_interlock);
de = vp->v_data; de = vp->v_data;
if (de != NULL) { if (de != NULL) {
MPASS(de->de_usecount == 0);
de->de_vnode = NULL; de->de_vnode = NULL;
vp->v_data = NULL; vp->v_data = NULL;
} }
}
static int
devfs_reclaim(struct vop_reclaim_args *ap)
{
struct vnode *vp;
vp = ap->a_vp;
mtx_lock(&devfs_de_interlock);
devfs_reclaiml(vp);
mtx_unlock(&devfs_de_interlock); mtx_unlock(&devfs_de_interlock);
return (0); return (0);
} }
static int static int
devfs_reclaim_vchr(struct vop_reclaim_args *ap) devfs_reclaim_vchr(struct vop_reclaim_args *ap)
{ {
struct vnode *vp; struct vnode *vp;
struct cdev *dev; struct cdev *dev;
vp = ap->a_vp; vp = ap->a_vp;
MPASS(vp->v_type == VCHR); MPASS(vp->v_type == VCHR);
devfs_reclaim(ap); mtx_lock(&devfs_de_interlock);
VI_LOCK(vp); VI_LOCK(vp);
devfs_usecount_subl(vp);
devfs_reclaiml(vp);
mtx_unlock(&devfs_de_interlock);
dev_lock(); dev_lock();
dev = vp->v_rdev; dev = vp->v_rdev;
vp->v_rdev = NULL; vp->v_rdev = NULL;
if (dev != NULL)
dev->si_usecount -= (vp->v_usecount > 0);
dev_unlock(); dev_unlock();
VI_UNLOCK(vp); VI_UNLOCK(vp);
if (dev != NULL) if (dev != NULL)
dev_rel(dev); dev_rel(dev);
return (0); return (0);
} }
static int static int
▲ Show 20 Lines • Show All 545 Lines • Show Last 20 Lines