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;
Date:   Thu, 7 Jul 2022 19:50:53 +0000
From:   Eric Biggers <ebiggers@kernel.org>
To:     Slava Bacherikov <slava@bacher09.org>
Cc:     tytso@mit.edu, linux-ext4@vger.kernel.org, krisman@collabora.com
Subject: Re: [PATCH v2] tune2fs: allow disabling casefold feature
Message-ID: <Ysc5HYPXUs6rW02S@gmail.com>
References: <YscmTC3Mk9OXqOgL@gmail.com>
 <20220707190456.64972-1-slava@bacher09.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220707190456.64972-1-slava@bacher09.org>
Precedence: bulk

On Thu, Jul 07, 2022 at 10:04:56PM +0300, Slava Bacherikov wrote:
> diff --git a/misc/tune2fs.8.in b/misc/tune2fs.8.in
> index 628dcdc0..8ef28860 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 could be disabled only if filesystem has no
> +directories with
> +.B F
> +attribute.

Please use present tense: "could" => "can".  Otherwise this can be interpreted
as the opposite of what you meant.

Also, "cleared" instead of "disabled", to be consistent with the rest of the
page.

>  static errcode_t disable_uninit_bg(ext2_filsys fs, __u32 csum_feature_flag)
>  {
>  	struct ext2_group_desc *gd;
> @@ -1554,6 +1590,20 @@ 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 couldn't be disabled when "
> +					"there are inodes with +F flag.\n"), stderr);
> +			return 1;
> +		}
> +		enabling_casefold = 0;

Likewise, "couldn't" => "can't".

Also, what are the semantics of disabling casefold, exactly?  Do the encoding
and encoding flags fields in the superblock also get cleared?

- Eric