Re: [PATCH 1/8] xfs: AIL doesn't need manual pushing

[Christoph Hellwig <hch@infradead.org>]

This looks very nice, but a bunch of minor comments:

> This is silly and expensive. The AIL is perfectly capable of
> calculating the push target itself, and it will always be running
> when the AIL contains objects.
> 
> Modify the AIL to calculate it's 25% push target before it starts a
> push using the same reserve grant head based calculation as is
> currently used, and remove all the places where we ask the AIL to
> push to a new 25% free target.
> 

I suspect some of these "the AIL" to xfsaild or te AIL pusher for
the sentences to make sense.

> +#include "xfs_trans_priv.h"

> +#include "xfs_log_priv.h"

> -			threshold_lsn = xlog_grant_push_threshold(log, 0);
> +			threshold_lsn = xfs_ail_push_target(log->l_ailp);

Should xfs_ail_push_target go into xfs_log.h instead of xfs_log_priv.h?


> +int xlog_space_left(struct xlog	 *log, atomic64_t *head);

Nit: odd formatting with the tab before the *log.

> +xfs_lsn_t
> +__xfs_ail_push_target(
> +	struct xfs_ail		*ailp)
> +{
> +	struct xlog	*log = ailp->ail_log;
> +	xfs_lsn_t	threshold_lsn = 0;
> +	xfs_lsn_t	last_sync_lsn;
> +	int		free_blocks;
> +	int		free_bytes;
> +	int		threshold_block;
> +	int		threshold_cycle;
> +	int		free_threshold;

This culd use a:

	lockdep_assert_held(&ailp->ail_lock);

to document the locking assumptions.

> +	free_bytes = xlog_space_left(log, &log->l_reserve_head.grant);
> +	free_blocks = BTOBBT(free_bytes);
> +
> +	/*
> +	 * Set the threshold for the minimum number of free blocks in the
> +	 * log to the maximum of what the caller needs, one quarter of the
> +	 * log, and 256 blocks.
> +	 */
> +	free_threshold = log->l_logBBsize >> 2;
> +	if (free_blocks >= free_threshold)

Nit: free_block is only used once, so there might not be much in a point
in keeping it as a logcal variable.

> +static inline void xfs_ail_push(struct xfs_ail *ailp)
> +{
> +	wake_up_process(ailp->ail_task);
> +}
> +
> +static inline void xfs_ail_push_all(struct xfs_ail *ailp)
> +{
> +	if (!test_and_set_bit(XFS_AIL_OPSTATE_PUSH_ALL, &ailp->ail_opstate))
> +		xfs_ail_push(ailp);
> +}
> +
> +xfs_lsn_t		__xfs_ail_push_target(struct xfs_ail *ailp);
> +static inline xfs_lsn_t xfs_ail_push_target(struct xfs_ail *ailp)
> +{
> +	xfs_lsn_t	lsn;
> +
> +	spin_lock(&ailp->ail_lock);
> +	lsn = __xfs_ail_push_target(ailp);
> +	spin_unlock(&ailp->ail_lock);
> +	return lsn;
> +}

Is there really any point in micro-optimizing these as inlines in
a header?