Re: [PATCH 23/26] xfs: reclaim inodes from the LRU

From: Christoph Hellwig <hch@infradead.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org, linux-mm@kvack.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 23/26] xfs: reclaim inodes from the LRU
Date: Fri, 11 Oct 2019 03:56:18 -0700	[thread overview]
Message-ID: <20191011105618.GE12811@infradead.org> (raw)
In-Reply-To: <20191009032124.10541-24-david@fromorbit.com>

> +++ b/fs/xfs/xfs_icache.c
> @@ -1193,7 +1193,7 @@ xfs_reclaim_inode(
>   *
>   * Return the number of inodes freed.
>   */
> -STATIC int
> +int
>  xfs_reclaim_inodes_ag(
>  	struct xfs_mount	*mp,
>  	int			flags,

This looks odd.  This function actually is unused now.  I think you
want to fold in the patch that removes it instead of this little hack
to make the compiler happy.

> -	xfs_reclaim_inodes_ag(mp, SYNC_WAIT, INT_MAX);
> +        struct xfs_ireclaim_args *ra = arg;
> +        struct inode		*inode = container_of(item, struct inode, i_lru);
> +        struct xfs_inode	*ip = XFS_I(inode);

Whitespace damage, and a line > 80 chars.

> +out_ifunlock:
> +	xfs_ifunlock(ip);

This error path will instantly deadlock, given that xfs_ifunlock takes
i_flags_lock through xfs_iflags_clear, and we already hold it here.

> +	/*
> +	 * Remove the inode from the per-AG radix tree.
> +	 *
> +	 * Because radix_tree_delete won't complain even if the item was never
> +	 * added to the tree assert that it's been there before to catch
> +	 * problems with the inode life time early on.
> +	 */
> +	pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ino));
> +	spin_lock(&pag->pag_ici_lock);
> +	if (!radix_tree_delete(&pag->pag_ici_root, XFS_INO_TO_AGINO(mp, ino)))
> +		ASSERT(0);

Well, it "complains" by returning NULL instead of the entry.  So I think
that comment could use some updates or simply be removed.

> +void
> +xfs_dispose_inodes(
> +	struct list_head	*freeable)
> +{
> +	while (!list_empty(freeable)) {
> +		struct inode *inode;
> +
> +		inode = list_first_entry(freeable, struct inode, i_lru);

This could use list_first_entry_or_null in the while loop, or not.
Or list_pop_entry if we had it, but Linus hates that :)

> +xfs_reclaim_inodes(
> +	struct xfs_mount	*mp)
> +{
> +	while (list_lru_count(&mp->m_inode_lru)) {
> +		struct xfs_ireclaim_args ra;
> +		long freed, to_free;
> +
> +		INIT_LIST_HEAD(&ra.freeable);
> +		ra.lowest_lsn = NULLCOMMITLSN;
> +		to_free = list_lru_count(&mp->m_inode_lru);

Do we want a helper to initialize the xfs_ireclaim_args?  That would
solve the "issue" of not initializing dirty_skipped in a few users
and make it a little easier to use.

> +
> +		freed = list_lru_walk(&mp->m_inode_lru, xfs_inode_reclaim_isolate,

Line > 80 chars.

> +static inline int __xfs_iflock_nowait(struct xfs_inode *ip)
> +{
> +	if (ip->i_flags & XFS_IFLOCK)
> +		return false;
> +	ip->i_flags |= XFS_IFLOCK;
> +	return true;
> +}

I wonder if simply open coding this would be simpler, given how magic
xfs_inode_reclaim_isolate already is, and given that we really shouldn't
use this helper anywhere else.