Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 1 Nov 2019 08:05:13 -0400
From:   Brian Foster <bfoster@redhat.com>
To:     Dave Chinner <david@fromorbit.com>
Cc:     linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 04/28] xfs: Improve metadata buffer reclaim accountability
Message-ID: <20191101120513.GD59146@bfoster>
References: <20191031234618.15403-1-david@fromorbit.com>
 <20191031234618.15403-5-david@fromorbit.com>
MIME-Version: 1.0
In-Reply-To: <20191031234618.15403-5-david@fromorbit.com>
User-Agent: Mutt/1.12.1 (2019-06-15)
Content-Type: text/plain; charset=WINDOWS-1252
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Nov 01, 2019 at 10:45:54AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>=20
> The buffer cache shrinker frees more than just the xfs_buf slab
> objects - it also frees the pages attached to the buffers. Make sure
> the memory reclaim code accounts for this memory being freed
> correctly, similar to how the inode shrinker accounts for pages
> freed from the page cache due to mapping invalidation.
>=20
> We also need to make sure that the mm subsystem knows these are
> reclaimable objects. We provide the memory reclaim subsystem with a
> a shrinker to reclaim xfs_bufs, so we should really mark the slab
> that way.
>=20
> We also have a lot of xfs_bufs in a busy system, spread them around
> like we do inodes.
>=20
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

I still don't see why we wouldn't set the spread flag on the bli cache
as well, but afaict it doesn't matter in most cases unless the spread
knob is enabled. Unless I'm misunderstanding how that works, I think the
commit log could be improved to describe that since to me it implies the
flag by itself has an effect, but otherwise the change seems fine:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_buf.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>=20
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 1e63dd3d1257..d34e5d2edacd 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -324,6 +324,9 @@ xfs_buf_free(
> =20
>  =09=09=09__free_page(page);
>  =09=09}
> +=09=09if (current->reclaim_state)
> +=09=09=09current->reclaim_state->reclaimed_slab +=3D
> +=09=09=09=09=09=09=09bp->b_page_count;
>  =09} else if (bp->b_flags & _XBF_KMEM)
>  =09=09kmem_free(bp->b_addr);
>  =09_xfs_buf_free_pages(bp);
> @@ -2061,7 +2064,8 @@ int __init
>  xfs_buf_init(void)
>  {
>  =09xfs_buf_zone =3D kmem_zone_init_flags(sizeof(xfs_buf_t), "xfs_buf",
> -=09=09=09=09=09=09KM_ZONE_HWALIGN, NULL);
> +=09=09=09KM_ZONE_HWALIGN | KM_ZONE_SPREAD | KM_ZONE_RECLAIM,
> +=09=09=09NULL);
>  =09if (!xfs_buf_zone)
>  =09=09goto out;
> =20
> --=20
> 2.24.0.rc0
>=20