On 2025/5/8 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;
}
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.
ah, I kept it with the older format. And they should check the unmatched
conflict between ctx and sbi.
Thanks,
Hongbo
+ 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
