Re: xfs_buftarg_isolate(): "Correctly invert xfs_buftarg LRU isolation logic"

[Brian Foster <bfoster@redhat.com>]

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@redhat.com> 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?).

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@redhat.com>
> > > 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@redhat.com>
> > >    Reviewed-by: Brian Foster <bfoster@redhat.com>
> > >    Reviewed-by: Christoph Hellwig <hch@lst.de>
> > >    Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > >    Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > >
> >