Subject: Re: fsync_range() system call
To: Klaus Klein <kleink@reziprozitaet.de>
From: Bill Studenmund <wrstuden@netbsd.org>
List: tech-kern
Date: 10/25/2003 10:26:15
--45Z9DzgjV8m4Oswq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sat, Oct 25, 2003 at 03:53:30AM +0200, Klaus Klein wrote:
> On Saturday 25 October 2003 00:23, Bill Studenmund wrote:
>=20
> > Index: include/unistd.h
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > RCS file: /cvsroot/wasabisrc/src/include/unistd.h,v
> > retrieving revision 1.1.1.4
> > diff -u -r1.1.1.4 unistd.h
> > --- include/unistd.h	12 May 2003 20:51:02 -0000	1.1.1.4
> > +++ include/unistd.h	17 Oct 2003 18:29:50 -0000
> > @@ -311,6 +314,7 @@
> >  void	 endusershell __P((void));
> >  int	 exect __P((const char *, char * const *, char * const *));
> >  int	 fchroot __P((int));
> > +int	 fsync_range(int, int, off_t, off_t);
> >  int	 getdomainname __P((char *, size_t));
> >  int	 getgrouplist __P((const char *, gid_t, gid_t *, int *));
> >  mode_t	 getmode __P((const void *, mode_t));
>=20
> You shoudln't mix K&R-escaped with ANSI within the same file.

Hmm.. Then I probably need a different file. Or a different place in this=
=20
file.

I deliberately didn't __P()  fsync_range as 1) it's my understanding it's
out of fashion, and 2) since off_t is bigger than an int, the K&R
just-name prototype will create an ABI difference between the caller and
callee.

The latter being why __P()  is going out of style, as I understand it. :-)

So which is more important? Style, or functionality?

> > Index: lib/libc/sys/fsync.2
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > RCS file: /cvsroot/wasabisrc/src/lib/libc/sys/fsync.2,v
> > retrieving revision 1.1.1.4
> > diff -u -r1.1.1.4 fsync.2
> > --- lib/libc/sys/fsync.2	17 Apr 2003 05:21:21 -0000	1.1.1.4
> > +++ lib/libc/sys/fsync.2	17 Oct 2003 18:29:54 -0000
> > @@ -38,6 +38,7 @@
> >  .Os
> >  .Sh NAME
> >  .Nm fsync
> > +.Nm fsync_range
> >  .Nd "synchronize a file's in-core state with that on disk"
> >  .Sh LIBRARY
> >  .Lb libc
>=20
> This should be ".Nm fsync ,".
>=20
>=20
> > Index: lib/libpthread/pthread_cancelstub.c
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > RCS file: /cvsroot/wasabisrc/src/lib/libpthread/pthread_cancelstub.c,v
> > retrieving revision 1.1.1.7
> > diff -u -r1.1.1.7 pthread_cancelstub.c
> > --- lib/libpthread/pthread_cancelstub.c	11 Mar 2003 02:46:28 -0000	1.1.=
1.7
> > +++ lib/libpthread/pthread_cancelstub.c	17 Oct 2003 18:29:56 -0000
> > @@ -69,6 +69,7 @@
> > @@ -157,6 +158,20 @@
> >  	return retval;
> >  }
> > =20
> > +int
> > +_fsync_range(int d, int f, off_t s, off_t e)
> > +{
> > +	int retval;
> > +	pthread_t self;
> > +
> > +	self =3D pthread__self();
> > +	pthread__testcancel(self);
> > +	retval =3D _sys_fsync(d, f, s, e);
> > +	pthread__testcancel(self);
> > +=09
> > +	return retval;
> > +}
> > +
> >  ssize_t
> >  msgrcv(int msgid, void *msgp, size_t msgsz, long msgtyp, int msgflg)
> >  {
>=20
> I think you want to call _sys_fsync_range(), here.

Yes, I do. :-)

> > Index: sys/kern/vfs_syscalls.c
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > RCS file: /cvsroot/wasabisrc/src/sys/kern/vfs_syscalls.c,v
> > retrieving revision 1.1.1.10
> > diff -u -r1.1.1.10 vfs_syscalls.c
> > --- sys/kern/vfs_syscalls.c	24 May 2003 16:16:42 -0000	1.1.1.10
> > +++ sys/kern/vfs_syscalls.c	17 Oct 2003 18:30:30 -0000
> > @@ -2867,6 +2867,76 @@
> [...]
>=20
> > +
> > +	if ((fp->f_flag & FWRITE) =3D=3D 0) {
> > +		// simple_unlock(&fp->f_slock);
>=20
> That looks like a leftover, given getvnode()'s implied FILE_USE().
> However, you've made me aware of a conformance deficiency in fdatasync(2)=
.=20
> :-)
>=20
> > +	flags =3D SCARG(uap, flags);
> > +	if ((flags & (FDATASYNC | FFILESYNC)) =3D=3D 0) {
> > +		/* Do we care? */
> > +	}
>=20
> We should (EINVAL); for a precedent, look at e.g. mlockall(2).
>=20
>=20
> As a general observation, I think the FSYNC_DATAONLY flag name/value shou=
ld=20
> be recycled for FSYNC_DATASYNC semantics; the current FSYNC_DATAONLY=20
> semantics are too aggressive for its (and fdatasync(2)'s) own good.

That's more of a change than I want to make right now. I'm not against it,=
=20
I just don't feel up to it now. :-)

Take care,

Bill

--45Z9DzgjV8m4Oswq
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.3 (NetBSD)

iD8DBQE/mrI3Wz+3JHUci9cRApPjAKCS1zjN3ed8665a4PzkEGSyujKHkgCfQwFI
0H0CQgDcvhs9ev7G3zaRn1g=
=phcL
-----END PGP SIGNATURE-----

--45Z9DzgjV8m4Oswq--