[PATCH v3 1/2] common/casefold: Add infrastructure to test filename casefold feature

[PATCH v3 1/2] common/casefold: Add infrastructure to test filename casefold feature

[Gabriel Krisman Bertazi]

Add a set of basic helper functions to simplify the testing of
casefolding capable filesystems.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 common/casefold | 92 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)
 create mode 100644 common/casefold

diff --git a/common/casefold b/common/casefold
new file mode 100644
index 000000000000..df01029a3d44
--- /dev/null
+++ b/common/casefold
@@ -0,0 +1,92 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2019 Collabora, Ltd.  All Rights Reserved.
+#
+# Common functions for testing filename casefold feature
+
+_has_casefold_kernel_support()
+{
+	case $FSTYP in
+		ext4)
+			test -f '/sys/fs/ext4/features/casefold'
+			;;
+		*)
+			# defaults to unsupported
+			false
+			;;
+	esac
+}
+
+_require_scratch_casefold()
+{
+	if ! _has_casefold_kernel_support ; then
+		_notrun "$FSTYP does not support casefold feature"
+	fi
+
+	if ! _scratch_mkfs_casefold &>>seqres.full; then
+		_notrun "$FSTYP userspace tools do not support casefold"
+	fi
+
+	# Make sure the kernel can mount a filesystem with the encoding
+	# defined by the userspace tools.  This will fail if
+	# the userspace tool used a more recent encoding than the one
+	# supported in kernel space.
+	if ! _try_scratch_mount &>>seqres.full; then
+		_notrun \
+		    "kernel can't mount filesystem with the encoding set by userspace"
+	fi
+	_scratch_unmount
+
+	# utilities used by casefold
+	_require_command "$CHATTR_PROG" chattr
+	_require_command "$LSATTR_PROG" lsattr
+}
+
+_scratch_mkfs_casefold()
+{
+	case $FSTYP in
+		ext4)
+			_scratch_mkfs -O casefold $*
+			;;
+		*)
+			_notrun "Don't know how to mkfs with casefold support on $FSTYP"
+			;;
+	esac
+}
+
+_scratch_mkfs_casefold_strict()
+{
+	case $FSTYP in
+		ext4)
+			_scratch_mkfs -O casefold -E encoding_flags=strict
+			;;
+		*)
+			_notrun \
+			    "Don't know how to mkfs with casefold-strict support on $FSTYP"
+			;;
+	esac
+}
+
+# To get the exact disk name, we need some method that does a
+# getdents() on the parent directory, such that we don't get
+# normalized/casefolded results.  'Find' works ok.
+_casefold_check_exact_name()
+{
+	local basedir=$1
+	local exact_name=$2
+	find ${basedir} | grep -q ${exact_name}
+}
+
+_casefold_set_attr()
+{
+	$CHATTR_PROG +F "${1}"
+}
+
+_casefold_unset_attr()
+{
+	$CHATTR_PROG -F "${1}"
+}
+
+_casefold_lsattr_dir()
+{
+	$LSATTR_PROG -ld "${1}" | _filter_spaces
+}
-- 
2.20.1

[PATCH v3 2/2] shared/012: Add tests for filename casefolding feature

[Gabriel Krisman Bertazi]

From: "Lakshmipathi.G" <lakshmipathi.ganapathi@collabora.co.uk>

This new test implements verification for the per-directory
case-insensitive feature, as supported by the reference implementation
in Ext4.

Signed-off-by: Lakshmipathi.G <lakshmipathi.ganapathi@collabora.co.uk>
Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
  [Rewrite to support feature design]
  [Refactor to simplify implementation]
---
 tests/shared/012     | 508 +++++++++++++++++++++++++++++++++++++++++++
 tests/shared/012.out |  16 ++
 tests/shared/group   |   1 +
 3 files changed, 525 insertions(+)
 create mode 100755 tests/shared/012
 create mode 100644 tests/shared/012.out

