Message-ID: <956192d3-5fb8-4ecc-8625-a34812df537b@igalia.com>
Date: Fri, 6 Sep 2024 11:59:45 -0300
From: André Almeida <andrealmeid@...lia.com>
To: Gabriel Krisman Bertazi <gabriel@...sman.be>
Cc: Hugh Dickins <hughd@...gle.com>, Andrew Morton
 <akpm@...ux-foundation.org>, Alexander Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
 krisman@...nel.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 linux-fsdevel@...r.kernel.org, kernel-dev@...lia.com,
 Daniel Rosenberg <drosen@...gle.com>, smcv@...labora.com,
 Christoph Hellwig <hch@....de>, Theodore Ts'o <tytso@....edu>
Subject: Re: [PATCH v3 6/9] tmpfs: Add casefold lookup support

Hey!

On 9/5/24 18:28, Gabriel Krisman Bertazi wrote:
> Hi,
>
> André Almeida <andrealmeid@...lia.com> writes:
>> @@ -3427,6 +3431,10 @@ shmem_mknod(struct mnt_idmap *idmap, struct inode *dir,
>>   	if (IS_ERR(inode))
>>   		return PTR_ERR(inode);
>>   
>> +	if (IS_ENABLED(CONFIG_UNICODE))
>> +		if (!generic_ci_validate_strict_name(dir, &dentry->d_name))
>> +			return -EINVAL;
>> +
> if (IS_ENABLED(CONFIG_UNICODE) &&
>      generic_ci_validate_strict_name(dir, &dentry->d_name))
>
>>   static const struct constant_table shmem_param_enums_huge[] = {
>> @@ -4081,9 +4111,62 @@ const struct fs_parameter_spec shmem_fs_parameters[] = {
>>   	fsparam_string("grpquota_block_hardlimit", Opt_grpquota_block_hardlimit),
>>   	fsparam_string("grpquota_inode_hardlimit", Opt_grpquota_inode_hardlimit),
>>   #endif
>> +	fsparam_string("casefold",	Opt_casefold_version),
>> +	fsparam_flag  ("casefold",	Opt_casefold),
>> +	fsparam_flag  ("strict_encoding", Opt_strict_encoding),
> I don't know if it is possible, but can we do it with a single parameter?

I tried, but when you use casefold with no args, the code fails 
somewhere before that, claiming that there's no arg.

>> +static int shmem_parse_opt_casefold(struct fs_context *fc, struct fs_parameter *param,
>> +				    bool latest_version)
> Instead of the boolean, can't you check if param->string != NULL? (real
> question, I never used fs_parameter.
>
>> +{
>> +	struct shmem_options *ctx = fc->fs_private;
>> +	unsigned int maj = 0, min = 0, rev = 0, version = 0;
>> +	struct unicode_map *encoding;
>> +	char *version_str = param->string + 5;
>> +	int ret;
> unsigned int version = UTF8_LATEST;
>
> and kill the if/else below:
>> +
>> +	if (latest_version) {
>> +		version = UTF8_LATEST;
>> +	} else {
>> +		if (strncmp(param->string, "utf8-", 5))
>> +			return invalfc(fc, "Only UTF-8 encodings are supported "
>> +				       "in the format: utf8-<version number>");
>> +
>> +		ret = utf8_parse_version(version_str, &maj, &min, &rev);
> utf8_parse_version interface could return UNICODE_AGE() already, so we hide the details
> from the caller. wdyt?

I like it!

>
>> +		if (ret)
>> +			return invalfc(fc, "Invalid UTF-8 version: %s", version_str);
>> +
>> +		version = UNICODE_AGE(maj, min, rev);
>> +	}
>> +
>> +	encoding = utf8_load(version);
>> +
>> +	if (IS_ERR(encoding)) {
>> +		if (latest_version)
>> +			return invalfc(fc, "Failed loading latest UTF-8 version");
>> +		else
>> +			return invalfc(fc, "Failed loading UTF-8 version: %s", version_str);
> The following covers both legs (untested):
>
> if (IS_ERR(encoding))
>    return invalfc(fc, "Failed loading UTF-8 version: utf8-%u.%u.%u\n"",
> 	           unicode_maj(version), unicode_min(version), unicode_rev(version));
>
>> +	if (latest_version)
>> +		pr_info("tmpfs: Using the latest UTF-8 version available");
>> +	else
>> +		pr_info("tmpfs: Using encoding provided by mount
>> options: %s\n", param->string);
> The following covers both legs (untested):
>
> pr_info (fc, "tmpfs: Using encoding : utf8-%u.%u.%u\n"
>           unicode_maj(version), unicode_min(version), unicode_rev(version));
>
>> +
>> +	ctx->encoding = encoding;
>> +
>> +	return 0;
>> +}
>> +#else
>> +static int shmem_parse_opt_casefold(struct fs_context *fc, struct fs_parameter *param,
>> +				    bool latest_version)
>> +{
>> +	return invalfc(fc, "tmpfs: No kernel support for casefold filesystems\n");
>> +}
> A message like "Kernel not built with CONFIG_UNICODE" immediately tells
> you how to fix it.
>
>> @@ -4515,6 +4610,16 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc)
>>   	}
>>   	sb->s_export_op = &shmem_export_ops;
>>   	sb->s_flags |= SB_NOSEC | SB_I_VERSION;
>> +
>> +#if IS_ENABLED(CONFIG_UNICODE)
>> +	if (ctx->encoding) {
>> +		sb->s_encoding = ctx->encoding;
>> +		generic_set_sb_d_ops(sb);
> This is the right place for setting d_ops (see the next comment), but you
> should be loading generic_ci_always_del_dentry_ops, right?
>
> Also, since generic_ci_always_del_dentry_ops is only used by this one,
> can you move it to this file?
>
>> +static struct dentry *shmem_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
>> +{
>> +	const struct dentry_operations *d_ops = &simple_dentry_operations;
>> +
>> +#if IS_ENABLED(CONFIG_UNICODE)
>> +	if (dentry->d_sb->s_encoding)
>> +		d_ops = &generic_ci_always_del_dentry_ops;
>> +#endif
> This needs to be done at mount time through sb->s_d_op. See
>
> https://lore.kernel.org/all/20240221171412.10710-1-krisman@suse.de/
>
> I suppose we can do it at mount-time for
> generic_ci_always_del_dentry_ops and simple_dentry_operations.
>
>> +
>> +	if (dentry->d_name.len > NAME_MAX)
>> +		return ERR_PTR(-ENAMETOOLONG);
>> +
>> +	if (!dentry->d_sb->s_d_op)
>> +		d_set_d_op(dentry, d_ops);
>> +
>> +	/*
>> +	 * For now, VFS can't deal with case-insensitive negative dentries, so
>> +	 * we prevent them from being created
>> +	 */
>> +	if (IS_ENABLED(CONFIG_UNICODE) && IS_CASEFOLDED(dir))
>> +		return NULL;
> Thinking out loud:
>
> I misunderstood always_delete_dentry before.  It removes negative
> dentries right after the lookup, since ->d_delete is called on dput.
>
> But you still need this check here, IMO, to prevent the negative dentry
> from ever being hashed. Otherwise it can be found by a concurrent
> lookup.  And you cannot drop ->d_delete from the case-insensitive
> operations too, because we still wants it for !IS_CASEFOLDED(dir).
>
> The window is that, without this code, the negative dentry dentry would
> be hashed in d_add() and a concurrent lookup might find it between that
> time and the d_put, where it is removed at the end of the concurrent
> lookup.
>
> All of this would hopefully go away with the negative dentry for
> casefolded directories.
>
>> +
>> +	d_add(dentry, NULL);
>> +
>> +	return NULL;
>> +}
> The sole reason you are doing this custom function is to exclude negative
> dentries from casefolded directories. I doubt we care about the extra
> check being done.  Can we just do it in simple_lookup?

So, in summary:

* set d_ops at mount time to generic_ci_always_del_dentry_ops
* use simple_lookup(), get rid of shmem_lookup()
* inside of simple_lookup(), add (IS_CASEFOLDED(dir)) return NULL

Right?

>> +
>>   static const struct inode_operations shmem_dir_inode_operations = {
>>   #ifdef CONFIG_TMPFS
>>   	.getattr	= shmem_getattr,
>>   	.create		= shmem_create,
>> -	.lookup		= simple_lookup,
>> +	.lookup		= shmem_lookup,
>>   	.link		= shmem_link,
>>   	.unlink		= shmem_unlink,
>>   	.symlink	= shmem_symlink,
>> @@ -4791,6 +4923,8 @@ int shmem_init_fs_context(struct fs_context *fc)
>>   	ctx->uid = current_fsuid();
>>   	ctx->gid = current_fsgid();
>>   
>> +	ctx->encoding = NULL;
>> +
>>   	fc->fs_private = ctx;
>>   	fc->ops = &shmem_fs_context_ops;
>>   	return 0;

Powered by blists - more mailing lists