On 2025/5/12 11:32, Chao Yu wrote:
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 <lihongb...@huawei.com>
...
+ 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");
Yeah, this makes it clearer. And don't think anyone would use the old
log as the criterion.
Thanks,
Hongbo
Then, user may get enough hint from log to update conflicted {no,}extensions
for mount.
Thanks,
Ditto,
thanks,
-Eric
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
