Re: [PATCH 4/6] xfs: reduce the absurdly large log reservations

[Dave Chinner <david@fromorbit.com>]

On Mon, Apr 25, 2022 at 04:47:49PM -0700, Darrick J. Wong wrote:
> On Sat, Apr 23, 2022 at 08:51:52AM +1000, Dave Chinner wrote:
> > On Thu, Apr 14, 2022 at 03:54:48PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Back in the early days of reflink and rmap development I set the
> > > transaction reservation sizes to be overly generous for rmap+reflink
> > > filesystems, and a little under-generous for rmap-only filesystems.
> > > 
> > > Since we don't need *eight* transaction rolls to handle three new log
> > > intent items, decrease the logcounts to what we actually need, and amend
> > > the shadow reservation computation function to reflect what we used to
> > > do so that the minimum log size doesn't change.
> > 
> > Yup, this will make a huge difference to the number of transactions
> > we can have in flight on reflink/rmap enabled filesystems.
> > 
> > Mostly looks good, some comments about code and comments below.
....
> > > -	/* Put everything back the way it was.  This goes at the end. */
> > > -	mp->m_rmap_maxlevels = rmap_maxlevels;
> > > +	/* Add one logcount for BUI items that appear with rmap or reflink. */
> > > +	if (xfs_has_reflink(mp) || xfs_has_rmapbt(mp)) {
> > > +		resp->tr_itruncate.tr_logcount++;
> > > +		resp->tr_write.tr_logcount++;
> > > +		resp->tr_qm_dqalloc.tr_logcount++;
> > > +	}
> > > +
> > > +	/* Add one logcount for refcount intent items. */
> > > +	if (xfs_has_reflink(mp)) {
> > > +		resp->tr_itruncate.tr_logcount++;
> > > +		resp->tr_write.tr_logcount++;
> > > +		resp->tr_qm_dqalloc.tr_logcount++;
> > > +	}
> > > +
> > > +	/* Add one logcount for rmap intent items. */
> > > +	if (xfs_has_rmapbt(mp)) {
> > > +		resp->tr_itruncate.tr_logcount++;
> > > +		resp->tr_write.tr_logcount++;
> > > +		resp->tr_qm_dqalloc.tr_logcount++;
> > > +	}
> > 
> > This would be much more concisely written as
> > 
> > 	count = 0;
> > 	if (xfs_has_reflink(mp) || xfs_has_rmapbt(mp)) {
> > 		count = 2;
> > 		if (xfs_has_reflink(mp) && xfs_has_rmapbt(mp))
> > 			count++;
> > 	}
> > 
> > 	resp->tr_itruncate.tr_logcount += count;
> > 	resp->tr_write.tr_logcount += count;
> > 	resp->tr_qm_dqalloc.tr_logcount += count;
> 
> I think I'd rather do:
> 
> 	/*
> 	 * Add one logcount for BUI items that appear with rmap or reflink,
> 	 * one logcount for refcount intent items, and one logcount for rmap
> 	 * intent items.
> 	 */
> 	if (xfs_has_reflink(mp) || xfs_has_rmapbt(mp))
> 		logcount_adj++;
> 	if (xfs_has_reflink(mp))
> 		logcount_adj++;
> 	if (xfs_has_rmapbt(mp))
> 		logcount_adj++;
> 
> 	resp->tr_itruncate.tr_logcount += logcount_adj;
> 	resp->tr_write.tr_logcount += logcount_adj;
> 	resp->tr_qm_dqalloc.tr_logcount += logcount_adj;
> 
> If you don't mind?

Sure, that's just as good.

--Dave.
-- 
Dave Chinner
david@fromorbit.com