On Tue, 2024-08-13 at 16:36 +0200, Mateusz Guzik wrote:
> Most commonly neither I_LRU_ISOLATING nor I_SYNC are set, but the stock
> kernel takes a back-to-back relock trip to check for them.
>
> It probably can be avoided altogether, but for now massage things back
> to just one lock acquire.
>
> Signed-off-by: Mateusz Guzik <mjguzik@xxxxxxxxx>
> ---
>
> there are smp_mb's in the area I'm going to look at removing at some
> point(tm), in the meantime I think this is an easy cleanup
>
> has a side effect of whacking a inode_wait_for_writeback which was only
> there to deal with not holding the lock
>
> fs/fs-writeback.c | 17 +++--------------
> fs/inode.c | 5 +++--
> 2 files changed, 6 insertions(+), 16 deletions(-)
>
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 4451ecff37c4..1a5006329f6f 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -1510,13 +1510,12 @@ static int write_inode(struct inode *inode, struct writeback_control *wbc)
> * Wait for writeback on an inode to complete. Called with i_lock held.
> * Caller must make sure inode cannot go away when we drop i_lock.
> */
> -static void __inode_wait_for_writeback(struct inode *inode)
> - __releases(inode->i_lock)
> - __acquires(inode->i_lock)
> +void inode_wait_for_writeback(struct inode *inode)
> {
> DEFINE_WAIT_BIT(wq, &inode->i_state, __I_SYNC);
> wait_queue_head_t *wqh;
>
> + lockdep_assert_held(&inode->i_lock);
> wqh = bit_waitqueue(&inode->i_state, __I_SYNC);
> while (inode->i_state & I_SYNC) {
> spin_unlock(&inode->i_lock);
> @@ -1526,16 +1525,6 @@ static void __inode_wait_for_writeback(struct inode *inode)
> }
> }
>
> -/*
> - * Wait for writeback on an inode to complete. Caller must have inode pinned.
> - */
> -void inode_wait_for_writeback(struct inode *inode)
> -{
> - spin_lock(&inode->i_lock);
> - __inode_wait_for_writeback(inode);
> - spin_unlock(&inode->i_lock);
> -}
> -
> /*
> * Sleep until I_SYNC is cleared. This function must be called with i_lock
> * held and drops it. It is aimed for callers not holding any inode reference
> @@ -1757,7 +1746,7 @@ static int writeback_single_inode(struct inode *inode,
> */
> if (wbc->sync_mode != WB_SYNC_ALL)
> goto out;
> - __inode_wait_for_writeback(inode);
> + inode_wait_for_writeback(inode);
> }
> WARN_ON(inode->i_state & I_SYNC);
> /*
> diff --git a/fs/inode.c b/fs/inode.c
> index 73183a499b1c..d48d29d39cd2 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -582,7 +582,7 @@ static void inode_unpin_lru_isolating(struct inode *inode)
>
> static void inode_wait_for_lru_isolating(struct inode *inode)
> {
> - spin_lock(&inode->i_lock);
> + lockdep_assert_held(&inode->i_lock);
> if (inode->i_state & I_LRU_ISOLATING) {
> DEFINE_WAIT_BIT(wq, &inode->i_state, __I_LRU_ISOLATING);
> wait_queue_head_t *wqh;
> @@ -593,7 +593,6 @@ static void inode_wait_for_lru_isolating(struct inode *inode)
> spin_lock(&inode->i_lock);
> WARN_ON(inode->i_state & I_LRU_ISOLATING);
> }
> - spin_unlock(&inode->i_lock);
> }
>
> /**
> @@ -765,6 +764,7 @@ static void evict(struct inode *inode)
>
> inode_sb_list_del(inode);
>
> + spin_lock(&inode->i_lock);
> inode_wait_for_lru_isolating(inode);
>
> /*
> @@ -774,6 +774,7 @@ static void evict(struct inode *inode)
> * the inode. We just have to wait for running writeback to finish.
> */
> inode_wait_for_writeback(inode);
> + spin_unlock(&inode->i_lock);
>
> if (op->evict_inode) {
> op->evict_inode(inode);
...and a nice cleanup to boot.
Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
