[PATCH 0/8 RESEND] Cleanup and improve sync (v4)

[PATCH 0/8 RESEND] Cleanup and improve sync (v4)

[Jan Kara]

  Hello,

  this is a fourth iteration of my series improving handling of sync syscall.
Since previous submission I have slightly improved cleaned up iteration loops
so that we don't have to pass void * around. Christoph also asked about why
we do non-blocking ->sync_fs() pass. My answer to it was:

I did also measurements where non-blocking ->sync_fs was removed and I didn't
see any regression with ext3, ext4, xfs, or btrfs. OTOH I can imagine *some*
filesystem can do an equivalent of filemap_fdatawrite() on some metadata for
non-blocking ->sync_fs and filemap_fdatawrite_and_wait() on the blocking one
and if there are more such filesystems on different backing storages the
performance difference can be noticeable (actually, checking the filesystems,
JFS and Ceph seem to be doing something like this). So I that's why I didn't
include the change in the end...

So Christoph, if you think we should get rid of non-blocking ->sync_fs, I can
include the patch but personally I think it has some use. Arguably a cleaner
interface for the users will be something like two methods ->sync_fs_begin
and ->sync_fs_end. Filesystems that don't have much to optimize in ->sync_fs()
would just use one of these functions.

I have run three tests below to verify performance impact of the patch series.
Each test has been run with 1, 2, and 4 filesystems mounted; test with 2
filesystems was run with each filesystem on a different disk, test with 4
filesystems had 2 filesystems on the first disk and 2 filesystems on the second
disk.

Test 1: Run 200 times sync with filesystem mounted to verify overhead of
  sync when there are no data to write.
Test 2: For each filesystem run a process creating 40 KB files, sleep
  for 3 seconds, run sync.
Test 3: For each filesystem run a process creating 20 GB file, sleep for
  5 seconds, run sync.

I have performed 10 runs of each test for xfs, ext3, ext4, and btrfs
filesystems.

Results of test 1
-----------------
Numbers are time it took 200 syncs to complete.
Character in braces is + if the time increased with 2*STDDEV reliability,
- if it decreased with 2*STDDEV reliability, 0 otherwise.
		      BASE		      PATCHED
FS		AVG      STDDEV         AVG      STDDEV
xfs, 1 disks	4.189300 0.051525	2.141300 0.063389 (-)
xfs, 2 disks	4.820600 0.019096	4.611400 0.066322 (-)
xfs, 4 disks	6.518300 1.440362	6.435700 0.510641 (0)
ext4, 1 disks	4.085000 0.011375	1.689500 0.001360 (-)
ext4, 2 disks	4.088100 0.006488	1.705000 0.026359 (-)
ext4, 4 disks	4.107300 0.011934	1.702900 0.001814 (-)
ext3, 1 disks	4.080200 0.009527	1.703400 0.030559 (-)
ext3, 2 disks	4.138300 0.143909	1.694000 0.001414 (-)
ext3, 4 disks	4.107200 0.002482	1.702900 0.007778 (-)
btrfs, 1 disks	11.214600 0.086619	8.737200 0.081076 (-)
btrfs, 2 disks	32.910000 0.162089	30.673400 0.538820 (-)
btrfs, 4 disks	67.987700 1.655654	67.247100 1.971887 (0)

So we see nice improvements almost all over the board.

Results of test 2
-----------------
Numbers are time it took sync to complete.

		    BASE		    PATCHED
FS		AVG      STDDEV         AVG      STDDEV
xfs, 1 disks	0.436000 0.012000	0.506000 0.014283 (+)
xfs, 2 disks	1.105000 0.055543	1.274000 0.244426 (0)
xfs, 4 disks	5.880000 2.997135	4.837000 3.875448 (0)
ext4, 1 disks	0.791000 0.055579	0.853000 0.042438 (0)
ext4, 2 disks	18.232000 13.505638	17.254000 2.000506 (0)
ext4, 4 disks	491.790000 218.565229	696.783000 234.933562 (0)
ext3, 1 disks	15.315000 2.065465	1.900000 0.184662 (-)
ext3, 2 disks	128.524000 18.090519	55.278000 1.530554 (-)
ext3, 4 disks	221.202000 30.090432	232.849000 68.745423 (0)
btrfs, 1 disks	0.452000 0.026000	0.494000 0.023749 (0)
btrfs, 2 disks	5.156000 4.530852	4.083000 1.560519 (0)
btrfs, 4 disks	31.154000 11.220828	36.987000 17.334126 (0)

Except for ext3 which got a nice boost here and XFS which seems to be a tad bit
slower, there are no changes that would stand out of the noise.

Results of test 3
-----------------
Numbers are time it took sync to complete.

		    BASE		    PATCHED
FS		AVG      STDDEV         AVG      STDDEV
xfs, 1 disks	12.083000 0.058660	10.898000 0.285475 (-)
xfs, 2 disks	20.182000 0.549614	14.977000 0.351114 (-)
xfs, 4 disks	35.814000 5.318310	28.452000 3.332281 (0)
ext4, 1 disks	32.956000 5.753789	20.865000 3.892098 (0)
ext4, 2 disks	34.922000 3.051966	27.411000 2.752978 (0)
ext4, 4 disks	44.508000 6.829004	28.360000 2.561437 (0)
ext3, 1 disks	23.475000 1.288885	17.116000 0.319631 (-)
ext3, 2 disks	43.508000 4.998647	41.547000 2.597976 (0)
ext3, 4 disks	92.130000 11.344117	79.362000 9.891208 (0)
btrfs, 1 disks	12.478000 0.394304	12.847000 0.171117 (0)
btrfs, 2 disks	15.030000 0.777817	18.014000 2.011418 (0)
btrfs, 4 disks	32.395000 4.248859	38.411000 3.179939 (0)

Here we see XFS and ext3 had some improvements, ext4 likely as well although
the results are relatively noisy.

								Honza

[PATCH 1/8] vfs: Move noop_backing_dev_info check from sync into writeback