diff --git a/tests/shared/012 b/tests/shared/012
new file mode 100755
index 000000000000..d7e9cb43f524
--- /dev/null
+++ b/tests/shared/012
@@ -0,0 +1,508 @@
+# SPDX-License-Identifier: GPL-2.0+
+#!/bin/bash
+# FSQA Test No. 012
+#
+# Test the basic functionality of filesystems with case-insensitive
+# support.
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+status=1 # failure is thea default
+
+. ./common/rc
+. ./common/filter
+. ./common/casefold
+. ./common/attr
+
+_supported_os Linux
+_require_scratch_nocheck
+_require_scratch_casefold
+_require_check_dmesg
+_require_attrs
+
+sdev=$(_short_dev ${SCRATCH_DEV})
+
+filename1="file.txt"
+filename2="FILE.TXT"
+
+pt_file1=$(echo -e "coração")
+pt_file2=$(echo -e "corac\xcc\xa7\xc3\xa3o" | tr a-z A-Z)
+
+fr_file2=$(echo -e "french_caf\xc3\xa9.txt")
+fr_file1=$(echo -e "french_cafe\xcc\x81.txt")
+
+ar_file1=$(echo -e "arabic_\xdb\x92\xd9\x94.txt")
+ar_file2=$(echo -e "arabic_\xdb\x93.txt" | tr a-z A-Z)
+
+jp_file1=$(echo -e "japanese_\xe3\x82\xb2.txt")
+jp_file2=$(echo -e "japanese_\xe3\x82\xb1\xe3\x82\x99.txt")
+
+# '\xc3\x00' is an invalid sequence. Despite that, the sequences
+# below could match, if we ignored the error.  But we don't want
+# to be greedy at normalization, so at the first error we treat
+# the entire sequence as an opaque blob.  Therefore, these two
+# must NOT match.
+blob_file1=$(echo -e "corac\xcc\xa7\xc3")
+blob_file2=$(echo -e "coraç\xc3")
+
+# Test helpers
+basic_create_lookup()
+{
+	local basedir=${1}
+	local exact=${2}
+	local lookup=${3}
+
+	touch "${basedir}/${exact}"
+	[ -f "${basedir}/${lookup}" ] || \
+		echo "lookup of ${exact} using ${lookup} failed"
+	_casefold_check_exact_name "${basedir}" "${exact}" || \
+		echo "Created file ${exact} with wrong name."
+}
+
+# CI search should fail.
+bad_basic_create_lookup()
+{
+	local basedir=${1}
+	local exact=${2}
+	local lookup=${3}
+
+	touch "${basedir}/${exact}"
+	[ -f "${basedir}/${lookup}" ] && \
+		echo "Lookup of ${exact} using ${lookup} should fail"
+}
+
+# Testcases
+test_casefold_lookup()
+{
+	local basedir=${SCRATCH_MNT}/casefold_lookup
+
+	mkdir -p ${basedir}
+	_casefold_set_attr ${basedir}
+
+	basic_create_lookup "${basedir}" "${filename1}" "${filename2}"
+	basic_create_lookup "${basedir}" "${pt_file1}" "${pt_file2}"
+	basic_create_lookup "${basedir}" "${fr_file1}" "${fr_file2}"
+	basic_create_lookup "${basedir}" "${ar_file1}" "${ar_file2}"
+	basic_create_lookup "${basedir}" "${jp_file1}" "${jp_file2}"
+}
+
+test_bad_casefold_lookup()
+{
+	local basedir=${SCRATCH_MNT}/casefold_lookup
+
+	mkdir -p ${basedir}
+
+	bad_basic_create_lookup ${basedir} ${blob_file1} ${blob_file2}
+}
+
+do_create_and_remove()
+{
+	local basedir=${1}
+	local exact=${2}
+	local casefold=${3}
+
+	basic_create_lookup ${basedir} ${exact} ${casefold}
+	rm -f ${basedir}/${exact}
+	[ -f ${basedir}/${exact} ] && \
+		echo "File ${exact} was not removed using exact name"
+
+	basic_create_lookup ${basedir} ${exact} ${casefold}
+	rm -f ${basedir}/${casefold}
+	[ -f ${basedir}/${exact} ] && \
+		echo "File ${exact} was not removed using inexact name"
+}
+
+# remove and recreate
+test_create_and_remove()
+{
+	local basedir=${SCRATCH_MNT}/create_and_remove
+	mkdir -p ${basedir}
+
+	_casefold_set_attr ${basedir}
+	do_create_and_remove "${basedir}" "${pt_file1}" "${pt_file2}"
+	do_create_and_remove "${basedir}" "${jp_file1}" "${jp_file2}"
+	do_create_and_remove "${basedir}" "${ar_file1}" "${ar_file2}"
+	do_create_and_remove "${basedir}" "${fr_file1}" "${fr_file2}"
+}
+
+test_casefold_flag_basic()
+{
+	local basedir=${SCRATCH_MNT}/basic
+
+	mkdir -p ${basedir}
+	_casefold_set_attr ${basedir}
+	_casefold_lsattr_dir ${basedir} | _filter_scratch
+
+	_casefold_unset_attr ${basedir}
+	_casefold_lsattr_dir ${basedir} | _filter_scratch
+}
+
+test_casefold_flag_removal()
+{
+	local basedir=${SCRATCH_MNT}/casefold_flag_removal
+
+	mkdir -p ${basedir}
+	_casefold_set_attr ${basedir}
+	_casefold_lsattr_dir ${basedir} | _filter_scratch
+
+	# Try to remove +F attribute on non empty directory
+	touch ${basedir}/${filename1}
+	_casefold_unset_attr ${basedir} &>/dev/null
+	_casefold_lsattr_dir ${basedir} | _filter_scratch
+}
+
+# Test Inheritance of casefold flag
+test_casefold_flag_inheritance()
+{
+	local basedir=${SCRATCH_MNT}/flag_inheritance
+	local dirpath1="d1/d2/d3"
+	local dirpath2="D1/D2/D3"
+
+	mkdir -p ${basedir}
+	_casefold_set_attr ${basedir}
+
+	mkdir -p ${basedir}/${dirpath1}
+	_casefold_lsattr_dir ${basedir}/${dirpath1} | _filter_scratch
+
+	[ -d ${basedir}/${dirpath2} ] || \
+		echo "Directory CI Lookup failed."
+	_casefold_check_exact_name "${basedir}" "${dirpath1}" || \
+		echo "Created directory with wrong name."
+
+	touch ${basedir}/${dirpath2}/${filename1}
+	[ -f ${basedir}/${dirpath1}/${filename2} ] || \
+		echo "Couldn't create file on casefolded parent."
+}
+
+# Test nesting of sensitive directory inside insensitive directory.
+test_nesting_sensitive_insensitive_tree_simple()
+{
+	local basedir=${SCRATCH_MNT}/sd1
+
+	mkdir -p ${basedir}
+	_casefold_set_attr ${basedir}
+
+	mkdir -p ${basedir}/sd1
+	_casefold_set_attr ${basedir}/sd1
+
+	mkdir ${basedir}/sd1/sd2
+	_casefold_unset_attr ${basedir}/sd1/sd2
+
+	touch ${basedir}/sd1/sd2/${filename1}
+	[ -f ${basedir}/sd1/sd2/${filename1} ] || \
+		echo "Exact nested file lookup failed."
+	[ -f ${basedir}/sd1/SD2/${filename1} ] || \
+		echo "Nested file lookup failed."
+	[ -f ${basedir}/sd1/SD2/${filename2} ] && \
+		echo "Wrong file lookup passed, should have fail."
+}
+
+test_nesting_sensitive_insensitive_tree_complex()
+{
+	# Test nested-directories
+	local basedir=${SCRATCH_MNT}/nesting
+
+	mkdir -p ${basedir}
+	_casefold_set_attr ${basedir}
+
+	mkdir ${basedir}/nd1
+	_casefold_set_attr ${basedir}/nd1
+	mkdir ${basedir}/nd1/nd2
+	_casefold_unset_attr ${basedir}/nd1/nd2
+	mkdir ${basedir}/nd1/nd2/nd3
+	_casefold_set_attr ${basedir}/nd1/nd2/nd3
+	mkdir ${basedir}/nd1/nd2/nd3/nd4
+	_casefold_unset_attr ${basedir}/nd1/nd2/nd3/nd4
+	mkdir ${basedir}/nd1/nd2/nd3/nd4/nd5
+	_casefold_set_attr ${basedir}/nd1/nd2/nd3/nd4/nd5
+
+	[ -d ${basedir}/ND1/ND2/nd3/ND4/nd5 ] || \
+		echo "Nest-dir Lookup failed."
+	[ -d ${basedir}/nd1/nd2/nd3/nd4/ND5 ] && \
+		echo "ND5: Nest-dir Lookup passed, it should fail."
+	[ -d ${basedir}/nd1/nd2/nd3/ND4/nd5 ] || \
+		echo "Nest-dir Lookup failed."
+	[ -d ${basedir}/nd1/nd2/ND3/nd4/ND5 ] && \
+		echo "ND3: Nest-dir Lookup passed, it should fail."
+}
+
+test_symlink_with_inexact_name()
+{
+	local basedir=${SCRATCH_MNT}/symlink
+
+	mkdir -p ${basedir}
+	_casefold_set_attr ${basedir}
+
+	mkdir ${basedir}/ind1
+	mkdir ${basedir}/ind2
+	_casefold_set_attr ${basedir}/ind1
+	touch ${basedir}/ind1/target
+
+	ln -s ${basedir}/ind1/TARGET ${basedir}/ind2/link
+	[ -L ${basedir}/ind2/link ] || echo "Not a symlink."
+	readlink -e ${basedir}/ind2/link | _filter_scratch
+}
+
+do_test_name_preserve()
+{
+	local basedir=${1}
+	local exact=${2}
+	local casefold=${3}
+
+	touch ${basedir}/${exact}
+	rm ${basedir}/${exact}
+
+	touch ${basedir}/${casefold}
+	_casefold_check_exact_name ${basedir} ${casefold} ||
+		echo "${casefold} was not created with exact name"
+}
+
+# Name-preserving tests
+# We create a file with a name, delete it and create again with an
+# equivalent name.  If the negative dentry wasn't invalidated, the
+# file might be created using $1 instead of $2.
+test_name_preserve()
+{
+	local basedir=${SCRATCH_MNT}/test_name_preserve
+
+	mkdir -p ${basedir}
+	_casefold_set_attr ${basedir}
+
+	do_test_name_preserve "${basedir}" "${pt_file1}" "${pt_file2}"
+	do_test_name_preserve "${basedir}" "${jp_file1}" "${jp_file2}"
+	do_test_name_preserve "${basedir}" "${ar_file1}" "${ar_file2}"
+	do_test_name_preserve "${basedir}" "${fr_file1}" "${fr_file2}"
+}
+
+do_test_dir_name_preserve()
+{
+	local basedir=${1}
+	local exact=${2}
+	local casefold=${3}
+
+	mkdir ${basedir}/${exact}
+	rmdir ${basedir}/${exact}
+
+	mkdir ${basedir}/${casefold}
+	_casefold_check_exact_name ${basedir} ${casefold} ||
+		echo "${casefold} was not created with exact name"
+}
+
+test_dir_name_preserve()
+{
+	local basedir=${SCRATCH_MNT}/"dir-test_name_preserve"
+
+	mkdir -p ${basedir}
+	_casefold_set_attr ${basedir}
+
+	do_test_dir_name_preserve "${basedir}" "${pt_file1}" "${pt_file2}"
+	do_test_dir_name_preserve "${basedir}" "${jp_file1}" "${jp_file2}"
+	do_test_dir_name_preserve "${basedir}" "${ar_file1}" "${ar_file2}"
+	do_test_dir_name_preserve "${basedir}" "${fr_file1}" "${fr_file2}"
+}
+
+test_name_reuse()
+{
+	local basedir=${SCRATCH_MNT}/reuse
+	local reuse1=fileX
+	local reuse2=FILEX
+
+	mkdir ${basedir}
+	_casefold_set_attr ${basedir}
+
+	touch ${basedir}/${reuse1}
+	rm -f ${basedir}/${reuse1} || echo "File lookup failed."
+	touch ${basedir}/${reuse2}
+	_casefold_check_exact_name "${basedir}" "${reuse2}" || \
+		echo "File created with wrong name"
+	_casefold_check_exact_name "${basedir}" "${reuse1}" && \
+		echo "File created with the old name"
+}
+
+test_create_with_same_name()
+{
+	local basedir=${SCRATCH_MNT}/same_name
+
+	mkdir ${basedir}
+	_casefold_set_attr ${basedir}
+
+	mkdir -p ${basedir}/same1/same1
+	touch ${basedir}/SAME1/sAME1/sAMe1
+	touch -c ${basedir}/SAME1/sAME1/same1 ||
+		echo "Would create a new file instead of using old one"
+}
+
+test_file_rename()
+{
+	local basedir=${SCRATCH_MNT}/rename
+
+	mkdir -p ${basedir}
+	_casefold_set_attr ${basedir}
+
+	# Move to an equivalent name should not work
+	mv ${basedir}/rename ${basedir}/rename 2>&1 | \
+		_filter_scratch
+
+	_casefold_check_exact_name ${basedir} "rename" || \
+		echo "Name shouldn't change."
+}
+
+# Test openfd with casefold.
+# 1. Delete a file after gettings its fd.
+# 2. Then create new dir with same name
+test_casefold_openfd()
+{
+	local basedir=${SCRATCH_MNT}/openfd
+	local ofd1="openfd"
+	local ofd2="OPENFD"
+
+	mkdir -p ${basedir}
+	_casefold_set_attr ${basedir}
+
+	exec 3<> ${basedir}/${ofd1}
+	rm -rf ${basedir}/${ofd1}
+	mkdir ${basedir}/${ofd2}
+	[ -d ${basedir}/${ofd2} ] || echo "Not a directory"
+	_casefold_check_exact_name ${basedir} "${ofd2}" ||
+		echo "openfd file was created using old name"
+	rm -rf ${basedir}/${ofd2}
+	exec 3>&-
+}
+
+# Test openfd with casefold.
+# 1. Delete a file after gettings its fd.
+# 2. Then create new file with same name
+# 3. Read from open-fd and write into new file.
+test_casefold_openfd2()
+{
+	local basedir=${SCRATCH_MNT}/openfd2
+	local ofd1="openfd"
+	local ofd2="OPENFD"
+
+	mkdir ${basedir}
+	_casefold_set_attr ${basedir}
+
+	date > ${basedir}/${ofd1}
+	exec 3<> ${basedir}/${ofd1}
+	rm -rf ${basedir}/${ofd1}
+	touch ${basedir}/${ofd1}
+	[ -f ${basedir}/${ofd2} ] || echo "Not a file"
+	read data <&3
+	echo $data >> ${basedir}/${ofd1}
+	exec 3>&-
+}
+
+test_hard_link_lookups()
+{
+	local basedir=${SCRATCH_MNT}/hard_link
+
+	mkdir ${basedir}
+	_casefold_set_attr ${basedir}
+
+	touch ${basedir}/h1
+	ln ${basedir}/H1 ${SCRATCH_MNT}/h1
+	cnt=`stat -c %h ${basedir}/h1`
+	[ $cnt -eq 1 ] && echo "Unable to create hardlink"
+
+	# Create hardlink for casefold dir file and inside regular dir.
+	touch ${SCRATCH_MNT}/h2
+	ln ${SCRATCH_MNT}/h2 ${basedir}/H2
+	cnt=`stat -c %h ${basedir}/h2`
+	[ $cnt -eq 1 ] && echo "Unable to create hardlink"
+}
+
+test_xattrs_lookups()
+{
+	local basedir=${SCRATCH_MNT}/xattrs
+
+	mkdir ${basedir}
+	_casefold_set_attr ${basedir}
+
+	mkdir -p ${basedir}/x
+
+	${SETFATTR_PROG} -n user.foo -v bar ${basedir}/x
+	${GETFATTR_PROG} --absolute-names -n user.foo \
+		${basedir}/x | _filter_scratch
+
+	touch ${basedir}/x/f1
+	${SETFATTR_PROG} -n user.foo -v bar ${basedir}/x/f1
+	${GETFATTR_PROG} --absolute-names -n user.foo \
+		${basedir}/x/f1 | _filter_scratch
+}
+
+test_lookup_large_directory()
+{
+	local basedir=${SCRATCH_MNT}/large
+
+	mkdir -p ${basedir}
+	_casefold_set_attr ${basedir}
+
+	touch $(seq -f "${basedir}/file%g" 0 2000)
+
+	# We really want to spawn a single process here, to speed up the
+	# test, but we don't want the output of 2k files, except for
+	# errors.
+	cat $(seq -f "${basedir}/FILE%g" 0 2000) || \
+		echo "Case on large dir failed"
+}
+
+test_strict_mode_invalid_filename()
+{
+	local basedir=${SCRATCH_MNT}/strict
+
+	mkdir -p ${basedir}
+	_casefold_set_attr ${basedir}
+
+	# These creation commands should fail, since we are on strict
+	# mode.
+	touch "${basedir}/${blob_file1}" 2>&1 | _filter_scratch
+	touch "${basedir}/${blob_file2}" 2>&1 | _filter_scratch
+}
+
+#############
+# Run tests #
+#############
+
+_scratch_mkfs_casefold >>$seqres.full 2>&1
+
+_scratch_mount
+
+_check_dmesg_for \
+	"\(${sdev}\): Using encoding defined by superblock: utf8" || \
+	_fail "Could not mount with encoding: utf8"
+
+test_casefold_flag_basic
+test_casefold_lookup
+test_bad_casefold_lookup
+test_create_and_remove
+test_casefold_flag_removal
+test_casefold_flag_inheritance
+test_nesting_sensitive_insensitive_tree_simple
+test_nesting_sensitive_insensitive_tree_complex
+test_symlink_with_inexact_name
+test_name_preserve
+test_dir_name_preserve
+test_name_reuse
+test_create_with_same_name
+test_file_rename
+test_casefold_openfd
+test_casefold_openfd2
+test_hard_link_lookups
+test_xattrs_lookups
+test_lookup_large_directory
+
+_scratch_unmount
+_check_scratch_fs
+
+# Test Strict Mode
+_scratch_mkfs_casefold_strict >>$seqres.full 2>&1
+_scratch_mount
+
+test_strict_mode_invalid_filename
+
+_scratch_unmount
+_check_scratch_fs
+
+status=0
+exit
diff --git a/tests/shared/012.out b/tests/shared/012.out
new file mode 100644
index 000000000000..881c375b5576
--- /dev/null
+++ b/tests/shared/012.out
@@ -0,0 +1,16 @@
+QA output created by 012
+SCRATCH_MNT/basic Extents, Casefold
+SCRATCH_MNT/basic Extents
+SCRATCH_MNT/casefold_flag_removal Extents, Casefold
+SCRATCH_MNT/casefold_flag_removal Extents, Casefold
+SCRATCH_MNT/flag_inheritance/d1/d2/d3 Extents, Casefold
+SCRATCH_MNT/symlink/ind1/TARGET
+mv: cannot stat 'SCRATCH_MNT/rename/rename': No such file or directory
+# file: SCRATCH_MNT/xattrs/x
+user.foo="bar"
+
+# file: SCRATCH_MNT/xattrs/x/f1
+user.foo="bar"
+
+touch: setting times of 'SCRATCH_MNT/strict/corac'$'\314\247\303': Invalid argument
+touch: setting times of 'SCRATCH_MNT/strict/cora'$'\303\247\303': Invalid argument
diff --git a/tests/shared/group b/tests/shared/group
index b091d9111359..53b4a356695d 100644
--- a/tests/shared/group
+++ b/tests/shared/group
@@ -14,6 +14,7 @@
 009 auto stress dedupe
 010 auto stress dedupe
 011 auto quick
