On 5/8/25 3:13 AM, Chao Yu wrote:
> On 4/24/25 01:08, Eric Sandeen wrote:
>> From: Hongbo Li <lihongbo22@xxxxxxxxxx>
...
>> + if (ctx->qname_mask) {
>> + for (i = 0; i < MAXQUOTAS; i++) {
>> + if (!(ctx->qname_mask & (1 << i)))
>> + continue;
>> +
>> + old_qname = F2FS_OPTION(sbi).s_qf_names[i];
>> + new_qname = F2FS_CTX_INFO(ctx).s_qf_names[i];
>> + if (quota_turnon &&
>> + !!old_qname != !!new_qname)
>> + goto err_jquota_change;
>> +
>> + if (old_qname) {
>> + if (strcmp(old_qname, new_qname) == 0) {
>> + ctx->qname_mask &= ~(1 << i);
>
> Needs to free and nully F2FS_CTX_INFO(ctx).s_qf_names[i]?
>
I will have to look into this. If s_qf_names are used/applied, they get
transferred to the sbi in f2fs_apply_quota_options and are freed in the
normal course of the fiesystem lifetime, i.e at unmount in f2fs_put_super.
That's the normal non-error lifecycle of the strings.
If they do not get transferred to the sbi in f2fs_apply_quota_options, they
remain on the ctx, and should get freed in f2fs_fc_free:
for (i = 0; i < MAXQUOTAS; i++)
kfree(F2FS_CTX_INFO(ctx).s_qf_names[i]);
It is possible to free them before f2fs_fc_free of course and that might
be an inconsistency in this function, because we do that in the other case
in the check_consistency function:
if (quota_feature) {
f2fs_info(sbi, "QUOTA feature is enabled, so ignore qf_name");
ctx->qname_mask &= ~(1 << i);
kfree(F2FS_CTX_INFO(ctx).s_qf_names[i]);
F2FS_CTX_INFO(ctx).s_qf_names[i] = NULL;
}
I'll have to look at it a bit more. But this is modeled on ext4's
ext4_check_quota_consistency(), and it does not do any freeing in that
function; it leaves freeing in error cases to when the fc / ctx gets freed.
But tl;dr: I think we can remove the kfree and "= NULL" in this function,
and defer the freeing in the error case.
>> +
>> +static inline void clear_compression_spec(struct f2fs_fs_context *ctx)
>> +{
>> + ctx->spec_mask &= ~(F2FS_SPEC_compress_algorithm
>> + | F2FS_SPEC_compress_log_size
>> + | F2FS_SPEC_compress_extension
>> + | F2FS_SPEC_nocompress_extension
>> + | F2FS_SPEC_compress_chksum
>> + | F2FS_SPEC_compress_mode);
>
> How about add a macro to include all compression macros, and use it to clean
> up above codes?
That's a good idea and probably easy enough to do without rebase pain.
>> +
>> + if (f2fs_test_compress_extension(F2FS_CTX_INFO(ctx).noextensions,
>> + F2FS_CTX_INFO(ctx).nocompress_ext_cnt,
>> + F2FS_CTX_INFO(ctx).extensions,
>> + F2FS_CTX_INFO(ctx).compress_ext_cnt)) {
>> + f2fs_err(sbi, "invalid compress or nocompress extension");
>
> Can you please describe what is detailed confliction in the log? e.g. new
> noext conflicts w/ new ext...
Hmm, let me think about this. I had not noticed it was calling
f2fs_test_compress_extension 3 times, I wonder if there is a better option.
I need to understand this approach better. Maybe Hongbo has thoughts.
>> + return -EINVAL;
>> + }
>> + if (f2fs_test_compress_extension(F2FS_CTX_INFO(ctx).noextensions,
>> + F2FS_CTX_INFO(ctx).nocompress_ext_cnt,
>> + F2FS_OPTION(sbi).extensions,
>> + F2FS_OPTION(sbi).compress_ext_cnt)) {
>> + f2fs_err(sbi, "invalid compress or nocompress extension");
>
> Ditto,
>
>> + return -EINVAL;
>> + }
>> + if (f2fs_test_compress_extension(F2FS_OPTION(sbi).noextensions,
>> + F2FS_OPTION(sbi).nocompress_ext_cnt,
>> + F2FS_CTX_INFO(ctx).extensions,
>> + F2FS_CTX_INFO(ctx).compress_ext_cnt)) {
>> + f2fs_err(sbi, "invalid compress or nocompress extension");
>
> Ditto,
thanks,
-Eric
