diff mbox series

[1/3] xfs: skip dquot reservations if quota is inactive

Message ID 20201001150310.141467-2-bfoster@redhat.com (mailing list archive)
State New
Headers show
Series xfs: rework quotaoff to avoid log deadlock | expand

Commit Message

Brian Foster Oct. 1, 2020, 3:03 p.m. UTC
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@redhat.com>
---
 fs/xfs/xfs_trans_dquot.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

Comments

Darrick J. Wong Oct. 7, 2020, 10:09 p.m. UTC | #1
On Thu, Oct 01, 2020 at 11:03:08AM -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@redhat.com>

Seems reasonable not to bother with the dqresv step if the quota type
isn't enabled.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--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 133fc6fc3edd..547ba824542e 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -39,14 +39,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(
> @@ -770,19 +768,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.25.4
>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index 133fc6fc3edd..547ba824542e 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -39,14 +39,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(
@@ -770,19 +768,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;