Commit 5afced3b authored by Jan Kara's avatar Jan Kara

writeback: Avoid skipping inode writeback

Inode's i_io_list list head is used to attach inode to several different
lists - wb->{b_dirty, b_dirty_time, b_io, b_more_io}. When flush worker
prepares a list of inodes to writeback e.g. for sync(2), it moves inodes
to b_io list. Thus it is critical for sync(2) data integrity guarantees
that inode is not requeued to any other writeback list when inode is
queued for processing by flush worker. That's the reason why
writeback_single_inode() does not touch i_io_list (unless the inode is
completely clean) and why __mark_inode_dirty() does not touch i_io_list
if I_SYNC flag is set.

However there are two flaws in the current logic:

1) When inode has only I_DIRTY_TIME set but it is already queued in b_io
list due to sync(2), concurrent __mark_inode_dirty(inode, I_DIRTY_SYNC)
can still move inode back to b_dirty list resulting in skipping
writeback of inode time stamps during sync(2).

2) When inode is on b_dirty_time list and writeback_single_inode() races
with __mark_inode_dirty() like:

writeback_single_inode()		__mark_inode_dirty(inode, I_DIRTY_PAGES)
  inode->i_state |= I_SYNC
  __writeback_single_inode()
					  inode->i_state |= I_DIRTY_PAGES;
					  if (inode->i_state & I_SYNC)
					    bail
  if (!(inode->i_state & I_DIRTY_ALL))
  - not true so nothing done

We end up with I_DIRTY_PAGES inode on b_dirty_time list and thus
standard background writeback will not writeback this inode leading to
possible dirty throttling stalls etc. (thanks to Martijn Coenen for this
analysis).

