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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxx
