On Sun, Jul 31, 2011 at 03:40:20PM -1000, Linus Torvalds wrote: > On Sun, Jul 31, 2011 at 3:28 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote: > > > > IOWs, what I'm asking is whether this "just move the inodes one at a > > time to a different queue" is just a bandaid for a particular > > symptom of a deeper problem we haven't realised existed.... > > Deeper problems in writeback? Unpossible. Heh. But that's exactly why I'd like to understand the problem fully. > The writeback code has pretty much always been just a collection of > "bandaids for particular symptoms of deeper problems". So let's just > say I'd not be shocked. But what else would you suggest? You could > just break out of the loop if you can't get the read lock, but while > the *common* case is likely that a lot of the inodes are on the same > filesystem, that's certainly not the only possible case. Right, but in this specific case of executing writeback_inodes_wb(), we can only be operating on a specific bdi without being told which sb to flush. If we are told which sb, then we go through __writeback_inodes_sb() and avoid the grab_super_passive() altogether because some other thread holds the s_umount lock. These no-specific-sb cases can come only from wb_check_background_flush() or wb_check_old_data_flush() which, by definition, are oppurtunist background asynchronous writeback executed only when there is no other work to do. Further, if there is new work queued while they are running, they abort. Hence if we can't grab the superblock here, it is simply another case of a "new work pending" interrupt, right? And so aborting the work is the correct thing to do? Especially as it avoids all the ordering problems of redirtying inodes and allows the writeback work to restart (form whatever context it is stared from next time) where it stopped. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
