xfs: Correctly invert xfs_buftarg LRU isolation logic
PATCH 1/1 xfs: Correctly invert xfs_buftarg LRU isolation logic
The function xfs_buftarg_isolate() used by xfs buffer schrinkers to determine whether a buffer should be isolated and disposed from LRU list, has inverted logic.
Excerpt from xfs_buftarg_isolate():
/*
* Decrement the b_lru_ref count unless the value is already
* zero. If the value is already zero, we need to reclaim the
* buffer, otherwise it gets another trip through the LRU.
*/
if (!atomic_add_unless(&bp->b_lru_ref, -1, 0)) {
spin_unlock(&bp->b_lock);
return LRU_ROTATE;
}However, as per documentation, atomic_add_unless() returns _zero_ if the atomic value was originally equal to the specified *unsless* value.
Ultimately causing a xfs_buffer with ->b_lru_ref == 0, to take another trip around LRU, while isolating buffers with non-zero b_lru_ref.
--- fs/xfs/xfs_buf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/xfs/xfs_buf.c +1 -1
@@ -1708,7 +1708,7 @@ xfs_buftarg_isolate(* zero. If the value is already zero, we need to reclaim the * buffer, otherwise it gets another trip through the LRU. */ -if (!atomic_add_unless (&bp->b_lru_ref,-1 ,0 )) { +if (atomic_add_unless (&bp->b_lru_ref,-1 ,0 )) {spin_unlock (&bp->b_lock);return LRU_ROTATE ; }
Signature · 1 line
2.14.3
Quoted 15 lines · Vratislav Bendel
The function
xfs_buftarg_isolate()used by xfs buffer schrinkers to determine whether a buffer should be isolated and disposed from LRU list, has inverted logic.Excerpt from
xfs_buftarg_isolate():/* * Decrement the b_lru_ref count unless the value is already * zero. If the value is already zero, we need to reclaim the * buffer, otherwise it gets another trip through the LRU. */ if (!atomic_add_unless(&bp->b_lru_ref, -1, 0)) { spin_unlock(&bp->b_lock); return LRU_ROTATE; }
Quoted 8 lines
However, as per documentation,
atomic_add_unless()returns _zero_ if the atomic value was originally equal to the specified *unsless* value.Ultimately causing a
xfs_bufferwith ->b_lru_ref == 0, to take another trip around LRU, while isolating buffers with non-zerob_lru_ref.Signed-off-by Vratislav Bendel <[email protected]>CC Brian Foster <[email protected]>
Can you add a respective Fixes: tag? Also what effects are observed by the user when this happens on the kernel log?
Luis
Quoted 15 lines · Vratislav Bendel
The function
xfs_buftarg_isolate()used by xfs buffer schrinkers to determine whether a buffer should be isolated and disposed from LRU list, has inverted logic.Excerpt from
xfs_buftarg_isolate():/* * Decrement the b_lru_ref count unless the value is already * zero. If the value is already zero, we need to reclaim the * buffer, otherwise it gets another trip through the LRU. */ if (!atomic_add_unless(&bp->b_lru_ref, -1, 0)) { spin_unlock(&bp->b_lock); return LRU_ROTATE; }
Quoted 3 lines
However, as per documentation,
atomic_add_unless()returns _zero_ if the atomic value was originally equal to the specified *unsless* value.
Nit: unless
Quoted 6 lines
Ultimately causing a
xfs_bufferwith ->b_lru_ref == 0, to take another trip around LRU, while isolating buffers with non-zerob_lru_ref.Signed-off-by Vratislav Bendel <[email protected]>CC Brian Foster <[email protected]>---
It might be worth pointing out in the commit log that currently isolated buffers end up right back on the LRU once they are released, because ->b_lru_ref remains elevated. Therefore, this patch essentially fixes that circuitous route by leaving them on the LRU as originally intended. Otherwise this looks Ok to me:
Thanks for sending the patch.
Brian
Quoted 23 lines
fs/xfs/xfs_buf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/xfs/xfs_buf.c +1
-1@@ -1708,7 +1708,7 @@ xfs_buftarg_isolate(* zero. If the value is already zero, we need to reclaim the * buffer, otherwise it gets another trip through the LRU. */ -if (!atomic_add_unless (&bp->b_lru_ref,-1 ,0 )) { +if (atomic_add_unless (&bp->b_lru_ref,-1 ,0 )) {spin_unlock (&bp->b_lock);return LRU_ROTATE ; }Signature · 6 lines
2.14.3 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 15 lines · Vratislav Bendel
The function
xfs_buftarg_isolate()used by xfs buffer schrinkers to determine whether a buffer should be isolated and disposed from LRU list, has inverted logic.Excerpt from
xfs_buftarg_isolate():/* * Decrement the b_lru_ref count unless the value is already * zero. If the value is already zero, we need to reclaim the * buffer, otherwise it gets another trip through the LRU. */ if (!atomic_add_unless(&bp->b_lru_ref, -1, 0)) { spin_unlock(&bp->b_lock); return LRU_ROTATE; }
Quoted 8 lines
However, as per documentation,
atomic_add_unless()returns _zero_ if the atomic value was originally equal to the specified *unsless* value.Ultimately causing a
xfs_bufferwith ->b_lru_ref == 0, to take another trip around LRU, while isolating buffers with non-zerob_lru_ref.Signed-off-by Vratislav Bendel <[email protected]>CC Brian Foster <[email protected]>
Looks ok, will test...
--D
Quoted 20 lines
--- fs/xfs/xfs_buf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/xfs/xfs_buf.c +1
-1@@ -1708,7 +1708,7 @@ xfs_buftarg_isolate(* zero. If the value is already zero, we need to reclaim the * buffer, otherwise it gets another trip through the LRU. */ -if (!atomic_add_unless (&bp->b_lru_ref,-1 ,0 )) { +if (atomic_add_unless (&bp->b_lru_ref,-1 ,0 )) {spin_unlock (&bp->b_lock);return LRU_ROTATE ; }Signature · 1 line
2.14.3
Quoted 19 lines · Darrick J. Wong
Quoted 18 lines · Vratislav Bendel
The function
xfs_buftarg_isolate()used by xfs buffer schrinkers to determine whether a buffer should be isolated and disposed from LRU list, has inverted logic.Excerpt from
xfs_buftarg_isolate():/* * Decrement the b_lru_ref count unless the value is already * zero. If the value is already zero, we need to reclaim the * buffer, otherwise it gets another trip through the LRU. */ if (!atomic_add_unless(&bp->b_lru_ref, -1, 0)) { spin_unlock(&bp->b_lock); return LRU_ROTATE; }However, as per documentation,
atomic_add_unless()returns _zero_ if the atomic value was originally equal to the specified *unsless* value.
Quoted 8 lines
Quoted 5 lines
Ultimately causing a
xfs_bufferwith ->b_lru_ref == 0, to take another trip around LRU, while isolating buffers with non-zerob_lru_ref.Signed-off-by Vratislav Bendel <[email protected]>CC Brian Foster <[email protected]>Looks ok, will test...
Reviewed-by Darrick J. Wong <[email protected]>
This tests ok, but please address Brian and Luis' comments before I put this in the upstream tream.
--D
Quoted 26 lines
--D
Quoted 20 lines
--- fs/xfs/xfs_buf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/xfs/xfs_buf.c +1
-1@@ -1708,7 +1708,7 @@ xfs_buftarg_isolate(* zero. If the value is already zero, we need to reclaim the * buffer, otherwise it gets another trip through the LRU. */ -if (!atomic_add_unless (&bp->b_lru_ref,-1 ,0 )) { +if (atomic_add_unless (&bp->b_lru_ref,-1 ,0 )) {spin_unlock (&bp->b_lock);return LRU_ROTATE ; }Signature · 1 line
2.14.3--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
(In response to Luis' comment:)
Quoted 1 line
Can you add a respective Fixes: tag?
It was apparently present since LRU was added to xfs buffer cache via:
commit 430cbeb86fdcbbdabea7d4aa65307de8de425350
[xfs: add a lru to the XFS buffer cache]
But I wouldn't say this patch "fixes" that commit. What do you think? Should a fixes tag be added in this case?
Quoted 2 lines
Also what effects are observed by
the user when this happens on the kernel log?
I haven't spotted any differences visible to user, nor in the kernel log.
(In response to Brian's comment:)
Quoted 4 lines
Quoted 3 lines
However, as per documentation,
atomic_add_unless()returns _zero_ if the atomic value was originally equal to the specified *unsless* value.Nit: unless
Thanks very much for feedback. Since it's my very first upstream commit-proposal,
I expected that some polish would be needed.
Quoted 5 lines
It might be worth pointing out in the commit log that currently isolated buffers end up right back on the LRU once they are released, because ->b_lru_ref remains elevated. Therefore, this patch essentially fixes that circuitous route by leaving them on the LRU as originally intended. Otherwise this looks Ok to me:
So the final commit message could be:
~~~
Currently the xfs_buftarg_isolate() is causing an xfs_buffer with zero b_lru_ref, to take another trip around LRU, while isolating buffers with non-zero b_lru_ref.
Additionally those isolated buffers end up right back on the LRU once they are released, because ->b_lru_ref remains elevated.
Fix that circuitous route by leaving them on the LRU as originally intended.
Quoted 3 lines
Quoted 1 line
Signed-off-by Vratislav Bendel <[email protected]>Reviewed-by Brian Foster <[email protected]>---
Darrick J. Wong
Reviewed-byQuoted 33 lines · Vratislav Bendel
(In response to Luis' comment:)
Quoted 1 line
Can you add a respective Fixes: tag?
It was apparently present since LRU was added to xfs buffer cache via:
commit430cbeb86fdcbbdabea7d4aa65307de8de425350
[xfs: add a lru to the XFS buffer cache]But I wouldn't say this patch "fixes" that commit. What do you think? Should a fixes tag be added in this case?
Quoted 2 lines
Also what effects are observed by
the user when this happens on the kernel log?I haven't spotted any differences visible to user, nor in the kernel log.
(In response to Brian's comment:)
Quoted 4 lines
Quoted 3 lines
However, as per documentation,
atomic_add_unless()returns _zero_ if the atomic value was originally equal to the specified *unsless* value.Nit: unless
Thanks very much for feedback. Since it's my very first upstream commit-proposal,
I expected that some polish would be needed.Quoted 5 lines
It might be worth pointing out in the commit log that currently isolated buffers end up right back on the LRU once they are released, because ->b_lru_ref remains elevated. Therefore, this patch essentially fixes that circuitous route by leaving them on the LRU as originally intended. Otherwise this looks Ok to me:
Quoted 3 lines
So the final commit message could be:
~~~
Currently thexfs_buftarg_isolate()is causing anxfs_buffer
"Due to an inverted logic mistake in xfs_buftarg_isolate()..."?
Quoted 1 line
with zero
b_lru_ref, to take another trip around LRU, while
^^^^ no need for this comma
Quoted 7 lines
isolating buffers with non-zero
b_lru_ref.Additionally those isolated buffers end up right back on the LRU once they are released, because ->b_lru_ref remains elevated.
Fix that circuitous route by leaving them on the LRU as originally intended.
Otherwise this seems fine to me; can you please resend the patch w/ updated change log and reviewed-by tags?
--D
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Signed-off-by Vratislav Bendel <[email protected]>Reviewed-by Brian Foster <[email protected]>---
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html