On 5/8/25 23:52, Eric Sandeen wrote:
> 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;
> }
Yes, I noticed such inconsistency, and I'm wondering why we handle ctx.s_qf_names
w/ different ways.
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;
}
For "quota_feature is on" case, as opt.s_qf_names is NULL, so if it doesn't
nully ctx.s_qf_names, it will fail below check which is not as expected. So
I doubt it should be handled separately.
/* Make sure we don't mix old and new quota format */
usr_qf_name = F2FS_OPTION(sbi).s_qf_names[USRQUOTA] ||
F2FS_CTX_INFO(ctx).s_qf_names[USRQUOTA];
grp_qf_name = F2FS_OPTION(sbi).s_qf_names[GRPQUOTA] ||
F2FS_CTX_INFO(ctx).s_qf_names[GRPQUOTA];
prj_qf_name = F2FS_OPTION(sbi).s_qf_names[PRJQUOTA] ||
F2FS_CTX_INFO(ctx).s_qf_names[PRJQUOTA];
usrquota = test_opt(sbi, USRQUOTA) ||
ctx_test_opt(ctx, F2FS_MOUNT_USRQUOTA);
grpquota = test_opt(sbi, GRPQUOTA) ||
ctx_test_opt(ctx, F2FS_MOUNT_GRPQUOTA);
prjquota = test_opt(sbi, PRJQUOTA) ||
ctx_test_opt(ctx, F2FS_MOUNT_PRJQUOTA);
if (usr_qf_name) {
ctx_clear_opt(ctx, F2FS_MOUNT_USRQUOTA);
usrquota = false;
}
if (grp_qf_name) {
ctx_clear_opt(ctx, F2FS_MOUNT_GRPQUOTA);
grpquota = false;
}
if (prj_qf_name) {
ctx_clear_opt(ctx, F2FS_MOUNT_PRJQUOTA);
prjquota = false;
}
if (usr_qf_name || grp_qf_name || prj_qf_name) {
if (grpquota || usrquota || prjquota) {
f2fs_err(sbi, "old and new quota format mixing");
return -EINVAL;
}
if (!(ctx->spec_mask & F2FS_SPEC_jqfmt ||
F2FS_OPTION(sbi).s_jquota_fmt)) {
f2fs_err(sbi, "journaled quota format not specified");
return -EINVAL;
}
}
>
> 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.
Maybe:
f2fs_err(sbi, "new noextensions conflicts w/ new extensions");
>
>>> + 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");
f2fs_err(sbi, "new noextensions conflicts w/ old extensions");
>>
>> 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");
f2fs_err(sbi, "new extensions conflicts w/ old noextensions");
Then, user may get enough hint from log to update conflicted {no,}extensions
for mount.
Thanks,
>>
>> Ditto,
>
> thanks,
> -Eric
>
