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

[Amir Goldstein <amir73il@gmail.com>]

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.

> 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.
Does it really matter if it's 1k or 2k??

Am I missing something?

Thanks,
Amir.