On Tue, Apr 06, 2021 at 10:42:36AM -0400, Brian Foster wrote:
> The dquot reservation helper currently performs the associated
> reservation for any provided dquots. The dquots could have been
> acquired from inode references or explicit dquot allocation
> requests. Some reservation callers may have already checked that the
> associated quota subsystem is active (xfs_qm_dqget() returns an
> error otherwise), while others might not have checked at all
> (xfs_trans_reserve_quota_nblks() passes the inode references).
> Further, subsequent dquot modifications do actually check that the
> associated quota is active before making transactional changes
> (xfs_trans_mod_dquot_byino()).
>
> Given all of that, the behavior to unconditionally perform
> reservation on any provided dquots is somewhat ad hoc. While it is
> currently harmless, it is not without side effect. If the quota is
> inactive by the time a transaction attempts a quota reservation, the
> dquot will be attached to the transaction and subsequently logged,
> even though no dquot modifications are ultimately made.
>
> This is a problem for upcoming quotaoff changes that intend to
> implement a strict transactional barrier for logging dquots during a
> quotaoff operation. If a dquot is logged after the subsystem
> deactivated and the barrier released, a subsequent log recovery can
> incorrectly replay dquot changes into the filesystem.
>
> Therefore, update the dquot reservation path to also check that a
> particular quota mode is active before associating a dquot with a
> transaction. This should have no noticeable impact on the current
> code that already accommodates checking active quota state at points
> before and after quota reservations are made.
>
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
If you end up reposting this, please change to:
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> ---
> fs/xfs/xfs_trans_dquot.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index 48e09ea30ee5..eec640999148 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -40,14 +40,12 @@ xfs_trans_dqjoin(
> }
>
> /*
> - * This is called to mark the dquot as needing
> - * to be logged when the transaction is committed. The dquot must
> - * already be associated with the given transaction.
> - * Note that it marks the entire transaction as dirty. In the ordinary
> - * case, this gets called via xfs_trans_commit, after the transaction
> - * is already dirty. However, there's nothing stop this from getting
> - * called directly, as done by xfs_qm_scall_setqlim. Hence, the TRANS_DIRTY
> - * flag.
> + * This is called to mark the dquot as needing to be logged when the transaction
> + * is committed. The dquot must already be associated with the given
> + * transaction. Note that it marks the entire transaction as dirty. In the
> + * ordinary case, this gets called via xfs_trans_commit, after the transaction
> + * is already dirty. However, there's nothing stop this from getting called
> + * directly, as done by xfs_qm_scall_setqlim. Hence, the TRANS_DIRTY flag.
> */
> void
> xfs_trans_log_dquot(
> @@ -743,19 +741,19 @@ xfs_trans_reserve_quota_bydquots(
>
> ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
>
> - if (udqp) {
> + if (XFS_IS_UQUOTA_ON(mp) && udqp) {
> error = xfs_trans_dqresv(tp, mp, udqp, nblks, ninos, flags);
> if (error)
> return error;
> }
>
> - if (gdqp) {
> + if (XFS_IS_GQUOTA_ON(mp) && gdqp) {
> error = xfs_trans_dqresv(tp, mp, gdqp, nblks, ninos, flags);
> if (error)
> goto unwind_usr;
> }
>
> - if (pdqp) {
> + if (XFS_IS_PQUOTA_ON(mp) && pdqp) {
> error = xfs_trans_dqresv(tp, mp, pdqp, nblks, ninos, flags);
> if (error)
> goto unwind_grp;
> --
> 2.26.3
>