+012 auto quick casefold
 032 mkfs auto quick
 272 auto enospc rw
 289 auto quick
-- 
2.20.1

Re: [PATCH v3 2/2] shared/012: Add tests for filename casefolding feature

[Eryu Guan]

On Wed, Jun 12, 2019 at 02:40:33PM -0400, Gabriel Krisman Bertazi wrote:
> From: "Lakshmipathi.G" <lakshmipathi.ganapathi@collabora.co.uk>
> 
> This new test implements verification for the per-directory
> case-insensitive feature, as supported by the reference implementation
> in Ext4.
> 
> Signed-off-by: Lakshmipathi.G <lakshmipathi.ganapathi@collabora.co.uk>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
>   [Rewrite to support feature design]
>   [Refactor to simplify implementation]

Test looks good to me, and test passes for me with v5.2-rc4 kernel and
latest e2fsprogs, thanks! Just that, I moved the test to generic, as we
have all the needed _require rules ready to _notrun on unsupported fs,
so it's ready to be generic. (Sorry I was not involved with the
ext4-shared-generic discussion in the first place)

Thanks,
Eryu

Re: [PATCH v3 2/2] shared/012: Add tests for filename casefolding feature

[Theodore Ts'o]

On Sun, Jun 16, 2019 at 10:44:40PM +0800, Eryu Guan wrote:
> On Wed, Jun 12, 2019 at 02:40:33PM -0400, Gabriel Krisman Bertazi wrote:
> Test looks good to me, and test passes for me with v5.2-rc4 kernel and
> latest e2fsprogs, thanks! Just that, I moved the test to generic, as we
> have all the needed _require rules ready to _notrun on unsupported fs,
> so it's ready to be generic. (Sorry I was not involved with the
> ext4-shared-generic discussion in the first place)

