Re: PATCH: Relax fdatasync checks to IEEE Std 1003.1-2008

[Yorick Hardy <yorickhardy%gmail.com@localhost>]

On 2020-05-24, Yorick Hardy wrote:
> Dear Greg and Paul,
> 
> On 2020-03-25, Paul Ripke wrote:
> > On Mon, Mar 16, 2020 at 08:47:27AM -0400, Greg Troxel wrote:
> > >   [lots of test reports about fdatasync patch]
> > > 
> > > Thanks -- that's enough for me to be comfortable.
> > > and it's been proposed for more than long enough, with no adverse
> > > comments, so I'll commit it soonish.
> > 
> > fwiw, I missed a comment at the top of the function... fixed in
> > attached patch.
> > 
> > -- 
> > Paul Ripke
> > "Great minds discuss ideas, average minds discuss events, small minds
> >  discuss people."
> > -- Disputed: Often attributed to Eleanor Roosevelt. 1948.
> 
> > diff --git a/lib/libc/sys/fdatasync.2 b/lib/libc/sys/fdatasync.2
> > index 3f12119f0dbb..20da609191f5 100644
> > --- a/lib/libc/sys/fdatasync.2
> > +++ b/lib/libc/sys/fdatasync.2
> > @@ -68,7 +68,7 @@ function will fail if:
> >  .It Bq Er EBADF
> >  The
> >  .Fa fd
> > -argument is not a valid file descriptor open for writing.
> > +argument is not a valid file descriptor.
> >  .It Bq Er EINVAL
> >  This implementation does not support synchronized I/O for this file.
> >  .It Bq Er ENOSYS
> > @@ -93,4 +93,4 @@ and outstanding I/O operations are not guaranteed to have been completed.
> >  The
> >  .Fn fdatasync
> >  function conforms to
> > -.St -p1003.1b-93 .
> > +.St -p1003.1-2008 .
> > diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
> > index d51beedbfca9..8cfe5abe6cf8 100644
> > --- a/sys/kern/vfs_syscalls.c
> > +++ b/sys/kern/vfs_syscalls.c
> > @@ -4059,8 +4059,7 @@ sys_fsync(struct lwp *l, const struct sys_fsync_args *uap, register_t *retval)
> >   * Sync a range of file data.  API modeled after that found in AIX.
> >   *
> >   * FDATASYNC indicates that we need only save enough metadata to be able
> > - * to re-read the written data.  Note we duplicate AIX's requirement that
> > - * the file be open for writing.
> > + * to re-read the written data.
> >   */
> >  /* ARGSUSED */
> >  int
> > @@ -4141,10 +4140,6 @@ sys_fdatasync(struct lwp *l, const struct sys_fdatasync_args *uap, register_t *r
> >  	/* fd_getvnode() will use the descriptor for us */
> >  	if ((error = fd_getvnode(SCARG(uap, fd), &fp)) != 0)
> >  		return (error);
> > -	if ((fp->f_flag & FWRITE) == 0) {
> > -		fd_putfile(SCARG(uap, fd));
> > -		return (EBADF);
> > -	}
> >  	vp = fp->f_vnode;
> >  	vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
> >  	error = VOP_FSYNC(vp, fp->f_cred, FSYNC_WAIT|FSYNC_DATAONLY, 0, 0);
> 
> On 2020-03-25, Greg Troxel wrote:
> > Paul Ripke <stix%stix.id.au@localhost> writes:
> > 
> > > On Mon, Mar 16, 2020 at 08:47:27AM -0400, Greg Troxel wrote:
> > >>   [lots of test reports about fdatasync patch]
> > >> 
> > >> Thanks -- that's enough for me to be comfortable.
> > >> and it's been proposed for more than long enough, with no adverse
> > >> comments, so I'll commit it soonish.
> > 
> > > fwiw, I missed a comment at the top of the function... fixed in
> > > attached patch.
> > 
> > I have committed your patch, exactly as you just sent it.  My full
> > release build worked and I have an anita test run in process, just in
> > case.
> > 
> > Thanks for perservering on this.  It takes many people to fix all the
> > loose ends in an operating system!
> 
> I have been trying to find the cause of PR kern/55237. I am not at all
> familiar with the code, so please forgive me for pointing fingers!
> 
> I think the call to fd_putfile results in a close of the fd, but that
> does not happen anymore? Should it?
> 
> Apologies again if this has nothing to do with kern/55237.

It was not the casue of kern/55237, apologies!

-- 
Kind regards,

Yorick Hardy