[Jan Kara]

In principle, a filesystem may want to have ->sync_fs() called during sync(1)
although it does not have a bdi (i.e. s_bdi is set to noop_backing_dev_info).
Only writeback code really needs bdi set to something reasonable. So move the
checks where they are more logical.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/fs-writeback.c |    5 +++++
 fs/sync.c         |    7 -------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 73c3992..2447206 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1242,6 +1242,8 @@ void writeback_inodes_sb_nr(struct super_block *sb,
 		.reason			= reason,
 	};
 
+	if (sb->s_bdi == &noop_backing_dev_info)
+		return;
 	WARN_ON(!rwsem_is_locked(&sb->s_umount));
 	bdi_queue_work(sb->s_bdi, &work);
 	wait_for_completion(&done);
@@ -1322,6 +1324,9 @@ void sync_inodes_sb(struct super_block *sb)
 		.reason		= WB_REASON_SYNC,
 	};
 
+	/* Nothing to do? */
+	if (sb->s_bdi == &noop_backing_dev_info)
+		return;
 	WARN_ON(!rwsem_is_locked(&sb->s_umount));
 
 	bdi_queue_work(sb->s_bdi, &work);
diff --git a/fs/sync.c b/fs/sync.c
index 101b8ef..878d721 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -30,13 +30,6 @@
  */
 static int __sync_filesystem(struct super_block *sb, int wait)
 {
-	/*
-	 * This should be safe, as we require bdi backing to actually
-	 * write out data in the first place
-	 */
-	if (sb->s_bdi == &noop_backing_dev_info)
-		return 0;
-
 	if (sb->s_qcop && sb->s_qcop->quota_sync)
 		sb->s_qcop->quota_sync(sb, -1, wait);
 
-- 
1.7.1

[PATCH 2/8] quota: Split dquot_quota_sync() to writeback and cache flushing part

[Jan Kara]

Split off part of dquot_quota_sync() which writes dquots into a quota file
to a separate function. In the next patch we will use the function from
filesystems and we do not want to abuse ->quota_sync quotactl callback more
than necessary.

Acked-by: Steven Whitehouse <swhiteho@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/gfs2/quota.c          |    4 ++--
 fs/gfs2/quota.h          |    2 +-
 fs/gfs2/super.c          |    2 +-
 fs/gfs2/sys.c            |    2 +-
 fs/quota/dquot.c         |   35 ++++++++++++++++++++++++++---------
 fs/quota/quota.c         |    4 ++--
 fs/sync.c                |    2 +-
 include/linux/quota.h    |    2 +-
 include/linux/quotaops.h |    3 ++-
 9 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index 7e528dc..a263ccb 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -1118,7 +1118,7 @@ void gfs2_quota_change(struct gfs2_inode *ip, s64 change,
 	}
 }
 
-int gfs2_quota_sync(struct super_block *sb, int type, int wait)
+int gfs2_quota_sync(struct super_block *sb, int type)
 {
 	struct gfs2_sbd *sdp = sb->s_fs_info;
 	struct gfs2_quota_data **qda;
@@ -1164,7 +1164,7 @@ int gfs2_quota_sync(struct super_block *sb, int type, int wait)
 
 static int gfs2_quota_sync_timeo(struct super_block *sb, int type)
 {
-	return gfs2_quota_sync(sb, type, 0);
+	return gfs2_quota_sync(sb, type);
 }
 
 int gfs2_quota_refresh(struct gfs2_sbd *sdp, int user, u32 id)
diff --git a/fs/gfs2/quota.h b/fs/gfs2/quota.h
index 90bf1c3..f25d98b 100644
--- a/fs/gfs2/quota.h
+++ b/fs/gfs2/quota.h
@@ -26,7 +26,7 @@ extern int gfs2_quota_check(struct gfs2_inode *ip, u32 uid, u32 gid);
 extern void gfs2_quota_change(struct gfs2_inode *ip, s64 change,
 			      u32 uid, u32 gid);
 
-extern int gfs2_quota_sync(struct super_block *sb, int type, int wait);
+extern int gfs2_quota_sync(struct super_block *sb, int type);
 extern int gfs2_quota_refresh(struct gfs2_sbd *sdp, int user, u32 id);
 
 extern int gfs2_quota_init(struct gfs2_sbd *sdp);
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 71e4209..af40287 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -838,7 +838,7 @@ static int gfs2_make_fs_ro(struct gfs2_sbd *sdp)
 	int error;
 
 	flush_workqueue(gfs2_delete_workqueue);
-	gfs2_quota_sync(sdp->sd_vfs, 0, 1);
+	gfs2_quota_sync(sdp->sd_vfs, 0);
 	gfs2_statfs_sync(sdp->sd_vfs, 0);
 
 	error = gfs2_glock_nq_init(sdp->sd_trans_gl, LM_ST_SHARED, GL_NOCACHE,
diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
index 443cabc..4e06db2 100644
--- a/fs/gfs2/sys.c
+++ b/fs/gfs2/sys.c
@@ -168,7 +168,7 @@ static ssize_t quota_sync_store(struct gfs2_sbd *sdp, const char *buf,
 	if (simple_strtol(buf, NULL, 0) != 1)
 		return -EINVAL;
 
-	gfs2_quota_sync(sdp->sd_vfs, 0, 1);
+	gfs2_quota_sync(sdp->sd_vfs, 0);
 	return len;
 }
 
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 5b572c8..35c4a47 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -595,12 +595,14 @@ out:
 }
 EXPORT_SYMBOL(dquot_scan_active);
 
-int dquot_quota_sync(struct super_block *sb, int type, int wait)
+/* Write all dquot structures to quota files */
+int dquot_writeback_dquots(struct super_block *sb, int type)
 {
 	struct list_head *dirty;
 	struct dquot *dquot;
 	struct quota_info *dqopt = sb_dqopt(sb);
 	int cnt;
+	int err, ret = 0;
 
 	mutex_lock(&dqopt->dqonoff_mutex);
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
@@ -624,7 +626,9 @@ int dquot_quota_sync(struct super_block *sb, int type, int wait)
 			atomic_inc(&dquot->dq_count);
 			spin_unlock(&dq_list_lock);
 			dqstats_inc(DQST_LOOKUPS);
-			sb->dq_op->write_dquot(dquot);
+			err = sb->dq_op->write_dquot(dquot);
+			if (!ret && err)
+				err = ret;
 			dqput(dquot);
 			spin_lock(&dq_list_lock);
 		}
@@ -638,7 +642,21 @@ int dquot_quota_sync(struct super_block *sb, int type, int wait)
 	dqstats_inc(DQST_SYNCS);
 	mutex_unlock(&dqopt->dqonoff_mutex);
 
-	if (!wait || (sb_dqopt(sb)->flags & DQUOT_QUOTA_SYS_FILE))
+	return ret;
+}
+EXPORT_SYMBOL(dquot_writeback_dquots);
+
+/* Write all dquot structures to disk and make them visible from userspace */
+int dquot_quota_sync(struct super_block *sb, int type)
+{
+	struct quota_info *dqopt = sb_dqopt(sb);
+	int cnt;
+	int ret;
+
+	ret = dquot_writeback_dquots(sb, type);
+	if (ret)
+		return ret;
+	if (dqopt->flags & DQUOT_QUOTA_SYS_FILE)
 		return 0;
 
 	/* This is not very clever (and fast) but currently I don't know about
@@ -652,18 +670,17 @@ int dquot_quota_sync(struct super_block *sb, int type, int wait)
 	 * Now when everything is written we can discard the pagecache so
 	 * that userspace sees the changes.
 	 */
-	mutex_lock(&sb_dqopt(sb)->dqonoff_mutex);
+	mutex_lock(&dqopt->dqonoff_mutex);
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
 		if (type != -1 && cnt != type)
 			continue;
 		if (!sb_has_quota_active(sb, cnt))
 			continue;
-		mutex_lock_nested(&sb_dqopt(sb)->files[cnt]->i_mutex,
-				  I_MUTEX_QUOTA);
-		truncate_inode_pages(&sb_dqopt(sb)->files[cnt]->i_data, 0);
-		mutex_unlock(&sb_dqopt(sb)->files[cnt]->i_mutex);
+		mutex_lock_nested(&dqopt->files[cnt]->i_mutex, I_MUTEX_QUOTA);
+		truncate_inode_pages(&dqopt->files[cnt]->i_data, 0);
+		mutex_unlock(&dqopt->files[cnt]->i_mutex);
 	}
