Re: [PATCH] Journalled quota (fwd)

[Andrew Morton]

Jan Kara <jack@xxxxxxx> wrote:
>
>   Here comes journalled quota patch for 2.6.3-rc2.

Could you please document the locking rules?  For example, functions such
as DQUOT_FREE_SPACE_NODIRTY() (and all similar) should have a little
comment above them describing the caller's locking responsibilities.

Because it looks to me like DQUOT_FREE_SPACE_NODIRTY() is supposed to be
called under i_lock, but will call dquot_free_space(), which does
down_read().


I didn't review your changes to the ext3 transaction space reservation
constants.  Did you get them right?  Mistakes here tend to take a long time
to show up.

In ext3_orphan_cleanup():

  - Local variable `i' is unused if !CONFIG_QUOTA and will generate a
    compiler warning.

  - This

	for (i=0; i < MAXQUOTAS; i++)

    introduces coding style inconsistency.  Please do

	for (i = 0; i < MAXQUOTAS; i++)

  - Please edit in an 80-column xterm.  Changes you have made to this
    filesystem are quite infuriating to those who _do_ use 80-cols and need
    to be cleaned up.

  - This

	for (i=0; i < MAXQUOTAS; i++)
		if (EXT3_SB(sb)->s_qf_names[i]) {
			int ret = ext3_quota_on_mount(sb, i);

    introduces coding style inconsistency.  Please do

	for (i=0; i < MAXQUOTAS; i++) {
		if (EXT3_SB(sb)->s_qf_names[i]) {
			int ret = ext3_quota_on_mount(sb, i);

    (several places)


Please document writes_to_blocks()


The locking in v2_commit_dquot() looks fishy.

The locking in dquot_mark_dquot_dirty() and in mark_info_dirty() also look
fishy.  For example:

	void mark_info_dirty(struct super_block *sb, int type)
	{
		spin_lock(&dq_data_lock);
		set_bit(DQF_INFO_DIRTY_B, &sb_dqopt(sb)->info[type].dqi_flags);
		spin_unlock(&dq_data_lock);
	}

what is the spinlock doing there?


I'm not really in a position to review the deadlockiness of this code
without some sort of documentation of the lock ranking (including where
journal_start() sits in that ranking).  Is that something you could add?

Thanks.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/