Hi Brian, On Tue, Oct 22, 2019 at 2:06 PM Brian Foster <bfoster@xxxxxxxxxx> wrote: > > On Tue, Oct 22, 2019 at 09:49:56AM +0300, Alex Lyakas wrote: > > Hi Brian, > > > > Thank you for your response. > > > > On Mon, Oct 21, 2019 at 3:47 PM Brian Foster <bfoster@xxxxxxxxxx> wrote: > > > > > > 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. > > > > I apologize for the confusion. By "freed" I really meant "taken off > > the LRU". I am aware of the fact that b_hold is controlling whether > > the buffer will be freed or not. > > > > Ok. > > > What I meant is that the commit description does not address the issue > > accurately. From the description one can understand that the only > > problem is the additional trip through the LRU. > > But in fact, due to this issue, buffers will spend one cycle less in > > the LRU than intended. If we initialize b_lru_ref to X, we intend the > > buffer to survive X shrinker calls, and on the X+1'th call to be taken > > off the LRU (and maybe freed). But with this issue, the buffer will be > > taken off the LRU and immediately re-added back. But this will happen > > X-1 times, because on the X'th time the b_lru_ref will be 0, and the > > buffer will not be readded to the LRU. So the buffer will survive X-1 > > shrinker calls and not X as intended. > > > > That sounds like a reasonable description of behavior to me. Personally, > I also think the commit log description is fine because I read it > generically as just pointing out the logic is backwards. Your > description above is more detailed and probably more technically > accurate, but I'm not sure if/why it matters at this point for anything > beyond just attempting to understand a historical bug (for stable > backport purposes?). Mostly I wanted to confirm my understanding. I have sent the stable patch with this fix. Thanks, Alex. > > Brian > > > Furthermore, if somehow we end up with the buffer sitting on the LRU > > and having b_lru_ref==0, this buffer will never be taken off the LRU, > > due to the bug. I am not sure this can happen, because by default > > b_lru_ref is set to 1. > > > > > > > > > > > If so, I think this fix should be backported to stable kernels. > > > > > > > > > > Seems reasonable to me. Feel free to post a patch. :) > > Will do. > > > > Thanks, > > Alex. > > > > > > > > > > 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> > > > > > > > >