-	mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
+	mutex_unlock(&dqopt->dqonoff_mutex);
 
 	return 0;
 }
diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 35f4b0e..0e17b99 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -48,7 +48,7 @@ static int check_quotactl_permission(struct super_block *sb, int type, int cmd,
 static void quota_sync_one(struct super_block *sb, void *arg)
 {
 	if (sb->s_qcop && sb->s_qcop->quota_sync)
-		sb->s_qcop->quota_sync(sb, *(int *)arg, 1);
+		sb->s_qcop->quota_sync(sb, *(int *)arg);
 }
 
 static int quota_sync_all(int type)
@@ -271,7 +271,7 @@ static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id,
 	case Q_SYNC:
 		if (!sb->s_qcop->quota_sync)
 			return -ENOSYS;
-		return sb->s_qcop->quota_sync(sb, type, 1);
+		return sb->s_qcop->quota_sync(sb, type);
 	case Q_XQUOTAON:
 	case Q_XQUOTAOFF:
 	case Q_XQUOTARM:
diff --git a/fs/sync.c b/fs/sync.c
index 878d721..faad2d8 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -31,7 +31,7 @@
 static int __sync_filesystem(struct super_block *sb, int wait)
 {
 	if (sb->s_qcop && sb->s_qcop->quota_sync)
-		sb->s_qcop->quota_sync(sb, -1, wait);
+		sb->s_qcop->quota_sync(sb, -1);
 
 	if (wait)
 		sync_inodes_sb(sb);
diff --git a/include/linux/quota.h b/include/linux/quota.h
index cb78556..8a43f08 100644
--- a/include/linux/quota.h
+++ b/include/linux/quota.h
@@ -329,7 +329,7 @@ struct quotactl_ops {
 	int (*quota_on)(struct super_block *, int, int, struct path *);
 	int (*quota_on_meta)(struct super_block *, int, int);
 	int (*quota_off)(struct super_block *, int);
-	int (*quota_sync)(struct super_block *, int, int);
+	int (*quota_sync)(struct super_block *, int);
 	int (*get_info)(struct super_block *, int, struct if_dqinfo *);
 	int (*set_info)(struct super_block *, int, struct if_dqinfo *);
 	int (*get_dqblk)(struct super_block *, int, qid_t, struct fs_disk_quota *);
diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
index d93f95e..e993d5c 100644
--- a/include/linux/quotaops.h
+++ b/include/linux/quotaops.h
@@ -83,7 +83,8 @@ int dquot_quota_on(struct super_block *sb, int type, int format_id,
 int dquot_quota_on_mount(struct super_block *sb, char *qf_name,
  	int format_id, int type);
 int dquot_quota_off(struct super_block *sb, int type);
-int dquot_quota_sync(struct super_block *sb, int type, int wait);
+int dquot_writeback_dquots(struct super_block *sb, int type);
+int dquot_quota_sync(struct super_block *sb, int type);
 int dquot_get_dqinfo(struct super_block *sb, int type, struct if_dqinfo *ii);
 int dquot_set_dqinfo(struct super_block *sb, int type, struct if_dqinfo *ii);
 int dquot_get_dqblk(struct super_block *sb, int type, qid_t id,
-- 
1.7.1

[PATCH 3/8] 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: Joel Becker <jlbec@evilplan.org>
CC: reiserfs-devel@vger.kernel.org
Acked-by: Steven Whitehouse <swhiteho@redhat.com>
Acked-by: Dave Kleikamp <shaggy@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
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 bd8ac16..adf3cde 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 922d289..b721140 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -2522,6 +2522,11 @@ static int ext3_sync_fs(struct super_block *sb, int wait)
 	tid_t target;
 
 	trace_ext3_sync_fs(sb, wait);
+	/*
+	 * 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 9953d80..a2d9098 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4304,6 +4304,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 af40287..3757a9a 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -952,6 +952,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 a44eff0..b9e0fca 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -602,6 +602,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 14363b9..dd7aec5 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 faad2d8..cda435f 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -30,9 +30,6 @@
  */
 static int __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

[PATCH 4/8] vfs: Reorder operations during sys_sync

[Jan Kara]

Change the order of operations during sync from

for_each_sb {
        writeback_inodes_sb();
        sync_fs(nowait);
        __sync_blockdev(nowait);
}
for_each_sb {
        sync_inodes_sb();
        sync_fs(wait);
        __sync_blockdev(wait);
}

to

for_each_sb
        writeback_inodes_sb();
for_each_sb
        sync_fs(nowait);
for_each_sb
        __sync_blockdev(nowait);
for_each_sb
        sync_inodes_sb();
for_each_sb
        sync_fs(wait);
for_each_sb
        __sync_blockdev(wait);

This is a preparation for the following patches in this series.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/sync.c |   46 ++++++++++++++++++++++++++++++++++------------
 1 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/fs/sync.c b/fs/sync.c
index cda435f..e62a57b 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -68,18 +68,28 @@ int sync_filesystem(struct super_block *sb)
 }
 EXPORT_SYMBOL_GPL(sync_filesystem);
 
-static void sync_one_sb(struct super_block *sb, void *arg)
+static void sync_inodes_one_sb(struct super_block *sb, void *arg)
 {
 	if (!(sb->s_flags & MS_RDONLY))
-		__sync_filesystem(sb, *(int *)arg);
+		sync_inodes_sb(sb);
 }
-/*
- * Sync all the data for all the filesystems (called by sys_sync() and
- * emergency sync)
- */
-static void sync_filesystems(int wait)
+
+static void writeback_inodes_one_sb(struct super_block *sb, void *arg)
+{
+	if (!(sb->s_flags & MS_RDONLY))
+		writeback_inodes_sb(sb);
+}
+
+static void sync_fs_one_sb(struct super_block *sb, void *arg)
 {
-	iterate_supers(sync_one_sb, &wait);
+	if (!(sb->s_flags & MS_RDONLY) && sb->s_op->sync_fs)
+		sb->s_op->sync_fs(sb, *(int *)arg);
+}
+
+static void sync_blkdev_one_sb(struct super_block *sb, void *arg)
+{
+	if (!(sb->s_flags & MS_RDONLY))
+		__sync_blockdev(sb->s_bdev, *(int *)arg);
 }
 
 /*
@@ -88,9 +98,15 @@ static void sync_filesystems(int wait)
  */
 SYSCALL_DEFINE0(sync)
 {
+	int nowait = 0, wait = 1;
+
 	wakeup_flusher_threads(0, WB_REASON_SYNC);
-	sync_filesystems(0);
-	sync_filesystems(1);
+	iterate_supers(writeback_inodes_one_sb, NULL);
+	iterate_supers(sync_fs_one_sb, &nowait);
+	iterate_supers(sync_blkdev_one_sb, &nowait);
+	iterate_supers(sync_inodes_one_sb, NULL);
+	iterate_supers(sync_fs_one_sb, &wait);
+	iterate_supers(sync_blkdev_one_sb, &wait);
 	if (unlikely(laptop_mode))
 		laptop_sync_completion();
 	return 0;
@@ -98,12 +114,18 @@ SYSCALL_DEFINE0(sync)
 
 static void do_sync_work(struct work_struct *work)
 {
+	int nowait = 0;
+
 	/*
 	 * Sync twice to reduce the possibility we skipped some inodes / pages
 	 * because they were temporarily locked
 	 */
-	sync_filesystems(0);
-	sync_filesystems(0);
+	iterate_supers(sync_inodes_one_sb, &nowait);
+	iterate_supers(sync_fs_one_sb, &nowait);
+	iterate_supers(sync_blkdev_one_sb, &nowait);
+	iterate_supers(sync_inodes_one_sb, &nowait);
+	iterate_supers(sync_fs_one_sb, &nowait);
+	iterate_supers(sync_blkdev_one_sb, &nowait);
 	printk("Emergency Sync complete\n");
 	kfree(work);
 }
-- 
1.7.1

[PATCH 5/8] vfs: Create function for iterating over block devices

[Jan Kara]

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/block_dev.c     |   36 ++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |    1 +
 2 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index b07f1da..4d6631e 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -196,6 +196,42 @@ blkdev_direct_IO(int rw, struct kiocb *iocb, const struct iovec *iov,
 				    nr_segs, blkdev_get_blocks, NULL, NULL, 0);
 }
 
+void iterate_bdevs(void (*func)(struct block_device *, void *), void *arg)
+{
+	struct inode *inode, *old_inode = NULL;
+
+	spin_lock(&inode_sb_list_lock);
+	list_for_each_entry(inode, &blockdev_superblock->s_inodes, i_sb_list) {
+		struct address_space *mapping = inode->i_mapping;
+
+		spin_lock(&inode->i_lock);
+		if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW) ||
+		    mapping->nrpages == 0) {
+			spin_unlock(&inode->i_lock);
+			continue;
+		}
+		__iget(inode);
+		spin_unlock(&inode->i_lock);
+		spin_unlock(&inode_sb_list_lock);
+		/*
+		 * We hold a reference to 'inode' so it couldn't have been
+		 * removed from s_inodes list while we dropped the
+		 * inode_sb_list_lock.  We cannot iput the inode now as we can
+		 * be holding the last reference and we cannot iput it under
+		 * inode_sb_list_lock. So we keep the reference and iput it
+		 * later.
+		 */
+		iput(old_inode);
+		old_inode = inode;
+
+		func(I_BDEV(inode), arg);
+
+		spin_lock(&inode_sb_list_lock);
+	}
+	spin_unlock(&inode_sb_list_lock);
+	iput(old_inode);
+}
+
 int __sync_blockdev(struct block_device *bdev, int wait)
 {
 	if (!bdev)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 0c4df26..0404e7c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2089,6 +2089,7 @@ extern void bd_set_size(struct block_device *, loff_t size);
 extern void bd_forget(struct inode *inode);
 extern void bdput(struct block_device *);
 extern void invalidate_bdev(struct block_device *);
+extern void iterate_bdevs(void (*)(struct block_device *, void *), void *);
 extern int sync_blockdev(struct block_device *bdev);
 extern struct super_block *freeze_bdev(struct block_device *);
 extern void emergency_thaw_all(void);
-- 
1.7.1

[PATCH 6/8] vfs: Make sys_sync writeout also block device inodes

[Jan Kara]

In case block device does not have filesystem mounted on it, sys_sync will just
ignore it and doesn't writeout its dirty pages. This is because writeback code
avoids writing inodes from superblock without backing device and
blockdev_superblock is such a superblock.  Since it's unexpected that sync
doesn't writeout dirty data for block devices be nice to users and change the
behavior to do so. So now we iterate over all block devices on blockdev_super
instead of iterating over all superblocks when syncing block devices.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/sync.c |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/fs/sync.c b/fs/sync.c
index e62a57b..ccaaa1b 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -86,10 +86,54 @@ static void sync_fs_one_sb(struct super_block *sb, void *arg)
 		sb->s_op->sync_fs(sb, *(int *)arg);
 }
 
-static void sync_blkdev_one_sb(struct super_block *sb, void *arg)
+/*
+ * We go through all existing block devices so that even devices without
+ * filesystem mounted are synced.
+ */
+static void sync_all_bdevs(int wait)
 {
-	if (!(sb->s_flags & MS_RDONLY))
-		__sync_blockdev(sb->s_bdev, *(int *)arg);
+	struct inode *inode, *old_inode = NULL;
+
+	spin_lock(&inode_sb_list_lock);
+	list_for_each_entry(inode, &blockdev_superblock->s_inodes, i_sb_list) {
+		struct address_space *mapping = inode->i_mapping;
+
+		spin_lock(&inode->i_lock);
+		if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW) ||
+		    mapping->nrpages == 0) {
+			spin_unlock(&inode->i_lock);
+			continue;
+		}
+		__iget(inode);
+		spin_unlock(&inode->i_lock);
+		spin_unlock(&inode_sb_list_lock);
+		/*
+		 * We hold a reference to 'inode' so it couldn't have been
+		 * removed from s_inodes list while we dropped the
+		 * inode_sb_list_lock.  We cannot iput the inode now as we can
+		 * be holding the last reference and we cannot iput it under
+		 * inode_sb_list_lock. So we keep the reference and iput it
+		 * later.
+		 */
+		iput(old_inode);
+		old_inode = inode;
+
+		__sync_blockdev(I_BDEV(inode), wait);
+
+		spin_lock(&inode_sb_list_lock);
+	}
+	spin_unlock(&inode_sb_list_lock);
+	iput(old_inode);
+}
+
+static void flush_one_bdev(struct block_device *bdev, void *arg)
+{
+	__sync_blockdev(bdev, 0);
+}
+
+static void sync_one_bdev(struct block_device *bdev, void *arg)
+{
+	sync_blockdev(bdev);
 }
 
 /*
@@ -103,10 +147,10 @@ SYSCALL_DEFINE0(sync)
 	wakeup_flusher_threads(0, WB_REASON_SYNC);
 	iterate_supers(writeback_inodes_one_sb, NULL);
 	iterate_supers(sync_fs_one_sb, &nowait);
-	iterate_supers(sync_blkdev_one_sb, &nowait);
+	iterate_bdevs(flush_one_bdev, NULL);
 	iterate_supers(sync_inodes_one_sb, NULL);
 	iterate_supers(sync_fs_one_sb, &wait);
-	iterate_supers(sync_blkdev_one_sb, &wait);
+	iterate_bdevs(sync_one_bdev, NULL);
 	if (unlikely(laptop_mode))
 		laptop_sync_completion();
 	return 0;
@@ -122,10 +166,10 @@ static void do_sync_work(struct work_struct *work)
 	 */
 	iterate_supers(sync_inodes_one_sb, &nowait);
 	iterate_supers(sync_fs_one_sb, &nowait);