Just to clear up my confusion, what's the distinction between shared
and generic?  Is it that if there are explicit "only run this test on
file systems xxx, yyy, and zzz declarations", then it should be
shared, and otherwise it should be in generic?

						- Ted

Re: [PATCH v3 2/2] shared/012: Add tests for filename casefolding feature

[Eryu Guan]

On Sun, Jun 16, 2019 at 04:01:54PM -0400, Theodore Ts'o wrote:
> On Sun, Jun 16, 2019 at 10:44:40PM +0800, Eryu Guan wrote:
> > On Wed, Jun 12, 2019 at 02:40:33PM -0400, Gabriel Krisman Bertazi wrote:
> > Test looks good to me, and test passes for me with v5.2-rc4 kernel and
> > latest e2fsprogs, thanks! Just that, I moved the test to generic, as we
> > have all the needed _require rules ready to _notrun on unsupported fs,
> > so it's ready to be generic. (Sorry I was not involved with the
> > ext4-shared-generic discussion in the first place)
> 
> Just to clear up my confusion, what's the distinction between shared
> and generic?  Is it that if there are explicit "only run this test on
> file systems xxx, yyy, and zzz declarations", then it should be
> shared, and otherwise it should be in generic?
> 
> 						- Ted

IMO, shared tests are generic tests that don't have proper _require
rules, so they're hard-coded with explicit "_supported_fs xxx yyy". With
proper _require rules, there should be no shared tests at all, and we'd
try avoid adding new shared tests if possible.

