Message-ID: <4bacbb26-01b4-4d42-b59b-36fc6ff8fc75@samsung.com>
Date: Tue, 2 Sep 2025 14:44:36 +0900
From: Sungjong Seo <sj1557.seo@...sung.com>
To: Ethan Ferguson <ethan.ferguson@...ier.com>
Cc: cpgs@...sung.com, linkinjeon@...nel.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, yuezhang.mo@...y.com, Sungjong Seo
	<sj1557.seo@...sung.com>
Subject: Re: [PATCH v4 0/1] exfat: Add support for FS_IOC_{GET,SET}FSLABEL



On 25. 9. 2. 03:02, Ethan Ferguson wrote:
> On 8/31/25 05:50, Sungjong Seo wrote:
>> Hi,
>>> Add support for reading / writing to the exfat volume label from the
>>> FS_IOC_GETFSLABEL and FS_IOC_SETFSLABEL ioctls
>>>
>>> Signed-off-by: Ethan Ferguson <ethan.ferguson@...ier.com>
>>> ---
>>>  fs/exfat/exfat_fs.h  |   5 +
>>>  fs/exfat/exfat_raw.h |   6 ++
>>>  fs/exfat/file.c      |  88 +++++++++++++++++
>>>  fs/exfat/super.c     | 224 +++++++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 323 insertions(+)
>>>
>>> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
>>> index f8ead4d47ef0..ed4b5ecb952b 100644
>>> --- a/fs/exfat/exfat_fs.h
>>> +++ b/fs/exfat/exfat_fs.h
>>> @@ -267,6 +267,7 @@ struct exfat_sb_info {
>>>  	struct buffer_head **vol_amap; /* allocation bitmap */
>>>
>>>  	unsigned short *vol_utbl; /* upcase table */
>>> +	unsigned short *volume_label; /* volume name */
>> Why is it necessary to allocate and cache it? I didn't find where to reuse
>> it.
>> Is there a reason why uniname is not used directly as an argument?
>> Is there something I missed?
>>
> I thought it might be prudent to store it in the sbi, in case other
> patches in the future might want to use the volume label. However, since
> this is not necessary, I can remove it if needed.
> 
> I chose not to use a uniname, as that would require allocating lots of
> data, when the maximum amount of bytes present in a volume label is 22.
> 

What I meant was that since uniname is already assigned within the new
ioctl, I think we can use it right away. Anyway, this variable in
sbi right now seems unnecessary.

>>>
>>>  	unsigned int clu_srch_ptr; /* cluster search pointer */
>>>  	unsigned int used_clusters; /* number of used clusters */
>>> @@ -431,6 +432,10 @@ static inline loff_t exfat_ondisk_size(const struct
>>> inode *inode)
[snip[
>>> diff --git a/fs/exfat/file.c b/fs/exfat/file.c
>>> index 538d2b6ac2ec..970e3ee57c43 100644
>>> --- a/fs/exfat/file.c
>>> +++ b/fs/exfat/file.c
>>> @@ -12,6 +12,7 @@
>>>  #include <linux/security.h>
>>>  #include <linux/msdos_fs.h>
>>>  #include <linux/writeback.h>
>>> +#include "../nls/nls_ucs2_utils.h"
>>>
>>>  #include "exfat_raw.h"
>>>  #include "exfat_fs.h"
>>> @@ -486,10 +487,93 @@ static int exfat_ioctl_shutdown(struct super_block
>>> *sb, unsigned long arg)
>>>  	return exfat_force_shutdown(sb, flags);
>>>  }
>>>
>>> +static int exfat_ioctl_get_volume_label(struct super_block *sb, unsigned
>>> long arg)
>>> +{
>>> +	int ret;
>>> +	char utf8[FSLABEL_MAX] = {0};
>>> +	struct exfat_uni_name *uniname;
>>> +	struct exfat_sb_info *sbi = EXFAT_SB(sb);
>>> +
>>> +	uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
>>> +	if (!uniname)
>>> +		return -ENOMEM;
>>> +
>>> +	ret = exfat_read_volume_label(sb);
>>> +	if (ret < 0)
>>> +		goto cleanup;
>>> +
>>> +	memcpy(uniname->name, sbi->volume_label,
>>> +	       EXFAT_VOLUME_LABEL_LEN * sizeof(short));
>>> +	uniname->name[EXFAT_VOLUME_LABEL_LEN] = 0x0000;
>>> +	uniname->name_len = UniStrnlen(uniname->name,
>>> EXFAT_VOLUME_LABEL_LEN);
>> The volume label length is stored on-disk. It makes sense to retrieve
>> it directly. This way, there is no need to unnecessarily include the 
>> NLS utility header file.
>>
> That's true, given I'll be removing the volume_label field from the
> superblock info struct, I can rework this function to output to a
> uniname struct.
>>> +
>>> +	ret = exfat_utf16_to_nls(sb, uniname, utf8, FSLABEL_MAX);
>>> +	if (ret < 0)
>>> +		goto cleanup;
>>> +
>>> +	if (copy_to_user((char __user *)arg, utf8, FSLABEL_MAX)) {
>>> +		ret = -EFAULT;
>>> +		goto cleanup;
>>> +	}
>>> +
>>> +	ret = 0;
>>> +
>>> +cleanup:
>>> +	kfree(uniname);
>>> +	return ret;
>>> +}
>>> +
>>> +static int exfat_ioctl_set_volume_label(struct super_block *sb,
>>> +					unsigned long arg,
>>> +					struct inode *root_inode)
>>> +{
>>> +	int ret, lossy;
>>> +	char utf8[FSLABEL_MAX];
>>> +	struct exfat_uni_name *uniname;
>>> +
>>> +	if (!capable(CAP_SYS_ADMIN))
>>> +		return -EPERM;
>>> +
>>> +	uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
>>> +	if (!uniname)
>>> +		return -ENOMEM;
>>> +
>>> +	if (copy_from_user(utf8, (char __user *)arg, FSLABEL_MAX)) {
>>> +		ret = -EFAULT;
>>> +		goto cleanup;
>>> +	}
>>> +
>>> +	if (utf8[0]) {
>>> +		ret = exfat_nls_to_utf16(sb, utf8, strnlen(utf8,
>>> FSLABEL_MAX),
>>> +					 uniname, &lossy);
>>> +		if (ret < 0)
>>> +			goto cleanup;
>>> +		else if (lossy & NLS_NAME_LOSSY) {
>>> +			ret = -EINVAL;
>>> +			goto cleanup;
>>> +		}
>>> +	} else {
>>> +		uniname->name[0] = 0x0000;
>>> +		uniname->name_len = 0;
>>> +	}
>>> +
>>> +	if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) {
>>> +		exfat_info(sb, "Volume label length too long, truncating");
>>> +		uniname->name_len = EXFAT_VOLUME_LABEL_LEN;
>>> +	}
>>> +
>>> +	ret = exfat_write_volume_label(sb, uniname, root_inode);
>>> +
>>> +cleanup:
>>> +	kfree(uniname);
>>> +	return ret;
>>> +}
>>> +
>>>  long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>>  {
>>>  	struct inode *inode = file_inode(filp);
>>>  	u32 __user *user_attr = (u32 __user *)arg;
>>> +	struct inode *root_inode = filp->f_path.mnt->mnt_root->d_inode;
>> From this point, there is no need to pass root_inode. The root_inode can be
>> obtained directly from sb->s_root->d_inode within the function.
>>
> Must have missed that, thank you!
>>>
>>>  	switch (cmd) {
>>>  	case FAT_IOCTL_GET_ATTRIBUTES:
>>> @@ -500,6 +584,10 @@ long exfat_ioctl(struct file *filp, unsigned int cmd,
>>> unsigned long arg)
>>>  		return exfat_ioctl_shutdown(inode->i_sb, arg);
>>>  	case FITRIM:
>>>  		return exfat_ioctl_fitrim(inode, arg);
>>> +	case FS_IOC_GETFSLABEL:
>>> +		return exfat_ioctl_get_volume_label(inode->i_sb, arg);
>>> +	case FS_IOC_SETFSLABEL:
>>> +		return exfat_ioctl_set_volume_label(inode->i_sb, arg,
>>> root_inode);
>>>  	default:
>>>  		return -ENOTTY;
>>>  	}
>>> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
>>> index 8926e63f5bb7..7931cdb4a1d1 100644
>>> --- a/fs/exfat/super.c
>>> +++ b/fs/exfat/super.c
>>> @@ -18,6 +18,7 @@
>>>  #include <linux/nls.h>
>>>  #include <linux/buffer_head.h>
>>>  #include <linux/magic.h>
>>> +#include "../nls/nls_ucs2_utils.h"
>>>
>>>  #include "exfat_raw.h"
>>>  #include "exfat_fs.h"
>>> @@ -573,6 +574,228 @@ static int exfat_verify_boot_region(struct
>>> super_block *sb)
>>>  	return 0;
>>>  }
>>>
>>> +static int exfat_get_volume_label_ptrs(struct super_block *sb,
>>> +				       struct buffer_head **out_bh,
>>> +				       struct exfat_dentry **out_dentry,
>>> +				       struct inode *root_inode)
>> Instead of passing root_inode, it seems more helpful to pass the
>> need_create condition to better understand the function's behavior.
>> As mentioned earlier, the root_inode can be found directly from
>> sb->s_root->d_inode.
>>
> Given I can now obtain the root inode due to your above suggestion, I
> can use need_create from my previous patch (v3).
>>> +{
>>> +	int i, ret;
>>> +	unsigned int type, old_clu;
>>> +	struct exfat_sb_info *sbi = EXFAT_SB(sb);
>>> +	struct exfat_chain clu;
>>> +	struct exfat_dentry *ep, *deleted_ep = NULL;
>>> +	struct buffer_head *bh, *deleted_bh;
>>> +
>>> +	clu.dir = sbi->root_dir;
>>> +	clu.flags = ALLOC_FAT_CHAIN;
>>> +
>>> +	while (clu.dir != EXFAT_EOF_CLUSTER) {
>>> +		for (i = 0; i < sbi->dentries_per_clu; i++) {
>>> +			ep = exfat_get_dentry(sb, &clu, i, &bh);
>>> +
>>> +			if (!ep) {
>>> +				ret = -EIO;
>>> +				goto end;
>>> +			}
>>> +
>>> +			type = exfat_get_entry_type(ep);
>>> +			if (type == TYPE_DELETED && !deleted_ep &&
>> root_inode)
>>> {
>>> +				deleted_ep = ep;
>>> +				deleted_bh = bh;
>>> +				continue;
>>> +			}
>>> +
>>> +			if (type == TYPE_UNUSED) {
>>> +				if (!root_inode) {
>>> +					brelse(bh);
>>> +					ret = -ENOENT;
>>> +					goto end;
>>> +				}
>> Too many unnecessary operations are being performed here.
>> 1. Since the VOLUME_LABEL entry requires only one empty entry, if a DELETED
>>     or UNUSED entry is found, it can be used directly.
>> 2. According to the exFAT specification, all entries after UNUSED are
>>     guaranteed to be UNUSED.
>>
>> Therefore, there is no need to allocate additional clusters or
>> mark the next entry as UNUSED here.
>>
>> In the case of need_create(as of now, root_inode is not null),
>> if the next cluster is EOF and TYPE_VOLUME, TYPE_DELETED, or TYPE_UNUSED
>> entries are not found, then a new cluster should be allocated.
>>
>> Lastly, if a new VOLUME_LABEL entry is created, initialization of
>> hint_femp is required.
>>
> Just so I'm sure, if the last dentry in a cluster is TYPE_UNUSED, we can
> safely overwrite that with the volume label (assuming no other
> TYPE_VOLUME or TYPE_DELETED have been found previously), and we don't
> have to allocate a new cluster? And we would only have to allocate a
> new cluster if there are no TYPE_UNUSED, TYPE_VOLUME, or TYPE_DELETED
> anywhere in the chain?
Yes, that's right. If the root directory has an empty entry to use as
a volume label, no additional cluster is required at this point.

> 
> As for hint_femp, should I set the root_inode's hint_femp field to
> EXFAT_HINT_NONE?
I think so.

> Thanks you for the sugestions,
> Ethan Ferguson

Thanks!

Powered by blists - more mailing lists