Re: [PATCH CANDIDATE 5.15, 6.1, 6.6] xfs: Increase XFS_QM_TRANS_MAXDQS to 5

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Sep 15, 2025 at 10:20:40PM +0200, Amir Goldstein wrote:
> On Mon, Sep 15, 2025 at 8:20 PM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Sat, Sep 13, 2025 at 05:05:02AM +0200, Amir Goldstein wrote:
> > > From: Allison Henderson <allison.henderson@oracle.com>
> > >
> > > [ Upstream  commit f103df763563ad6849307ed5985d1513acc586dd ]
> > >
> > > With parent pointers enabled, a rename operation can update up to 5
> > > inodes: src_dp, target_dp, src_ip, target_ip and wip.  This causes
> > > their dquots to a be attached to the transaction chain, so we need
> > > to increase XFS_QM_TRANS_MAXDQS.  This patch also add a helper
> > > function xfs_dqlockn to lock an arbitrary number of dquots.
> > >
> > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > >
> > > [amir: backport to kernels prior to parent pointers to fix an old bug]
> > >
> > > A rename operation of a directory (i.e. mv A/C/ B/) may end up changing
> > > three different dquot accounts under the following conditions:
> > > 1. user (or group) quotas are enabled
> > > 2. A/ B/ and C/ have different owner uids (or gids)
> > > 3. A/ blocks shrinks after remove of entry C/
> > > 4. B/ blocks grows before adding of entry C/
> > > 5. A/ ino <= XFS_DIR2_MAX_SHORT_INUM
> > > 6. B/ ino > XFS_DIR2_MAX_SHORT_INUM
> > > 7. C/ is converted from sf to block format, because its parent entry
> > >    needs to be stored as 8 bytes (see xfs_dir2_sf_replace_needblock)
> > >
> > > When all conditions are met (observed in the wild) we get this assertion:
> > >
> > > XFS: Assertion failed: qtrx, file: fs/xfs/xfs_trans_dquot.c, line: 207
> > >
> > > The upstream commit fixed this bug as a side effect, so decided to apply
> > > it as is rather than changing XFS_QM_TRANS_MAXDQS to 3 in stable kernels.
> >
> > Heh.  Indeed, you only need MAXDQS==5 for filesystems that support
> > parent pointers, because only on those filesystems can you end up
> > needing to allocate a xattr block either to the new whiteout file or
> > free one from the file being unlinked.
> >
> > > The Fixes commit below is NOT the commit that introduced the bug, but
> > > for some reason, which is not explained in the commit message, it fixes
> > > the comment to state that highest number of dquots of one type is 3 and
> > > not 2 (which leads to the assertion), without actually fixing it.
> >
> > Agree.
> >
> > > The change of wording from "usr, grp OR prj" to "usr, grp and prj"
> > > suggests that there may have been a confusion between "the number of
> > > dquote of one type" and "the number of dquot types" (which is also 3),
> > > so the comment change was only accidentally correct.
> >
> > I interpret the "OR" -> "and" change to reflect the V4 -> V5 transition
> > where you actually can have all three dquot types because group/project
> > quota are no longer mutually exclusive.
> >
> > The "...involved in a transaction is 3" part I think is separate, and
> > strange that XFS_QM_TRANS_MAXDQS wasn't updated.
> >
> > > Fixes: 10f73d27c8e9 ("xfs: fix the comment explaining xfs_trans_dqlockedjoin")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >
> > > Christoph,
> > >
> > > This is a cognitive challenge. can you say what you where thinking in
> > > 2013 when making the comment change in the Fixes commit?
> > > Is my speculation above correct?
> > >
> > > Catherine and Leah,
> > >
> > > I decided that cherry-pick this upstream commit as is with a commit
> > > message addendum was the best stable tree strategy.
> > > The commit applies cleanly to 5.15.y, so I assume it does for 6.6 and
> > > 6.1 as well. I ran my tests on 5.15.y and nothing fell out, but did not
> > > try to reproduce these complex assertion in a test.
> > >
> > > Could you take this candidate backport patch to a spin on your test
> > > branch?
> > >
> > > What do you all think about this?
> >
> > I only think you need MAXDQS==5 for 6.12 to handle parent pointers.
> >
> 
> Yes, of course. I just preferred to keep the 5 to avoid deviating from
> the upstream commit if there is no good reason to do so.

<shrug> What do Greg and Sasha think about this?  If they don't mind
this then I guess I don't either. ;)

> > The older kernels could have it set to 3 instead.  struct xfs_dqtrx on a
> > 6.17-rc6 kernel is 88 bytes.  Stuffing 9 of them into struct
> > xfs_dquot_acct instead of 15 means that the _acct struct is only 792
> > bytes instead of 1392, which means we can use the 1k slab instead of the
> > 2k slab.
> 
> Huh? there is only one xfs_dquot_acct per transaction.

Yes, but there can be a lot of transactions in flight.

> Does it really matter if it's 1k or 2k??
> 
> Am I missing something?

It seems silly to waste so much memory on a scenario that can't happen
just so we can say that we hammered in a less appropriate solution.

--D

> Thanks,
> Amir.
>