Re: [PATCH v3] _require_sparse_files: rewrite as a direct test instead of a black list

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Dec 19, 2023 at 04:57:20AM +0800, Alexander Patrakov wrote:
> _require_sparse_files was implemented as a list of filesystems known not to
> support sparse files, and therefore it missed some cases.
> 
> However, if sparse files do not work as expected during a test, the risk
> is that the test will write out to the disk all the zeros that would
> normally be unwritten. This amounts to at least 4 TB for the generic/129
> test, and therefore there is a significant media wear-out concern here.
> 
> Adding more filesystems to the list of exclusions would not scale and
> would not work anyway because CIFS backed by SAMBA is safe, while CIFS
> backed by Windows Server 2022 is not (because the specific write
> patterns found in generic/014 and generic/129 cause it to ignore the
> otherwise-supported request to make a file sparse).
> 
> Mitigate this risk by rewriting the check as a small-scale test that
> reliably triggers Windows misbehavior. The black list becomes unneeded
> because the same test creates and detects non-sparse files on exfat and
> hfsplus.
> 
> Signed-off-by: Alexander Patrakov <patrakov@gmail.com>

Looks good, thanks for replacing the FSTYP test with a functional test.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  common/rc | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index cc92fe06..a9e0ba7e 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2870,17 +2870,30 @@ _require_fs_space()
>  #
>  # Check if the filesystem supports sparse files.
>  #
> -# Unfortunately there is no better way to do this than a manual black list.
> +# Filesystems (such as CIFS mounted from a Windows server) that generally
> +# support sparse files but are tricked into creating a non-sparse file by one
> +# of the tests are treated here as not supporting sparse files. This special
> +# treatment is done due to media wear-out concerns -- e.g., generic/129 would
> +# write multiple terabytes of zeros if allowed to run on a filesystem that
> +# ignores the request to make a file sparse.
>  #
>  _require_sparse_files()
>  {
> -    case $FSTYP in
> -    hfsplus|exfat)
> -        _notrun "Sparse files not supported by this filesystem type: $FSTYP"
> -	;;
> -    *)
> -        ;;
> -    esac
> +    local testfile="$TEST_DIR/$$.sparsefiletest"
> +    rm -f "$testfile"
> +
> +    # The write size and offset are specifically chosen to trick the Windows
> +    # SMB server implementation into dishonoring the request to create a sparse
> +    # file, while still fitting into the 64 kb SMB1 maximum request size.
> +    # This also creates a non-sparse file on vfat, exfat, and hfsplus.
> +    $XFS_IO_PROG -f -c 'pwrite -b 51200 -S 0x61 1638400 51200' "$testfile" >/dev/null
> +
> +    resulting_file_size_kb=$( du -sk "$testfile" | cut -f 1 )
> +    rm -f "$testfile"
> +
> +    # The threshold of 1 MB allows for filesystems with such large clusters.
> +    [ $resulting_file_size_kb -gt 1024 ] && \
> +	_notrun "Sparse files are not supported or do not work as expected"
>  }
>  
>  _require_debugfs()
> -- 
> 2.43.0
> 
>