Skip to contentSkip to conversation

sys_sync fixes and changes

RFC PATCH In review Nick Piggin 2 participants 8 messages Nov 10, 2008 – Nov 12, 2008

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover sys_sync fixes and changes

Hi,

Unfortunately I had hoped to have my fsync patches for comment by now, but I have run into some problems with sys_sync and this WB_SYNC_HOLD thing.

But that code seems to have a problem with data integrity anyway, and fixing that seems to get things closer to working with my fsync design (without hobbling the alternative proposal), and not to mention simplifies the more common writeback and single-file fsync cases by removing WB_SYNC_HOLD. (sync mode now becomes a very simple "data integrity" or "not data integrity" flag).

I have 3 patches to this end, and would be interested in hearing comments on them.

Thanks,
Nick

#1 lore raw

RFC PATCH 1/3 fs: remove WB_SYNC_HOLD

Remove WB_SYNC_HOLD. The primary motiviation is the design of my anti-starvation code for fsync. It requires taking an inode lock over the sync operation, so we could run into lock ordering problems with multiple inodes. It is possible to take a single global lock to solve the ordering problem, but then that would prevent a future nice implementation of "sync multiple inodes" based on lock order via inode address.

Seems like a backward step to remove this, but actually it is busted
anyway: we can't use the inode lists for data integrity wait: an inode can be taken off the dirty lists but still be under writeback. In order to satisfy data integrity semantics, we should wait for it to finish writeback, but if we only search the dirty lists, we'll miss it.

It would be possible to have a "writeback" list, for sys_sync, I suppose. But why complicate things by prematurely optimise? For unmounting, we could avoid the "livelock avoidance" code, which would be easier, but again premature IMO.

Fixing the data integrity problem will come next.

---

