Message ID | 1600765442-12146-4-git-send-email-kaixuxia@tencent.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | xfs: random fixes for disk quota | expand |
On Tue, Sep 22, 2020 at 05:04:02PM +0800, xiakaixu1987@gmail.com wrote: > From: Kaixu Xia <kaixuxia@tencent.com> > > We attach the dquot(s) to the given inode and return udquot, gdquot > or pdquot with references taken by dqget or dqhold in xfs_qm_vop_dqalloc() > funciton. Actually, we only need to do dqget or dqhold for the specified > dquots, it is unnecessary if the passed-in O_{u,g,p}dqpp value is NULL. When would a caller pass in (for example) XFS_QMOPT_UQUOTA, a different uid than the one currently associated with the inode, but a null O_udqpp? It doesn't make sense to say "Prepare to change this file's uid, but don't give me the dquot of the new uid." None of the callers do that today, so I don't see the point of this patch. Perhaps the function could ASSERT the arguments a little more closely? --D > Signed-off-by: Kaixu Xia <kaixuxia@tencent.com> > --- > fs/xfs/xfs_qm.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c > index 44509decb4cd..38380fc29b4d 100644 > --- a/fs/xfs/xfs_qm.c > +++ b/fs/xfs/xfs_qm.c > @@ -1661,7 +1661,7 @@ xfs_qm_vop_dqalloc( > } > } > > - if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) { > + if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp) && O_udqpp) { > if (!uid_eq(inode->i_uid, uid)) { > /* > * What we need is the dquot that has this uid, and > @@ -1694,7 +1694,7 @@ xfs_qm_vop_dqalloc( > uq = xfs_qm_dqhold(ip->i_udquot); > } > } > - if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) { > + if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp) && O_gdqpp) { > if (!gid_eq(inode->i_gid, gid)) { > xfs_iunlock(ip, lockflags); > error = xfs_qm_dqget(mp, from_kgid(user_ns, gid), > @@ -1711,7 +1711,7 @@ xfs_qm_vop_dqalloc( > gq = xfs_qm_dqhold(ip->i_gdquot); > } > } > - if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) { > + if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp) && O_pdqpp) { > if (ip->i_d.di_projid != prid) { > xfs_iunlock(ip, lockflags); > error = xfs_qm_dqget(mp, prid, > @@ -1733,16 +1733,11 @@ xfs_qm_vop_dqalloc( > xfs_iunlock(ip, lockflags); > if (O_udqpp) > *O_udqpp = uq; > - else > - xfs_qm_dqrele(uq); > if (O_gdqpp) > *O_gdqpp = gq; > - else > - xfs_qm_dqrele(gq); > if (O_pdqpp) > *O_pdqpp = pq; > - else > - xfs_qm_dqrele(pq); > + > return 0; > > error_rele: > -- > 2.20.0 >
On 2020/9/23 0:18, Darrick J. Wong wrote: > On Tue, Sep 22, 2020 at 05:04:02PM +0800, xiakaixu1987@gmail.com wrote: >> From: Kaixu Xia <kaixuxia@tencent.com> >> >> We attach the dquot(s) to the given inode and return udquot, gdquot >> or pdquot with references taken by dqget or dqhold in xfs_qm_vop_dqalloc() >> funciton. Actually, we only need to do dqget or dqhold for the specified >> dquots, it is unnecessary if the passed-in O_{u,g,p}dqpp value is NULL. > > When would a caller pass in (for example) XFS_QMOPT_UQUOTA, a different > uid than the one currently associated with the inode, but a null > O_udqpp? It doesn't make sense to say "Prepare to change this file's > uid, but don't give me the dquot of the new uid." > > None of the callers do that today, so I don't see the point of this > patch. Perhaps the function could ASSERT the arguments a little more > closely? Yeah, ASSERT the arguments is better, will do that in the next version. Thanks, Kaixu > > --D > >> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com> >> --- >> fs/xfs/xfs_qm.c | 13 ++++--------- >> 1 file changed, 4 insertions(+), 9 deletions(-) >> >> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c >> index 44509decb4cd..38380fc29b4d 100644 >> --- a/fs/xfs/xfs_qm.c >> +++ b/fs/xfs/xfs_qm.c >> @@ -1661,7 +1661,7 @@ xfs_qm_vop_dqalloc( >> } >> } >> >> - if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) { >> + if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp) && O_udqpp) { >> if (!uid_eq(inode->i_uid, uid)) { >> /* >> * What we need is the dquot that has this uid, and >> @@ -1694,7 +1694,7 @@ xfs_qm_vop_dqalloc( >> uq = xfs_qm_dqhold(ip->i_udquot); >> } >> } >> - if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) { >> + if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp) && O_gdqpp) { >> if (!gid_eq(inode->i_gid, gid)) { >> xfs_iunlock(ip, lockflags); >> error = xfs_qm_dqget(mp, from_kgid(user_ns, gid), >> @@ -1711,7 +1711,7 @@ xfs_qm_vop_dqalloc( >> gq = xfs_qm_dqhold(ip->i_gdquot); >> } >> } >> - if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) { >> + if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp) && O_pdqpp) { >> if (ip->i_d.di_projid != prid) { >> xfs_iunlock(ip, lockflags); >> error = xfs_qm_dqget(mp, prid, >> @@ -1733,16 +1733,11 @@ xfs_qm_vop_dqalloc( >> xfs_iunlock(ip, lockflags); >> if (O_udqpp) >> *O_udqpp = uq; >> - else >> - xfs_qm_dqrele(uq); >> if (O_gdqpp) >> *O_gdqpp = gq; >> - else >> - xfs_qm_dqrele(gq); >> if (O_pdqpp) >> *O_pdqpp = pq; >> - else >> - xfs_qm_dqrele(pq); >> + >> return 0; >> >> error_rele: >> -- >> 2.20.0 >>
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c index 44509decb4cd..38380fc29b4d 100644 --- a/fs/xfs/xfs_qm.c +++ b/fs/xfs/xfs_qm.c @@ -1661,7 +1661,7 @@ xfs_qm_vop_dqalloc( } } - if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) { + if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp) && O_udqpp) { if (!uid_eq(inode->i_uid, uid)) { /* * What we need is the dquot that has this uid, and @@ -1694,7 +1694,7 @@ xfs_qm_vop_dqalloc( uq = xfs_qm_dqhold(ip->i_udquot); } } - if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) { + if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp) && O_gdqpp) { if (!gid_eq(inode->i_gid, gid)) { xfs_iunlock(ip, lockflags); error = xfs_qm_dqget(mp, from_kgid(user_ns, gid), @@ -1711,7 +1711,7 @@ xfs_qm_vop_dqalloc( gq = xfs_qm_dqhold(ip->i_gdquot); } } - if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) { + if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp) && O_pdqpp) { if (ip->i_d.di_projid != prid) { xfs_iunlock(ip, lockflags); error = xfs_qm_dqget(mp, prid, @@ -1733,16 +1733,11 @@ xfs_qm_vop_dqalloc( xfs_iunlock(ip, lockflags); if (O_udqpp) *O_udqpp = uq; - else - xfs_qm_dqrele(uq); if (O_gdqpp) *O_gdqpp = gq; - else - xfs_qm_dqrele(gq); if (O_pdqpp) *O_pdqpp = pq; - else - xfs_qm_dqrele(pq); + return 0; error_rele: