* [PATCH 1/3] xfs: directly return if the delta equal to zero
2020-09-22 9:03 [RFC PATCH 0/3]xfs: random fixes for disk quota xiakaixu1987
@ 2020-09-22 9:04 ` xiakaixu1987
2020-09-22 16:19 ` Darrick J. Wong
2020-09-22 17:43 ` Brian Foster
2020-09-22 9:04 ` [PATCH 2/3] xfs: remove the unused parameter id from xfs_qm_dqattach_one xiakaixu1987
` (2 subsequent siblings)
3 siblings, 2 replies; 15+ messages in thread
From: xiakaixu1987 @ 2020-09-22 9:04 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
It is useless to go on when the variable delta equal to zero in
xfs_trans_mod_dquot(), so just return if the value equal to zero.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
fs/xfs/xfs_trans_dquot.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index 133fc6fc3edd..23c34af71825 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -215,10 +215,11 @@ xfs_trans_mod_dquot(
if (qtrx->qt_dquot == NULL)
qtrx->qt_dquot = dqp;
- if (delta) {
- trace_xfs_trans_mod_dquot_before(qtrx);
- trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
- }
+ if (!delta)
+ return;
+
+ trace_xfs_trans_mod_dquot_before(qtrx);
+ trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
switch (field) {
@@ -284,8 +285,7 @@ xfs_trans_mod_dquot(
ASSERT(0);
}
- if (delta)
- trace_xfs_trans_mod_dquot_after(qtrx);
+ trace_xfs_trans_mod_dquot_after(qtrx);
tp->t_flags |= XFS_TRANS_DQ_DIRTY;
}
--
2.20.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 1/3] xfs: directly return if the delta equal to zero
2020-09-22 9:04 ` [PATCH 1/3] xfs: directly return if the delta equal to zero xiakaixu1987
@ 2020-09-22 16:19 ` Darrick J. Wong
2020-09-22 17:43 ` Brian Foster
1 sibling, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2020-09-22 16:19 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
On Tue, Sep 22, 2020 at 05:04:00PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> It is useless to go on when the variable delta equal to zero in
> xfs_trans_mod_dquot(), so just return if the value equal to zero.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
Yeah, that makes sense.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_trans_dquot.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index 133fc6fc3edd..23c34af71825 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -215,10 +215,11 @@ xfs_trans_mod_dquot(
> if (qtrx->qt_dquot == NULL)
> qtrx->qt_dquot = dqp;
>
> - if (delta) {
> - trace_xfs_trans_mod_dquot_before(qtrx);
> - trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
> - }
> + if (!delta)
> + return;
> +
> + trace_xfs_trans_mod_dquot_before(qtrx);
> + trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
>
> switch (field) {
>
> @@ -284,8 +285,7 @@ xfs_trans_mod_dquot(
> ASSERT(0);
> }
>
> - if (delta)
> - trace_xfs_trans_mod_dquot_after(qtrx);
> + trace_xfs_trans_mod_dquot_after(qtrx);
>
> tp->t_flags |= XFS_TRANS_DQ_DIRTY;
> }
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/3] xfs: directly return if the delta equal to zero
2020-09-22 9:04 ` [PATCH 1/3] xfs: directly return if the delta equal to zero xiakaixu1987
2020-09-22 16:19 ` Darrick J. Wong
@ 2020-09-22 17:43 ` Brian Foster
2020-09-23 2:42 ` kaixuxia
2020-09-23 7:01 ` Christoph Hellwig
1 sibling, 2 replies; 15+ messages in thread
From: Brian Foster @ 2020-09-22 17:43 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, darrick.wong, Kaixu Xia
On Tue, Sep 22, 2020 at 05:04:00PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> It is useless to go on when the variable delta equal to zero in
> xfs_trans_mod_dquot(), so just return if the value equal to zero.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> ---
> fs/xfs/xfs_trans_dquot.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index 133fc6fc3edd..23c34af71825 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -215,10 +215,11 @@ xfs_trans_mod_dquot(
> if (qtrx->qt_dquot == NULL)
> qtrx->qt_dquot = dqp;
>
> - if (delta) {
> - trace_xfs_trans_mod_dquot_before(qtrx);
> - trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
> - }
> + if (!delta)
> + return;
> +
This does slightly change behavior in that this function currently
unconditionally results in logging the associated dquot in the
transaction. I'm not sure anything really depends on that with a delta
== 0, but it might be worth documenting in the commit log.
Also, it does seem a little odd to bail out after we've potentially
allocated ->t_dqinfo as well as assigned the current dquot a slot in the
transaction. I think that means the effect of this change is lost if
another dquot happens to be modified (with delta != 0) in the same
transaction (which might also be an odd thing to do).
Brian
> + trace_xfs_trans_mod_dquot_before(qtrx);
> + trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
>
> switch (field) {
>
> @@ -284,8 +285,7 @@ xfs_trans_mod_dquot(
> ASSERT(0);
> }
>
> - if (delta)
> - trace_xfs_trans_mod_dquot_after(qtrx);
> + trace_xfs_trans_mod_dquot_after(qtrx);
>
> tp->t_flags |= XFS_TRANS_DQ_DIRTY;
> }
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/3] xfs: directly return if the delta equal to zero
2020-09-22 17:43 ` Brian Foster
@ 2020-09-23 2:42 ` kaixuxia
2020-09-23 13:27 ` Brian Foster
2020-09-23 7:01 ` Christoph Hellwig
1 sibling, 1 reply; 15+ messages in thread
From: kaixuxia @ 2020-09-23 2:42 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs, darrick.wong, Kaixu Xia
On 2020/9/23 1:43, Brian Foster wrote:
> On Tue, Sep 22, 2020 at 05:04:00PM +0800, xiakaixu1987@gmail.com wrote:
>> From: Kaixu Xia <kaixuxia@tencent.com>
>>
>> It is useless to go on when the variable delta equal to zero in
>> xfs_trans_mod_dquot(), so just return if the value equal to zero.
>>
>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
>> ---
>> fs/xfs/xfs_trans_dquot.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
>> index 133fc6fc3edd..23c34af71825 100644
>> --- a/fs/xfs/xfs_trans_dquot.c
>> +++ b/fs/xfs/xfs_trans_dquot.c
>> @@ -215,10 +215,11 @@ xfs_trans_mod_dquot(
>> if (qtrx->qt_dquot == NULL)
>> qtrx->qt_dquot = dqp;
>>
>> - if (delta) {
>> - trace_xfs_trans_mod_dquot_before(qtrx);
>> - trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
>> - }
>> + if (!delta)
>> + return;
>> +
>
>
> This does slightly change behavior in that this function currently
> unconditionally results in logging the associated dquot in the
> transaction. I'm not sure anything really depends on that with a delta
> == 0, but it might be worth documenting in the commit log.
>> Also, it does seem a little odd to bail out after we've potentially
> allocated ->t_dqinfo as well as assigned the current dquot a slot in the
> transaction. I think that means the effect of this change is lost if
> another dquot happens to be modified (with delta != 0) in the same
> transaction (which might also be an odd thing to do).
>
Since the dquot value doesn't changes if the delta == 0, we shouldn't
set the XFS_TRANS_DQ_DIRTY flag to current transaction. Maybe we should
do the judgement at the beginning of the function, we will do nothing if
the delta == 0. Just like this,
xfs_trans_mod_dquot(
{
...
if (!delta)
return;
if (tp->t_dqinfo == NULL)
xfs_trans_alloc_dqinfo(tp);
...
}
I'm not sure...What's your opinion about that?
Thanks,
Kaixu
> Brian
>
>> + trace_xfs_trans_mod_dquot_before(qtrx);
>> + trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
>>
>> switch (field) {
>>
>> @@ -284,8 +285,7 @@ xfs_trans_mod_dquot(
>> ASSERT(0);
>> }
>>
>> - if (delta)
>> - trace_xfs_trans_mod_dquot_after(qtrx);
>> + trace_xfs_trans_mod_dquot_after(qtrx);
>>
>> tp->t_flags |= XFS_TRANS_DQ_DIRTY;
>> }
>> --
>> 2.20.0
>>
>
--
kaixuxia
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/3] xfs: directly return if the delta equal to zero
2020-09-23 2:42 ` kaixuxia
@ 2020-09-23 13:27 ` Brian Foster
2020-09-23 16:09 ` Darrick J. Wong
0 siblings, 1 reply; 15+ messages in thread
From: Brian Foster @ 2020-09-23 13:27 UTC (permalink / raw)
To: kaixuxia; +Cc: linux-xfs, darrick.wong, Kaixu Xia
On Wed, Sep 23, 2020 at 10:42:10AM +0800, kaixuxia wrote:
>
>
> On 2020/9/23 1:43, Brian Foster wrote:
> > On Tue, Sep 22, 2020 at 05:04:00PM +0800, xiakaixu1987@gmail.com wrote:
> >> From: Kaixu Xia <kaixuxia@tencent.com>
> >>
> >> It is useless to go on when the variable delta equal to zero in
> >> xfs_trans_mod_dquot(), so just return if the value equal to zero.
> >>
> >> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> >> ---
> >> fs/xfs/xfs_trans_dquot.c | 12 ++++++------
> >> 1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> >> index 133fc6fc3edd..23c34af71825 100644
> >> --- a/fs/xfs/xfs_trans_dquot.c
> >> +++ b/fs/xfs/xfs_trans_dquot.c
> >> @@ -215,10 +215,11 @@ xfs_trans_mod_dquot(
> >> if (qtrx->qt_dquot == NULL)
> >> qtrx->qt_dquot = dqp;
> >>
> >> - if (delta) {
> >> - trace_xfs_trans_mod_dquot_before(qtrx);
> >> - trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
> >> - }
> >> + if (!delta)
> >> + return;
> >> +
> >
> >
> > This does slightly change behavior in that this function currently
> > unconditionally results in logging the associated dquot in the
> > transaction. I'm not sure anything really depends on that with a delta
> > == 0, but it might be worth documenting in the commit log.
> >> Also, it does seem a little odd to bail out after we've potentially
> > allocated ->t_dqinfo as well as assigned the current dquot a slot in the
> > transaction. I think that means the effect of this change is lost if
> > another dquot happens to be modified (with delta != 0) in the same
> > transaction (which might also be an odd thing to do).
> >
> Since the dquot value doesn't changes if the delta == 0, we shouldn't
> set the XFS_TRANS_DQ_DIRTY flag to current transaction. Maybe we should
> do the judgement at the beginning of the function, we will do nothing if
> the delta == 0. Just like this,
>
> xfs_trans_mod_dquot(
> {
> ...
> if (!delta)
> return;
> if (tp->t_dqinfo == NULL)
> xfs_trans_alloc_dqinfo(tp);
> ...
> }
>
> I'm not sure...What's your opinion about that?
>
Yes, I think that makes more sense than bailing out after at least.
Otherwise if some other path sets XFS_TRANS_DQ_DIRTY, then this dquot is
still associated with the transaction. I'm not sure that's currently
possible, but it's an odd wart where the current code is at least
readable/predictable. That said, note again that this changes behavior,
so it's not quite sufficient for the commit log description to just say
bail out early since delta is zero. That much is obvious from the code
change. We need to audit the behavior change and provide a few sentences
in the commit log description to explain why it is safe.
Brian
> Thanks,
> Kaixu
>
> > Brian
> >
> >> + trace_xfs_trans_mod_dquot_before(qtrx);
> >> + trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
> >>
> >> switch (field) {
> >>
> >> @@ -284,8 +285,7 @@ xfs_trans_mod_dquot(
> >> ASSERT(0);
> >> }
> >>
> >> - if (delta)
> >> - trace_xfs_trans_mod_dquot_after(qtrx);
> >> + trace_xfs_trans_mod_dquot_after(qtrx);
> >>
> >> tp->t_flags |= XFS_TRANS_DQ_DIRTY;
> >> }
> >> --
> >> 2.20.0
> >>
> >
>
> --
> kaixuxia
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/3] xfs: directly return if the delta equal to zero
2020-09-23 13:27 ` Brian Foster
@ 2020-09-23 16:09 ` Darrick J. Wong
0 siblings, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2020-09-23 16:09 UTC (permalink / raw)
To: Brian Foster; +Cc: kaixuxia, linux-xfs, Kaixu Xia
On Wed, Sep 23, 2020 at 09:27:45AM -0400, Brian Foster wrote:
> On Wed, Sep 23, 2020 at 10:42:10AM +0800, kaixuxia wrote:
> >
> >
> > On 2020/9/23 1:43, Brian Foster wrote:
> > > On Tue, Sep 22, 2020 at 05:04:00PM +0800, xiakaixu1987@gmail.com wrote:
> > >> From: Kaixu Xia <kaixuxia@tencent.com>
> > >>
> > >> It is useless to go on when the variable delta equal to zero in
> > >> xfs_trans_mod_dquot(), so just return if the value equal to zero.
> > >>
> > >> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> > >> ---
> > >> fs/xfs/xfs_trans_dquot.c | 12 ++++++------
> > >> 1 file changed, 6 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> > >> index 133fc6fc3edd..23c34af71825 100644
> > >> --- a/fs/xfs/xfs_trans_dquot.c
> > >> +++ b/fs/xfs/xfs_trans_dquot.c
> > >> @@ -215,10 +215,11 @@ xfs_trans_mod_dquot(
> > >> if (qtrx->qt_dquot == NULL)
> > >> qtrx->qt_dquot = dqp;
> > >>
> > >> - if (delta) {
> > >> - trace_xfs_trans_mod_dquot_before(qtrx);
> > >> - trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
> > >> - }
> > >> + if (!delta)
> > >> + return;
> > >> +
> > >
> > >
> > > This does slightly change behavior in that this function currently
> > > unconditionally results in logging the associated dquot in the
> > > transaction. I'm not sure anything really depends on that with a delta
> > > == 0, but it might be worth documenting in the commit log.
> > >> Also, it does seem a little odd to bail out after we've potentially
> > > allocated ->t_dqinfo as well as assigned the current dquot a slot in the
> > > transaction. I think that means the effect of this change is lost if
> > > another dquot happens to be modified (with delta != 0) in the same
> > > transaction (which might also be an odd thing to do).
> > >
> > Since the dquot value doesn't changes if the delta == 0, we shouldn't
> > set the XFS_TRANS_DQ_DIRTY flag to current transaction. Maybe we should
> > do the judgement at the beginning of the function, we will do nothing if
> > the delta == 0. Just like this,
> >
> > xfs_trans_mod_dquot(
> > {
> > ...
> > if (!delta)
> > return;
> > if (tp->t_dqinfo == NULL)
> > xfs_trans_alloc_dqinfo(tp);
> > ...
> > }
> >
> > I'm not sure...What's your opinion about that?
> >
>
> Yes, I think that makes more sense than bailing out after at least.
> Otherwise if some other path sets XFS_TRANS_DQ_DIRTY, then this dquot is
> still associated with the transaction. I'm not sure that's currently
> possible, but it's an odd wart where the current code is at least
> readable/predictable. That said, note again that this changes behavior,
> so it's not quite sufficient for the commit log description to just say
> bail out early since delta is zero. That much is obvious from the code
> change. We need to audit the behavior change and provide a few sentences
> in the commit log description to explain why it is safe.
Agreed. Sorry I didn't notice the TRANS_DQ_DIRTY thing earlier. :/
TBH I wonder if we even need that flag, since the only thing it seems to
do nowadays is shortcut checking if tp->t_dqinfo == NULL in
xfs_trans_apply_dquot_deltas and its unreserve variant.
--D
>
> Brian
>
> > Thanks,
> > Kaixu
> >
> > > Brian
> > >
> > >> + trace_xfs_trans_mod_dquot_before(qtrx);
> > >> + trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
> > >>
> > >> switch (field) {
> > >>
> > >> @@ -284,8 +285,7 @@ xfs_trans_mod_dquot(
> > >> ASSERT(0);
> > >> }
> > >>
> > >> - if (delta)
> > >> - trace_xfs_trans_mod_dquot_after(qtrx);
> > >> + trace_xfs_trans_mod_dquot_after(qtrx);
> > >>
> > >> tp->t_flags |= XFS_TRANS_DQ_DIRTY;
> > >> }
> > >> --
> > >> 2.20.0
> > >>
> > >
> >
> > --
> > kaixuxia
> >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] xfs: directly return if the delta equal to zero
2020-09-22 17:43 ` Brian Foster
2020-09-23 2:42 ` kaixuxia
@ 2020-09-23 7:01 ` Christoph Hellwig
1 sibling, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2020-09-23 7:01 UTC (permalink / raw)
To: Brian Foster; +Cc: xiakaixu1987, linux-xfs, darrick.wong, Kaixu Xia
On Tue, Sep 22, 2020 at 01:43:47PM -0400, Brian Foster wrote:
> This does slightly change behavior in that this function currently
> unconditionally results in logging the associated dquot in the
> transaction. I'm not sure anything really depends on that with a delta
> == 0, but it might be worth documenting in the commit log.
>
> Also, it does seem a little odd to bail out after we've potentially
> allocated ->t_dqinfo as well as assigned the current dquot a slot in the
> transaction. I think that means the effect of this change is lost if
> another dquot happens to be modified (with delta != 0) in the same
> transaction (which might also be an odd thing to do).
Yes, it seems like we should probably bail out at the very beginning for
delta == 0, and document what kind of changes this theoretically causes,
and why they don't matter.
Btw, the function could really use a reindent, the formatting is very
strange.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] xfs: remove the unused parameter id from xfs_qm_dqattach_one
2020-09-22 9:03 [RFC PATCH 0/3]xfs: random fixes for disk quota xiakaixu1987
2020-09-22 9:04 ` [PATCH 1/3] xfs: directly return if the delta equal to zero xiakaixu1987
@ 2020-09-22 9:04 ` xiakaixu1987
2020-09-22 16:20 ` Darrick J. Wong
2020-09-23 7:02 ` Christoph Hellwig
2020-09-22 9:04 ` [PATCH 3/3] xfs: only do dqget or dqhold for the specified dquots xiakaixu1987
2020-09-23 16:30 ` [RFC PATCH 0/3]xfs: random fixes for disk quota Darrick J. Wong
3 siblings, 2 replies; 15+ messages in thread
From: xiakaixu1987 @ 2020-09-22 9:04 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
Since we never use the second parameter id, so remove it from
xfs_qm_dqattach_one() function.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
fs/xfs/xfs_qm.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 41a459ffd1f2..44509decb4cd 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -249,7 +249,6 @@ xfs_qm_unmount_quotas(
STATIC int
xfs_qm_dqattach_one(
struct xfs_inode *ip,
- xfs_dqid_t id,
xfs_dqtype_t type,
bool doalloc,
struct xfs_dquot **IO_idqpp)
@@ -330,23 +329,23 @@ xfs_qm_dqattach_locked(
ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
- error = xfs_qm_dqattach_one(ip, i_uid_read(VFS_I(ip)),
- XFS_DQTYPE_USER, doalloc, &ip->i_udquot);
+ error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_USER,
+ doalloc, &ip->i_udquot);
if (error)
goto done;
ASSERT(ip->i_udquot);
}
if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
- error = xfs_qm_dqattach_one(ip, i_gid_read(VFS_I(ip)),
- XFS_DQTYPE_GROUP, doalloc, &ip->i_gdquot);
+ error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_GROUP,
+ doalloc, &ip->i_gdquot);
if (error)
goto done;
ASSERT(ip->i_gdquot);
}
if (XFS_IS_PQUOTA_ON(mp) && !ip->i_pdquot) {
- error = xfs_qm_dqattach_one(ip, ip->i_d.di_projid, XFS_DQTYPE_PROJ,
+ error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_PROJ,
doalloc, &ip->i_pdquot);
if (error)
goto done;
--
2.20.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 2/3] xfs: remove the unused parameter id from xfs_qm_dqattach_one
2020-09-22 9:04 ` [PATCH 2/3] xfs: remove the unused parameter id from xfs_qm_dqattach_one xiakaixu1987
@ 2020-09-22 16:20 ` Darrick J. Wong
2020-09-23 7:02 ` Christoph Hellwig
1 sibling, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2020-09-22 16:20 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
On Tue, Sep 22, 2020 at 05:04:01PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> Since we never use the second parameter id, so remove it from
> xfs_qm_dqattach_one() function.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
Heh, yep.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_qm.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 41a459ffd1f2..44509decb4cd 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -249,7 +249,6 @@ xfs_qm_unmount_quotas(
> STATIC int
> xfs_qm_dqattach_one(
> struct xfs_inode *ip,
> - xfs_dqid_t id,
> xfs_dqtype_t type,
> bool doalloc,
> struct xfs_dquot **IO_idqpp)
> @@ -330,23 +329,23 @@ xfs_qm_dqattach_locked(
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>
> if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
> - error = xfs_qm_dqattach_one(ip, i_uid_read(VFS_I(ip)),
> - XFS_DQTYPE_USER, doalloc, &ip->i_udquot);
> + error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_USER,
> + doalloc, &ip->i_udquot);
> if (error)
> goto done;
> ASSERT(ip->i_udquot);
> }
>
> if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
> - error = xfs_qm_dqattach_one(ip, i_gid_read(VFS_I(ip)),
> - XFS_DQTYPE_GROUP, doalloc, &ip->i_gdquot);
> + error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_GROUP,
> + doalloc, &ip->i_gdquot);
> if (error)
> goto done;
> ASSERT(ip->i_gdquot);
> }
>
> if (XFS_IS_PQUOTA_ON(mp) && !ip->i_pdquot) {
> - error = xfs_qm_dqattach_one(ip, ip->i_d.di_projid, XFS_DQTYPE_PROJ,
> + error = xfs_qm_dqattach_one(ip, XFS_DQTYPE_PROJ,
> doalloc, &ip->i_pdquot);
> if (error)
> goto done;
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/3] xfs: remove the unused parameter id from xfs_qm_dqattach_one
2020-09-22 9:04 ` [PATCH 2/3] xfs: remove the unused parameter id from xfs_qm_dqattach_one xiakaixu1987
2020-09-22 16:20 ` Darrick J. Wong
@ 2020-09-23 7:02 ` Christoph Hellwig
1 sibling, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2020-09-23 7:02 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, darrick.wong, Kaixu Xia
On Tue, Sep 22, 2020 at 05:04:01PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> Since we never use the second parameter id, so remove it from
> xfs_qm_dqattach_one() function.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] xfs: only do dqget or dqhold for the specified dquots
2020-09-22 9:03 [RFC PATCH 0/3]xfs: random fixes for disk quota xiakaixu1987
2020-09-22 9:04 ` [PATCH 1/3] xfs: directly return if the delta equal to zero xiakaixu1987
2020-09-22 9:04 ` [PATCH 2/3] xfs: remove the unused parameter id from xfs_qm_dqattach_one xiakaixu1987
@ 2020-09-22 9:04 ` xiakaixu1987
2020-09-22 16:18 ` Darrick J. Wong
2020-09-23 16:30 ` [RFC PATCH 0/3]xfs: random fixes for disk quota Darrick J. Wong
3 siblings, 1 reply; 15+ messages in thread
From: xiakaixu1987 @ 2020-09-22 9:04 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
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.
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] xfs: only do dqget or dqhold for the specified dquots
2020-09-22 9:04 ` [PATCH 3/3] xfs: only do dqget or dqhold for the specified dquots xiakaixu1987
@ 2020-09-22 16:18 ` Darrick J. Wong
2020-09-23 3:11 ` kaixuxia
0 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2020-09-22 16:18 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/3] xfs: only do dqget or dqhold for the specified dquots
2020-09-22 16:18 ` Darrick J. Wong
@ 2020-09-23 3:11 ` kaixuxia
0 siblings, 0 replies; 15+ messages in thread
From: kaixuxia @ 2020-09-23 3:11 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, Kaixu Xia
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
>>
--
kaixuxia
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH 0/3]xfs: random fixes for disk quota
2020-09-22 9:03 [RFC PATCH 0/3]xfs: random fixes for disk quota xiakaixu1987
` (2 preceding siblings ...)
2020-09-22 9:04 ` [PATCH 3/3] xfs: only do dqget or dqhold for the specified dquots xiakaixu1987
@ 2020-09-23 16:30 ` Darrick J. Wong
3 siblings, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2020-09-23 16:30 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
On Tue, Sep 22, 2020 at 05:03:59PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> Hi all,
>
> This patchset include random fixes and code cleanups for disk quota.
> In order to make it easier to track, I bundle them up and put all
> the scattered patches into a single patchset.
FWIW I've applied the second patch, so you can drop it from the next
resend.
--D
> Kaixu Xia (3):
> xfs: directly return if the delta equal to zero
> xfs: remove the unused parameter id from xfs_qm_dqattach_one
> xfs: only do dqget or dqhold for the specified dquots
>
> fs/xfs/xfs_qm.c | 24 +++++++++---------------
> fs/xfs/xfs_trans_dquot.c | 12 ++++++------
> 2 files changed, 15 insertions(+), 21 deletions(-)
>
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 15+ messages in thread