Thanks,
Eryu

Removing the shared class of tests

[Theodore Ts'o]

On Thu, Jun 20, 2019 at 07:29:03PM +0800, Eryu Guan wrote:
> 
> IMO, shared tests are generic tests that don't have proper _require
> rules, so they're hard-coded with explicit "_supported_fs xxx yyy". With
> proper _require rules, there should be no shared tests at all, and we'd
> try avoid adding new shared tests if possible.

Thanks for the clarification, that makes sense!

I can see some shared tests that we can probably move out, actually.
shared/00[134] and shared/272 make no sense at all for ext2.  The ext3
file system was removed in 2015, in the 4.3 kernel, and since 2009
(ten years ago) in 2.6.33, the ext4 implementation could be used to
support ext3 (and I believe many if not all enterprise distros been
taking advantage of this long before 2015, so they only had to update
patches for ext4).

(If anything, we might be better served by a two line patch to check
so that simply included the ext4 group when FSTYP == ext3.  That way
we will run more tests on those systems which still support the ext3
emulated-by-ext4 mode.)

The shared/002 test could be moved to generic if we had a way for file
systems to declare how many xattrs per file they support.

The shared/006 test needs some way of descriminating which inodes have
a fixed number of inodes, since it fills a small file system until it
runs out of space and then runs fsck on it.  Actually, if we make the
test file system smaller, so it runs in finite time, we could probably
just run it on all file systems, since checking to see what file
systems which don't have a fixed inode table (e.g., btrfs) do under
ENOSPC when creating tons of inodes probably makes sense there for
those file systems as well.

I'm not sure why shared/011 is only run on ext4 and btrfs.  Does
cgroup-aware writeback not work on other file systems?

The shared/{008,009,010} tests could be moved to generic if we added
_require_dedup.  The shared/298 tests just needs a _require_fstrim.

The bottom line is I think if this is something we care about, we can
probably move out nearly all of the tests from shared.  Should I start
sending patches?  :-)

						- Ted

