[PATCH 4/5] quota: Move quota syncing to ->sync_fs method

[PATCH 4/5] quota: Move quota syncing to ->sync_fs method

[Jan Kara]

Since the moment writes to quota files are using block device page cache and
space for quota structures is reserved at the moment they are first accessed we
have no reason to sync quota before inode writeback. In fact this order is now
only harmful since quota information can easily change during inode writeback
(either because conversion of delayed-allocated extents or simply because of
allocation of new blocks for simple filesystems not using page_mkwrite).

So move syncing of quota information after writeback of inodes into ->sync_fs
method. This way we do not have to use ->quota_sync callback which is primarily
intended for use by quotactl syscall anyway and we get rid of calling
->sync_fs() twice unnecessarily. We skip quota syncing for OCFS2 since it does
proper quota journalling in all cases (unlike ext3, ext4, and reiserfs which
also support legacy non-journalled quotas) and thus there are no dirty quota
structures.

CC: "Theodore Ts'o" <tytso@mit.edu>
CC: Steven Whitehouse <swhiteho@redhat.com>
CC: Dave Kleikamp <shaggy@kernel.org>
CC: Joel Becker <jlbec@evilplan.org>
CC: reiserfs-devel@vger.kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/super.c     |    6 ++++++
 fs/ext3/super.c     |    5 +++++
 fs/ext4/super.c     |    5 +++++
 fs/gfs2/super.c     |    2 ++
 fs/jfs/super.c      |    5 +++++
 fs/reiserfs/super.c |    5 +++++
 fs/sync.c           |    3 ---
 7 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 1dd62ed..174782f 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -1180,6 +1180,12 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
 	struct ext2_sb_info *sbi = EXT2_SB(sb);
 	struct ext2_super_block *es = EXT2_SB(sb)->s_es;
 