-	iterate_supers(sync_blkdev_one_sb, &nowait);
+	sync_all_bdevs(nowait);
 	iterate_supers(sync_inodes_one_sb, &nowait);
 	iterate_supers(sync_fs_one_sb, &nowait);
-	iterate_supers(sync_blkdev_one_sb, &nowait);
+	sync_all_bdevs(nowait);
 	printk("Emergency Sync complete\n");
 	kfree(work);
 }
-- 
1.7.1

[PATCH 7/8] vfs: Remove unnecessary flushing of block devices

[Jan Kara]

It is not necessary to write block devices twice. The reason why we first did
flush and then proper sync is that
  for_each_bdev() {
    write_bdev()
    wait_for_completion()
  }
is much slower than
  for_each_bdev()
    write_bdev()
  for_each_bdev()
    wait_for_completion()
when there is bigger amount of data. But as is seen in the above, there's no real
need to scan pages and submit them twice. We just need to separate the submission
and waiting part. This patch does that.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/sync.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/sync.c b/fs/sync.c
index ccaaa1b..d8b9dd1 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -126,14 +126,14 @@ static void sync_all_bdevs(int wait)
 	iput(old_inode);
 }
 
-static void flush_one_bdev(struct block_device *bdev, void *arg)
+static void fdatawrite_one_bdev(struct block_device *bdev, void *arg)
 {
-	__sync_blockdev(bdev, 0);
+	filemap_fdatawrite(bdev->bd_inode->i_mapping);
 }
 
