| Message ID | 20240813143626.1573445-1-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vfs: drop one lock trip in evict() | expand |
在 2024/8/13 22:36, Mateusz Guzik 写道: > 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@gmail.com> > --- Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com> > > 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); >
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@gmail.com> > --- > > 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@kernel.org>
On Tue, 13 Aug 2024 16:36:26 +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. > > > [...] Applied to the vfs.fixes branch of the vfs/vfs.git tree. Patches in the vfs.fixes branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.fixes [1/1] vfs: drop one lock trip in evict() https://git.kernel.org/vfs/vfs/c/8b30d568bd8d
On Tue 13-08-24 16:36:26, 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@gmail.com> Back from vacation so not sure if this is still actual but the patch looks good to me. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > > 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); > -- > 2.43.0 >
On Mon, Aug 26, 2024 at 10:27:46PM GMT, Jan Kara wrote: > On Tue 13-08-24 16:36:26, 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@gmail.com> > > Back from vacation so not sure if this is still actual but the patch looks > good to me. Feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> It's all still in vfs.misc so we can still add RvBs. So go ahead and ack whatever you want to ack. :)
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);
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@gmail.com> --- 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(-)