Revert "writeback: plug writeback at a high level"

torvalds · torvalds · commit 0ba13fd19d39 · 2015-09-11T13:26:39.000-07:00
This reverts commit d353d75. Doing the block layer plug/unplug inside writeback_sb_inodes() is broken, because that function is actually called with a spinlock held: wb->list_lock, as pointed out by Chris Mason. Chris suggested just dropping and re-taking the spinlock around the blk_finish_plug() call (the plgging itself can happen under the spinlock), and that would technically work, but is just disgusting. We do something fairly similar - but not quite as disgusting because we at least have a better reason for it - in writeback_single_inode(), so it's not like the caller can depend on the lock being held over the call, but in this case there just isn't any good reason for that "release and re-take the lock" pattern. [ In general, we should really strive to avoid the "release and retake" pattern for locks, because in the general case it can easily cause subtle bugs when the caller caches any state around the call that might be invalidated by dropping the lock even just temporarily. ] But in this case, the plugging should be easy to just move up to the callers before the spinlock is taken, which should even improve the effectiveness of the plug. So there is really no good reason to play games with locking here. I'll send off a test-patch so that Dave Chinner can verify that that plug movement works. In the meantime this just reverts the problematic commit and adds a comment to the function so that we hopefully don't make this mistake again. Reported-by: Chris Mason <clm@fb.com> Cc: Josef Bacik <jbacik@fb.com> Cc: Dave Chinner <david@fromorbit.com> Cc: Neil Brown <neilb@suse.de> Cc: Jan Kara <jack@suse.cz> Cc: Christoph Hellwig <hch@lst.de> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
@@ -1380,6 +1380,10 @@ static long writeback_chunk_size(struct bdi_writeback *wb,
  * Write a portion of b_io inodes which belong to @sb.
  *
  * Return the number of pages and/or inodes written.
+ *
+ * NOTE! This is called with wb->list_lock held, and will
+ * unlock and relock that for each inode it ends up doing
+ * IO for.
  */
 static long writeback_sb_inodes(struct super_block *sb,
 				struct bdi_writeback *wb,
@@ -1398,9 +1402,7 @@ static long writeback_sb_inodes(struct super_block *sb,
 	unsigned long start_time = jiffies;
 	long write_chunk;
 	long wrote = 0;  /* count both pages and inodes */
-	struct blk_plug plug;
 
-	blk_start_plug(&plug);
 	while (!list_empty(&wb->b_io)) {
 		struct inode *inode = wb_inode(wb->b_io.prev);
 
@@ -1498,7 +1500,6 @@ static long writeback_sb_inodes(struct super_block *sb,
 				break;
 		}
 	}
-	blk_finish_plug(&plug);
 	return wrote;
 }
 

Original file line number	Diff line number	Diff line change
`@@ -1380,6 +1380,10 @@ static long writeback_chunk_size(struct bdi_writeback *wb,`
`1380`	`1380`	`* Write a portion of b_io inodes which belong to @sb.`
`1381`	`1381`	`*`
`1382`	`1382`	`* Return the number of pages and/or inodes written.`
	`1383`	`+ *`
	`1384`	`+ * NOTE! This is called with wb->list_lock held, and will`
	`1385`	`+ * unlock and relock that for each inode it ends up doing`
	`1386`	`+ * IO for.`
`1383`	`1387`	`*/`
`1384`	`1388`	`static long writeback_sb_inodes(struct super_block *sb,`
`1385`	`1389`	`struct bdi_writeback *wb,`
`@@ -1398,9 +1402,7 @@ static long writeback_sb_inodes(struct super_block *sb,`
`1398`	`1402`	`unsigned long start_time = jiffies;`
`1399`	`1403`	`long write_chunk;`
`1400`	`1404`	`long wrote = 0; /* count both pages and inodes */`
`1401`		`- struct blk_plug plug;`
`1402`	`1405`
`1403`		`- blk_start_plug(&plug);`
`1404`	`1406`	`while (!list_empty(&wb->b_io)) {`
`1405`	`1407`	`struct inode *inode = wb_inode(wb->b_io.prev);`
`1406`	`1408`
`@@ -1498,7 +1500,6 @@ static long writeback_sb_inodes(struct super_block *sb,`
`1498`	`1500`	`break;`
`1499`	`1501`	`}`
`1500`	`1502`	`}`
`1501`		`- blk_finish_plug(&plug);`
`1502`	`1503`	`return wrote;`
`1503`	`1504`	`}`
`1504`	`1505`