On Wed, Sep 09, 2020 at 11:00:35AM -0400, Brian Foster wrote: > On Wed, Sep 09, 2020 at 07:07:20AM +1000, Dave Chinner wrote: > > On Tue, Sep 08, 2020 at 11:56:02AM -0400, Brian Foster wrote: > > > - xfs_trans_mod_dquot_byino() (via xfs_bmapi_write() -> ... -> xfs_bmap_btalloc() -> > > > xfs_bmap_btalloc_accounting()) skips accounting the allocated blocks > > > to the group dquot because it is not enabled > > > > Right, the reservation functions need to do the same thing as > > xfs_trans_mod_dquot_byino(). I simply missed that for the > > reservation functions. i.e. Adding the same style of check like: > > > > if (XFS_IS_UQUOTA_ON(mp) && udq) > > > > before doing anything with user quota will avoid this problem as > > we are already in transaction context and the UQUOTA on or off state > > will not change until the transaction ends. > > > > > concept itself. It seems like we should be able to head this issue off > > > somewhere in this sequence (i.e., checking the appropriate flag before > > > the dquot is attached), but it also seems like the quotaoff start/end > > > plus various quota flags all fit together a certain way and I feel like > > > some pieces of the puzzle are still missing from a design standpoint... > > > > I can't think of anything that is missing - the quota off barrier > > gives us an atomic quota state change w.r.t. running transactions, > > so we just need to make sure we check the quota state before joining > > anything quota related to a transaction rather than assume that the > > presence of a dquot attached to an inode means that quotas are on. > > > > This gets back to my earlier questions around the various quota flags. > If I trace through the code of some operations, it seems like this > approach should work (once this logging issue is addressed, and more > testing required of course). However if I refer back to the runtime > macro comment: > > /* > * Checking XFS_IS_*QUOTA_ON() while holding any inode lock guarantees > * quota will be not be switched off as long as that inode lock is held. > */ > > This will technically no longer be the case because the updated quotaoff > will clear all of the flags before cycling any ilocks and detaching > dquots. I'm aware it will drain the transaction subsystem, but does > anything else depend on not seeing such a state change with an inode > lock held? I haven't seen anything so far that would conflict, but the > comment here is rather vague on details. Not that I know of. I would probably rewrite the above comment as: /* * Checking XFS_IS_*QUOTA_ON() while inside an active quota modifying * transaction context guarantees quota will be not be switched until after the * entire rolling transaction chain is completed. */ To clarify the situation. Having the inode locked will now only guarantee that the dquot will not go away while the inode is locked, it doesn't guarantee that quota will not switch off any more. > Conversely, if not, I'm wondering whether there's a need for an ACTIVE > flag at all if we'd clear it at the same time as the ACCT|ENFD flags > during quotaoff anyways. It sounds like the answer to both those > questions is no based on your previous responses, perhaps reason being > that the transaction drain on the quotaoff side effectively replaces the > need for this rule on the general transaction side. Hm? Note that I > wouldn't remove the ACTIVE flag immediately anyways, but I want to make > sure the concern is clear.. Yes, I think you are right - the ACTIVE flag could probably away as it doesn't really play a part in the quota-off dance anymore. We'd still need the IS_QUOTA_ON() checks, but they'd look at ACCT|ENFD instead... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