Re: Removing the shared class of tests

[Darrick J. Wong]

On Thu, Jun 20, 2019 at 12:21:16PM -0400, Theodore Ts'o wrote:
> On Thu, Jun 20, 2019 at 07:29:03PM +0800, Eryu Guan wrote:
> > 
> > IMO, shared tests are generic tests that don't have proper _require
> > rules, so they're hard-coded with explicit "_supported_fs xxx yyy". With
> > proper _require rules, there should be no shared tests at all, and we'd
> > try avoid adding new shared tests if possible.
> 
> Thanks for the clarification, that makes sense!
> 
> I can see some shared tests that we can probably move out, actually.
> shared/00[134] and shared/272 make no sense at all for ext2.  The ext3
> file system was removed in 2015, in the 4.3 kernel, and since 2009
> (ten years ago) in 2.6.33, the ext4 implementation could be used to
> support ext3 (and I believe many if not all enterprise distros been
> taking advantage of this long before 2015, so they only had to update
> patches for ext4).
> 
> (If anything, we might be better served by a two line patch to check
> so that simply included the ext4 group when FSTYP == ext3.  That way
> we will run more tests on those systems which still support the ext3
> emulated-by-ext4 mode.)
> 
> The shared/002 test could be moved to generic if we had a way for file
> systems to declare how many xattrs per file they support.
> 
> The shared/006 test needs some way of descriminating which inodes have
> a fixed number of inodes, since it fills a small file system until it
> runs out of space and then runs fsck on it.  Actually, if we make the
> test file system smaller, so it runs in finite time, we could probably
> just run it on all file systems, since checking to see what file
> systems which don't have a fixed inode table (e.g., btrfs) do under
> ENOSPC when creating tons of inodes probably makes sense there for
> those file systems as well.

xfs doesn't have a fixed inode table either, so ... that sounds like a
good idea.

> I'm not sure why shared/011 is only run on ext4 and btrfs.  Does
> cgroup-aware writeback not work on other file systems?

IIRC it doesn't work on xfs because the author never quite answered
Dave's question about whether or not it would cause ... io priority
inversions or something?  There was some unanswered question (iirc) so
nobody RVB'd the patch and it never went upstream.

> The shared/{008,009,010} tests could be moved to generic if we added
> _require_dedup.  The shared/298 tests just needs a _require_fstrim.

FWIW shared/010 is blacklisted on my systems because of its poor "kill
everything and wait for the processes to exit" code.  The test starts
fsstress and then "while [ -e $dupe_run ]; do duperemove; done" loops.

When end_test runs, it'll remove the $dupe_run flag file and wait for
all the duperemoves to finish.  Unfortunately by this time the
duperemoves and the fsstress threads are duking it out for file locks
and it takes forever for duperemove to finish scanning and exit.
Consequently this test sometimes runs for a very long time.

I /think/ the answer is for end_test to send SIGTERM or something to the
duperemoves and wait for them to exit sooner than later.

