Received-SPF: pass (google.com: domain of linux-nfs+bounces-1195-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 18 Jan 2024 11:36:01 +1100
From: David Disseldorp <ddiss@suse.de>
To: Jeff Layton <jlayton@kernel.org>
Cc: fstests@vger.kernel.org, linux-nfs@vger.kernel.org
Subject: Re: [PATCH fstests] common/rc: NFSv2/3 do not support negative
 timestamps
Message-ID: <20240118113601.760290cb@echidna>
In-Reply-To: <20240116173127.238994-1-jlayton@kernel.org>
References: <20240116173127.238994-1-jlayton@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Tue, 16 Jan 2024 12:31:27 -0500, Jeff Layton wrote:

> The NFSv2 and v3 protocols use unsigned values for timestamps. Fix
> _require_negative_timestamps() to check the NFS version and _notrun if
> it's 2 or 3.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  common/rc | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index a9e0ba7e22f1..d4ac0744fab0 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2902,6 +2902,27 @@ _require_debugfs()
>      [ -d "$DEBUGFS_MNT/boot_params" ] || _notrun "Debugfs not mounted"
>  }
>  
> +#
> +# Return the version of NFS in use on the mount on $1. Returns 0
> +# if it's not NFS.
> +#
> +_nfs_version()
> +{
> +	local mountpoint=$1
> +	local nfsvers=""
> +
> +	case "$FSTYP" in
> +	nfs*)
> +		nfsvers=`_mount | grep $1 | sed -n 's/^.*vers=\([0-9.]*\).*$/\1/p'`
> +		;;
> +	*)
> +		nfsvers="0"
> +		;;
> +	esac
> +
> +	echo "$nfsvers"
> +}
> +
>  # The default behavior of SEEK_HOLE is to always return EOF.
>  # Filesystems that implement non-default behavior return the offset
>  # of holes with SEEK_HOLE. There is no way to query the filesystem
> @@ -2925,7 +2946,7 @@ _fstyp_has_non_default_seek_data_hole()
>  	nfs*)
>  		# NFSv2, NFSv3, and NFSv4.0/4.1 only support default behavior of SEEK_HOLE,
>  		# while NFSv4.2 supports non-default behavior
> -		local nfsvers=`_mount() | grep $TEST_DEV | sed -n 's/^.*vers=\([0-9.]*\).*$/\1/p'`
> +		local nfsvers=$( _nfs_version "$TEST_DIR" )
>  		[ "$nfsvers" = "4.2" ]
>  		return $?
>  		;;
> @@ -5129,6 +5150,16 @@ _require_negative_timestamps() {
>  	ceph|exfat)
>  		_notrun "$FSTYP does not support negative timestamps"
>  		;;
> +	nfs*)
> +		#
> +		# NFSv2/3 timestamps use 32-bit unsigned values, and so
> +		# cannot represent values prior to the epoch
> +		#
> +		local nfsvers=$( _nfs_version "$TEST_DIR" )
> +		if [ "$nfsvers" = "2" -o "$nfsvers" = "3" ]; then
> +			_notrun "$FSTYP version $nfsvers does not support negative timestamps"
> +		fi
> +		;;
>  	esac
>  }

Nit: It looks like there's quite some overlap with the existing
_is_nfs_version() / _fs_options() helpers, but it's still an improvement
on what's currently there...
Reviewed-by: David Disseldorp <ddiss@suse.de>