Re: [PATCH] ext4: Always journal quota file modifications

[Dmitry Monakhov <dmonakhov@openvz.org>]

tytso@mit.edu writes:

> On Wed, Jun 02, 2010 at 04:23:13PM +0200, Jan Kara wrote:
>> When journaled quota options are not specified, we do writes
>> to quota files just in data=ordered mode. This actually causes
>> warnings from JBD2 about dirty journaled buffer because ext4_getblk
>> unconditionally treats a block allocated by it as metadata. Since
>> quota actually is filesystem metadata, the easiest way to get rid
>> of the warning is to always treat quota writes as metadata...
>> 
>> Signed-off-by: Jan Kara <jack@suse.cz>
>
> I'm worried about this patch in the short-term.  In the long-term I
> think the quota file should become a special file much like the
> journal, and then this makes a huge amount of sense.  But I worry
> about what might happen if (a) someone tries writing to the quota file
> directly from userspace, maybe right before quota is enabled (and
> before delayed allocation writes complete, such that some writes are
> happening via the journal in ext4_quota_write and some w/o going
> through the journal in ext4_writepage), and (b) what happens if quota
> is disabled, the quota file is deleted, and some blocks get reused ---
> and then system crashes before a journal commit can happen.
>
> All of these problems go away if the quota file isn't visible from
> userspace, and it becomes a special file.  In the short term I think
> we could make this change, but I think we would also have to (1) treat
> the quota file as immutable while quotas are enabled (so it cannot be
> opened for writing), (2) force an fsync of the quota file and a
> journal commit before enabling quotas, and (3) force a journal commit
> after disabling quotas.
3'rd is already true
int vfs_quota_disable(...)
{...
        /* Sync the superblock so that buffers with quota data are written to
         * disk (and so userspace sees correct data afterwards). */
        if (sb->s_op->sync_fs)
                sb->s_op->sync_fs(sb, 1);
        sync_blockdev(sb->s_bdev);
        /* Now the quota files are just ordinary files and we can set the
         * inode flags back. Moreover we discard the pagecache so that
...}
>
> The other thing we might try that might mostly fix things is to change
> ext4_should_journal_data() in ext4_jbd2.h to return true if it's a
> quota file --- but we don't know which files are the quota files when
> quotas are disabled, so we would still need to do (2) and (3).  But
> this would allow us to write to the quota file while quotas are
> enabled, if we think this is necessary --- although I think it's a bad
> idea, so I'd be in favor of simply not allowing quota files to be
> writable from userspace while quotas are enabled.  Jan, is this going
> to cause any problems with quotautils?
>
> OTOH, I think we have similar races with journaled quotas, and no one
> has complained (although the vast majority of the quota documentation
> on various HOWTO pages still don't talk about journaled quotas, so I
> don't know how many people are using journaled quotas.  :-/ )
>
>>   Ted, this patch fixes some JBD2 warning for me when running XFSQA
>> with quotas enabled. I think this is a move into a direction you are
>> trying to achieve as well. Will you merge the patch or should I do it?
>
> I'm happy to carry the patch, since I Have Plans to try to make quotas
> be a first class supported filesystem feature (i.e., make the quota
> file a special file, and make quota files be always journaled if they
> are journaled, and make the !@#! magic quota options handling in
> /proc/mounts go away) in the 2.6.36 timeframe.
>
> So the question is should we try to merge something like this for
> 2.6.35 or 2.6.35.y, and if so, how much bullet-proofing do we feel is
> necessary for some of these races that I've outlined above.
>
> 	      	      	    	       	    - Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html