+	/*
+	 * Write quota structures to quota file, sync_blockdev() will write
+	 * them to disk later
+	 */
+	dquot_writeback_dquots(sb, -1);
+
 	spin_lock(&sbi->s_lock);
 	if (es->s_state & cpu_to_le16(EXT2_VALID_FS)) {
 		ext2_debug("setting valid to 0\n");
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index aad153e..a91869c 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -2507,6 +2507,11 @@ static int ext3_sync_fs(struct super_block *sb, int wait)
 {
 	tid_t target;
 
+	/*
+	 * Writeback quota in non-journalled quota case - journalled quota has
+	 * no dirty dquots
+	 */
+	dquot_writeback_dquots(sb, -1);
 	if (journal_start_commit(EXT3_SB(sb)->s_journal, &target)) {
 		if (wait)
 			log_wait_commit(EXT3_SB(sb)->s_journal, target);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 9ea71aa..c81ab5f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4199,6 +4199,11 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
 
 	trace_ext4_sync_fs(sb, wait);
 	flush_workqueue(sbi->dio_unwritten_wq);
+	/*
+	 * Writeback quota in non-journalled quota case - journalled quota has
+	 * no dirty dquots
+	 */
+	dquot_writeback_dquots(sb, -1);
 	if (jbd2_journal_start_commit(sbi->s_journal, &target)) {
 		if (wait)
 			jbd2_log_wait_commit(sbi->s_journal, target);
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index e361610..faf6e83 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -921,6 +921,8 @@ restart:
 static int gfs2_sync_fs(struct super_block *sb, int wait)
 {
 	struct gfs2_sbd *sdp = sb->s_fs_info;
+
+	gfs2_quota_sync(sb, -1);
 	if (wait && sdp)
 		gfs2_log_flush(sdp, NULL);
 	return 0;
diff --git a/fs/jfs/super.c b/fs/jfs/super.c
index 06c8a67..a2c9cc1 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -603,6 +603,11 @@ static int jfs_sync_fs(struct super_block *sb, int wait)
 
 	/* log == NULL indicates read-only mount */
 	if (log) {
+		/*
+		 * Write quota structures to quota file, sync_blockdev() will
+		 * write them to disk later
+		 */
+		dquot_writeback_dquots(sb, -1);
 		jfs_flush_journal(log, wait);
 		jfs_syncpt(log, 0);
 	}
diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
index aa91089..a2bfc97 100644
--- a/fs/reiserfs/super.c
+++ b/fs/reiserfs/super.c
@@ -66,6 +66,11 @@ static int reiserfs_sync_fs(struct super_block *s, int wait)
 {
 	struct reiserfs_transaction_handle th;
 
+	/*
+	 * Writeback quota in non-journalled quota case - journalled quota has
+	 * no dirty dquots
+	 */
+	dquot_writeback_dquots(s, -1);
 	reiserfs_write_lock(s);
 	if (!journal_begin(&th, s, 1))
 		if (!journal_end_sync(&th, s, 1))
diff --git a/fs/sync.c b/fs/sync.c
index 2b7d569..f07f991 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -30,9 +30,6 @@
  */
 static void __sync_filesystem(struct super_block *sb, int wait)
 {
-	if (sb->s_qcop && sb->s_qcop->quota_sync)
-		sb->s_qcop->quota_sync(sb, -1);
-
 	if (wait)
 		sync_inodes_sb(sb);
 	else
-- 
1.7.1

Re: [PATCH 4/5] quota: Move quota syncing to ->sync_fs method

[Steven Whitehouse]

Hi,

On Wed, 2011-07-27 at 00:38 +0200, Jan Kara wrote:
> Since the moment writes to quota files are using block device page cache and
> space for quota structures is reserved at the moment they are first accessed we
> have no reason to sync quota before inode writeback. In fact this order is now
> only harmful since quota information can easily change during inode writeback
> (either because conversion of delayed-allocated extents or simply because of
> allocation of new blocks for simple filesystems not using page_mkwrite).
> 
> So move syncing of quota information after writeback of inodes into ->sync_fs
> method. This way we do not have to use ->quota_sync callback which is primarily
> intended for use by quotactl syscall anyway and we get rid of calling
> ->sync_fs() twice unnecessarily. We skip quota syncing for OCFS2 since it does
> proper quota journalling in all cases (unlike ext3, ext4, and reiserfs which
> also support legacy non-journalled quotas) and thus there are no dirty quota
> structures.
> 
> CC: "Theodore Ts'o" <tytso@mit.edu>
> CC: Steven Whitehouse <swhiteho@redhat.com>
> CC: Dave Kleikamp <shaggy@kernel.org>
> CC: Joel Becker <jlbec@evilplan.org>
> CC: reiserfs-devel@vger.kernel.org
> Signed-off-by: Jan Kara <jack@suse.cz>

Acked-by: Steven Whitehouse <swhiteho@redhat.com>

Steve.

> ---
>  fs/ext2/super.c     |    6 ++++++
>  fs/ext3/super.c     |    5 +++++
>  fs/ext4/super.c     |    5 +++++
>  fs/gfs2/super.c     |    2 ++
>  fs/jfs/super.c      |    5 +++++
>  fs/reiserfs/super.c |    5 +++++
>  fs/sync.c           |    3 ---
>  7 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> index 1dd62ed..174782f 100644
> --- a/fs/ext2/super.c
> +++ b/fs/ext2/super.c
> @@ -1180,6 +1180,12 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
>  	struct ext2_sb_info *sbi = EXT2_SB(sb);
>  	struct ext2_super_block *es = EXT2_SB(sb)->s_es;
>  
> +	/*
> +	 * Write quota structures to quota file, sync_blockdev() will write
> +	 * them to disk later
> +	 */
> +	dquot_writeback_dquots(sb, -1);
> +
>  	spin_lock(&sbi->s_lock);
>  	if (es->s_state & cpu_to_le16(EXT2_VALID_FS)) {
>  		ext2_debug("setting valid to 0\n");
> diff --git a/fs/ext3/super.c b/fs/ext3/super.c
> index aad153e..a91869c 100644
> --- a/fs/ext3/super.c
> +++ b/fs/ext3/super.c
> @@ -2507,6 +2507,11 @@ static int ext3_sync_fs(struct super_block *sb, int wait)
>  {
>  	tid_t target;
>  
> +	/*
> +	 * Writeback quota in non-journalled quota case - journalled quota has
> +	 * no dirty dquots
> +	 */
> +	dquot_writeback_dquots(sb, -1);
>  	if (journal_start_commit(EXT3_SB(sb)->s_journal, &target)) {
>  		if (wait)
>  			log_wait_commit(EXT3_SB(sb)->s_journal, target);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 9ea71aa..c81ab5f 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4199,6 +4199,11 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
>  
>  	trace_ext4_sync_fs(sb, wait);
>  	flush_workqueue(sbi->dio_unwritten_wq);
> +	/*
> +	 * Writeback quota in non-journalled quota case - journalled quota has
> +	 * no dirty dquots
> +	 */
> +	dquot_writeback_dquots(sb, -1);
>  	if (jbd2_journal_start_commit(sbi->s_journal, &target)) {
>  		if (wait)
>  			jbd2_log_wait_commit(sbi->s_journal, target);
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index e361610..faf6e83 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -921,6 +921,8 @@ restart:
>  static int gfs2_sync_fs(struct super_block *sb, int wait)
>  {
>  	struct gfs2_sbd *sdp = sb->s_fs_info;
> +
> +	gfs2_quota_sync(sb, -1);
>  	if (wait && sdp)
>  		gfs2_log_flush(sdp, NULL);
>  	return 0;
> diff --git a/fs/jfs/super.c b/fs/jfs/super.c
> index 06c8a67..a2c9cc1 100644
> --- a/fs/jfs/super.c
> +++ b/fs/jfs/super.c
> @@ -603,6 +603,11 @@ static int jfs_sync_fs(struct super_block *sb, int wait)
>  
>  	/* log == NULL indicates read-only mount */
>  	if (log) {
> +		/*
> +		 * Write quota structures to quota file, sync_blockdev() will
> +		 * write them to disk later
> +		 */
> +		dquot_writeback_dquots(sb, -1);
>  		jfs_flush_journal(log, wait);
>  		jfs_syncpt(log, 0);
>  	}
> diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
> index aa91089..a2bfc97 100644
> --- a/fs/reiserfs/super.c
> +++ b/fs/reiserfs/super.c
> @@ -66,6 +66,11 @@ static int reiserfs_sync_fs(struct super_block *s, int wait)
>  {
>  	struct reiserfs_transaction_handle th;
>  
> +	/*
> +	 * Writeback quota in non-journalled quota case - journalled quota has
> +	 * no dirty dquots
> +	 */
> +	dquot_writeback_dquots(s, -1);
>  	reiserfs_write_lock(s);
>  	if (!journal_begin(&th, s, 1))
>  		if (!journal_end_sync(&th, s, 1))
> diff --git a/fs/sync.c b/fs/sync.c
> index 2b7d569..f07f991 100644
> --- a/fs/sync.c
> +++ b/fs/sync.c
> @@ -30,9 +30,6 @@
>   */
>  static void __sync_filesystem(struct super_block *sb, int wait)
>  {
> -	if (sb->s_qcop && sb->s_qcop->quota_sync)
> -		sb->s_qcop->quota_sync(sb, -1);
> -
>  	if (wait)
>  		sync_inodes_sb(sb);
>  	else

Re: [PATCH 4/5] quota: Move quota syncing to ->sync_fs method

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 4/5] quota: Move quota syncing to ->sync_fs method

[Dave Kleikamp]

On 07/26/2011 05:38 PM, Jan Kara wrote:
> Since the moment writes to quota files are using block device page cache and
> space for quota structures is reserved at the moment they are first accessed we
> have no reason to sync quota before inode writeback. In fact this order is now
> only harmful since quota information can easily change during inode writeback
> (either because conversion of delayed-allocated extents or simply because of
> allocation of new blocks for simple filesystems not using page_mkwrite).
> 
> So move syncing of quota information after writeback of inodes into ->sync_fs
> method. This way we do not have to use ->quota_sync callback which is primarily
> intended for use by quotactl syscall anyway and we get rid of calling
> ->sync_fs() twice unnecessarily. We skip quota syncing for OCFS2 since it does
> proper quota journalling in all cases (unlike ext3, ext4, and reiserfs which
> also support legacy non-journalled quotas) and thus there are no dirty quota
> structures.
> 
> CC: "Theodore Ts'o" <tytso@mit.edu>
> CC: Steven Whitehouse <swhiteho@redhat.com>
> CC: Dave Kleikamp <shaggy@kernel.org>
> CC: Joel Becker <jlbec@evilplan.org>
> CC: reiserfs-devel@vger.kernel.org
> Signed-off-by: Jan Kara <jack@suse.cz>

Acked-by: Dave Kleikamp <dave.kleikamp@oracle.com>

> ---
>  fs/ext2/super.c     |    6 ++++++
>  fs/ext3/super.c     |    5 +++++
>  fs/ext4/super.c     |    5 +++++
>  fs/gfs2/super.c     |    2 ++
>  fs/jfs/super.c      |    5 +++++
>  fs/reiserfs/super.c |    5 +++++
>  fs/sync.c           |    3 ---
>  7 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> index 1dd62ed..174782f 100644
> --- a/fs/ext2/super.c
> +++ b/fs/ext2/super.c
> @@ -1180,6 +1180,12 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
>  	struct ext2_sb_info *sbi = EXT2_SB(sb);
>  	struct ext2_super_block *es = EXT2_SB(sb)->s_es;
>  
> +	/*
> +	 * Write quota structures to quota file, sync_blockdev() will write
> +	 * them to disk later
> +	 */
> +	dquot_writeback_dquots(sb, -1);
> +
>  	spin_lock(&sbi->s_lock);
>  	if (es->s_state & cpu_to_le16(EXT2_VALID_FS)) {
>  		ext2_debug("setting valid to 0\n");
> diff --git a/fs/ext3/super.c b/fs/ext3/super.c
> index aad153e..a91869c 100644
> --- a/fs/ext3/super.c
> +++ b/fs/ext3/super.c
> @@ -2507,6 +2507,11 @@ static int ext3_sync_fs(struct super_block *sb, int wait)
>  {
>  	tid_t target;
>  
> +	/*
> +	 * Writeback quota in non-journalled quota case - journalled quota has
> +	 * no dirty dquots
> +	 */
> +	dquot_writeback_dquots(sb, -1);
>  	if (journal_start_commit(EXT3_SB(sb)->s_journal, &target)) {
>  		if (wait)
>  			log_wait_commit(EXT3_SB(sb)->s_journal, target);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 9ea71aa..c81ab5f 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4199,6 +4199,11 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
>  
>  	trace_ext4_sync_fs(sb, wait);
>  	flush_workqueue(sbi->dio_unwritten_wq);
> +	/*
> +	 * Writeback quota in non-journalled quota case - journalled quota has
> +	 * no dirty dquots
> +	 */
> +	dquot_writeback_dquots(sb, -1);
>  	if (jbd2_journal_start_commit(sbi->s_journal, &target)) {
>  		if (wait)
>  			jbd2_log_wait_commit(sbi->s_journal, target);
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index e361610..faf6e83 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -921,6 +921,8 @@ restart:
>  static int gfs2_sync_fs(struct super_block *sb, int wait)
>  {
>  	struct gfs2_sbd *sdp = sb->s_fs_info;
> +
> +	gfs2_quota_sync(sb, -1);
>  	if (wait && sdp)
>  		gfs2_log_flush(sdp, NULL);
>  	return 0;
> diff --git a/fs/jfs/super.c b/fs/jfs/super.c
> index 06c8a67..a2c9cc1 100644
> --- a/fs/jfs/super.c
> +++ b/fs/jfs/super.c
> @@ -603,6 +603,11 @@ static int jfs_sync_fs(struct super_block *sb, int wait)
>  
>  	/* log == NULL indicates read-only mount */
>  	if (log) {
> +		/*
> +		 * Write quota structures to quota file, sync_blockdev() will
> +		 * write them to disk later
> +		 */
> +		dquot_writeback_dquots(sb, -1);
>  		jfs_flush_journal(log, wait);
>  		jfs_syncpt(log, 0);
>  	}
> diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
> index aa91089..a2bfc97 100644
> --- a/fs/reiserfs/super.c
> +++ b/fs/reiserfs/super.c
> @@ -66,6 +66,11 @@ static int reiserfs_sync_fs(struct super_block *s, int wait)
>  {
>  	struct reiserfs_transaction_handle th;
>  
> +	/*
> +	 * Writeback quota in non-journalled quota case - journalled quota has
> +	 * no dirty dquots
> +	 */
> +	dquot_writeback_dquots(s, -1);
>  	reiserfs_write_lock(s);
>  	if (!journal_begin(&th, s, 1))
>  		if (!journal_end_sync(&th, s, 1))
> diff --git a/fs/sync.c b/fs/sync.c
> index 2b7d569..f07f991 100644
> --- a/fs/sync.c
> +++ b/fs/sync.c
> @@ -30,9 +30,6 @@
>   */
>  static void __sync_filesystem(struct super_block *sb, int wait)
>  {
> -	if (sb->s_qcop && sb->s_qcop->quota_sync)
> -		sb->s_qcop->quota_sync(sb, -1);
> -
>  	if (wait)
>  		sync_inodes_sb(sb);
>  	else