Fix these problems by tracking whether inode is queued in b_io or
b_more_io lists in a new I_SYNC_QUEUED flag. When this flag is set, we
know flush worker has queued inode and we should not touch i_io_list.
On the other hand we also know that once flush worker is done with the
inode it will requeue the inode to appropriate dirty list. When
I_SYNC_QUEUED is not set, __mark_inode_dirty() can (and must) move inode
to appropriate dirty list.
Reported-by: default avatarMartijn Coenen <maco@android.com>
Reviewed-by: default avatarMartijn Coenen <maco@android.com>
Tested-by: default avatarMartijn Coenen <maco@android.com>
Reviewed-by: default avatarChristoph Hellwig <hch@lst.de>
Fixes: 0ae45f63 ("vfs: add support for a lazytime mount option")
CC: stable@vger.kernel.org
Signed-off-by: default avatarJan Kara <jack@suse.cz>
parent b35250c0
...@@ -146,6 +146,7 @@ static void inode_io_list_del_locked(struct inode *inode, ...@@ -146,6 +146,7 @@ static void inode_io_list_del_locked(struct inode *inode,
assert_spin_locked(&wb->list_lock); assert_spin_locked(&wb->list_lock);
assert_spin_locked(&inode->i_lock); assert_spin_locked(&inode->i_lock);
inode->i_state &= ~I_SYNC_QUEUED;
list_del_init(&inode->i_io_list); list_del_init(&inode->i_io_list);
wb_io_lists_depopulated(wb); wb_io_lists_depopulated(wb);
} }
...@@ -1187,6 +1188,7 @@ static void redirty_tail_locked(struct inode *inode, struct bdi_writeback *wb) ...@@ -1187,6 +1188,7 @@ static void redirty_tail_locked(struct inode *inode, struct bdi_writeback *wb)
inode->dirtied_when = jiffies; inode->dirtied_when = jiffies;
} }
inode_io_list_move_locked(inode, wb, &wb->b_dirty); inode_io_list_move_locked(inode, wb, &wb->b_dirty);
inode->i_state &= ~I_SYNC_QUEUED;
} }
static void redirty_tail(struct inode *inode, struct bdi_writeback *wb) static void redirty_tail(struct inode *inode, struct bdi_writeback *wb)
...@@ -1262,8 +1264,11 @@ static int move_expired_inodes(struct list_head *delaying_queue, ...@@ -1262,8 +1264,11 @@ static int move_expired_inodes(struct list_head *delaying_queue,
break; break;
list_move(&inode->i_io_list, &tmp); list_move(&inode->i_io_list, &tmp);
moved++; moved++;
spin_lock(&inode->i_lock);
if (flags & EXPIRE_DIRTY_ATIME) if (flags & EXPIRE_DIRTY_ATIME)
set_bit(__I_DIRTY_TIME_EXPIRED, &inode->i_state); inode->i_state |= I_DIRTY_TIME_EXPIRED;
inode->i_state |= I_SYNC_QUEUED;
spin_unlock(&inode->i_lock);
if (sb_is_blkdev_sb(inode->i_sb)) if (sb_is_blkdev_sb(inode->i_sb))
continue; continue;
if (sb && sb != inode->i_sb) if (sb && sb != inode->i_sb)
...@@ -1438,6 +1443,7 @@ static void requeue_inode(struct inode *inode, struct bdi_writeback *wb, ...@@ -1438,6 +1443,7 @@ static void requeue_inode(struct inode *inode, struct bdi_writeback *wb,
} else if (inode->i_state & I_DIRTY_TIME) { } else if (inode->i_state & I_DIRTY_TIME) {
inode->dirtied_when = jiffies; inode->dirtied_when = jiffies;
inode_io_list_move_locked(inode, wb, &wb->b_dirty_time); inode_io_list_move_locked(inode, wb, &wb->b_dirty_time);
inode->i_state &= ~I_SYNC_QUEUED;
} else { } else {
/* The inode is clean. Remove from writeback lists. */ /* The inode is clean. Remove from writeback lists. */
inode_io_list_del_locked(inode, wb); inode_io_list_del_locked(inode, wb);
...@@ -2301,11 +2307,12 @@ void __mark_inode_dirty(struct inode *inode, int flags) ...@@ -2301,11 +2307,12 @@ void __mark_inode_dirty(struct inode *inode, int flags)
inode->i_state |= flags; inode->i_state |= flags;
/* /*
* If the inode is being synced, just update its dirty state. * If the inode is queued for writeback by flush worker, just
* The unlocker will place the inode on the appropriate * update its dirty state. Once the flush worker is done with
* superblock list, based upon its state. * the inode it will place it on the appropriate superblock
* list, based upon its state.
*/ */
if (inode->i_state & I_SYNC) if (inode->i_state & I_SYNC_QUEUED)
goto out_unlock_inode; goto out_unlock_inode;
/* /*
......
...@@ -2157,6 +2157,10 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src, ...@@ -2157,6 +2157,10 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
* *
* I_DONTCACHE Evict inode as soon as it is not used anymore. * I_DONTCACHE Evict inode as soon as it is not used anymore.
* *
* I_SYNC_QUEUED Inode is queued in b_io or b_more_io writeback lists.
* Used to detect that mark_inode_dirty() should not move
* inode between dirty lists.
*
* Q: What is the difference between I_WILL_FREE and I_FREEING? * Q: What is the difference between I_WILL_FREE and I_FREEING?
*/ */
#define I_DIRTY_SYNC (1 << 0) #define I_DIRTY_SYNC (1 << 0)
...@@ -2174,12 +2178,12 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src, ...@@ -2174,12 +2178,12 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
#define I_DIO_WAKEUP (1 << __I_DIO_WAKEUP) #define I_DIO_WAKEUP (1 << __I_DIO_WAKEUP)
#define I_LINKABLE (1 << 10) #define I_LINKABLE (1 << 10)
#define I_DIRTY_TIME (1 << 11) #define I_DIRTY_TIME (1 << 11)
#define __I_DIRTY_TIME_EXPIRED 12 #define I_DIRTY_TIME_EXPIRED (1 << 12)
#define I_DIRTY_TIME_EXPIRED (1 << __I_DIRTY_TIME_EXPIRED)
#define I_WB_SWITCH (1 << 13) #define I_WB_SWITCH (1 << 13)
#define I_OVL_INUSE (1 << 14) #define I_OVL_INUSE (1 << 14)
#define I_CREATING (1 << 15) #define I_CREATING (1 << 15)
#define I_DONTCACHE (1 << 16) #define I_DONTCACHE (1 << 16)
#define I_SYNC_QUEUED (1 << 17)
#define I_DIRTY_INODE (I_DIRTY_SYNC | I_DIRTY_DATASYNC) #define I_DIRTY_INODE (I_DIRTY_SYNC | I_DIRTY_DATASYNC)
#define I_DIRTY (I_DIRTY_INODE | I_DIRTY_PAGES) #define I_DIRTY (I_DIRTY_INODE | I_DIRTY_PAGES)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment