* [PATCH v6 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation
2020-10-16 3:38 [PATCH v6 0/3] xfs: random fixes for disk quota xiakaixu1987
@ 2020-10-16 3:38 ` xiakaixu1987
2020-10-26 22:52 ` Darrick J. Wong
2020-10-16 3:38 ` [PATCH v6 2/3] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag xiakaixu1987
2020-10-16 3:38 ` [PATCH v6 3/3] xfs: directly return if the delta equal to zero xiakaixu1987
2 siblings, 1 reply; 9+ messages in thread
From: xiakaixu1987 @ 2020-10-16 3:38 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
The function xfs_trans_mod_dquot_byino() wraps around
xfs_trans_mod_dquot() to account for quotas, and also there is the
function call chain xfs_trans_reserve_quota_bydquots -> xfs_trans_dqresv
-> xfs_trans_mod_dquot, both of them do the duplicated null check and
allocation. Thus we can delete the duplicated operation from them.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_trans_dquot.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index fe45b0c3970c..67f1e275b34d 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -143,9 +143,6 @@ xfs_trans_mod_dquot_byino(
xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
return;
- if (tp->t_dqinfo == NULL)
- xfs_trans_alloc_dqinfo(tp);
-
if (XFS_IS_UQUOTA_ON(mp) && ip->i_udquot)
(void) xfs_trans_mod_dquot(tp, ip->i_udquot, field, delta);
if (XFS_IS_GQUOTA_ON(mp) && ip->i_gdquot)
@@ -698,7 +695,6 @@ xfs_trans_dqresv(
* because we don't have the luxury of a transaction envelope then.
*/
if (tp) {
- ASSERT(tp->t_dqinfo);
ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
if (nblks != 0)
xfs_trans_mod_dquot(tp, dqp,
@@ -752,9 +748,6 @@ xfs_trans_reserve_quota_bydquots(
if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
return 0;
- if (tp && tp->t_dqinfo == NULL)
- xfs_trans_alloc_dqinfo(tp);
-
ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
if (udqp) {
--
2.20.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation
2020-10-16 3:38 ` [PATCH v6 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation xiakaixu1987
@ 2020-10-26 22:52 ` Darrick J. Wong
2020-11-12 2:53 ` kaixuxia
0 siblings, 1 reply; 9+ messages in thread
From: Darrick J. Wong @ 2020-10-26 22:52 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
On Fri, Oct 16, 2020 at 11:38:26AM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> The function xfs_trans_mod_dquot_byino() wraps around
> xfs_trans_mod_dquot() to account for quotas, and also there is the
> function call chain xfs_trans_reserve_quota_bydquots -> xfs_trans_dqresv
> -> xfs_trans_mod_dquot, both of them do the duplicated null check and
> allocation. Thus we can delete the duplicated operation from them.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
HAH this got all the way to v6, sorry I suck. :(
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_trans_dquot.c | 7 -------
> 1 file changed, 7 deletions(-)
>
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index fe45b0c3970c..67f1e275b34d 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -143,9 +143,6 @@ xfs_trans_mod_dquot_byino(
> xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
> return;
>
> - if (tp->t_dqinfo == NULL)
> - xfs_trans_alloc_dqinfo(tp);
> -
> if (XFS_IS_UQUOTA_ON(mp) && ip->i_udquot)
> (void) xfs_trans_mod_dquot(tp, ip->i_udquot, field, delta);
> if (XFS_IS_GQUOTA_ON(mp) && ip->i_gdquot)
> @@ -698,7 +695,6 @@ xfs_trans_dqresv(
> * because we don't have the luxury of a transaction envelope then.
> */
> if (tp) {
> - ASSERT(tp->t_dqinfo);
> ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
> if (nblks != 0)
> xfs_trans_mod_dquot(tp, dqp,
> @@ -752,9 +748,6 @@ xfs_trans_reserve_quota_bydquots(
> if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
> return 0;
>
> - if (tp && tp->t_dqinfo == NULL)
> - xfs_trans_alloc_dqinfo(tp);
> -
> ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
>
> if (udqp) {
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation
2020-10-26 22:52 ` Darrick J. Wong
@ 2020-11-12 2:53 ` kaixuxia
2020-11-13 1:55 ` Darrick J. Wong
0 siblings, 1 reply; 9+ messages in thread
From: kaixuxia @ 2020-11-12 2:53 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, Kaixu Xia
On 2020/10/27 6:52, Darrick J. Wong wrote:
> On Fri, Oct 16, 2020 at 11:38:26AM +0800, xiakaixu1987@gmail.com wrote:
>> From: Kaixu Xia <kaixuxia@tencent.com>
>>
>> The function xfs_trans_mod_dquot_byino() wraps around
>> xfs_trans_mod_dquot() to account for quotas, and also there is the
>> function call chain xfs_trans_reserve_quota_bydquots -> xfs_trans_dqresv
>> -> xfs_trans_mod_dquot, both of them do the duplicated null check and
>> allocation. Thus we can delete the duplicated operation from them.
>>
>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> HAH this got all the way to v6, sorry I suck. :(
>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Hi Darrick,
There are some patches that have been reviewed but not been merged
into xfs for-next branch, I will reply to them.
Sorry for the noise:)
Thanks,
Kaixu
>
> --D
>
>> ---
>> fs/xfs/xfs_trans_dquot.c | 7 -------
>> 1 file changed, 7 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
>> index fe45b0c3970c..67f1e275b34d 100644
>> --- a/fs/xfs/xfs_trans_dquot.c
>> +++ b/fs/xfs/xfs_trans_dquot.c
>> @@ -143,9 +143,6 @@ xfs_trans_mod_dquot_byino(
>> xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
>> return;
>>
>> - if (tp->t_dqinfo == NULL)
>> - xfs_trans_alloc_dqinfo(tp);
>> -
>> if (XFS_IS_UQUOTA_ON(mp) && ip->i_udquot)
>> (void) xfs_trans_mod_dquot(tp, ip->i_udquot, field, delta);
>> if (XFS_IS_GQUOTA_ON(mp) && ip->i_gdquot)
>> @@ -698,7 +695,6 @@ xfs_trans_dqresv(
>> * because we don't have the luxury of a transaction envelope then.
>> */
>> if (tp) {
>> - ASSERT(tp->t_dqinfo);
>> ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
>> if (nblks != 0)
>> xfs_trans_mod_dquot(tp, dqp,
>> @@ -752,9 +748,6 @@ xfs_trans_reserve_quota_bydquots(
>> if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
>> return 0;
>>
>> - if (tp && tp->t_dqinfo == NULL)
>> - xfs_trans_alloc_dqinfo(tp);
>> -
>> ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
>>
>> if (udqp) {
>> --
>> 2.20.0
>>
--
kaixuxia
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation
2020-11-12 2:53 ` kaixuxia
@ 2020-11-13 1:55 ` Darrick J. Wong
2020-11-13 3:26 ` kaixuxia
0 siblings, 1 reply; 9+ messages in thread
From: Darrick J. Wong @ 2020-11-13 1:55 UTC (permalink / raw)
To: kaixuxia; +Cc: linux-xfs, Kaixu Xia
On Thu, Nov 12, 2020 at 10:53:39AM +0800, kaixuxia wrote:
>
>
> On 2020/10/27 6:52, Darrick J. Wong wrote:
> > On Fri, Oct 16, 2020 at 11:38:26AM +0800, xiakaixu1987@gmail.com wrote:
> >> From: Kaixu Xia <kaixuxia@tencent.com>
> >>
> >> The function xfs_trans_mod_dquot_byino() wraps around
> >> xfs_trans_mod_dquot() to account for quotas, and also there is the
> >> function call chain xfs_trans_reserve_quota_bydquots -> xfs_trans_dqresv
> >> -> xfs_trans_mod_dquot, both of them do the duplicated null check and
> >> allocation. Thus we can delete the duplicated operation from them.
> >>
> >> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> >> Reviewed-by: Brian Foster <bfoster@redhat.com>
> >> Reviewed-by: Christoph Hellwig <hch@lst.de>
> >
> > HAH this got all the way to v6, sorry I suck. :(
> >
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>
> Hi Darrick,
>
> There are some patches that have been reviewed but not been merged
> into xfs for-next branch, I will reply to them.
> Sorry for the noise:)
Same situation here -- these are 5.11 cleanups and I'm still working on
bug fixes for 5.10. If you have time to review patches, can you please
have a look at the unreviewed patches in the series "xfs: fix various
scrub problems", please?
--D
> Thanks,
> Kaixu
> >
> > --D
> >
> >> ---
> >> fs/xfs/xfs_trans_dquot.c | 7 -------
> >> 1 file changed, 7 deletions(-)
> >>
> >> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> >> index fe45b0c3970c..67f1e275b34d 100644
> >> --- a/fs/xfs/xfs_trans_dquot.c
> >> +++ b/fs/xfs/xfs_trans_dquot.c
> >> @@ -143,9 +143,6 @@ xfs_trans_mod_dquot_byino(
> >> xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
> >> return;
> >>
> >> - if (tp->t_dqinfo == NULL)
> >> - xfs_trans_alloc_dqinfo(tp);
> >> -
> >> if (XFS_IS_UQUOTA_ON(mp) && ip->i_udquot)
> >> (void) xfs_trans_mod_dquot(tp, ip->i_udquot, field, delta);
> >> if (XFS_IS_GQUOTA_ON(mp) && ip->i_gdquot)
> >> @@ -698,7 +695,6 @@ xfs_trans_dqresv(
> >> * because we don't have the luxury of a transaction envelope then.
> >> */
> >> if (tp) {
> >> - ASSERT(tp->t_dqinfo);
> >> ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
> >> if (nblks != 0)
> >> xfs_trans_mod_dquot(tp, dqp,
> >> @@ -752,9 +748,6 @@ xfs_trans_reserve_quota_bydquots(
> >> if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
> >> return 0;
> >>
> >> - if (tp && tp->t_dqinfo == NULL)
> >> - xfs_trans_alloc_dqinfo(tp);
> >> -
> >> ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
> >>
> >> if (udqp) {
> >> --
> >> 2.20.0
> >>
>
> --
> kaixuxia
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation
2020-11-13 1:55 ` Darrick J. Wong
@ 2020-11-13 3:26 ` kaixuxia
0 siblings, 0 replies; 9+ messages in thread
From: kaixuxia @ 2020-11-13 3:26 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, Kaixu Xia
On 2020/11/13 9:55, Darrick J. Wong wrote:
> On Thu, Nov 12, 2020 at 10:53:39AM +0800, kaixuxia wrote:
>>
>>
>> On 2020/10/27 6:52, Darrick J. Wong wrote:
>>> On Fri, Oct 16, 2020 at 11:38:26AM +0800, xiakaixu1987@gmail.com wrote:
>>>> From: Kaixu Xia <kaixuxia@tencent.com>
>>>>
>>>> The function xfs_trans_mod_dquot_byino() wraps around
>>>> xfs_trans_mod_dquot() to account for quotas, and also there is the
>>>> function call chain xfs_trans_reserve_quota_bydquots -> xfs_trans_dqresv
>>>> -> xfs_trans_mod_dquot, both of them do the duplicated null check and
>>>> allocation. Thus we can delete the duplicated operation from them.
>>>>
>>>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
>>>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>>
>>> HAH this got all the way to v6, sorry I suck. :(
>>>
>>> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> Hi Darrick,
>>
>> There are some patches that have been reviewed but not been merged
>> into xfs for-next branch, I will reply to them.
>> Sorry for the noise:)
>
> Same situation here -- these are 5.11 cleanups and I'm still working on
> bug fixes for 5.10. If you have time to review patches, can you please
> have a look at the unreviewed patches in the series "xfs: fix various
> scrub problems", please?
> Hi Darrick,
Thank you for the invitation! I am very interested and happy to review
these patches. I will review them when I have time:)
Thanks,
Kaixu
> --D
>
>> Thanks,
>> Kaixu
>>>
>>> --D
>>>
>>>> ---
>>>> fs/xfs/xfs_trans_dquot.c | 7 -------
>>>> 1 file changed, 7 deletions(-)
>>>>
>>>> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
>>>> index fe45b0c3970c..67f1e275b34d 100644
>>>> --- a/fs/xfs/xfs_trans_dquot.c
>>>> +++ b/fs/xfs/xfs_trans_dquot.c
>>>> @@ -143,9 +143,6 @@ xfs_trans_mod_dquot_byino(
>>>> xfs_is_quota_inode(&mp->m_sb, ip->i_ino))
>>>> return;
>>>>
>>>> - if (tp->t_dqinfo == NULL)
>>>> - xfs_trans_alloc_dqinfo(tp);
>>>> -
>>>> if (XFS_IS_UQUOTA_ON(mp) && ip->i_udquot)
>>>> (void) xfs_trans_mod_dquot(tp, ip->i_udquot, field, delta);
>>>> if (XFS_IS_GQUOTA_ON(mp) && ip->i_gdquot)
>>>> @@ -698,7 +695,6 @@ xfs_trans_dqresv(
>>>> * because we don't have the luxury of a transaction envelope then.
>>>> */
>>>> if (tp) {
>>>> - ASSERT(tp->t_dqinfo);
>>>> ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
>>>> if (nblks != 0)
>>>> xfs_trans_mod_dquot(tp, dqp,
>>>> @@ -752,9 +748,6 @@ xfs_trans_reserve_quota_bydquots(
>>>> if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
>>>> return 0;
>>>>
>>>> - if (tp && tp->t_dqinfo == NULL)
>>>> - xfs_trans_alloc_dqinfo(tp);
>>>> -
>>>> ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
>>>>
>>>> if (udqp) {
>>>> --
>>>> 2.20.0
>>>>
>>
>> --
>> kaixuxia
--
kaixuxia
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v6 2/3] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag
2020-10-16 3:38 [PATCH v6 0/3] xfs: random fixes for disk quota xiakaixu1987
2020-10-16 3:38 ` [PATCH v6 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation xiakaixu1987
@ 2020-10-16 3:38 ` xiakaixu1987
2020-10-26 22:52 ` Darrick J. Wong
2020-10-16 3:38 ` [PATCH v6 3/3] xfs: directly return if the delta equal to zero xiakaixu1987
2 siblings, 1 reply; 9+ messages in thread
From: xiakaixu1987 @ 2020-10-16 3:38 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
Nowadays the only things that the XFS_TRANS_DQ_DIRTY flag seems to do
are indicates the tp->t_dqinfo->dqs[XFS_QM_TRANS_{USR,GRP,PRJ}] values
changed and check in xfs_trans_apply_dquot_deltas() and the unreserve
variant xfs_trans_unreserve_and_mod_dquots(). Actually, we also can
use the tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag, that
is to say, we allocate the new tp->t_dqinfo only when the qtrx values
changed, so the tp->t_dqinfo value isn't NULL equals the XFS_TRANS_DQ_DIRTY
flag is set, we only need to check if tp->t_dqinfo == NULL in
xfs_trans_apply_dquot_deltas() and its unreserve variant to determine
whether lock all of the dquots and join them to the transaction.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_shared.h | 1 -
fs/xfs/xfs_inode.c | 8 +-------
fs/xfs/xfs_trans_dquot.c | 13 ++-----------
3 files changed, 3 insertions(+), 19 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
index c795ae47b3c9..8c61a461bf7b 100644
--- a/fs/xfs/libxfs/xfs_shared.h
+++ b/fs/xfs/libxfs/xfs_shared.h
@@ -62,7 +62,6 @@ void xfs_log_get_max_trans_res(struct xfs_mount *mp,
#define XFS_TRANS_SB_DIRTY 0x02 /* superblock is modified */
#define XFS_TRANS_PERM_LOG_RES 0x04 /* xact took a permanent log res */
#define XFS_TRANS_SYNC 0x08 /* make commit synchronous */
-#define XFS_TRANS_DQ_DIRTY 0x10 /* at least one dquot in trx dirty */
#define XFS_TRANS_RESERVE 0x20 /* OK to use reserved data blocks */
#define XFS_TRANS_NO_WRITECOUNT 0x40 /* do not elevate SB writecount */
#define XFS_TRANS_RES_FDBLKS 0x80 /* reserve newly freed blocks */
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 20bb5fae0d00..16885624015e 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -959,7 +959,6 @@ xfs_dir_ialloc(
xfs_buf_t *ialloc_context = NULL;
int code;
void *dqinfo;
- uint tflags;
tp = *tpp;
ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
@@ -1018,12 +1017,9 @@ xfs_dir_ialloc(
* and attach it to the next transaction.
*/
dqinfo = NULL;
- tflags = 0;
if (tp->t_dqinfo) {
dqinfo = (void *)tp->t_dqinfo;
tp->t_dqinfo = NULL;
- tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
- tp->t_flags &= ~(XFS_TRANS_DQ_DIRTY);
}
code = xfs_trans_roll(&tp);
@@ -1031,10 +1027,8 @@ xfs_dir_ialloc(
/*
* Re-attach the quota info that we detached from prev trx.
*/
- if (dqinfo) {
+ if (dqinfo)
tp->t_dqinfo = dqinfo;
- tp->t_flags |= tflags;
- }
if (code) {
xfs_buf_relse(ialloc_context);
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index 67f1e275b34d..0ebfd7930382 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -84,13 +84,6 @@ xfs_trans_dup_dqinfo(
xfs_trans_alloc_dqinfo(ntp);
- /*
- * Because the quota blk reservation is carried forward,
- * it is also necessary to carry forward the DQ_DIRTY flag.
- */
- if (otp->t_flags & XFS_TRANS_DQ_DIRTY)
- ntp->t_flags |= XFS_TRANS_DQ_DIRTY;
-
for (j = 0; j < XFS_QM_TRANS_DQTYPES; j++) {
oqa = otp->t_dqinfo->dqs[j];
nqa = ntp->t_dqinfo->dqs[j];
@@ -270,8 +263,6 @@ xfs_trans_mod_dquot(
if (delta)
trace_xfs_trans_mod_dquot_after(qtrx);
-
- tp->t_flags |= XFS_TRANS_DQ_DIRTY;
}
@@ -348,7 +339,7 @@ xfs_trans_apply_dquot_deltas(
int64_t totalbdelta;
int64_t totalrtbdelta;
- if (!(tp->t_flags & XFS_TRANS_DQ_DIRTY))
+ if (!tp->t_dqinfo)
return;
ASSERT(tp->t_dqinfo);
@@ -490,7 +481,7 @@ xfs_trans_unreserve_and_mod_dquots(
struct xfs_dqtrx *qtrx, *qa;
bool locked;
- if (!tp->t_dqinfo || !(tp->t_flags & XFS_TRANS_DQ_DIRTY))
+ if (!tp->t_dqinfo)
return;
for (j = 0; j < XFS_QM_TRANS_DQTYPES; j++) {
--
2.20.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v6 2/3] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag
2020-10-16 3:38 ` [PATCH v6 2/3] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag xiakaixu1987
@ 2020-10-26 22:52 ` Darrick J. Wong
0 siblings, 0 replies; 9+ messages in thread
From: Darrick J. Wong @ 2020-10-26 22:52 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
On Fri, Oct 16, 2020 at 11:38:27AM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> Nowadays the only things that the XFS_TRANS_DQ_DIRTY flag seems to do
> are indicates the tp->t_dqinfo->dqs[XFS_QM_TRANS_{USR,GRP,PRJ}] values
> changed and check in xfs_trans_apply_dquot_deltas() and the unreserve
> variant xfs_trans_unreserve_and_mod_dquots(). Actually, we also can
> use the tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag, that
> is to say, we allocate the new tp->t_dqinfo only when the qtrx values
> changed, so the tp->t_dqinfo value isn't NULL equals the XFS_TRANS_DQ_DIRTY
> flag is set, we only need to check if tp->t_dqinfo == NULL in
> xfs_trans_apply_dquot_deltas() and its unreserve variant to determine
> whether lock all of the dquots and join them to the transaction.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Still yay,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/libxfs/xfs_shared.h | 1 -
> fs/xfs/xfs_inode.c | 8 +-------
> fs/xfs/xfs_trans_dquot.c | 13 ++-----------
> 3 files changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index c795ae47b3c9..8c61a461bf7b 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -62,7 +62,6 @@ void xfs_log_get_max_trans_res(struct xfs_mount *mp,
> #define XFS_TRANS_SB_DIRTY 0x02 /* superblock is modified */
> #define XFS_TRANS_PERM_LOG_RES 0x04 /* xact took a permanent log res */
> #define XFS_TRANS_SYNC 0x08 /* make commit synchronous */
> -#define XFS_TRANS_DQ_DIRTY 0x10 /* at least one dquot in trx dirty */
> #define XFS_TRANS_RESERVE 0x20 /* OK to use reserved data blocks */
> #define XFS_TRANS_NO_WRITECOUNT 0x40 /* do not elevate SB writecount */
> #define XFS_TRANS_RES_FDBLKS 0x80 /* reserve newly freed blocks */
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 20bb5fae0d00..16885624015e 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -959,7 +959,6 @@ xfs_dir_ialloc(
> xfs_buf_t *ialloc_context = NULL;
> int code;
> void *dqinfo;
> - uint tflags;
>
> tp = *tpp;
> ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> @@ -1018,12 +1017,9 @@ xfs_dir_ialloc(
> * and attach it to the next transaction.
> */
> dqinfo = NULL;
> - tflags = 0;
> if (tp->t_dqinfo) {
> dqinfo = (void *)tp->t_dqinfo;
> tp->t_dqinfo = NULL;
> - tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
> - tp->t_flags &= ~(XFS_TRANS_DQ_DIRTY);
> }
>
> code = xfs_trans_roll(&tp);
> @@ -1031,10 +1027,8 @@ xfs_dir_ialloc(
> /*
> * Re-attach the quota info that we detached from prev trx.
> */
> - if (dqinfo) {
> + if (dqinfo)
> tp->t_dqinfo = dqinfo;
> - tp->t_flags |= tflags;
> - }
>
> if (code) {
> xfs_buf_relse(ialloc_context);
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index 67f1e275b34d..0ebfd7930382 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -84,13 +84,6 @@ xfs_trans_dup_dqinfo(
>
> xfs_trans_alloc_dqinfo(ntp);
>
> - /*
> - * Because the quota blk reservation is carried forward,
> - * it is also necessary to carry forward the DQ_DIRTY flag.
> - */
> - if (otp->t_flags & XFS_TRANS_DQ_DIRTY)
> - ntp->t_flags |= XFS_TRANS_DQ_DIRTY;
> -
> for (j = 0; j < XFS_QM_TRANS_DQTYPES; j++) {
> oqa = otp->t_dqinfo->dqs[j];
> nqa = ntp->t_dqinfo->dqs[j];
> @@ -270,8 +263,6 @@ xfs_trans_mod_dquot(
>
> if (delta)
> trace_xfs_trans_mod_dquot_after(qtrx);
> -
> - tp->t_flags |= XFS_TRANS_DQ_DIRTY;
> }
>
>
> @@ -348,7 +339,7 @@ xfs_trans_apply_dquot_deltas(
> int64_t totalbdelta;
> int64_t totalrtbdelta;
>
> - if (!(tp->t_flags & XFS_TRANS_DQ_DIRTY))
> + if (!tp->t_dqinfo)
> return;
>
> ASSERT(tp->t_dqinfo);
> @@ -490,7 +481,7 @@ xfs_trans_unreserve_and_mod_dquots(
> struct xfs_dqtrx *qtrx, *qa;
> bool locked;
>
> - if (!tp->t_dqinfo || !(tp->t_flags & XFS_TRANS_DQ_DIRTY))
> + if (!tp->t_dqinfo)
> return;
>
> for (j = 0; j < XFS_QM_TRANS_DQTYPES; j++) {
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v6 3/3] xfs: directly return if the delta equal to zero
2020-10-16 3:38 [PATCH v6 0/3] xfs: random fixes for disk quota xiakaixu1987
2020-10-16 3:38 ` [PATCH v6 1/3] xfs: delete duplicated tp->t_dqinfo null check and allocation xiakaixu1987
2020-10-16 3:38 ` [PATCH v6 2/3] xfs: check tp->t_dqinfo value instead of the XFS_TRANS_DQ_DIRTY flag xiakaixu1987
@ 2020-10-16 3:38 ` xiakaixu1987
2 siblings, 0 replies; 9+ messages in thread
From: xiakaixu1987 @ 2020-10-16 3:38 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
The xfs_trans_mod_dquot() function will allocate new tp->t_dqinfo if
it is NULL and make the changes in the tp->t_dqinfo->dqs[XFS_QM_TRANS
_{USR,GRP,PRJ}]. Nowadays seems none of the callers want to join the
dquots to the transaction and push them to device when the delta is
zero. Actually, most of time the caller would check the delta and go
on only when the delta value is not zero, so we should bail out when
it is zero.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_trans_dquot.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index 0ebfd7930382..28b8ac701919 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -194,6 +194,9 @@ xfs_trans_mod_dquot(
ASSERT(XFS_IS_QUOTA_RUNNING(tp->t_mountp));
qtrx = NULL;
+ if (!delta)
+ return;
+
if (tp->t_dqinfo == NULL)
xfs_trans_alloc_dqinfo(tp);
/*
@@ -205,10 +208,8 @@ 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);
- }
+ trace_xfs_trans_mod_dquot_before(qtrx);
+ trace_xfs_trans_mod_dquot(tp, dqp, field, delta);
switch (field) {
/* regular disk blk reservation */
@@ -261,8 +262,7 @@ xfs_trans_mod_dquot(
ASSERT(0);
}
- if (delta)
- trace_xfs_trans_mod_dquot_after(qtrx);
+ trace_xfs_trans_mod_dquot_after(qtrx);
}
@@ -687,14 +687,9 @@ xfs_trans_dqresv(
*/
if (tp) {
ASSERT(flags & XFS_QMOPT_RESBLK_MASK);
- if (nblks != 0)
- xfs_trans_mod_dquot(tp, dqp,
- flags & XFS_QMOPT_RESBLK_MASK,
- nblks);
- if (ninos != 0)
- xfs_trans_mod_dquot(tp, dqp,
- XFS_TRANS_DQ_RES_INOS,
- ninos);
+ xfs_trans_mod_dquot(tp, dqp, flags & XFS_QMOPT_RESBLK_MASK,
+ nblks);
+ xfs_trans_mod_dquot(tp, dqp, XFS_TRANS_DQ_RES_INOS, ninos);
}
ASSERT(dqp->q_blk.reserved >= dqp->q_blk.count);
ASSERT(dqp->q_rtb.reserved >= dqp->q_rtb.count);
--
2.20.0
^ permalink raw reply related [flat|nested] 9+ messages in thread