Re: falloc and related stuff
Struct file again.
f_flag isn’t modified often, so it’s modifacation can be atomic.
f_msgcount and f_rxfer, f_wxfer, f_seek, f_rbytes, f_wbytes can be protected by
rwlock.
f_offset protection is actual for vnodes only.
FIF_MARK and FIF_DEFER flags are used only by unpc garbage collector. This
flags can
be moved out from f_iflags, for example to f_unpc_flags, and use their own
protection.
FIF_HASLOCK checked only in vn_closefile(), but this flag doesn’t indicate
actual vnode lock
state, because VOP_ADVLOCK()’s return value is not checked. May be it can be
replaced by
new vn_islocked() function, which will check actual vnode lock possibility and
lock state?
only FIF_LARVAL remains in f_iflags, this flag sets only once, so it’s
modification can be atomic.
f_count may be modified and checked under rwlock, but I think atomic ops are
better on smp,
afaik, with uniprocessor kernel simple increment/decrement over volatile
variable will be enough.
This modification doesn’t break pstat, all related FIF_* flags can be set in
kinfo_file struct.
f_offset protection can be done like in patch below. it is just
proof-of-concept. I think f_offset
protection stuff can be moved to external struct, which will be stored in hash
with fp address as key.
FIF_MARK and FIF_DEFER stuff can be moved to external struct too, I suppose.
Index: compat/common/compat_dir.c
===
RCS file: /cvs/src/sys/compat/common/compat_dir.c,v
retrieving revision 1.11
diff -u -p -r1.11 compat_dir.c
--- compat/common/compat_dir.c 16 Dec 2014 21:25:28 - 1.11
+++ compat/common/compat_dir.c 9 Apr 2015 10:40:55 -
@@ -51,7 +51,6 @@ readdir_with_callback(struct file *fp, o
struct iovec aiov;
int eofflag = 0;
int error, len, reclen;
- off_t newoff = *off;
struct vnode *vp;
struct vattr va;
@@ -84,10 +83,16 @@ again:
auio.uio_segflg = UIO_SYSSPACE;
auio.uio_procp = curproc;
auio.uio_resid = buflen;
- auio.uio_offset = newoff;
-
+ if (fp-f_offset == off)
+ foffset_lock(fp, auio.uio_offset);
+ else
+ auio.uio_offset = *off;
error = VOP_READDIR(vp, auio, fp-f_cred, eofflag);
- *off = auio.uio_offset;
+ if (fp-f_offset == off)
+ foffset_unlock(fp, auio.uio_offset);
+ else
+ *off = auio.uio_offset;
+
if (error)
goto out;
Index: kern/kern_descrip.c
===
RCS file: /cvs/src/sys/kern/kern_descrip.c,v
retrieving revision 1.116
diff -u -p -r1.116 kern_descrip.c
--- kern/kern_descrip.c 19 Jan 2015 01:19:17 - 1.116
+++ kern/kern_descrip.c 9 Apr 2015 10:41:10 -
@@ -61,6 +61,7 @@
#include sys/event.h
#include sys/pool.h
#include sys/ktrace.h
+#include sys/rwlock.h
#include sys/pipe.h
@@ -451,9 +452,9 @@ restart:
if (fl.l_start == 0 fl.l_len 0) {
/* lockf(3) compliance hack */
fl.l_len = -fl.l_len;
- fl.l_start = fp-f_offset - fl.l_len;
+ fl.l_start = foffset_get(fp) - fl.l_len;
} else
- fl.l_start += fp-f_offset;
+ fl.l_start += foffset_get(fp);
}
switch (fl.l_type) {
@@ -514,9 +515,9 @@ restart:
if (fl.l_start == 0 fl.l_len 0) {
/* lockf(3) compliance hack */
fl.l_len = -fl.l_len;
- fl.l_start = fp-f_offset - fl.l_len;
+ fl.l_start = foffset_get(fp) - fl.l_len;
} else
- fl.l_start += fp-f_offset;
+ fl.l_start += foffset_get(fp);
}
if (fl.l_type != F_RDLCK
fl.l_type != F_WRLCK
@@ -869,6 +870,7 @@ restart:
*/
nfiles++;
fp = pool_get(file_pool, PR_WAITOK|PR_ZERO);
+ rw_init(fp-f_offset_lck, f_offset_lck);
fp-f_iflags = FIF_LARVAL;
if ((fq = p-p_fd-fd_ofiles[0]) != NULL) {
LIST_INSERT_AFTER(fq, fp, f_list);
@@ -1125,6 +1127,47 @@ fdrop(struct file *fp, struct proc *p)
pool_put(file_pool, fp);
return (error);
+}
+
+off_t
+foffset_get(struct file *fp)
+{
+ off_t offset;
+
+ rw_enter_read(fp-f_offset_lck);
+ offset = fp-f_offset;
+ rw_exit_read(fp-f_offset_lck);
+
+ return offset;
+}
+
+void
+foffset_lock(struct file *fp, off_t *foffset)
+{
+ KASSERT(foffset != NULL);
+ rw_enter_write(fp-f_offset_lck);
+ while (fp-f_offset_lckf FOFFSET_LOCKED) {
+ fp-f_offset_lckf |= FOFFSET_LOCK_WAITING;
+
Re: falloc and related stuff
On 08 Apr 2015, at 02:31, Philip Guenther guent...@gmail.com wrote:
On Tue, Apr 7, 2015 at 3:57 PM, Kanonenvogel kanonenvogel@gmail.com
wrote:
I have idea to modify falloc() function and related logic.
Now, after successful faclloc call, we have half-initialized struct file
object, protected by FIF_LARVAL flag.
I want to initialise struct file object within falloc() and then put it to
fd_ofiles array and filehead list. This modification
permits to avoid half-initialization state and remove FIF_LARVAL flag and
related logic.
The hard case is blocking opens of fifos, as the underlying operation
involves a change visible to another process and that therefore cannot
be rolled back.
Philip Guenther
Ok, I understood.
And what about make struct file more smp friendly? For example, make operations
under
f_iflags and f_flags atomic?
Index: kern/kern_descrip.c
===
RCS file: /cvs/src/sys/kern/kern_descrip.c,v
retrieving revision 1.116
diff -u -p -r1.116 kern_descrip.c
--- kern/kern_descrip.c 19 Jan 2015 01:19:17 - 1.116
+++ kern/kern_descrip.c 8 Apr 2015 09:21:19 -
@@ -61,6 +61,7 @@
#include sys/event.h
#include sys/pool.h
#include sys/ktrace.h
+#include sys/atomic.h
#include sys/pipe.h
@@ -332,6 +333,7 @@ sys_fcntl(struct proc *p, void *v, regis
int i, tmp, newmin, flg = F_POSIX;
struct flock fl;
int error = 0;
+ int oflag, nflag;
restart:
if ((fp = fd_getfile(fdp, fd)) == NULL)
@@ -384,8 +386,12 @@ restart:
break;
case F_SETFL:
- fp-f_flag = ~FCNTLFLAGS;
- fp-f_flag |= FFLAGS((long)SCARG(uap, arg)) FCNTLFLAGS;
+ do {
+ oflag = fp-f_flag;
+ nflag = oflag ~FCNTLFLAGS;
+ nflag |= FFLAGS((long)SCARG(uap, arg)) FCNTLFLAGS;
+ } while (atomic_cas_uint(fp-f_flag, oflag, nflag) != oflag);
+
tmp = fp-f_flag FNONBLOCK;
error = (*fp-f_ops-fo_ioctl)(fp, FIONBIO, (caddr_t)tmp, p);
if (error)
@@ -394,7 +400,7 @@ restart:
error = (*fp-f_ops-fo_ioctl)(fp, FIOASYNC, (caddr_t)tmp, p);
if (!error)
break;
- fp-f_flag = ~FNONBLOCK;
+ atomic_clearbits_int(fp-f_flag, FNONBLOCK);
tmp = 0;
(void) (*fp-f_ops-fo_ioctl)(fp, FIONBIO, (caddr_t)tmp, p);
break;
@@ -1160,7 +1166,7 @@ sys_flock(struct proc *p, void *v, regis
lf.l_len = 0;
if (how LOCK_UN) {
lf.l_type = F_UNLCK;
- fp-f_iflags = ~FIF_HASLOCK;
+ atomic_clearbits_int(fp-f_iflags, FIF_HASLOCK);
error = VOP_ADVLOCK(vp, (caddr_t)fp, F_UNLCK, lf, F_FLOCK);
goto out;
}
@@ -1172,7 +1178,7 @@ sys_flock(struct proc *p, void *v, regis
error = EINVAL;
goto out;
}
- fp-f_iflags |= FIF_HASLOCK;
+ atomic_setbits_int(fp-f_iflags, FIF_HASLOCK);
if (how LOCK_NB)
error = VOP_ADVLOCK(vp, (caddr_t)fp, F_SETLK, lf, F_FLOCK);
else
Index: kern/sys_generic.c
===
RCS file: /cvs/src/sys/kern/sys_generic.c,v
retrieving revision 1.96
diff -u -p -r1.96 sys_generic.c
--- kern/sys_generic.c 12 Feb 2015 22:27:04 - 1.96
+++ kern/sys_generic.c 8 Apr 2015 09:21:19 -
@@ -52,6 +52,7 @@
#include sys/stat.h
#include sys/malloc.h
#include sys/poll.h
+#include sys/atomic.h
#ifdef KTRACE
#include sys/ktrace.h
#endif
@@ -456,17 +457,17 @@ sys_ioctl(struct proc *p, void *v, regis
case FIONBIO:
if ((tmp = *(int *)data) != 0)
- fp-f_flag |= FNONBLOCK;
+ atomic_setbits_int(fp-f_flag, FNONBLOCK);
else
- fp-f_flag = ~FNONBLOCK;
+ atomic_clearbits_int(fp-f_flag, FNONBLOCK);
error = (*fp-f_ops-fo_ioctl)(fp, FIONBIO, (caddr_t)tmp, p);
break;
case FIOASYNC:
if ((tmp = *(int *)data) != 0)
- fp-f_flag |= FASYNC;
+ atomic_setbits_int(fp-f_flag, FASYNC);
else
- fp-f_flag = ~FASYNC;
+ atomic_clearbits_int(fp-f_flag, FASYNC);
error = (*fp-f_ops-fo_ioctl)(fp, FIOASYNC, (caddr_t)tmp, p);
break;
Index: kern/uipc_usrreq.c
===
RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v
retrieving revision 1.79
diff -u -p -r1.79 uipc_usrreq.c
--- kern/uipc_usrreq.c 11 Dec 2014 19:21:57 - 1.79
+++ kern/uipc_usrreq.c 8 Apr 2015 09:21:19 -
@@ -884,11 +884,11 @@ unp_gc(void)
unp_gcing = 1;
unp_defer = 0;
Re: falloc and related stuff
kanonenvogel@gmail.com wrote: On 08 Apr 2015, at 02:31, Philip Guenther guent...@gmail.com wrote: On Tue, Apr 7, 2015 at 3:57 PM, Kanonenvogel kanonenvogel@gmail.com wrote: I have idea to modify falloc() function and related logic. Now, after successful faclloc call, we have half-initialized struct file object, protected by FIF_LARVAL flag. I want to initialise struct file object within falloc() and then put it to fd_ofiles array and filehead list. This modification permits to avoid half-initialization state and remove FIF_LARVAL flag and related logic. The hard case is blocking opens of fifos, as the underlying operation involves a change visible to another process and that therefore cannot be rolled back. Philip Guenther Ok, I understood. And what about make struct file more smp friendly? For example, make operations under f_iflags and f_flags atomic? This can help, but not always. The atomic functions are quite expensive on some architectures, so we don't want to just use them everywhere. Also, this only helps if you're sure that the code reading the flag will do so in an smp safe way. In many cases, the reading code will also need to acquire a lock in order to correctly do something after reading the flag. From the diff context, it looks like most of this code will definitely already have some other lock.
Re: falloc and related stuff
On 08 Apr 2015, at 17:33, kanonenvogel@gmail.com wrote: Is it a good idea? bad idea because of sys_pread
Re: falloc and related stuff
On 08 Apr 2015, at 17:33, kanonenvogel@gmail.com wrote: Is it a good idea? bad idea because of sys_pread/sys_pwrite
Re: falloc and related stuff
On 08 Apr 2015, at 15:03, Ted Unangst t...@tedunangst.com wrote: The atomic functions are quite expensive on some architectures, so we don't want to just use them everywhere. So, rwlock is better here? Also, can you explain this lines from finishdup() function (openbsd-5., file kern/kern_descrip.c, lines 576-577): fp-f_count++; FRELE(fp, p); it looks useless, or I misunderstood something?
Re: falloc and related stuff
On 08 Apr 2015, at 15:03, Ted Unangst t...@tedunangst.com wrote:
Also, this only helps if you're sure that the code reading the flag will do so
in an smp safe way. In many cases, the reading code will also need to acquire
a lock in order to correctly do something after reading the flag. From the
diff context, it looks like most of this code will definitely already have
some other lock.
What do you think about f_offset protection? Lock file struct object within
of_read or fo_write routine?
For example for vn_read()
int
vn_read(struct file *fp, off_t *poff, struct uio *uio, struct ucred *cred)
{
struct vnode *vp = (struct vnode *)fp-f_data;
int error = 0;
size_t count = uio-uio_resid;
struct proc *p = uio-uio_procp;
FILE_LOCK(fp);
/* no wrap around of offsets except on character devices */
if (vp-v_type != VCHR count LLONG_MAX - *poff) {
FILE_UNLOCK(fp);
return (EINVAL);
}
vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, p);
uio-uio_offset = *poff;
if (vp-v_type != VDIR)
error = VOP_READ(vp, uio,
(fp-f_flag FNONBLOCK) ? IO_NDELAY : 0, cred);
*poff += count - uio-uio_resid;
VOP_UNLOCK(vp, 0, p);
FILE_UNLOCK(fp);
return (error);
}
Is it a good idea?
Re: falloc and related stuff
On Tue, Apr 7, 2015 at 3:57 PM, Kanonenvogel kanonenvogel@gmail.com wrote: I have idea to modify falloc() function and related logic. Now, after successful faclloc call, we have half-initialized struct file object, protected by FIF_LARVAL flag. I want to initialise struct file object within falloc() and then put it to fd_ofiles array and filehead list. This modification permits to avoid half-initialization state and remove FIF_LARVAL flag and related logic. The hard case is blocking opens of fifos, as the underlying operation involves a change visible to another process and that therefore cannot be rolled back. Philip Guenther
falloc and related stuff
Hello. I have idea to modify falloc() function and related logic. Now, after successful faclloc call, we have half-initialized struct file object, protected by FIF_LARVAL flag. I want to initialise struct file object within falloc() and then put it to fd_ofiles array and filehead list. This modification permits to avoid half-initialization state and remove FIF_LARVAL flag and related logic. After, I want to protect struct file by rwlock, add something like SY_NOLOCK flag and logic and begin to move related syscalls from KERNEL_LOCK. What do you think about?