-static void sync_one_bdev(struct block_device *bdev, void *arg)
+static void fdatawait_one_bdev(struct block_device *bdev, void *arg)
 {
-	sync_blockdev(bdev);
+	filemap_fdatawait(bdev->bd_inode->i_mapping);
 }
 
 /*
@@ -147,10 +147,10 @@ SYSCALL_DEFINE0(sync)
 	wakeup_flusher_threads(0, WB_REASON_SYNC);
 	iterate_supers(writeback_inodes_one_sb, NULL);
 	iterate_supers(sync_fs_one_sb, &nowait);
-	iterate_bdevs(flush_one_bdev, NULL);
 	iterate_supers(sync_inodes_one_sb, NULL);
 	iterate_supers(sync_fs_one_sb, &wait);
-	iterate_bdevs(sync_one_bdev, NULL);
+	iterate_bdevs(fdatawrite_one_bdev, NULL);
+	iterate_bdevs(fdatawait_one_bdev, NULL);
 	if (unlikely(laptop_mode))
 		laptop_sync_completion();
 	return 0;
-- 
1.7.1

[PATCH 8/8] vfs: Avoid unnecessary WB_SYNC_NONE writeback during sys_sync and reorder sync passes

[Jan Kara]

wakeup_flusher_threads(0) will queue work doing complete writeback for each
flusher thread. Thus there is not much point in submitting another work doing
full inode WB_SYNC_NONE writeback by writeback_inodes_sb().

After this change it does not make sense to call nonblocking ->sync_fs and
block device flush before calling sync_inodes_sb() because
wakeup_flusher_threads() is completely asynchronous and thus these functions
would be called in parallel with inode writeback running which will effectively
void any work they do. So we move sync_inodes_sb() call before these two
functions.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/sync.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/fs/sync.c b/fs/sync.c
index d8b9dd1..5a2f1ba 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -74,12 +74,6 @@ static void sync_inodes_one_sb(struct super_block *sb, void *arg)
 		sync_inodes_sb(sb);
 }
 
-static void writeback_inodes_one_sb(struct super_block *sb, void *arg)
-{
-	if (!(sb->s_flags & MS_RDONLY))
-		writeback_inodes_sb(sb);
-}
-
 static void sync_fs_one_sb(struct super_block *sb, void *arg)
 {
 	if (!(sb->s_flags & MS_RDONLY) && sb->s_op->sync_fs)
@@ -137,17 +131,22 @@ static void fdatawait_one_bdev(struct block_device *bdev, void *arg)
 }
 
 /*
- * sync everything.  Start out by waking pdflush, because that writes back
- * all queues in parallel.
+ * Sync everything. We start by waking flusher threads so that most of
+ * writeback runs on all devices in parallel. Then we sync all inodes reliably
+ * which effectively also waits for all flusher threads to finish doing
+ * writeback. At this point all data is on disk so metadata should be stable
+ * and we tell filesystems to sync their metadata via ->sync_fs() calls.
+ * Finally, we writeout all block devices because some filesystems (e.g. ext2)
+ * just write metadata (such as inodes or bitmaps) to block device page cache
+ * and do not sync it on their own in ->sync_fs().
  */
 SYSCALL_DEFINE0(sync)
 {
 	int nowait = 0, wait = 1;
 
 	wakeup_flusher_threads(0, WB_REASON_SYNC);
-	iterate_supers(writeback_inodes_one_sb, NULL);
-	iterate_supers(sync_fs_one_sb, &nowait);
 	iterate_supers(sync_inodes_one_sb, NULL);
+	iterate_supers(sync_fs_one_sb, &nowait);
 	iterate_supers(sync_fs_one_sb, &wait);
 	iterate_bdevs(fdatawrite_one_bdev, NULL);
 	iterate_bdevs(fdatawait_one_bdev, NULL);
-- 
1.7.1

Re: [PATCH 6/8] vfs: Make sys_sync writeout also block device inodes

[Curt Wohlgemuth]

Hi Jan:

Was there ever a resolution for this patch and the series?  I looked
for but found no ack/nack for V4 of your "Cleanup and improve sync"
patch series.

We've been carrying a crappy hack/patch to sync all block device
inodes in order to allow lilo to not break, and it'd be nice to have a
real fix for it upstream.

Thanks,
Curt


On Thu, Jan 5, 2012 at 3:46 PM, Jan Kara <jack@suse.cz> wrote:
> In case block device does not have filesystem mounted on it, sys_sync will just
> ignore it and doesn't writeout its dirty pages. This is because writeback code
> avoids writing inodes from superblock without backing device and
> blockdev_superblock is such a superblock.  Since it's unexpected that sync
> doesn't writeout dirty data for block devices be nice to users and change the
> behavior to do so. So now we iterate over all block devices on blockdev_super
> instead of iterating over all superblocks when syncing block devices.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/sync.c |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 51 insertions(+), 7 deletions(-)
>
> diff --git a/fs/sync.c b/fs/sync.c
> index e62a57b..ccaaa1b 100644
> --- a/fs/sync.c
> +++ b/fs/sync.c
> @@ -86,10 +86,54 @@ static void sync_fs_one_sb(struct super_block *sb, void *arg)
>                sb->s_op->sync_fs(sb, *(int *)arg);
>  }
>
> -static void sync_blkdev_one_sb(struct super_block *sb, void *arg)
> +/*
> + * We go through all existing block devices so that even devices without
> + * filesystem mounted are synced.
> + */
> +static void sync_all_bdevs(int wait)
>  {
> -       if (!(sb->s_flags & MS_RDONLY))
> -               __sync_blockdev(sb->s_bdev, *(int *)arg);
> +       struct inode *inode, *old_inode = NULL;
> +
> +       spin_lock(&inode_sb_list_lock);
> +       list_for_each_entry(inode, &blockdev_superblock->s_inodes, i_sb_list) {
> +               struct address_space *mapping = inode->i_mapping;
> +
> +               spin_lock(&inode->i_lock);
> +               if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW) ||
> +                   mapping->nrpages == 0) {
> +                       spin_unlock(&inode->i_lock);
> +                       continue;
> +               }
> +               __iget(inode);
> +               spin_unlock(&inode->i_lock);
> +               spin_unlock(&inode_sb_list_lock);
> +               /*
> +                * We hold a reference to 'inode' so it couldn't have been
> +                * removed from s_inodes list while we dropped the
> +                * inode_sb_list_lock.  We cannot iput the inode now as we can
> +                * be holding the last reference and we cannot iput it under
> +                * inode_sb_list_lock. So we keep the reference and iput it
> +                * later.
> +                */
> +               iput(old_inode);
> +               old_inode = inode;
> +
> +               __sync_blockdev(I_BDEV(inode), wait);
> +
> +               spin_lock(&inode_sb_list_lock);
> +       }
> +       spin_unlock(&inode_sb_list_lock);
> +       iput(old_inode);
> +}
> +
> +static void flush_one_bdev(struct block_device *bdev, void *arg)
> +{
> +       __sync_blockdev(bdev, 0);
> +}
> +
> +static void sync_one_bdev(struct block_device *bdev, void *arg)
> +{
> +       sync_blockdev(bdev);
>  }
>
>  /*
> @@ -103,10 +147,10 @@ SYSCALL_DEFINE0(sync)
>        wakeup_flusher_threads(0, WB_REASON_SYNC);
>        iterate_supers(writeback_inodes_one_sb, NULL);
>        iterate_supers(sync_fs_one_sb, &nowait);
> -       iterate_supers(sync_blkdev_one_sb, &nowait);
> +       iterate_bdevs(flush_one_bdev, NULL);
>        iterate_supers(sync_inodes_one_sb, NULL);
>        iterate_supers(sync_fs_one_sb, &wait);
> -       iterate_supers(sync_blkdev_one_sb, &wait);
> +       iterate_bdevs(sync_one_bdev, NULL);
>        if (unlikely(laptop_mode))
>                laptop_sync_completion();
>        return 0;
> @@ -122,10 +166,10 @@ static void do_sync_work(struct work_struct *work)
>         */
>        iterate_supers(sync_inodes_one_sb, &nowait);
>        iterate_supers(sync_fs_one_sb, &nowait);
> -       iterate_supers(sync_blkdev_one_sb, &nowait);
> +       sync_all_bdevs(nowait);
>        iterate_supers(sync_inodes_one_sb, &nowait);
>        iterate_supers(sync_fs_one_sb, &nowait);
> -       iterate_supers(sync_blkdev_one_sb, &nowait);
> +       sync_all_bdevs(nowait);
>        printk("Emergency Sync complete\n");
>        kfree(work);
>  }
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] vfs: Make sys_sync writeout also block device inodes

