Re: [PATCH RFC 4/4] xfs: include an allocfree res for inobt modifications

[Dave Chinner <david@fromorbit.com>]

On Tue, Nov 28, 2017 at 10:49:22AM -0500, Brian Foster wrote:
> On Tue, Nov 28, 2017 at 10:27:19AM +1100, Dave Chinner wrote:
> > On Mon, Nov 27, 2017 at 03:24:34PM -0500, Brian Foster wrote:
> > > Analysis of recent reports of log reservation overruns and code
> > > inspection has uncovered that the reservations associated with inode
> > > operations may not cover the worst case scenarios. In particular,
> > > many cases only include one allocfree res. for a particular
> > > operation even though said operations may also entail AGFL fixups
> > > and inode btree block allocations in addition to the actual inode
> > > chunk allocation. This can easily turn into two or three block
> > > allocations (or frees) per operation.
> > > 
> > > In theory, the only way to define the worst case reservation is to
> > > include an allocfree res for each individual allocation in a
> > > transaction. Since that is impractical (we can perform multiple agfl
> > > fixups per tx and not every allocation is going to result in a full
> > > tree operation), implement a reasonable compromise that addresses
> > > the deficiency in practice without blowing out the size of the
> > > transactions.
> > > 
> > > Refactor the inode transaction reservation code to include one
> > > allocfree res. per inode btree modification to cover allocations
> > > required by the tree itself. This essentially separates the
> > > reservation required to allocate the physical inode chunk from
> > > additional reservation required to perform inobt record
> > > insertion/removal.
> > 
> > I think you missed the most important reason the inobt/finobt
> > modifications need there own allocfree reservation - btree
> > modifications that cause btree blocks to be freed do not use defered
> > ops and so the freeing of blocks occurs directly within the primary
> > transaction. Hence the primary transaction reservation must take
> > this into account ....
> > 
> > > Apply the same logic to the finobt reservation.
> > > This results in killing off the finobt modify condition because we
> > > no longer assume that the broader transaction reservation will cover
> > > finobt block allocations.
> > > 
> > > Suggested-by: Dave Chinner <david@fromorbit.com>
> > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > 
> > Code looks fine. Comments below are for another follow-on patch.
> > 
> 
> Actually, what do you think of the following variant (compile tested
> only)? This facilites another cleanup patch I just wrote to kill off
> about half of the (now effectively duplicate) xfs_calc_create_*()
> helpers because the finobt and inode chunk res. helpers only include the
> associated reservation based on the associated feature bits.

Yup, makes a lot of sense to do that.

FWIW, because we've got so many alloc vs free type reservations,
would it make more sense to do something like:

#define _ALLOC	true
#define _FREE	false

And use those in the callers rather than true/false?

i.e. this code remains the same in that it takes an "bool alloc"
flag as a parameter:

> +STATIC uint
> +xfs_calc_inode_chunk_res(
> +	struct xfs_mount	*mp,
> +	bool			alloc)
> +{
> +	uint			res, size = 0;
> +
> +	res = xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> +			       XFS_FSB_TO_B(mp, 1));
> +	if (alloc) {
> +		/* icreate tx uses ordered buffers */
> +		if (xfs_sb_version_hascrc(&mp->m_sb))
> +			return res;
> +		size = XFS_FSB_TO_B(mp, 1);
> +	}
> +
> +	res += xfs_calc_buf_res(mp->m_ialloc_blks, size);
> +	return res;
> +}

but makes the callers  look like:

>  STATIC uint
> @@ -396,9 +430,7 @@ xfs_calc_create_resv_alloc(
>  {
>  	return xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
>  		mp->m_sb.sb_sectsize +
> -		xfs_calc_buf_res(mp->m_ialloc_blks, XFS_FSB_TO_B(mp, 1)) +
> -		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> -				 XFS_FSB_TO_B(mp, 1)) +
> +		xfs_calc_inode_chunk_res(mp, true) +

		xfs_calc_inode_chunk_res(mp, _ALLOC) +
		.....

and

> @@ -511,7 +542,7 @@ xfs_calc_ifree_reservation(
>  		xfs_calc_buf_res(3, mp->m_sb.sb_sectsize) +
>  		xfs_calc_iunlink_remove_reservation(mp) +
>  		xfs_calc_buf_res(1, 0) +
> -		xfs_calc_buf_res(mp->m_ialloc_blks, 0) +
> +		xfs_calc_inode_chunk_res(mp, false) +

		xfs_calc_inode_chunk_res(mp, _FREE) +
		.....

That would make it very clear what type of reservation we are
acutally expecting to take out in the calculation. i.e. the code
is now clearly self documenting :)

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com