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

[Dave Chinner <david@fromorbit.com>]

On Sun, Jul 10, 2022 at 11:02:24PM -0700, Christoph Hellwig wrote:
> 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.

Sure. This is all pretty rough "I just go this working" code right
now.

> 
> > +#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?

No.

The AIL should not be visible to anything except the log - the
xfs_mount and the transaction subsystem should know nothing about it
at all. And even with that isolation, the only external interfaces
should be the the commit completion inserting items into the AIL and
the AIL updating the amount of space it consumes.

Overall, our log/trans/log_item/ail headers and code structure is a
total mess.

The "trans_ail" namesapce needs to go away entirely - the AIL
has nothing to do with transactions at all - a rename of the
xfs_trans_ail.c file to xfs_ail.c needs to be done, the APIs need to
be renamed, and an xfs_ail.h needs to be created to expose the
-minimal- API needed for external interfacing with the AIL and get
all that stuff out of xfs_trans_priv.h.

Similarly, all the log reservation, ticket and grant head
reservation stuff needs to be moved out of xfs_log.c and into it's
own file source and header files.

xfs_log.h then should only include external interfaces to the log
(i.e. external calls from xfs_mount context into internal xlog
context) and xfs_log_priv.h only contains internal xlog context
structures and APIs.

Then we can separate the struct xfs_log_item and all the log item
futzing out of xfs_trans.h into xfs_log_item.h.

At the end of this, xfs_trans_priv.h should go away entirely,
xfs_trans.h is only used in front end transaction code,
xfs_log_item.h is only used back end transaction, CIL and AIL code,
xfs_log.h is only needed by the few places in high level code that
interface directly with the log, etc.

You can probably see me starting this re-org in this patchset by
moving and renaming the AIL bulk insert function - it has nothing to
do with transactions, but it is tightly intertwined with internal
CIL structures (the lv chain), log items and the AIL. Hence it
belongs in the CIL code, not the transaction code....

> > +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.

Sure, but this all ends up changing
> 
> > +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?

It's not micro-optimisation - it's simply retaining the existing API
to reduce the amount of cognitive effort needed to understand the
accounting changes in the patchset. That's the important thing here
- the patch set is making a fundamental accounting change to
changing a core, proven algorithm that has been in production for
a couple of years short of 3 decades....

And keep in mind that, as per above, changing the API will come with
restructuring the code, so it's not something I want to change right
now.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com