fs/fs-writeback.c +2 -10
Index: linux-2.6/fs/fs-writeback.c
===================================================================
@@ -421,9 +421,6 @@ __writeback_single_inode(struct inode *i
  * If we're a pdlfush thread, then implement pdflush collision avoidance
  * against the entire list.
  *
- * WB_SYNC_HOLD is a hack for sys_sync(): reattach the inode to sb->s_dirty so
- * that it can be located for waiting on in __writeback_single_inode().
- *
  * If `bdi' is non-zero then we're being asked to writeback a specific queue.
  * This function assumes that the blockdev superblock's inodes are backed by
  * a variety of queues, so all inodes are searched.  For other superblocks,
@@ -499,10 +496,6 @@ void generic_sync_sb_inodes(struct super
 		__iget(inode);
 		pages_skipped = wbc->pages_skipped;
 		__writeback_single_inode(inode, wbc);
-		if (wbc->sync_mode == WB_SYNC_HOLD) {
-			inode->dirtied_when = jiffies;
-			list_move(&inode->i_list, &sb->s_dirty);
-		}
 		if (current_is_pdflush())
 			writeback_release(bdi);
 		if (wbc->pages_skipped != pages_skipped) {
@@ -588,8 +581,7 @@ restart:
 
 /*
  * writeback and wait upon the filesystem's dirty inodes.  The caller will
- * do this in two passes - one to write, and one to wait.  WB_SYNC_HOLD is
- * used to park the written inodes on sb->s_dirty for the wait pass.
+ * do this in two passes - one to write, and one to wait.
  *
  * A finite limit is set on the number of pages which will be written.
  * To prevent infinite livelock of sys_sync().
@@ -600,7 +592,7 @@ restart:
 void sync_inodes_sb(struct super_block *sb, int wait)
 {
 	struct writeback_control wbc = {
-		.sync_mode	= wait ? WB_SYNC_ALL : WB_SYNC_HOLD,
+		.sync_mode	= wait ? WB_SYNC_ALL : WB_SYNC_NONE,
 		.range_start	= 0,
 		.range_end	= LLONG_MAX,
 	};
include/linux/writeback.h +0 -1
Index: linux-2.6/include/linux/writeback.h
===================================================================
@@ -30,7 +30,6 @@ static inline int task_is_pdflush(struct
 enum writeback_sync_modes {
 	WB_SYNC_NONE,	/* Don't wait on anything */
 	WB_SYNC_ALL,	/* Wait on every mapping */
-	WB_SYNC_HOLD,	/* Hold the inode on sb_dirty for sys_sync() */
 };
 
 /*
#2 lore raw

RFC PATCH 2/3 fs: sync_sb_inodes fix

In reply to Nick Piggin

Fix data integrity semantics required by sys_sync, by iterating over all inodes and waiting for any writeback pages after the initial writeout. Comments explain the exact problem.

---

fs/fs-writeback.c +46 -7
Index: linux-2.6/fs/fs-writeback.c
===================================================================
@@ -440,6 +440,7 @@ void generic_sync_sb_inodes(struct super
 				struct writeback_control *wbc)
 {
 	const unsigned long start = jiffies;	/* livelock avoidance */
+	int sync = wbc->sync_mode == WB_SYNC_ALL;
 
 	spin_lock(&inode_lock);
 	if (!wbc->for_kupdate || list_empty(&sb->s_io))
@@ -516,7 +517,42 @@ void generic_sync_sb_inodes(struct super
 		if (!list_empty(&sb->s_more_io))
 			wbc->more_io = 1;
 	}
-	spin_unlock(&inode_lock);
+
+	if (unlikely(sync)) {
+		struct inode *inode, *old_inode = NULL;
+
+		/*
+		 * Data integrity sync. Must wait for all pages under writeback,
+		 * because there may have been pages dirtied before our sync
+		 * call, but which had writeout started before we write it out.
+		 * In which case, the inode may not be on the dirty list, but
+		 * we still have to wait for that writeout.
+		 */
+		list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
+			struct address_space *mapping;
+
+			if (inode->i_state & (I_FREEING|I_WILL_FREE))
+				continue;
+			mapping = inode->i_mapping;
+			if (mapping->nrpages == 0)
+				continue;
+			__iget(inode);
+			spin_unlock(&inode_lock);
+			/* see dquot.c comment */
+			iput(old_inode);
+			old_inode = inode;
+
+			filemap_fdatawait(mapping);
+
+			cond_resched();
+
+			spin_lock(&inode_lock);
+		}
+		spin_unlock(&inode_lock);
+		iput(old_inode);
+	} else
+		spin_unlock(&inode_lock);
+
 	return;		/* Leave any unwritten inodes on s_io */
 }
 EXPORT_SYMBOL_GPL(generic_sync_sb_inodes);
@@ -596,13 +632,16 @@ void sync_inodes_sb(struct super_block *
 		.range_start	= 0,
 		.range_end	= LLONG_MAX,
 	};
-	unsigned long nr_dirty = global_page_state(NR_FILE_DIRTY);
-	unsigned long nr_unstable = global_page_state(NR_UNSTABLE_NFS);
 
-	wbc.nr_to_write = nr_dirty + nr_unstable +
-			(inodes_stat.nr_inodes - inodes_stat.nr_unused) +
-			nr_dirty + nr_unstable;
-	wbc.nr_to_write += wbc.nr_to_write / 2;		/* Bit more for luck */
+	if (!wait) {
+		unsigned long nr_dirty = global_page_state(NR_FILE_DIRTY);
+		unsigned long nr_unstable = global_page_state(NR_UNSTABLE_NFS);
+
+		wbc.nr_to_write = nr_dirty + nr_unstable +
+			(inodes_stat.nr_inodes - inodes_stat.nr_unused);
+	} else
+		wbc.nr_to_write = LONG_MAX; /* doesn't actually matter */
+
 	sync_sb_inodes(sb, &wbc);
 }
 
#3 lore raw

Re: [rfc][patch 2/3] fs: sync_sb_inodes fix

Quoted 19 lines · Nick Piggin

Fix data integrity semantics required by sys_sync, by iterating over all inodes and waiting for any writeback pages after the initial writeout. Comments explain the exact problem.

---
Index: linux-2.6/fs/fs-writeback.c
===================================================================

fs/fs-writeback.c +1 -0
@@ -440,6 +440,7 @@ void generic_sync_sb_inodes(struct super
 				struct writeback_control *wbc)
 {
 	const unsigned long start = jiffies;	/* livelock avoidance */
+	int sync = wbc->sync_mode == WB_SYNC_ALL;
 
 	spin_lock(&inode_lock);
 	if (!wbc->for_kupdate || list_empty(&sb->s_io))

fs/fs-writeback.c void generic_sync_sb_inodes(struct super · line 521

Quoted 6 lines
fs/fs-writeback.c +2 -1
@@ -516,7 +517,42 @@ void generic_sync_sb_inodes(struct super
 		if (!list_empty(&sb->s_more_io))
 			wbc->more_io = 1;
 	}
-	spin_unlock(&inode_lock);
+
+	if (unlikely(sync)) {

Please don't mark things like this unlikely. Static branch prediction is really nasty on many cpus when the prediction was wrong, and dynamic branch brediction can do thing much better.

In general never use unlikely for something like this which can happen with a normal workload.

fs/fs-writeback.c void generic_sync_sb_inodes(struct super · line 541

Quoted 13 lines
fs/fs-writeback.c +13 -0
+		 * we still have to wait for that writeout.
+		 */
+		list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
+			struct address_space *mapping;
+
+			if (inode->i_state & (I_FREEING|I_WILL_FREE))
+				continue;
+			mapping = inode->i_mapping;
+			if (mapping->nrpages == 0)
+				continue;
+			__iget(inode);
+			spin_unlock(&inode_lock);
+			/* see dquot.c comment */

Not an exactly useful comment :)

#4 lore raw

Re: [rfc][patch 2/3] fs: sync_sb_inodes fix

Quoted 17 lines · Christoph Hellwig
Quoted 16 lines · Nick Piggin

Fix data integrity semantics required by sys_sync, by iterating over all inodes and waiting for any writeback pages after the initial writeout. Comments explain the exact problem.

---
Index: linux-2.6/fs/fs-writeback.c
===================================================================

fs/fs-writeback.c +1 -0
@@ -440,6 +440,7 @@ void generic_sync_sb_inodes(struct super
 				struct writeback_control *wbc)
 {
 	const unsigned long start = jiffies;	/* livelock avoidance */
+	int sync = wbc->sync_mode == WB_SYNC_ALL;
 
Quoted 13 lines

fs/fs-writeback.c void generic_sync_sb_inodes(struct super · line 521

Quoted 9 lines
fs/fs-writeback.c +2 -1
 	spin_lock(&inode_lock);
 	if (!wbc->for_kupdate || list_empty(&sb->s_io))
@@ -516,7 +517,42 @@ void generic_sync_sb_inodes(struct super
 		if (!list_empty(&sb->s_more_io))
 			wbc->more_io = 1;
 	}
-	spin_unlock(&inode_lock);
+
+	if (unlikely(sync)) {

Please don't mark things like this unlikely. Static branch prediction is really nasty on many cpus when the prediction was wrong, and dynamic branch brediction can do thing much better.

There's a few aspects to this :)

Static branch hint instruction could indeed turn off dynamic prediction so in that cause it would always mispredict even if the workload was continually going the other way.

Quoted 2 lines

In general never use unlikely for something like this which can happen with a normal workload.

However, I think you have to consider frequency, and also relative importance of each case. This is going to be taken on sys_sync, and sys_umount. And not taken for pdflush writeout.

I think sync and umount are both much less frequent, and also much less important to be fast.

It can allow gcc to be much smarter about code genreation too. Although AFAIK it mainly just moves the block out of the icache hot path, it could in future do things like switch off predicated instructions for both branches, and also do better register allocation. Just what gcc does today can be fairly significant.

But... I don't feel strongly about this particular path, so I can remove it.

Quoted 7 lines
Quoted 7 lines
fs/fs-writeback.c +7 -0
+		 * we still have to wait for that writeout.
+		 */
+		list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
+			struct address_space *mapping;
+
+			if (inode->i_state & (I_FREEING|I_WILL_FREE))
+				continue;
Quoted 8 lines

fs/fs-writeback.c void generic_sync_sb_inodes(struct super · line 541

Quoted 6 lines
fs/fs-writeback.c +6 -0
+			mapping = inode->i_mapping;
+			if (mapping->nrpages == 0)
+				continue;
+			__iget(inode);
+			spin_unlock(&inode_lock);
+			/* see dquot.c comment */

Not an exactly useful comment :)

/* see fs/dquot.c:add_dquot_ref() comment */ ?

Thanks, I will repost with these changes after waiting for more feedback.

#5 lore raw

Re: [rfc][patch 2/3] fs: sync_sb_inodes fix

Quoted 3 lines · Nick Piggin

Static branch hint instruction could indeed turn off dynamic prediction so in that cause it would always mispredict even if the workload was continually going the other way.

It's not just turning them off. In many cpu microarchitectures a wrong static prediction means an pipeline stall that's much worse than a dynamic mispredict because that information is interpreted much deeper in the pipeline.

Please just don't use likely/unlikely unless really nessecary.

Quoted 3 lines

However, I think you have to consider frequency, and also relative importance of each case. This is going to be taken on sys_sync, and sys_umount. And not taken for pdflush writeout.

Yes. So dynamic branch prediction will get this perfectly right on any reasonable CPU.

Quoted 7 lines
Quoted 3 lines

fs/fs-writeback.c void generic_sync_sb_inodes(struct super · line 541

Quoted 1 line
fs/fs-writeback.c +1 -0
+			/* see dquot.c comment */

Not an exactly useful comment :)

/* see fs/dquot.c:add_dquot_ref() comment */ ?

Thanks, I will repost with these changes after waiting for more feedback.

Please just copy the comment over. Looking somewhere else to follow a comment is not very nice (and this is the more important file compared to dquot.c anyway :))

#6 lore raw

Re: [rfc][patch 2/3] fs: sync_sb_inodes fix

Quoted 9 lines · Christoph Hellwig
Quoted 3 lines · Nick Piggin

Static branch hint instruction could indeed turn off dynamic prediction so in that cause it would always mispredict even if the workload was continually going the other way.

It's not just turning them off. In many cpu microarchitectures a wrong static prediction means an pipeline stall that's much worse than a dynamic mispredict because that information is interpreted much deeper in the pipeline.

Which ones? AFAIK, dynamic branch prediction happens no later than instruction decode, and can be earlier even than instruction fetch.

Quoted 1 line

Please just don't use likely/unlikely unless really nessecary.

Sure. It isn't really necessary here.

Quoted 6 lines
Quoted 3 lines

However, I think you have to consider frequency, and also relative importance of each case. This is going to be taken on sys_sync, and sys_umount. And not taken for pdflush writeout.

Yes. So dynamic branch prediction will get this perfectly right on any reasonable CPU.

Or, it will not have a relevant entry in the branch predictor, and short forward jumps tend to default to unlikely, so it might miss every time. Anyway... I won't argue any more :) I've gotten rid of it.

Quoted 11 lines
Quoted 7 lines
Quoted 3 lines

fs/fs-writeback.c void generic_sync_sb_inodes(struct super · line 541

Quoted 1 line
fs/fs-writeback.c +1 -0
+			/* see dquot.c comment */

Not an exactly useful comment :)

/* see fs/dquot.c:add_dquot_ref() comment */ ?

Thanks, I will repost with these changes after waiting for more feedback.

Please just copy the comment over. Looking somewhere else to follow a comment is not very nice (and this is the more important file compared to dquot.c anyway :))

OK.

#7 lore raw

RFC PATCH 3/3 fs: sys_sync fix

In reply to Nick Piggin

s_syncing livelock avoidance was breaking data integrity guarantee of sys_sync, by allowing sys_sync to skip writing or waiting for superblocks if there is a concurrent sys_sync happening.

This livelock avoidance is much less important now that we don't have the get_super_to_sync() call after every sb that we sync. This was replaced by __put_super_and_need_restart, which only triggers on unmount.

---

fs/fs-writeback.c +1 -19
Index: linux-2.6/fs/fs-writeback.c
===================================================================
@@ -645,18 +645,6 @@ void sync_inodes_sb(struct super_block *
 	sync_sb_inodes(sb, &wbc);
 }
 
-/*
- * Rather lame livelock avoidance.
- */
-static void set_sb_syncing(int val)
-{
-	struct super_block *sb;
-	spin_lock(&sb_lock);
-	list_for_each_entry_reverse(sb, &super_blocks, s_list)
-		sb->s_syncing = val;
-	spin_unlock(&sb_lock);
-}
-
 /**
  * sync_inodes - writes all inodes to disk
  * @wait: wait for completion
@@ -683,9 +671,6 @@ static void __sync_inodes(int wait)
 	spin_lock(&sb_lock);
 restart:
 	list_for_each_entry(sb, &super_blocks, s_list) {
-		if (sb->s_syncing)
-			continue;
-		sb->s_syncing = 1;
 		sb->s_count++;
 		spin_unlock(&sb_lock);
 		down_read(&sb->s_umount);
@@ -703,13 +688,10 @@ restart:
 
 void sync_inodes(int wait)
 {
-	set_sb_syncing(0);
 	__sync_inodes(0);
 
-	if (wait) {
-		set_sb_syncing(0);
+	if (wait)
 		__sync_inodes(1);
-	}
 }
 
 /**
include/linux/fs.h +0 -1
Index: linux-2.6/include/linux/fs.h
===================================================================
@@ -1122,7 +1122,6 @@ struct super_block {
 	struct rw_semaphore	s_umount;
 	struct mutex		s_lock;
 	int			s_count;
-	int			s_syncing;
 	int			s_need_sync_fs;
 	atomic_t		s_active;
 #ifdef CONFIG_SECURITY