Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Gabriel Krisman Bertazi <krisman@collabora.com>
To:     Slava Bacherikov <slava@bacher09.org>
Cc:     ebiggers@kernel.org, tytso@mit.edu, linux-ext4@vger.kernel.org
Subject: Re: [PATCH] tune2fs: allow disabling casefold feature
Organization: Collabora
References: <Ysc5HYPXUs6rW02S@gmail.com>
        <20220707204137.71311-1-slava@bacher09.org>
Date:   Thu, 07 Jul 2022 17:10:52 -0400
In-Reply-To: <20220707204137.71311-1-slava@bacher09.org> (Slava Bacherikov's
        message of "Thu, 7 Jul 2022 23:41:37 +0300")
Message-ID: <87pmig39yb.fsf@collabora.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Slava Bacherikov <slava@bacher09.org> writes:

> Casefold can be safely disabled if there are no directories with +F
> attribute ( EXT4_CASEFOLD_FL ). This checks all inodes for that flag and in
> case there isn't any, it disables casefold FS feature. When FS has
> directories with +F attributes, user could convert these directories,
> probably by mounting FS and executing some script or by doing it
> manually. Afterwards, it would be possible to disable casefold FS flag
> via tune2fs.
>
> Signed-off-by: Slava Bacherikov <slava@bacher09.org>

I think this is a safe thing to do, gave it a quick spin and the patch
now looks good to me.

Reviewed-by: Gabriel Krisman Bertazi <krisman@collabora.com>

Thank you,

> ---
>  misc/tune2fs.8.in |  6 ++++--
>  misc/tune2fs.c    | 54 ++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 57 insertions(+), 3 deletions(-)
>
> diff --git a/misc/tune2fs.8.in b/misc/tune2fs.8.in
> index 628dcdc0..dcf108c1 100644
> --- a/misc/tune2fs.8.in
> +++ b/misc/tune2fs.8.in
> @@ -593,8 +593,10 @@ Enable the file system to be larger than 2^32 blocks.
>  .TP
>  .B casefold
>  Enable support for file system level casefolding.
> -.B Tune2fs
> -currently only supports setting this file system feature.
> +The option can be cleared only if filesystem has no
> +directories with
> +.B F
> +attribute.
>  .TP
>  .B dir_index
>  Use hashed b-trees to speed up lookups for large directories.
> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index 6c162ba5..a8355619 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -204,7 +204,8 @@ static __u32 clear_ok_features[3] = {
>  		EXT4_FEATURE_INCOMPAT_FLEX_BG |
>  		EXT4_FEATURE_INCOMPAT_MMP |
>  		EXT4_FEATURE_INCOMPAT_64BIT |
> -		EXT4_FEATURE_INCOMPAT_CSUM_SEED,
> +		EXT4_FEATURE_INCOMPAT_CSUM_SEED |
> +		EXT4_FEATURE_INCOMPAT_CASEFOLD,
>  	/* R/O compat */
>  	EXT2_FEATURE_RO_COMPAT_LARGE_FILE |
>  		EXT4_FEATURE_RO_COMPAT_HUGE_FILE|
> @@ -1020,6 +1021,41 @@ out:
>  	return retval;
>  }
>  
> +static int has_casefold_inode(ext2_filsys fs)
> +{
> +	int length = EXT2_INODE_SIZE(fs->super);
> +	struct ext2_inode *inode = NULL;
> +	ext2_inode_scan	scan;
> +	errcode_t	retval;
> +	ext2_ino_t	ino;
> +	int found_casefold = 0;
> +
> +	retval = ext2fs_get_mem(length, &inode);
> +	if (retval)
> +		fatal_err(retval, "while allocating memory");
> +
> +	retval = ext2fs_open_inode_scan(fs, 0, &scan);
> +	if (retval)
> +		fatal_err(retval, "while opening inode scan");
> +
> +	do {
> +		retval = ext2fs_get_next_inode_full(scan, &ino, inode, length);
> +		if (retval)
> +			fatal_err(retval, "while getting next inode");
> +		if (!ino)
> +			break;
> +
> +		if(inode->i_flags & EXT4_CASEFOLD_FL) {
> +			found_casefold = 1;
> +			break;
> +		}
> +	} while(1);
> +
> +	ext2fs_free_mem(&inode);
> +	ext2fs_close_inode_scan(scan);
> +	return found_casefold;
> +}
> +
>  static errcode_t disable_uninit_bg(ext2_filsys fs, __u32 csum_feature_flag)
>  {
>  	struct ext2_group_desc *gd;
> @@ -1554,6 +1590,22 @@ mmp_error:
>  		enabling_casefold = 1;
>  	}
>  
> +	if (FEATURE_OFF(E2P_FEATURE_INCOMPAT, EXT4_FEATURE_INCOMPAT_CASEFOLD)) {
> +		if (mount_flags & EXT2_MF_MOUNTED) {
> +			fputs(_("The casefold feature may only be disabled when "
> +				"the filesystem is unmounted.\n"), stderr);
> +			return 1;
> +		}
> +		if (has_casefold_inode(fs)) {
> +			fputs(_("The casefold feature can't be cleared when "
> +					"there are inodes with +F flag.\n"), stderr);
> +			return 1;
> +		}
> +		fs->super->s_encoding = 0;
> +		fs->super->s_encoding_flags = 0;
> +		enabling_casefold = 0;
> +	}
> +
>  	if (FEATURE_ON(E2P_FEATURE_INCOMPAT,
>  		EXT4_FEATURE_INCOMPAT_CSUM_SEED)) {
>  		if (!ext2fs_has_feature_metadata_csum(sb)) {

-- 
Gabriel Krisman Bertazi