Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
fork

Configure Feed

Select the types of activity you want to include in your feed.

Revert "writeback: plug writeback at a high level"

This reverts commit d353d7587d02116b9732d5c06615aed75a4d3a47.

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>

+4 -3
+4 -3
fs/fs-writeback.c
··· 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, ··· 1402 1398 unsigned long start_time = jiffies; 1403 1399 long write_chunk; 1404 1400 long wrote = 0; /* count both pages and inodes */ 1405 - struct blk_plug plug; 1406 1401 1407 - blk_start_plug(&plug); 1408 1402 while (!list_empty(&wb->b_io)) { 1409 1403 struct inode *inode = wb_inode(wb->b_io.prev); 1410 1404 ··· 1500 1498 break; 1501 1499 } 1502 1500 } 1503 - blk_finish_plug(&plug); 1504 1501 return wrote; 1505 1502 } 1506 1503