On Sun, Oct 20, 2019 at 05:54:03PM +0300, Alex Lyakas wrote: > Hello Vratislav, Brian, > > This is with regards to commit "xfs: Correctly invert xfs_buftarg LRU > isolation logic" [1]. > > I am hitting this issue in kernel 4.14. However, after some debugging, I do > not fully agree with the commit message, describing the effect of this > defect. > > In case b_lru_ref > 1, then indeed this xfs_buf will be taken off the LRU > list, and immediately added back to it, with b_lru_ref being lesser by 1 > now. > > In case b_lru_ref==1, then this xfs_buf will be similarly isolated (due to a > bug), and xfs_buf_rele() will be called on it. But now its b_lru_ref==0. In > this case, xfs_buf_rele() will free the buffer, rather than re-adding it > back to the LRU. This is a problem, because we intended for this buffer to > have another trip on the LRU. Only when b_lru_ref==0 upon entry to > xfs_buftarg_isolate(), we want to free the buffer. So we are freeing the > buffer one trip too early in this case. > > In case b_lru_ref==0 (somehow), then due to a bug, this xfs_buf will not be > removed off the LRU. It will remain sitting in the LRU with b_lru_ref==0. On > next shrinker call, this xfs_buff will also remain on the LRU, due to the > same bug. So this xfs_buf will be freed only on unmount or if > xfs_buf_stale() is called on it. > > Do you agree with the above? > I'm not really sure how you're inferring what cases free the buffer vs. what don't based on ->b_lru_ref. A buffer is freed when its reference count (->b_hold) drops to zero unless ->b_lru_ref is non-zero, at which point the buffer goes on the LRU and the LRU itself takes a ->b_hold reference to keep the buffer from being freed. This reference is not associated with how many cycles through the LRU a buffer is allowed before it is dropped from the LRU, which is what ->b_lru_ref tracks. Since the LRU holds a (single) reference to the buffer just like any other user of the buffer, it doesn't make any direct decision on when to free a buffer or not. In other words, the bug fixed by this patch is related to when we decide to drop the buffer from the LRU based on the LRU ref count. If the LRU ->b_hold reference happens to be the last on the buffer when it is dropped from the LRU, then the buffer is freed. > If so, I think this fix should be backported to stable kernels. > Seems reasonable to me. Feel free to post a patch. :) Brian > Thanks, > Alex. > > [1] > commit 19957a181608d25c8f4136652d0ea00b3738972d > Author: Vratislav Bendel <vbendel@xxxxxxxxxx> > Date: Tue Mar 6 17:07:44 2018 -0800 > > xfs: Correctly invert xfs_buftarg LRU isolation logic > > Due to an inverted logic mistake in xfs_buftarg_isolate() > the xfs_buffers with zero b_lru_ref will 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. > > Signed-off-by: Vratislav Bendel <vbendel@xxxxxxxxxx> > Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx> > Reviewed-by: Christoph Hellwig <hch@xxxxxx> > Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> >
