Thanks for the feedback. I will address all the comments in next revision.
Some comments inline:
On Tue, Nov 8, 2011 at 5:55 AM, Jan Kara <jack@xxxxxxx> wrote:
> Thanks for the patch. I did a detailed review of it. I have quite some
> comments but all of them are easily fixable so I think we should be mostly
> ready in the next iteration.
>
> On Thu 03-11-11 19:15:20, Aditya Kali wrote:
>> +static const struct quotactl_ops ext4_new_qctl_operations = {
> ext4_new_qctl_operations is rather poor name. What if you need to create
> even newer set of operations in future? So I'd rather name them by "what
> they do" - so something like ext4_sysfile_qctl_operations - meaning that these
> operations handle quota stored in hidden system files.
>
>> + .quota_on_meta = ext4_quota_on_meta,
>> + .quota_off = ext4_quota_off,
>> + .quota_sync = dquot_quota_sync,
>> + .get_info = dquot_get_dqinfo,
>> + .set_info = dquot_set_dqinfo,
>> + .get_dqblk = dquot_get_dqblk,
>> + .set_dqblk = dquot_set_dqblk
> ..
>> @@ -3610,6 +3623,15 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>> #ifdef CONFIG_QUOTA
>> sb->s_qcop = &ext4_qctl_operations;
>> sb->dq_op = &ext4_quota_operations;
>> +
>> + if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_QUOTA)) {
>> + /* Use new qctl operations with quota on function that does not
>> + * require user specified quota file path. */
>> + sb->s_qcop = &ext4_new_qctl_operations;
>> +
>> + sbi->s_qf_inums[USRQUOTA] = es->s_usr_quota_inum;
>> + sbi->s_qf_inums[GRPQUOTA] = es->s_grp_quota_inum;
> You need to convert from ondisk format here - i.e. use le32_to_cpu().
>
>> + }
>> #endif
>> memcpy(sb->s_uuid, es->s_uuid, sizeof(es->s_uuid));
>>
>> @@ -3815,8 +3837,21 @@ no_journal:
>> } else
>> descr = "out journal";
>>
>> - ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. "
>> - "Opts: %s%s%s", descr, sbi->s_es->s_mount_opts,
>> +#ifdef CONFIG_QUOTA
>> + /* Enable quota usage during mount. */
>> + if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_QUOTA) &&
>> + !(sb->s_flags & MS_RDONLY)) {
>> + ext4_quota_enable(sb, USRQUOTA, QFMT_VFS_V1,
>> + DQUOT_USAGE_ENABLED);
>> + ext4_quota_enable(sb, GRPQUOTA, QFMT_VFS_V1,
>> + DQUOT_USAGE_ENABLED);
>> + }
> You should check the return value here and fail the mount in case turning
> quotas on fails... Also you should set DQUOT_QUOTA_SYS_FILE in
> sb_dqopt(sb)->flags before calling ext4_quota_enable() to indicate that
> quota file is a hidden system file and generic quota code can then skip
> some rather costly steps during quota handling to keep user visible and
> kernel visible content in sync.
>
>
>> +#endif /* CONFIG_QUOTA */
>> +
>> + ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. quota=%s. "
>> + "Opts: %s%s%s", descr,
>> + sb_any_quota_loaded(sb) ? "on" : "off",
>> + sbi->s_es->s_mount_opts,
>> *sbi->s_es->s_mount_opts ? "; " : "", orig_data);
> Should info about quotas really be in the message? After all it's just
> another feature and we definitely don't report all the features filesystem
> has set.
>
>> @@ -4183,6 +4218,12 @@ static int ext4_commit_super(struct super_block *sb, int sync)
>> es->s_free_inodes_count =
>> cpu_to_le32(percpu_counter_sum_positive(
>> &EXT4_SB(sb)->s_freeinodes_counter));
>> +#ifdef CONFIG_QUOTA
>> + if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_QUOTA)) {
>> + es->s_usr_quota_inum = EXT4_SB(sb)->s_qf_inums[USRQUOTA];
>> + es->s_grp_quota_inum = EXT4_SB(sb)->s_qf_inums[GRPQUOTA];
>> + }
>> +#endif
> Is there really need to reset these numbers? They should be stable
> during the whole lifetime of a filesystem, no (modulo changes by tune2fs)?
> If I'm missing something and numbers indeed can change, you should convert
> them to on disk format - i.e. cpu_to_le32().
>
Currently tune2fs does not support enabling quota feature on mounted
filesystem. (I think we should probably make it enable quotas on a
read-only mount though). Even in that case, this is incorrect. I will
remove this.
>> @@ -4814,6 +4855,59 @@ static int ext4_quota_on(struct super_block *sb, int type, int format_id,
>> return dquot_quota_on(sb, type, format_id, path);
>> }
>>
>> +static int ext4_quota_enable(struct super_block *sb, int type, int format_id,
>> + unsigned int flags)
>> +{
>> + struct ext4_sb_info *sbi = EXT4_SB(sb);
>> + struct inode *qf_inode;
>> + int err;
>> +
>> + if (!sbi->s_qf_inums[type])
>> + return -EPERM;
>> +
>> + qf_inode = ext4_iget(sb, sbi->s_qf_inums[type]);
>> + if (IS_ERR(qf_inode)) {
>> + ext4_error(sb, "Bad quota inode # %lu", sbi->s_qf_inums[type]);
>> + return PTR_ERR(qf_inode);
>> + }
>> +
>> + /*
>> + * When we journal data on quota file, we have to flush journal to see
>> + * all updates to the file when we bypass pagecache...
>> + */
>> + if (sbi->s_journal &&
>> + ext4_should_journal_data(qf_inode)) {
>> + /*
>> + * We don't need to lock updates but journal_flush() could
>> + * otherwise be livelocked...
>> + */
>> + jbd2_journal_lock_updates(sbi->s_journal);
>> + err = jbd2_journal_flush(sbi->s_journal);
>> + jbd2_journal_unlock_updates(sbi->s_journal);
>> + if (err)
>> + return err;
>> + }
> Why do you have the above check? Since this function is called only for
> reserved quota inodes which are hidden, ext4_should_journal_data() will
> never be true for them.
>
This inode could refer to a user-visible file if existing quota file
was used by tune2fs. We will need to remove the journal flag on the
inode if present at tune2fs time.
>> +
>> + err = dquot_enable(qf_inode, type, format_id, flags);
>> + iput(qf_inode);
>> +
>> + return err;
>> +}
>> @@ -4837,7 +4931,12 @@ static int ext4_quota_off(struct super_block *sb, int type)
>> ext4_journal_stop(handle);
>>
>> out:
>> - return dquot_quota_off(sb, type);
>> + /* Disable only the limits if QUOTA feature is set. */
>> + if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_QUOTA))
>> + return dquot_disable(sb, type, DQUOT_LIMITS_ENABLED);
>> + else
>> + return dquot_disable(sb, type,
>> + DQUOT_USAGE_ENABLED | DQUOT_LIMITS_ENABLED);
>> }
> Hmm, most of the work ext4_quota_off() does is unnecessary when quota is
> handled internally - there's no need to sync the filesystem, no need to
> update mtime or ctime... So I'd rather provide a new ext4_quota_off()
> function in case quota feature is enabled and that would just call
> dquot_disable(sb, type, DQUOT_LIMITS_ENABLED).
>
> Honza
> --
> Jan Kara <jack@xxxxxxx>
> SUSE Labs, CR
>
--
Aditya
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