[Jan Kara]

  Hi Curt,

On Wed 20-06-12 07:23:56, Curt Wohlgemuth wrote:
> Was there ever a resolution for this patch and the series?  I looked
> for but found no ack/nack for V4 of your "Cleanup and improve sync"
> patch series.
> 
> We've been carrying a crappy hack/patch to sync all block device
> inodes in order to allow lilo to not break, and it'd be nice to have a
> real fix for it upstream.
  The patch set works, just I'm missing some final review and someone (Al?)
has to pick it up and merge it. Anyway I plan to repost the series soon to
hopefully get things moving again...

								Honza

> On Thu, Jan 5, 2012 at 3:46 PM, Jan Kara <jack@suse.cz> wrote:
> > In case block device does not have filesystem mounted on it, sys_sync will just
> > ignore it and doesn't writeout its dirty pages. This is because writeback code
> > avoids writing inodes from superblock without backing device and
> > blockdev_superblock is such a superblock.  Since it's unexpected that sync
> > doesn't writeout dirty data for block devices be nice to users and change the
> > behavior to do so. So now we iterate over all block devices on blockdev_super
> > instead of iterating over all superblocks when syncing block devices.
> >
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/sync.c |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
> >  1 files changed, 51 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/sync.c b/fs/sync.c
> > index e62a57b..ccaaa1b 100644
> > --- a/fs/sync.c
> > +++ b/fs/sync.c
> > @@ -86,10 +86,54 @@ static void sync_fs_one_sb(struct super_block *sb, void *arg)
> >                sb->s_op->sync_fs(sb, *(int *)arg);
> >  }
> >
> > -static void sync_blkdev_one_sb(struct super_block *sb, void *arg)
> > +/*
> > + * We go through all existing block devices so that even devices without
> > + * filesystem mounted are synced.
> > + */
> > +static void sync_all_bdevs(int wait)
> >  {
> > -       if (!(sb->s_flags & MS_RDONLY))
> > -               __sync_blockdev(sb->s_bdev, *(int *)arg);
> > +       struct inode *inode, *old_inode = NULL;
> > +
> > +       spin_lock(&inode_sb_list_lock);
> > +       list_for_each_entry(inode, &blockdev_superblock->s_inodes, i_sb_list) {
> > +               struct address_space *mapping = inode->i_mapping;
> > +
> > +               spin_lock(&inode->i_lock);
> > +               if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW) ||
> > +                   mapping->nrpages == 0) {
> > +                       spin_unlock(&inode->i_lock);
> > +                       continue;
> > +               }
> > +               __iget(inode);
> > +               spin_unlock(&inode->i_lock);
> > +               spin_unlock(&inode_sb_list_lock);
> > +               /*
> > +                * We hold a reference to 'inode' so it couldn't have been
> > +                * removed from s_inodes list while we dropped the
> > +                * inode_sb_list_lock.  We cannot iput the inode now as we can
> > +                * be holding the last reference and we cannot iput it under
> > +                * inode_sb_list_lock. So we keep the reference and iput it
> > +                * later.
> > +                */
> > +               iput(old_inode);
> > +               old_inode = inode;
> > +
> > +               __sync_blockdev(I_BDEV(inode), wait);
> > +
> > +               spin_lock(&inode_sb_list_lock);
> > +       }
> > +       spin_unlock(&inode_sb_list_lock);
> > +       iput(old_inode);
> > +}
> > +
> > +static void flush_one_bdev(struct block_device *bdev, void *arg)
> > +{
> > +       __sync_blockdev(bdev, 0);
> > +}
> > +
> > +static void sync_one_bdev(struct block_device *bdev, void *arg)
> > +{
> > +       sync_blockdev(bdev);
> >  }
> >
> >  /*
> > @@ -103,10 +147,10 @@ SYSCALL_DEFINE0(sync)
> >        wakeup_flusher_threads(0, WB_REASON_SYNC);
> >        iterate_supers(writeback_inodes_one_sb, NULL);
> >        iterate_supers(sync_fs_one_sb, &nowait);
> > -       iterate_supers(sync_blkdev_one_sb, &nowait);
> > +       iterate_bdevs(flush_one_bdev, NULL);
> >        iterate_supers(sync_inodes_one_sb, NULL);
> >        iterate_supers(sync_fs_one_sb, &wait);
> > -       iterate_supers(sync_blkdev_one_sb, &wait);
> > +       iterate_bdevs(sync_one_bdev, NULL);
> >        if (unlikely(laptop_mode))
> >                laptop_sync_completion();
> >        return 0;
> > @@ -122,10 +166,10 @@ static void do_sync_work(struct work_struct *work)
> >         */
> >        iterate_supers(sync_inodes_one_sb, &nowait);
> >        iterate_supers(sync_fs_one_sb, &nowait);
> > -       iterate_supers(sync_blkdev_one_sb, &nowait);
> > +       sync_all_bdevs(nowait);
> >        iterate_supers(sync_inodes_one_sb, &nowait);
> >        iterate_supers(sync_fs_one_sb, &nowait);
> > -       iterate_supers(sync_blkdev_one_sb, &nowait);
> > +       sync_all_bdevs(nowait);
> >        printk("Emergency Sync complete\n");
> >        kfree(work);
> >  }
> > --
> > 1.7.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] vfs: Make sys_sync writeout also block device inodes

