Re: [PATCH v2 6/8] tmpfs: Add flag FS_CASEFOLD_FL support for tmpfs dirs

[André Almeida]

Hi Krisman,

Thanks for the feedback!

Em 03/09/2024 13:15, Gabriel Krisman Bertazi escreveu:
> André Almeida <andrealmeid@xxxxxxxxxx> writes:
>
> > Enable setting flag FS_CASEFOLD_FL for tmpfs directories, when tmpfs is
> > mounted with casefold support. A special check is need for this flag,
> > since it can't be set for non-empty directories.
> >
> > Signed-off-by: André Almeida <andrealmeid@xxxxxxxxxx>

[...]

> > +
> > +	if (fsflags & FS_CASEFOLD_FL) {
> > +		if (!sb->s_encoding)
> > +			return -EOPNOTSUPP;
> > +
> > +		if (!S_ISDIR(inode->i_mode))
> > +			return -ENOTDIR;
> > +
> > +		if (dentry && !simple_empty(dentry))
> > +			return -ENOTEMPTY;
> > +
> > +		i_flags |= S_CASEFOLD;
> > +	} else if (old & S_CASEFOLD) {
> > +		if (dentry && !simple_empty(dentry))
> > +			return -ENOTEMPTY;
>
> We don't want to fail if a directory already has the S_CASEFOLD
> flag and we are not flipping it in the current operation.  Something like:
>
> if ((fsflags ^ old) & S_CASEFOLD) {
> 	if (!sb->s_encoding)
> 		return -EOPNOTSUPP;
>
> 	if (!S_ISDIR(inode->i_mode))
> 		return -ENOTDIR;
>
> 	if (dentry && !simple_empty(dentry))
> 		return -ENOTEMPTY;
>          i_flags |= fsflags & S_CASEFOLD;
> }

You are right, it's broken and failing for directories with S_CASEFOLD. 
Here's a small test showing that we can't add the +d attribute to a 
non-empty CI folder (+d doesn't require the directory to be empty):

folder ) mkdir A
folder ) mkdir A/B
folder ) chattr +d A/B
folder ) chattr +d A
chattr: Directory not empty while setting flags on A

However, FS_CASEFOLD_FL != S_CASEFOLD and the set of values for 
inode->i_flags (var old) and fsflags aren't the same, so your proposed 
snippet didn't work. I see that ext4 has a very similar code as your 
proposal, but I think they do something different with the flag values.

I rewrote my code separating the three possible paths and it worked:

/* inheritance from parent dir/keeping the same flags path */
if ((fsflags & FS_CASEFOLD_FL) && (old & S_CASEFOLD))
	i_flags |= S_CASEFOLD;

/* removing flag path */
if (!(fsflags & FS_CASEFOLD_FL) && (old & S_CASEFOLD))
	if (dentry && !simple_empty(dentry))
		return -ENOTEMPTY;

/* adding flag path */
if ((fsflags & FS_CASEFOLD_FL) && !(old & S_CASEFOLD)) {
	if (!sb->s_encoding)
		return -EOPNOTSUPP;

	if (!S_ISDIR(inode->i_mode))
		return -ENOTDIR;

	if (dentry && !simple_empty(dentry))
		return -ENOTEMPTY;

	i_flags |= S_CASEFOLD;
}

In that way, the `chattr +d` call doesn't fall into the simple_empty() 
check. I simplified the code like this for the v3:

if (fsflags & FS_CASEFOLD_FL) {
	if (!(old & S_CASEFOLD)) {
		if (!sb->s_encoding)
			return -EOPNOTSUPP;

		if (!S_ISDIR(inode->i_mode))
			return -ENOTDIR;

		if (dentry && !simple_empty(dentry))
			return -ENOTEMPTY;
	}

	i_flags |= S_CASEFOLD;
} else if (old & S_CASEFOLD) {
	if (dentry && !simple_empty(dentry))
		return -ENOTEMPTY;
}