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

[Alex Lyakas <alex@zadara.com>]

Hi Brian,

On Tue, Oct 22, 2019 at 2:06 PM Brian Foster <bfoster@redhat.com> 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@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?).
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@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>
> > > >
> > >
>