diff mbox series

[v2] writeback: Fix inode->i_io_list not be protected by inode->i_lock error

Message ID 20220504075421.105494-1-sunjunchao2870@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] writeback: Fix inode->i_io_list not be protected by inode->i_lock error | expand

Commit Message

Julian Sun May 4, 2022, 7:54 a.m. UTC 
Commit b35250c0816c ("writeback: Protect inode->i_io_list with
inode->i_lock") made inode->i_io_list not only protected by
wb->list_lock but also inode->i_lock, but
inode_io_list_move_locked() was missed. Add lock there and also
update comment describing things protected by inode->i_lock.

Fixes: b35250c0816c ("writeback: Protect inode->i_io_list with inode->i_lock")
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Jchao Sun <sunjunchao2870@gmail.com>
---
 fs/fs-writeback.c | 11 +++++------
 fs/inode.c        |  2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

Comments

Jan Kara May 4, 2022, 2:39 p.m. UTC | #1 
On Wed 04-05-22 00:54:21, Jchao Sun wrote:
> Commit b35250c0816c ("writeback: Protect inode->i_io_list with
> inode->i_lock") made inode->i_io_list not only protected by
> wb->list_lock but also inode->i_lock, but
> inode_io_list_move_locked() was missed. Add lock there and also
> update comment describing things protected by inode->i_lock.
> 
> Fixes: b35250c0816c ("writeback: Protect inode->i_io_list with inode->i_lock")
> Reviewed-by: Jan Kara <jack@suse.cz>

You have significantly modified the patch since last time (which is good
since 0-day testing has found a problem I have missed). But in such case
please do not add Reviewed-by tags (and also we usually drop already
existing ones in the patch) because the patch needs a new review.

> @@ -317,6 +318,7 @@ locked_inode_to_wb_and_lock_list(struct inode *inode)
>  		/* i_wb may have changed inbetween, can't use inode_to_wb() */
>  		if (likely(wb == inode->i_wb)) {
>  			wb_put(wb);	/* @inode already has ref */
> +			spin_lock(&inode->i_lock);
>  			return wb;
>  		}
>  

Please no. I agree the inode->i_lock and wb->list_lock handling in
fs/fs-writeback.c is ugly and perhaps deserves a cleanup but this isn't
making it easier to understand and it definitely belongs to a separate
patch. Also the problem in __mark_inode_dirty() is deeper than just not
holding inode->i_lock when calling inode_io_list_move_locked(). The problem
is that locked_inode_to_wb_and_lock_list() drops inode->i_lock and at that
moment inode can be moved to new lists, writeback can be started on it,
etc. So all the checks we have performed upto that moment are not valid
anymore. So what we need to do is to move
locked_inode_to_wb_and_lock_list() call up, perhaps just after
"inode->i_state |= flags;". Then lock inode->i_lock again, and then perform
all the checks we need and list moving etc.

> Sry for my insufficient tests and any inconvenience in lauguage, because
> my mother tongue is not english. It's my first commit to linux kernel
> community, some strage for me...

Yeah, no problem. You are doing good :)

								Honza
diff mbox series

Patch

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 591fe9cf1659..30f9698e7c2c 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -120,6 +120,7 @@  static bool inode_io_list_move_locked(struct inode *inode,
 				      struct list_head *head)
 {
 	assert_spin_locked(&wb->list_lock);
+	assert_spin_locked(&inode->i_lock);
 
 	list_move(&inode->i_io_list, head);
 
@@ -293,8 +294,8 @@  static void inode_cgwb_move_to_attached(struct inode *inode,
  * @inode: inode of interest with i_lock held
  *
  * Returns @inode's wb with its list_lock held.  @inode->i_lock must be
- * held on entry and is released on return.  The returned wb is guaranteed
- * to stay @inode's associated wb until its list_lock is released.
+ * held on entry.  The returned wb is guaranteed to stay @inode's associated
+ * wb until its list_lock is released.
  */
 static struct bdi_writeback *
 locked_inode_to_wb_and_lock_list(struct inode *inode)
@@ -317,6 +318,7 @@  locked_inode_to_wb_and_lock_list(struct inode *inode)
 		/* i_wb may have changed inbetween, can't use inode_to_wb() */
 		if (likely(wb == inode->i_wb)) {
 			wb_put(wb);	/* @inode already has ref */
+			spin_lock(&inode->i_lock);
 			return wb;
 		}
 
@@ -1141,7 +1143,6 @@  locked_inode_to_wb_and_lock_list(struct inode *inode)
 {
 	struct bdi_writeback *wb = inode_to_wb(inode);
 
-	spin_unlock(&inode->i_lock);
 	spin_lock(&wb->list_lock);
 	return wb;
 }
@@ -1152,6 +1153,7 @@  static struct bdi_writeback *inode_to_wb_and_lock_list(struct inode *inode)
 	struct bdi_writeback *wb = inode_to_wb(inode);
 
 	spin_lock(&wb->list_lock);
+	spin_lock(&inode->i_lock);
 	return wb;
 }
 
@@ -1233,7 +1235,6 @@  void inode_io_list_del(struct inode *inode)
 	struct bdi_writeback *wb;
 
 	wb = inode_to_wb_and_lock_list(inode);
-	spin_lock(&inode->i_lock);
 
 	inode->i_state &= ~I_SYNC_QUEUED;
 	list_del_init(&inode->i_io_list);
@@ -1704,7 +1705,6 @@  static int writeback_single_inode(struct inode *inode,
 	wbc_detach_inode(wbc);
 
 	wb = inode_to_wb_and_lock_list(inode);
-	spin_lock(&inode->i_lock);
 	/*
 	 * If the inode is now fully clean, then it can be safely removed from
 	 * its writeback list (if any).  Otherwise the flusher threads are
@@ -1875,7 +1875,6 @@  static long writeback_sb_inodes(struct super_block *sb,
 		 * have been switched to another wb in the meantime.
 		 */
 		tmp_wb = inode_to_wb_and_lock_list(inode);
-		spin_lock(&inode->i_lock);
 		if (!(inode->i_state & I_DIRTY_ALL))
 			wrote++;
 		requeue_inode(inode, tmp_wb, &wbc);
diff --git a/fs/inode.c b/fs/inode.c
index 9d9b422504d1..bd4da9c5207e 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -27,7 +27,7 @@ 
  * Inode locking rules:
  *
  * inode->i_lock protects:
- *   inode->i_state, inode->i_hash, __iget()
+ *   inode->i_state, inode->i_hash, __iget(), inode->i_io_list
  * Inode LRU list locks protect:
  *   inode->i_sb->s_inode_lru, inode->i_lru
  * inode->i_sb->s_inode_list_lock protects: