Re: fdexpand() & open file tracking
On 02/07/18(Mon) 11:22, Martin Pieuchot wrote:
> Let's use a array of structures to keep track of open files. This will
> allow us to simplify locking when expending it.
>
> While here consistently use M_ZERO when (re)allocating M_FILEDESC
> structures. I doubt the memset() solution makes any difference these
> days and the resulting code is simpler to understand.
>
> Finally passes a 'struct filedesc' to fdexpand() to be coherent with
> the other functions.
Updated diff now that Visa committed his nice refcounting code using
atomics.
Since flags are now part of the `fd_ofiles' array we can use the new
spinning `fd_fplock' to serialize access to them instead of fdplock().
Comments, oks?
Index: kern/kern_descrip.c
===
RCS file: /cvs/src/sys/kern/kern_descrip.c,v
retrieving revision 1.175
diff -u -p -r1.175 kern_descrip.c
--- kern/kern_descrip.c 2 Jul 2018 14:36:33 - 1.175
+++ kern/kern_descrip.c 2 Jul 2018 15:36:46 -
@@ -235,7 +235,7 @@ fd_getfile(struct filedesc *fdp, int fd)
return (NULL);
mtx_enter(>fd_fplock);
- fp = fdp->fd_ofiles[fd];
+ fp = fdp->fd_ofiles[fd].fn_fp;
if (fp != NULL)
atomic_inc_int(>f_count);
mtx_leave(>fd_fplock);
@@ -288,7 +288,7 @@ restart:
if ((error = fdalloc(p, 0, )) != 0) {
FRELE(fp, p);
if (error == ENOSPC) {
- fdexpand(p);
+ fdexpand(fdp);
fdpunlock(fdp);
goto restart;
}
@@ -363,7 +363,7 @@ restart:
if ((error = fdalloc(p, new, )) != 0) {
FRELE(fp, p);
if (error == ENOSPC) {
- fdexpand(p);
+ fdexpand(fdp);
fdpunlock(fdp);
goto restart;
}
@@ -375,8 +375,11 @@ restart:
}
/* No need for FRELE(), finishdup() uses current ref. */
error = finishdup(p, fp, old, new, retval, 1);
- if (!error && flags & O_CLOEXEC)
- fdp->fd_ofileflags[new] |= UF_EXCLOSE;
+ if (!error && flags & O_CLOEXEC) {
+ mtx_enter(>fd_fplock);
+ fdp->fd_ofiles[new].fn_flags |= UF_EXCLOSE;
+ mtx_leave(>fd_fplock);
+ }
out:
fdpunlock(fdp);
@@ -423,7 +426,7 @@ restart:
if ((error = fdalloc(p, newmin, )) != 0) {
FRELE(fp, p);
if (error == ENOSPC) {
- fdexpand(p);
+ fdexpand(fdp);
fdpunlock(fdp);
goto restart;
}
@@ -431,26 +434,29 @@ restart:
/* No need for FRELE(), finishdup() uses current ref. */
error = finishdup(p, fp, fd, i, retval, 0);
- if (!error && SCARG(uap, cmd) == F_DUPFD_CLOEXEC)
- fdp->fd_ofileflags[i] |= UF_EXCLOSE;
+ if (!error && SCARG(uap, cmd) == F_DUPFD_CLOEXEC) {
+ mtx_enter(>fd_fplock);
+ fdp->fd_ofiles[i].fn_flags |= UF_EXCLOSE;
+ mtx_leave(>fd_fplock);
+ }
}
fdpunlock(fdp);
return (error);
case F_GETFD:
- fdplock(fdp);
- *retval = fdp->fd_ofileflags[fd] & UF_EXCLOSE ? 1 : 0;
- fdpunlock(fdp);
+ mtx_enter(>fd_fplock);
+ *retval = fdp->fd_ofiles[fd].fn_flags & UF_EXCLOSE ? 1 : 0;
+ mtx_leave(>fd_fplock);
break;
case F_SETFD:
- fdplock(fdp);
+ mtx_enter(>fd_fplock);
if ((long)SCARG(uap, arg) & 1)
- fdp->fd_ofileflags[fd] |= UF_EXCLOSE;
+ fdp->fd_ofiles[fd].fn_flags |= UF_EXCLOSE;
else
- fdp->fd_ofileflags[fd] &= ~UF_EXCLOSE;
- fdpunlock(fdp);
+ fdp->fd_ofiles[fd].fn_flags &= ~UF_EXCLOSE;
+ mtx_leave(>fd_fplock);
break;
case F_GETFL:
@@ -679,10 +685,10 @@ finishdup(struct proc *p, struct file *f
* the function no longer creates a new reference to the old file.
*/
mtx_enter(>fd_fplock);
- fdp->fd_ofiles[new] = fp;
+ fdp->fd_ofiles[new].fn_fp = fp;
+ fdp->fd_ofiles[new].fn_flags =
+ fdp->fd_ofiles[old].fn_flags & ~UF_EXCLOSE;
mtx_leave(>fd_fplock);
-
- fdp->fd_ofileflags[new] = fdp->fd_ofileflags[old] & ~UF_EXCLOSE;
*retval = new;
if (oldfp != NULL) {
@@ -703,7 +709,8 @@ fdinsert(struct filedesc
fdexpand() & open file tracking
Let's use a array of structures to keep track of open files. This will
allow us to simplify locking when expending it.
While here consistently use M_ZERO when (re)allocating M_FILEDESC
structures. I doubt the memset() solution makes any difference these
days and the resulting code is simpler to understand.
Finally passes a 'struct filedesc' to fdexpand() to be coherent with
the other functions.
Comments? Oks?
Index: kern/kern_descrip.c
===
RCS file: /cvs/src/sys/kern/kern_descrip.c,v
retrieving revision 1.173
diff -u -p -r1.173 kern_descrip.c
--- kern/kern_descrip.c 1 Jul 2018 16:33:15 - 1.173
+++ kern/kern_descrip.c 2 Jul 2018 09:12:30 -
@@ -229,7 +229,7 @@ fd_getfile(struct filedesc *fdp, int fd)
return (NULL);
mtx_enter();
- fp = fdp->fd_ofiles[fd];
+ fp = fdp->fd_files[fd].fn_fp;
if (fp != NULL)
fp->f_count++;
mtx_leave();
@@ -282,7 +282,7 @@ restart:
if ((error = fdalloc(p, 0, )) != 0) {
FRELE(fp, p);
if (error == ENOSPC) {
- fdexpand(p);
+ fdexpand(fdp);
fdpunlock(fdp);
goto restart;
}
@@ -357,7 +357,7 @@ restart:
if ((error = fdalloc(p, new, )) != 0) {
FRELE(fp, p);
if (error == ENOSPC) {
- fdexpand(p);
+ fdexpand(fdp);
fdpunlock(fdp);
goto restart;
}
@@ -370,7 +370,7 @@ restart:
/* No need for FRELE(), finishdup() uses current ref. */
error = finishdup(p, fp, old, new, retval, 1);
if (!error && flags & O_CLOEXEC)
- fdp->fd_ofileflags[new] |= UF_EXCLOSE;
+ fdp->fd_files[new].fn_flags |= UF_EXCLOSE;
out:
fdpunlock(fdp);
@@ -417,7 +417,7 @@ restart:
if ((error = fdalloc(p, newmin, )) != 0) {
FRELE(fp, p);
if (error == ENOSPC) {
- fdexpand(p);
+ fdexpand(fdp);
fdpunlock(fdp);
goto restart;
}
@@ -426,7 +426,7 @@ restart:
error = finishdup(p, fp, fd, i, retval, 0);
if (!error && SCARG(uap, cmd) == F_DUPFD_CLOEXEC)
- fdp->fd_ofileflags[i] |= UF_EXCLOSE;
+ fdp->fd_files[i].fn_flags |= UF_EXCLOSE;
}
fdpunlock(fdp);
@@ -434,16 +434,16 @@ restart:
case F_GETFD:
fdplock(fdp);
- *retval = fdp->fd_ofileflags[fd] & UF_EXCLOSE ? 1 : 0;
+ *retval = fdp->fd_files[fd].fn_flags & UF_EXCLOSE ? 1 : 0;
fdpunlock(fdp);
break;
case F_SETFD:
fdplock(fdp);
if ((long)SCARG(uap, arg) & 1)
- fdp->fd_ofileflags[fd] |= UF_EXCLOSE;
+ fdp->fd_files[fd].fn_flags |= UF_EXCLOSE;
else
- fdp->fd_ofileflags[fd] &= ~UF_EXCLOSE;
+ fdp->fd_files[fd].fn_flags &= ~UF_EXCLOSE;
fdpunlock(fdp);
break;
@@ -668,8 +668,8 @@ finishdup(struct proc *p, struct file *f
fd_used(fdp, new);
}
- fdp->fd_ofiles[new] = fp;
- fdp->fd_ofileflags[new] = fdp->fd_ofileflags[old] & ~UF_EXCLOSE;
+ fdp->fd_files[new].fn_fp = fp;
+ fdp->fd_files[new].fn_flags = fdp->fd_files[old].fn_flags & ~UF_EXCLOSE;
*retval = new;
if (oldfp != NULL) {
@@ -690,15 +690,15 @@ fdinsert(struct filedesc *fdp, int fd, i
mtx_enter();
if ((fp->f_iflags & FIF_INSERTED) == 0) {
fp->f_iflags |= FIF_INSERTED;
- if ((fq = fdp->fd_ofiles[0]) != NULL) {
+ if ((fq = fdp->fd_files[0].fn_fp) != NULL) {
LIST_INSERT_AFTER(fq, fp, f_list);
} else {
LIST_INSERT_HEAD(, fp, f_list);
}
}
- KASSERT(fdp->fd_ofiles[fd] == NULL);
- fdp->fd_ofiles[fd] = fp;
- fdp->fd_ofileflags[fd] |= (flags & UF_EXCLOSE);
+ KASSERT(fdp->fd_files[fd].fn_fp == NULL);
+ fdp->fd_files[fd].fn_fp = fp;
+ fdp->fd_files[fd].fn_flags |= (flags & UF_EXCLOSE);
mtx_leave();
}
@@ -706,8 +706,8 @@ void
fdremove(struct filedesc *fdp, int fd)
{
fdpassertlocked(fdp);
- fdp->fd_ofiles[fd] = NULL;
- fdp->fd_ofileflags[fd] = 0;
+ fdp->fd_files[fd].fn_fp = NULL;
+ fdp->fd_files[fd].fn_flags = 0;
fd_unused(fdp, fd);
}
@@ -872,9 +872,9 @@ restart:
