On Fri 20-05-11 05:31:19, Wu Fengguang wrote: > > > > @@ -419,6 +419,15 @@ writeback_single_inode(struct inode *ino > > > > spin_lock(&inode->i_lock); > > > > inode->i_state &= ~I_SYNC; > > > > if (!(inode->i_state & I_FREEING)) { > > > > + /* > > > > + * Sync livelock prevention. Each inode is tagged and synced in > > > > + * one shot, so we can unconditionally update its dirty time to > > > > + * prevent syncing it again. Note that time ordering of b_dirty > > > > + * list will be kept because the following code either removes > > > > + * the inode from b_dirty or calls redirty_tail(). > > > > + */ > > > > + if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_sync) > > > > + inode->dirtied_when = jiffies; > > > > > > Shouldn't this update only ocur if the inode is still dirty? > > > > Yeah, that would be better even though the current form won't lead to > > errors. > > > > Let's add one more test (inode->i_state & I_DIRTY)? > > (I was actually aware of the trade offs and didn't bother to add it..) > > Oops, the (inode->i_state & I_DIRTY) test is not enough because > when the inode is actively dirtied, I_DIRTY_PAGES won't be set when > the flusher is writing out the pages with I_SYNC set... Well, rather on contrary I_DIRTY_PAGES won't be set when noone dirtied any page after we cleared I_DIRTY_PAGES. > Well it would look clumsy to add another mapping_tagged(mapping, > PAGECACHE_TAG_DIRTY) test. So I tend to revert to the original scheme > of updating ->dirtied_when only on newly dirtied pages. It's the > simplest form that can avoid unnecessarily polluting ->dirtied_when. Hmm, but won't now something like: while true; do touch f; done livelock sync? We always manage to write something - 1 inode - and the inode will never be clean (before the IO completes, the other process manages to dirty the inode again with very high probability). Honza > > @@ -432,6 +432,15 @@ writeback_single_inode(struct inode *ino > requeue_io(inode); > } else { > /* > + * Sync livelock prevention. Each inode is > + * tagged and synced in one shot, so we can > + * unconditionally update its dirty time to > + * prevent syncing it again. > + */ > + if (wbc->sync_mode == WB_SYNC_ALL || > + wbc->tagged_writepages) > + inode->dirtied_when = jiffies; > + /* > * Writeback blocked by something other than > * congestion. Delay the inode for some time to > * avoid spinning on the CPU (100% iowait) > > Thanks, > Fengguang -- Jan Kara <jack@xxxxxxx> SUSE Labs, CR -- 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