On Wed, Apr 18, 2018 at 09:55:11AM -0700, Darrick J. Wong wrote: > On Wed, Apr 18, 2018 at 11:33:17AM -0400, Brian Foster wrote: > > On Tue, Apr 17, 2018 at 07:39:39PM -0700, Darrick J. Wong wrote: > > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > > > > > > Create a new QMOPT flag to signal that we've already locked the quota > > > inode. This will be used by the quota scrub code refactoring to avoid > > > dropping the quota ip lock during scrub. The flag is incompatible with > > > the DQALLOC flag because dquot allocation creates a transaction, and we > > > shouldn't be doing that with the ILOCK held. > > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > > > --- > > > fs/xfs/libxfs/xfs_quota_defs.h | 2 ++ > > > fs/xfs/xfs_dquot.c | 32 ++++++++++++++++++++------------ > > > 2 files changed, 22 insertions(+), 12 deletions(-) > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h > > > index bb1b13a..cfc9938 100644 > > > --- a/fs/xfs/libxfs/xfs_quota_defs.h > > > +++ b/fs/xfs/libxfs/xfs_quota_defs.h > > > @@ -107,6 +107,8 @@ typedef uint16_t xfs_qwarncnt_t; > > > * to a single function. None of these XFS_QMOPT_* flags are meant to have > > > * persistent values (ie. their values can and will change between versions) > > > */ > > > +/* Quota ip already locked. Cannot be used with DQALLOC. */ > > > +#define XFS_QMOPT_QUOTIP_LOCKED 0x0000001 > > > #define XFS_QMOPT_DQALLOC 0x0000002 /* alloc dquot ondisk if needed */ > > > #define XFS_QMOPT_UQUOTA 0x0000004 /* user dquot requested */ > > > #define XFS_QMOPT_PQUOTA 0x0000008 /* project dquot requested */ > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > > > index a7daef9..ed2e37c 100644 > > > --- a/fs/xfs/xfs_dquot.c > > > +++ b/fs/xfs/xfs_dquot.c > > > @@ -417,18 +417,20 @@ xfs_qm_dqtobp( > > > struct xfs_mount *mp = dqp->q_mount; > > > xfs_dqid_t id = be32_to_cpu(dqp->q_core.d_id); > > > struct xfs_trans *tp = (tpp ? *tpp : NULL); > > > - uint lock_mode; > > > + uint lock_mode = 0; > > > > > > quotip = xfs_quota_inode(dqp->q_mount, dqp->dq_flags); > > > dqp->q_fileoffset = (xfs_fileoff_t)id / mp->m_quotainfo->qi_dqperchunk; > > > > > > - lock_mode = xfs_ilock_data_map_shared(quotip); > > > + if (!(flags & XFS_QMOPT_QUOTIP_LOCKED)) > > > + lock_mode = xfs_ilock_data_map_shared(quotip); > > > if (!xfs_this_quota_on(dqp->q_mount, dqp->dq_flags)) { > > > /* > > > * Return if this type of quotas is turned off while we > > > * didn't have the quota inode lock. > > > */ > > > - xfs_iunlock(quotip, lock_mode); > > > + if (lock_mode) > > > + xfs_iunlock(quotip, lock_mode); > > > return -ESRCH; > > > } > > > > > > @@ -438,7 +440,8 @@ xfs_qm_dqtobp( > > > error = xfs_bmapi_read(quotip, dqp->q_fileoffset, > > > XFS_DQUOT_CLUSTER_SIZE_FSB, &map, &nmaps, 0); > > > > > > - xfs_iunlock(quotip, lock_mode); > > > + if (lock_mode) > > > + xfs_iunlock(quotip, lock_mode); > > > if (error) > > > return error; > > > > > > @@ -458,7 +461,7 @@ xfs_qm_dqtobp( > > > */ > > > if (!(flags & XFS_QMOPT_DQALLOC)) > > > return -ENOENT; > > > - > > > + ASSERT(!(flags & XFS_QMOPT_QUOTIP_LOCKED)); > > > ASSERT(tp); > > > error = xfs_qm_dqalloc(tpp, mp, dqp, quotip, > > > dqp->q_fileoffset, &bp); > > > @@ -553,6 +556,7 @@ xfs_qm_dqread( > > > trace_xfs_dqread(dqp); > > > > > > if (flags & XFS_QMOPT_DQALLOC) { > > > + ASSERT(!(flags & XFS_QMOPT_QUOTIP_LOCKED)); > > > > Perhaps we should just have an explicit check for both flags above and > > return with -EINVAL if necessary? > > Sounds like a good idea. > > > > error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_dqalloc, > > > XFS_QM_DQALLOC_SPACE_RES(mp), 0, 0, &tp); > > > if (error) > > > @@ -634,11 +638,12 @@ static int > > > xfs_dq_get_next_id( > > > struct xfs_mount *mp, > > > uint type, > > > - xfs_dqid_t *id) > > > + xfs_dqid_t *id, > > > + uint flags) > > > { > > > struct xfs_inode *quotip = xfs_quota_inode(mp, type); > > > xfs_dqid_t next_id = *id + 1; /* simple advance */ > > > - uint lock_flags; > > > + uint lock_flags = 0; > > > struct xfs_bmbt_irec got; > > > struct xfs_iext_cursor cur; > > > xfs_fsblock_t start; > > > @@ -657,7 +662,8 @@ xfs_dq_get_next_id( > > > /* Nope, next_id is now past the current chunk, so find the next one */ > > > start = (xfs_fsblock_t)next_id / mp->m_quotainfo->qi_dqperchunk; > > > > > > - lock_flags = xfs_ilock_data_map_shared(quotip); > > > + if (!(flags & XFS_QMOPT_QUOTIP_LOCKED)) > > > + lock_flags = xfs_ilock_data_map_shared(quotip); > > > if (!(quotip->i_df.if_flags & XFS_IFEXTENTS)) { > > > error = xfs_iread_extents(NULL, quotip, XFS_DATA_FORK); > > > if (error) > > > @@ -673,7 +679,8 @@ xfs_dq_get_next_id( > > > error = -ENOENT; > > > } > > > > > > - xfs_iunlock(quotip, lock_flags); > > > + if (lock_flags) > > > + xfs_iunlock(quotip, lock_flags); > > > > > > return error; > > > } > > > @@ -733,7 +740,8 @@ xfs_qm_dqget( > > > > Earlier code in this function drops/reacquires the ip ILOCK with a note > > in the comment around lock ordering with the quota inode. Assuming an > > inode is passed, is that lock cycle safe if the caller also has the > > quotip held locked? > > I'm not sure; for the case that I want to support (iterate dquots > without a specific file inode in mind) I don't need (ip != NULL) at all. > So my answer is that I don't want to support this scenario at all. :) > Ok, works for me. > Ok, so at the top of _dqget, add: > > if (flags & _QUOTIP_LOCKED) { > ASSERT(ip == NULL); > ASSERT(!(flags & _DQALLOC)); > if (ip || (flags & DQALLOC)) > return -EINVAL; > } > It might be better to split the DQALLOC part off to xfs_qm_dqread() since it is not static and that's where we act on the dqalloc flag. Brian > --D > > > > > Brian > > > > > if (XFS_IS_DQUOT_UNINITIALIZED(dqp)) { > > > xfs_dqunlock(dqp); > > > mutex_unlock(&qi->qi_tree_lock); > > > - error = xfs_dq_get_next_id(mp, type, &id); > > > + error = xfs_dq_get_next_id(mp, type, &id, > > > + flags); > > > if (error) > > > return error; > > > goto restart; > > > @@ -768,7 +776,7 @@ xfs_qm_dqget( > > > > > > /* If we are asked to find next active id, keep looking */ > > > if (error == -ENOENT && (flags & XFS_QMOPT_DQNEXT)) { > > > - error = xfs_dq_get_next_id(mp, type, &id); > > > + error = xfs_dq_get_next_id(mp, type, &id, flags); > > > if (!error) > > > goto restart; > > > } > > > @@ -827,7 +835,7 @@ xfs_qm_dqget( > > > if (flags & XFS_QMOPT_DQNEXT) { > > > if (XFS_IS_DQUOT_UNINITIALIZED(dqp)) { > > > xfs_qm_dqput(dqp); > > > - error = xfs_dq_get_next_id(mp, type, &id); > > > + error = xfs_dq_get_next_id(mp, type, &id, flags); > > > if (error) > > > return error; > > > goto restart; > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > > the body of a message to majordomo@xxxxxxxxxxxxxxx > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@xxxxxxxxxxxxxxx > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html