On Tue, Sep 08, 2020 at 11:56:02AM -0400, Brian Foster wrote: > On Sat, Sep 05, 2020 at 08:29:36AM +1000, Dave Chinner wrote: > > IOWs, the barrier mechanism I added was designed to provide the > > exact "no dquots are logged after the quotaoff item is committed to > > the log" invariant you describe. It's basically the same mechanism > > we use for draining direct IO via taking IOLOCKs to prevent new > > submissions and calling inode_dio_wait() to drain everything in > > flight.... > > > > Right, I follow all of the above. I've been experimenting with an > approach that just freezes all transactions as opposed to only quota > transactions just to reduce the amount of code involved. What I'm trying > to point out is that I don't think this quotaoff logic alone is > sufficient to prevent dquot log ordering problems. > > Consider the following example scenario: > > - fs mounted w/ user+group quotas enabled > - inode 0x123 is in-core w/ user+group dquots already attached > - user executes 'xfs_quota -xc "off -g" <mnt>' to turn off group quotas > - quotaoff drains all outstanding transactions, clears (group) quota > flag, logs quotaoff start/end ... > > Meanwhile.. > > - user executes an fallocate request on inode 0x123, which blocks down > in xfs_alloc_file_space() -> xfs_trans_alloc() due to the quotaoff in > progress. > - quotaoff releases the trans barrier and begins doing its dquot > flush/purge thing.. > - falloc grabs the 0x123 ilock and xfs_trans_reserve_quota_bydquots() -> > xfs_trans_dqresv() -> xfs_trans_mod_dquot() joins the user/group > dquots to the transaction quota ctx because they are still attached to > the inode at this point (and user quota is still enabled), hence quota > blocks are reserved in both There's the bug. The patch I wrote needs to ensure that the quotas are enabled when attaching the dquot to the dqinfo. The code currently checks for global "quota on" state, but it doesn't check individual quota state... > - 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. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