[Al Viro]

On Wed, Jun 20, 2012 at 10:03:36PM +0200, Jan Kara wrote:
>   Hi Curt,
> 
> On Wed 20-06-12 07:23:56, Curt Wohlgemuth wrote:
> > Was there ever a resolution for this patch and the series?  I looked
> > for but found no ack/nack for V4 of your "Cleanup and improve sync"
> > patch series.
> > 
> > We've been carrying a crappy hack/patch to sync all block device
> > inodes in order to allow lilo to not break, and it'd be nice to have a
> > real fix for it upstream.
>   The patch set works, just I'm missing some final review and someone (Al?)
> has to pick it up and merge it. Anyway I plan to repost the series soon to
> hopefully get things moving again...

Please, rebase and send them my way.

Re: [PATCH 6/8] vfs: Make sys_sync writeout also block device inodes

[Jan Kara]

On Fri 22-06-12 11:30:41, Al Viro wrote:
> On Wed, Jun 20, 2012 at 10:03:36PM +0200, Jan Kara wrote:
> >   Hi Curt,
> > 
> > On Wed 20-06-12 07:23:56, Curt Wohlgemuth wrote:
> > > Was there ever a resolution for this patch and the series?  I looked
> > > for but found no ack/nack for V4 of your "Cleanup and improve sync"
> > > patch series.
> > > 
> > > We've been carrying a crappy hack/patch to sync all block device
> > > inodes in order to allow lilo to not break, and it'd be nice to have a
> > > real fix for it upstream.
> >   The patch set works, just I'm missing some final review and someone (Al?)
> > has to pick it up and merge it. Anyway I plan to repost the series soon to
> > hopefully get things moving again...
> 
> Please, rebase and send them my way.
  OK, I've rebased the patches, updated the measurements (numbers have