> The bottom line is I think if this is something we care about, we can
> probably move out nearly all of the tests from shared.  Should I start
> sending patches?  :-)

Sounds good to me....

--D

> 
> 						- Ted

Re: Removing the shared class of tests

[Theodore Ts'o]

On Thu, Jun 20, 2019 at 10:50:35AM -0700, Darrick J. Wong wrote:
> > The shared/006 test needs some way of descriminating which inodes have
> > a fixed number of inodes, since it fills a small file system until it
> > runs out of space and then runs fsck on it.  Actually, if we make the
> > test file system smaller, so it runs in finite time, we could probably
> > just run it on all file systems, since checking to see what file
> > systems which don't have a fixed inode table (e.g., btrfs) do under
> > ENOSPC when creating tons of inodes probably makes sense there for
> > those file systems as well.
> 
> xfs doesn't have a fixed inode table either, so ... that sounds like a
> good idea.

Which is amusing, given that shared/006 declares that it is supported
for xfs.  So it might just work on btrfs w/o any changes; although
maybe it just takes too long to run.  :-)

	      	      	    	- Ted

Re: Removing the shared class of tests

[Christoph Hellwig]

On Thu, Jun 20, 2019 at 10:50:35AM -0700, Darrick J. Wong wrote:
> > I'm not sure why shared/011 is only run on ext4 and btrfs.  Does
> > cgroup-aware writeback not work on other file systems?
> 
> IIRC it doesn't work on xfs because the author never quite answered
> Dave's question about whether or not it would cause ... io priority
> inversions or something?  There was some unanswered question (iirc) so
> nobody RVB'd the patch and it never went upstream.

I've got a new and tested patch to support cgroup writeback on xfs that
I'll send out today.  But even with that lots of file systems don't
support cgroup writeback and there is no easy way to autodetect the
support.  As far as I an tell ext2 and f2fs also support cgroup
writeback, so they should be added a well.


As for the higher level question?  The shared tests always confused the
heck out of me.  generic with the right feature checks seem like a much
better idea.

Re: Removing the shared class of tests

[Theodore Ts'o]

On Mon, Jun 24, 2019 at 12:16:10AM -0700, Christoph Hellwig wrote:
> 
> As for the higher level question?  The shared tests always confused the
> heck out of me.  generic with the right feature checks seem like a much
> better idea.

Agreed.  I've sent out a patch series to bring the number of patches
in shared down to four.  Here's what's left:

shared/002 --- needs a feature test to somehow determine whether a
	file system supports thousads of xattrs in a file (currently
	on btrfs and xfs)

shared/011 --- needs some way of determining that a file system
	supports cgroup-aware writeback (currently enabled only for
	ext4 and btrfs).  Do we consider lack of support of
	cgroup-aware writeback a bug?  If so, maybe it doesn't need a
	feature test at all?

shared/032 --- needs a feature test to determine whether or not a file
	system's mkfs supports detection of "foreign file systems".
	e.g., whether or not it warns if you try overwrite a file
	system w/o another file system.  (Currently enabled by xfs and
	btrfs; it doesn't work for ext[234] because e2fsprogs, because
	I didn't want to break existing shell scripts, only warns when
	it is used interactively.  We could a way to force it to be
	activated it points out this tests is fundamentally testing
	implementation choices of the userspace utilities of a file
	system.  Does it belong in xfstests?   : ¯\_(ツ)_/¯ )

shared/289 --- contains ext4, xfs, and btrfs mechanisms for
	determining blocks which are unallocated.  Has hard-coded
	invocations to dumpe2fs, xfs_db, and /bin/btrfs.

These don't have obvious solutions.  We could maybe add a _notrun if
adding the thousands of xattrs fails with an ENOSPC or related error
(f2fs uses something else).

Maybe we just move shared/011 and move it generic/ w/o a feature test.

Maybe we remove shared/032 altogether, since for e2fsprogs IMHO
the right place to put it is the regression test in e2fsprogs --- but
I know xfs has a different test philosophy for xfsprogs; and tha begs
the question of what to do for mkfs.btrfs.

And maybe we just split up shared/289 to three different tests in
ext4/, xfs/, and btrfs/, since it would make the test script much
simpler to understand?

What do people think?

						- Ted

Re: Removing the shared class of tests

[Darrick J. Wong]

On Mon, Jun 24, 2019 at 09:07:30AM -0400, Theodore Ts'o wrote:
> On Mon, Jun 24, 2019 at 12:16:10AM -0700, Christoph Hellwig wrote:
> > 
> > As for the higher level question?  The shared tests always confused the
> > heck out of me.  generic with the right feature checks seem like a much
> > better idea.
> 
> Agreed.  I've sent out a patch series to bring the number of patches
> in shared down to four.  Here's what's left:
> 
> shared/002 --- needs a feature test to somehow determine whether a
> 	file system supports thousads of xattrs in a file (currently
> 	on btrfs and xfs)

I don't know of a good way to do that other than trying it.

> shared/011 --- needs some way of determining that a file system
> 	supports cgroup-aware writeback (currently enabled only for
> 	ext4 and btrfs).  Do we consider lack of support of
> 	cgroup-aware writeback a bug?  If so, maybe it doesn't need a
> 	feature test at all?

...but for the ones that do, we need a test to make sure the reported
accounting values aren't totally off in the stratosphere.

I wonder, could we add a _require_scratch_cgroupwb that would assign a
new cgroup, try to write a fixed amount of data (~64k) and then _notrun
if the cgroup write back thing reported zero bytes written?

> shared/032 --- needs a feature test to determine whether or not a file
> 	system's mkfs supports detection of "foreign file systems".
> 	e.g., whether or not it warns if you try overwrite a file
> 	system w/o another file system.  (Currently enabled by xfs and
> 	btrfs; it doesn't work for ext[234] because e2fsprogs, because
> 	I didn't want to break existing shell scripts, only warns when
> 	it is used interactively.  We could a way to force it to be
> 	activated it points out this tests is fundamentally testing
> 	implementation choices of the userspace utilities of a file
> 	system.  Does it belong in xfstests?   : ¯\_(ツ)_/¯ )
> 
> shared/289 --- contains ext4, xfs, and btrfs mechanisms for
> 	determining blocks which are unallocated.  Has hard-coded
> 	invocations to dumpe2fs, xfs_db, and /bin/btrfs.

Huh?  shared/289 looks like a pure ext* test to me....

# Copyright (c) 2012 Red Hat, Inc.  All Rights Reserved.
#
# FS QA Test No. 289
#
# Test overhead & df output for extN filesystems

<confused>

> These don't have obvious solutions.  We could maybe add a _notrun if
> adding the thousands of xattrs fails with an ENOSPC or related error
> (f2fs uses something else).
> 
> Maybe we just move shared/011 and move it generic/ w/o a feature test.
> 
> Maybe we remove shared/032 altogether, since for e2fsprogs IMHO
> the right place to put it is the regression test in e2fsprogs --- but
> I know xfs has a different test philosophy for xfsprogs; and tha begs
> the question of what to do for mkfs.btrfs.

<shrug> I'm fine with leaving the test there for xfs since that's where
we put all the xfsprogs tests anyway. :)

