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

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

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.

-- 
Kind regards,

Yorick Hardy