changed somewhat since last time), and sent them your way.

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 3/8] 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: Joel Becker <jlbec@evilplan.org>
CC: reiserfs-devel@vger.kernel.org
Acked-by: Steven Whitehouse <swhiteho@redhat.com>
Acked-by: Dave Kleikamp <shaggy@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
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 b3621cb..5df3d2d 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -1184,6 +1184,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 8c3a44b..4ac304c 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -2526,6 +2526,11 @@ static int ext3_sync_fs(struct super_block *sb, int wait)
 	tid_t target;
 
 	trace_ext3_sync_fs(sb, wait);
+	/*
+	 * 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 eb7aa3e..d875940 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4325,6 +4325,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 313c329..f3d6bbf 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -952,6 +952,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 4a82950..c55c745 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -601,6 +601,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 651ce76..7a37dab 100644
--- a/fs/reiserfs/super.c
+++ b/fs/reiserfs/super.c
@@ -68,6 +68,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 cae145d..66acd2b 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -29,9 +29,6 @@
  */
 static int __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

[PATCH 3/8] 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: Joel Becker <jlbec@evilplan.org>
CC: reiserfs-devel@vger.kernel.org
Acked-by: Steven Whitehouse <swhiteho@redhat.com>
Acked-by: Dave Kleikamp <shaggy@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
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 bd8ac16..adf3cde 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 922d289..b721140 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -2522,6 +2522,11 @@ static int ext3_sync_fs(struct super_block *sb, int wait)
 	tid_t target;
 
 	trace_ext3_sync_fs(sb, wait);
+	/*
+	 * 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 9953d80..a2d9098 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4304,6 +4304,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 af40287..3757a9a 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -952,6 +952,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 a44eff0..b9e0fca 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -602,6 +602,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 14363b9..dd7aec5 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 faad2d8..cda435f 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -30,9 +30,6 @@
  */
 static int __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