--D

> And maybe we just split up shared/289 to three different tests in
> ext4/, xfs/, and btrfs/, since it would make the test script much
> simpler to understand?
> 
> What do people think?
> 
> 						- Ted

Re: Removing the shared class of tests

[Theodore Ts'o]

On Mon, Jun 24, 2019 at 10:05:15AM -0700, Darrick J. Wong wrote:
> > shared/289 --- contains ext4, xfs, and btrfs mechanisms for
> > 	determining blocks which are unallocated.  Has hard-coded
> > 	invocations to dumpe2fs, xfs_db, and /bin/btrfs.
> 
> Huh?  shared/289 looks like a pure ext* test to me....

Sorry, typo.  That should have been shared/298.

I've already moved shared/289 to ext4 in my proposed patches to move
tests out of shared/.

				- Ted

Re: Removing the shared class of tests

[Eryu Guan]

On Mon, Jun 24, 2019 at 09:07:30AM -0400, Theodore Ts'o wrote:
> On Mon, Jun 24, 2019 at 12:16:10AM -0700, Christoph Hellwig wrote:
> > 
> > As for the higher level question?  The shared tests always confused the
> > heck out of me.  generic with the right feature checks seem like a much
> > better idea.
> 
> Agreed.  I've sent out a patch series to bring the number of patches
> in shared down to four.  Here's what's left:
> 
> shared/002 --- needs a feature test to somehow determine whether a
> 	file system supports thousads of xattrs in a file (currently
> 	on btrfs and xfs)

Another option would be just whitelist btrfs and xfs in a require rule,
we already have few require rules work like that, e.g.
_fstyp_has_non_default_seek_data_hole(), this is not ideal, but works in
such corner cases.

Thanks,
Eryu

> 
> shared/011 --- needs some way of determining that a file system
> 	supports cgroup-aware writeback (currently enabled only for
> 	ext4 and btrfs).  Do we consider lack of support of
> 	cgroup-aware writeback a bug?  If so, maybe it doesn't need a
> 	feature test at all?
> 
> shared/032 --- needs a feature test to determine whether or not a file
> 	system's mkfs supports detection of "foreign file systems".
> 	e.g., whether or not it warns if you try overwrite a file
> 	system w/o another file system.  (Currently enabled by xfs and
> 	btrfs; it doesn't work for ext[234] because e2fsprogs, because
> 	I didn't want to break existing shell scripts, only warns when
> 	it is used interactively.  We could a way to force it to be
> 	activated it points out this tests is fundamentally testing
> 	implementation choices of the userspace utilities of a file
> 	system.  Does it belong in xfstests?   : ¯\_(ツ)_/¯ )
> 
> shared/289 --- contains ext4, xfs, and btrfs mechanisms for
> 	determining blocks which are unallocated.  Has hard-coded
> 	invocations to dumpe2fs, xfs_db, and /bin/btrfs.
> 
> These don't have obvious solutions.  We could maybe add a _notrun if
> adding the thousands of xattrs fails with an ENOSPC or related error
> (f2fs uses something else).
> 
> Maybe we just move shared/011 and move it generic/ w/o a feature test.
> 
> Maybe we remove shared/032 altogether, since for e2fsprogs IMHO
> the right place to put it is the regression test in e2fsprogs --- but
> I know xfs has a different test philosophy for xfsprogs; and tha begs
> the question of what to do for mkfs.btrfs.
> 
> And maybe we just split up shared/289 to three different tests in
> ext4/, xfs/, and btrfs/, since it would make the test script much
> simpler to understand?
> 
> What do people think?
> 
> 						- Ted