* [PATCH] xfs: remove the unused SYNCHRONIZE macro
2020-09-16 11:19 [RFC PATCH 0/9]xfs: random fixes and code cleanup xiakaixu1987
@ 2020-09-16 11:19 ` xiakaixu1987
2020-09-16 21:28 ` Darrick J. Wong
2020-09-17 8:03 ` Christoph Hellwig
2020-09-16 11:19 ` [PATCH] xfs: use the existing type definition for di_projid xiakaixu1987
` (7 subsequent siblings)
8 siblings, 2 replies; 26+ messages in thread
From: xiakaixu1987 @ 2020-09-16 11:19 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
There are no callers of the SYNCHRONIZE() macro, so remove it.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
fs/xfs/xfs_linux.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index ab737fed7b12..ad1009778d33 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -123,7 +123,6 @@ typedef __u32 xfs_nlink_t;
#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */
#define EFSBADCRC EBADMSG /* Bad CRC detected */
-#define SYNCHRONIZE() barrier()
#define __return_address __builtin_return_address(0)
/*
--
2.20.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH] xfs: remove the unused SYNCHRONIZE macro
2020-09-16 11:19 ` [PATCH] xfs: remove the unused SYNCHRONIZE macro xiakaixu1987
@ 2020-09-16 21:28 ` Darrick J. Wong
2020-09-17 8:03 ` Christoph Hellwig
1 sibling, 0 replies; 26+ messages in thread
From: Darrick J. Wong @ 2020-09-16 21:28 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
On Wed, Sep 16, 2020 at 07:19:04PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> There are no callers of the SYNCHRONIZE() macro, so remove it.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_linux.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index ab737fed7b12..ad1009778d33 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -123,7 +123,6 @@ typedef __u32 xfs_nlink_t;
> #define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */
> #define EFSBADCRC EBADMSG /* Bad CRC detected */
>
> -#define SYNCHRONIZE() barrier()
> #define __return_address __builtin_return_address(0)
>
> /*
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xfs: remove the unused SYNCHRONIZE macro
2020-09-16 11:19 ` [PATCH] xfs: remove the unused SYNCHRONIZE macro xiakaixu1987
2020-09-16 21:28 ` Darrick J. Wong
@ 2020-09-17 8:03 ` Christoph Hellwig
1 sibling, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2020-09-17 8:03 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, darrick.wong, Kaixu Xia
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] xfs: use the existing type definition for di_projid
2020-09-16 11:19 [RFC PATCH 0/9]xfs: random fixes and code cleanup xiakaixu1987
2020-09-16 11:19 ` [PATCH] xfs: remove the unused SYNCHRONIZE macro xiakaixu1987
@ 2020-09-16 11:19 ` xiakaixu1987
2020-09-16 21:30 ` Darrick J. Wong
2020-09-17 8:06 ` Christoph Hellwig
2020-09-16 11:19 ` [PATCH] xfs: remove the unnecessary xfs_dqid_t type cast xiakaixu1987
` (6 subsequent siblings)
8 siblings, 2 replies; 26+ messages in thread
From: xiakaixu1987 @ 2020-09-16 11:19 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
We have already defined the project ID type prid_t, so maybe should
use it here.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
fs/xfs/libxfs/xfs_inode_buf.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
index 536666143fe7..ef5eaf33d146 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.h
+++ b/fs/xfs/libxfs/xfs_inode_buf.h
@@ -17,7 +17,7 @@ struct xfs_dinode;
*/
struct xfs_icdinode {
uint16_t di_flushiter; /* incremented on flush */
- uint32_t di_projid; /* owner's project id */
+ prid_t di_projid; /* owner's project id */
xfs_fsize_t di_size; /* number of bytes in file */
xfs_rfsblock_t di_nblocks; /* # of direct & btree blocks used */
xfs_extlen_t di_extsize; /* basic/minimum extent size for file */
--
2.20.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH] xfs: use the existing type definition for di_projid
2020-09-16 11:19 ` [PATCH] xfs: use the existing type definition for di_projid xiakaixu1987
@ 2020-09-16 21:30 ` Darrick J. Wong
2020-09-17 8:06 ` Christoph Hellwig
1 sibling, 0 replies; 26+ messages in thread
From: Darrick J. Wong @ 2020-09-16 21:30 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
On Wed, Sep 16, 2020 at 07:19:05PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> We have already defined the project ID type prid_t, so maybe should
> use it here.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
Looks ok to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/libxfs/xfs_inode_buf.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> index 536666143fe7..ef5eaf33d146 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.h
> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> @@ -17,7 +17,7 @@ struct xfs_dinode;
> */
> struct xfs_icdinode {
> uint16_t di_flushiter; /* incremented on flush */
> - uint32_t di_projid; /* owner's project id */
> + prid_t di_projid; /* owner's project id */
> xfs_fsize_t di_size; /* number of bytes in file */
> xfs_rfsblock_t di_nblocks; /* # of direct & btree blocks used */
> xfs_extlen_t di_extsize; /* basic/minimum extent size for file */
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xfs: use the existing type definition for di_projid
2020-09-16 11:19 ` [PATCH] xfs: use the existing type definition for di_projid xiakaixu1987
2020-09-16 21:30 ` Darrick J. Wong
@ 2020-09-17 8:06 ` Christoph Hellwig
1 sibling, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2020-09-17 8:06 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, darrick.wong, Kaixu Xia
On Wed, Sep 16, 2020 at 07:19:05PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> We have already defined the project ID type prid_t, so maybe should
> use it here.
Looks ok, but I have a long pending series I need to resubmit to
kill of xfs_icdinode, for which this would just create churn..
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] xfs: remove the unnecessary xfs_dqid_t type cast
2020-09-16 11:19 [RFC PATCH 0/9]xfs: random fixes and code cleanup xiakaixu1987
2020-09-16 11:19 ` [PATCH] xfs: remove the unused SYNCHRONIZE macro xiakaixu1987
2020-09-16 11:19 ` [PATCH] xfs: use the existing type definition for di_projid xiakaixu1987
@ 2020-09-16 11:19 ` xiakaixu1987
2020-09-16 21:30 ` Darrick J. Wong
2020-09-17 8:07 ` Christoph Hellwig
2020-09-16 11:19 ` [PATCH] xfs: do the assert for all the log done items in xfs_trans_cancel xiakaixu1987
` (5 subsequent siblings)
8 siblings, 2 replies; 26+ messages in thread
From: xiakaixu1987 @ 2020-09-16 11:19 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
Since the type prid_t and xfs_dqid_t both are uint32_t, seems the
type cast is unnecessary, so remove it.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
fs/xfs/xfs_qm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 3f82e0c92c2d..41a459ffd1f2 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1715,7 +1715,7 @@ xfs_qm_vop_dqalloc(
if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) {
if (ip->i_d.di_projid != prid) {
xfs_iunlock(ip, lockflags);
- error = xfs_qm_dqget(mp, (xfs_dqid_t)prid,
+ error = xfs_qm_dqget(mp, prid,
XFS_DQTYPE_PROJ, true, &pq);
if (error) {
ASSERT(error != -ENOENT);
--
2.20.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH] xfs: remove the unnecessary xfs_dqid_t type cast
2020-09-16 11:19 ` [PATCH] xfs: remove the unnecessary xfs_dqid_t type cast xiakaixu1987
@ 2020-09-16 21:30 ` Darrick J. Wong
2020-09-17 8:07 ` Christoph Hellwig
1 sibling, 0 replies; 26+ messages in thread
From: Darrick J. Wong @ 2020-09-16 21:30 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
n Wed, Sep 16, 2020 at 07:19:06PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> Since the type prid_t and xfs_dqid_t both are uint32_t, seems the
> type cast is unnecessary, so remove it.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_qm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 3f82e0c92c2d..41a459ffd1f2 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1715,7 +1715,7 @@ xfs_qm_vop_dqalloc(
> if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) {
> if (ip->i_d.di_projid != prid) {
> xfs_iunlock(ip, lockflags);
> - error = xfs_qm_dqget(mp, (xfs_dqid_t)prid,
> + error = xfs_qm_dqget(mp, prid,
> XFS_DQTYPE_PROJ, true, &pq);
> if (error) {
> ASSERT(error != -ENOENT);
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xfs: remove the unnecessary xfs_dqid_t type cast
2020-09-16 11:19 ` [PATCH] xfs: remove the unnecessary xfs_dqid_t type cast xiakaixu1987
2020-09-16 21:30 ` Darrick J. Wong
@ 2020-09-17 8:07 ` Christoph Hellwig
1 sibling, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2020-09-17 8:07 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, darrick.wong, Kaixu Xia
On Wed, Sep 16, 2020 at 07:19:06PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> Since the type prid_t and xfs_dqid_t both are uint32_t, seems the
> type cast is unnecessary, so remove it.
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] xfs: do the assert for all the log done items in xfs_trans_cancel
2020-09-16 11:19 [RFC PATCH 0/9]xfs: random fixes and code cleanup xiakaixu1987
` (2 preceding siblings ...)
2020-09-16 11:19 ` [PATCH] xfs: remove the unnecessary xfs_dqid_t type cast xiakaixu1987
@ 2020-09-16 11:19 ` xiakaixu1987
2020-09-17 0:10 ` Darrick J. Wong
2020-09-16 11:19 ` [PATCH] xfs: add the XFS_ITEM_LOG_INTENT flag to mark the log intent item xiakaixu1987
` (4 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: xiakaixu1987 @ 2020-09-16 11:19 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
We should do the assert for all the log done items if they appear
here. This patch also add the XFS_ITEM_LOG_DONE flag to check if
the item is a log done item.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
fs/xfs/xfs_bmap_item.c | 2 +-
fs/xfs/xfs_extfree_item.c | 2 +-
fs/xfs/xfs_refcount_item.c | 2 +-
fs/xfs/xfs_rmap_item.c | 2 +-
fs/xfs/xfs_trans.c | 2 +-
fs/xfs/xfs_trans.h | 4 ++++
6 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index ec3691372e7c..2e49f48666f1 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -202,7 +202,7 @@ xfs_bud_item_release(
}
static const struct xfs_item_ops xfs_bud_item_ops = {
- .flags = XFS_ITEM_RELEASE_WHEN_COMMITTED,
+ .flags = XFS_ITEM_LOG_DONE_FLAG,
.iop_size = xfs_bud_item_size,
.iop_format = xfs_bud_item_format,
.iop_release = xfs_bud_item_release,
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 6cb8cd11072a..f2c6cb67262e 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -307,7 +307,7 @@ xfs_efd_item_release(
}
static const struct xfs_item_ops xfs_efd_item_ops = {
- .flags = XFS_ITEM_RELEASE_WHEN_COMMITTED,
+ .flags = XFS_ITEM_LOG_DONE_FLAG,
.iop_size = xfs_efd_item_size,
.iop_format = xfs_efd_item_format,
.iop_release = xfs_efd_item_release,
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index ca93b6488377..551bcc93acdd 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -208,7 +208,7 @@ xfs_cud_item_release(
}
static const struct xfs_item_ops xfs_cud_item_ops = {
- .flags = XFS_ITEM_RELEASE_WHEN_COMMITTED,
+ .flags = XFS_ITEM_LOG_DONE_FLAG,
.iop_size = xfs_cud_item_size,
.iop_format = xfs_cud_item_format,
.iop_release = xfs_cud_item_release,
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index dc5b0753cd51..427f90ef4509 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -231,7 +231,7 @@ xfs_rud_item_release(
}
static const struct xfs_item_ops xfs_rud_item_ops = {
- .flags = XFS_ITEM_RELEASE_WHEN_COMMITTED,
+ .flags = XFS_ITEM_LOG_DONE_FLAG,
.iop_size = xfs_rud_item_size,
.iop_format = xfs_rud_item_format,
.iop_release = xfs_rud_item_release,
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 4257fdb03778..d33d0ba6f3bd 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -1050,7 +1050,7 @@ xfs_trans_cancel(
struct xfs_log_item *lip;
list_for_each_entry(lip, &tp->t_items, li_trans)
- ASSERT(!(lip->li_type == XFS_LI_EFD));
+ ASSERT(!(lip->li_ops->flags & XFS_ITEM_LOG_DONE));
}
#endif
xfs_trans_unreserve_and_mod_sb(tp);
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 7fb82eb92e65..b92138b13c40 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -85,6 +85,10 @@ struct xfs_item_ops {
* intents that never need to be written back in place.
*/
#define XFS_ITEM_RELEASE_WHEN_COMMITTED (1 << 0)
+#define XFS_ITEM_LOG_DONE (1 << 1) /* log done item */
+
+#define XFS_ITEM_LOG_DONE_FLAG (XFS_ITEM_RELEASE_WHEN_COMMITTED | \
+ XFS_ITEM_LOG_DONE)
void xfs_log_item_init(struct xfs_mount *mp, struct xfs_log_item *item,
int type, const struct xfs_item_ops *ops);
--
2.20.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH] xfs: do the assert for all the log done items in xfs_trans_cancel
2020-09-16 11:19 ` [PATCH] xfs: do the assert for all the log done items in xfs_trans_cancel xiakaixu1987
@ 2020-09-17 0:10 ` Darrick J. Wong
2020-09-17 8:06 ` kaixuxia
2020-09-17 8:08 ` Christoph Hellwig
0 siblings, 2 replies; 26+ messages in thread
From: Darrick J. Wong @ 2020-09-17 0:10 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
On Wed, Sep 16, 2020 at 07:19:07PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> We should do the assert for all the log done items if they appear
> here. This patch also add the XFS_ITEM_LOG_DONE flag to check if
> the item is a log done item.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> ---
> fs/xfs/xfs_bmap_item.c | 2 +-
> fs/xfs/xfs_extfree_item.c | 2 +-
> fs/xfs/xfs_refcount_item.c | 2 +-
> fs/xfs/xfs_rmap_item.c | 2 +-
> fs/xfs/xfs_trans.c | 2 +-
> fs/xfs/xfs_trans.h | 4 ++++
> 6 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
> index ec3691372e7c..2e49f48666f1 100644
> --- a/fs/xfs/xfs_bmap_item.c
> +++ b/fs/xfs/xfs_bmap_item.c
> @@ -202,7 +202,7 @@ xfs_bud_item_release(
> }
>
> static const struct xfs_item_ops xfs_bud_item_ops = {
> - .flags = XFS_ITEM_RELEASE_WHEN_COMMITTED,
> + .flags = XFS_ITEM_LOG_DONE_FLAG,
> .iop_size = xfs_bud_item_size,
> .iop_format = xfs_bud_item_format,
> .iop_release = xfs_bud_item_release,
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 6cb8cd11072a..f2c6cb67262e 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -307,7 +307,7 @@ xfs_efd_item_release(
> }
>
> static const struct xfs_item_ops xfs_efd_item_ops = {
> - .flags = XFS_ITEM_RELEASE_WHEN_COMMITTED,
> + .flags = XFS_ITEM_LOG_DONE_FLAG,
> .iop_size = xfs_efd_item_size,
> .iop_format = xfs_efd_item_format,
> .iop_release = xfs_efd_item_release,
> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
> index ca93b6488377..551bcc93acdd 100644
> --- a/fs/xfs/xfs_refcount_item.c
> +++ b/fs/xfs/xfs_refcount_item.c
> @@ -208,7 +208,7 @@ xfs_cud_item_release(
> }
>
> static const struct xfs_item_ops xfs_cud_item_ops = {
> - .flags = XFS_ITEM_RELEASE_WHEN_COMMITTED,
> + .flags = XFS_ITEM_LOG_DONE_FLAG,
> .iop_size = xfs_cud_item_size,
> .iop_format = xfs_cud_item_format,
> .iop_release = xfs_cud_item_release,
> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
> index dc5b0753cd51..427f90ef4509 100644
> --- a/fs/xfs/xfs_rmap_item.c
> +++ b/fs/xfs/xfs_rmap_item.c
> @@ -231,7 +231,7 @@ xfs_rud_item_release(
> }
>
> static const struct xfs_item_ops xfs_rud_item_ops = {
> - .flags = XFS_ITEM_RELEASE_WHEN_COMMITTED,
> + .flags = XFS_ITEM_LOG_DONE_FLAG,
> .iop_size = xfs_rud_item_size,
> .iop_format = xfs_rud_item_format,
> .iop_release = xfs_rud_item_release,
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 4257fdb03778..d33d0ba6f3bd 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -1050,7 +1050,7 @@ xfs_trans_cancel(
> struct xfs_log_item *lip;
>
> list_for_each_entry(lip, &tp->t_items, li_trans)
> - ASSERT(!(lip->li_type == XFS_LI_EFD));
> + ASSERT(!(lip->li_ops->flags & XFS_ITEM_LOG_DONE));
> }
> #endif
> xfs_trans_unreserve_and_mod_sb(tp);
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 7fb82eb92e65..b92138b13c40 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -85,6 +85,10 @@ struct xfs_item_ops {
> * intents that never need to be written back in place.
> */
> #define XFS_ITEM_RELEASE_WHEN_COMMITTED (1 << 0)
> +#define XFS_ITEM_LOG_DONE (1 << 1) /* log done item */
> +
> +#define XFS_ITEM_LOG_DONE_FLAG (XFS_ITEM_RELEASE_WHEN_COMMITTED | \
> + XFS_ITEM_LOG_DONE)
Please don't go adding more flags for a debugging check.
You /could/ just detect intent-done items by the fact that their item
ops don't have unpin or push methods, kind of like what we do for
detecting intent items in log recovery.
Oh wait, you mowed /that/ down too.
--D
>
> void xfs_log_item_init(struct xfs_mount *mp, struct xfs_log_item *item,
> int type, const struct xfs_item_ops *ops);
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xfs: do the assert for all the log done items in xfs_trans_cancel
2020-09-17 0:10 ` Darrick J. Wong
@ 2020-09-17 8:06 ` kaixuxia
2020-09-17 8:08 ` Christoph Hellwig
1 sibling, 0 replies; 26+ messages in thread
From: kaixuxia @ 2020-09-17 8:06 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, Kaixu Xia
On 2020/9/17 8:10, Darrick J. Wong wrote:
> On Wed, Sep 16, 2020 at 07:19:07PM +0800, xiakaixu1987@gmail.com wrote:
>> From: Kaixu Xia <kaixuxia@tencent.com>
>>
>> We should do the assert for all the log done items if they appear
>> here. This patch also add the XFS_ITEM_LOG_DONE flag to check if
>> the item is a log done item.
>>
>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
>> ---
>> fs/xfs/xfs_bmap_item.c | 2 +-
>> fs/xfs/xfs_extfree_item.c | 2 +-
>> fs/xfs/xfs_refcount_item.c | 2 +-
>> fs/xfs/xfs_rmap_item.c | 2 +-
>> fs/xfs/xfs_trans.c | 2 +-
>> fs/xfs/xfs_trans.h | 4 ++++
>> 6 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
>> index ec3691372e7c..2e49f48666f1 100644
>> --- a/fs/xfs/xfs_bmap_item.c
>> +++ b/fs/xfs/xfs_bmap_item.c
>> @@ -202,7 +202,7 @@ xfs_bud_item_release(
>> }
>>
>> static const struct xfs_item_ops xfs_bud_item_ops = {
>> - .flags = XFS_ITEM_RELEASE_WHEN_COMMITTED,
>> + .flags = XFS_ITEM_LOG_DONE_FLAG,
>> .iop_size = xfs_bud_item_size,
>> .iop_format = xfs_bud_item_format,
>> .iop_release = xfs_bud_item_release,
>> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
>> index 6cb8cd11072a..f2c6cb67262e 100644
>> --- a/fs/xfs/xfs_extfree_item.c
>> +++ b/fs/xfs/xfs_extfree_item.c
>> @@ -307,7 +307,7 @@ xfs_efd_item_release(
>> }
>>
>> static const struct xfs_item_ops xfs_efd_item_ops = {
>> - .flags = XFS_ITEM_RELEASE_WHEN_COMMITTED,
>> + .flags = XFS_ITEM_LOG_DONE_FLAG,
>> .iop_size = xfs_efd_item_size,
>> .iop_format = xfs_efd_item_format,
>> .iop_release = xfs_efd_item_release,
>> diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
>> index ca93b6488377..551bcc93acdd 100644
>> --- a/fs/xfs/xfs_refcount_item.c
>> +++ b/fs/xfs/xfs_refcount_item.c
>> @@ -208,7 +208,7 @@ xfs_cud_item_release(
>> }
>>
>> static const struct xfs_item_ops xfs_cud_item_ops = {
>> - .flags = XFS_ITEM_RELEASE_WHEN_COMMITTED,
>> + .flags = XFS_ITEM_LOG_DONE_FLAG,
>> .iop_size = xfs_cud_item_size,
>> .iop_format = xfs_cud_item_format,
>> .iop_release = xfs_cud_item_release,
>> diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
>> index dc5b0753cd51..427f90ef4509 100644
>> --- a/fs/xfs/xfs_rmap_item.c
>> +++ b/fs/xfs/xfs_rmap_item.c
>> @@ -231,7 +231,7 @@ xfs_rud_item_release(
>> }
>>
>> static const struct xfs_item_ops xfs_rud_item_ops = {
>> - .flags = XFS_ITEM_RELEASE_WHEN_COMMITTED,
>> + .flags = XFS_ITEM_LOG_DONE_FLAG,
>> .iop_size = xfs_rud_item_size,
>> .iop_format = xfs_rud_item_format,
>> .iop_release = xfs_rud_item_release,
>> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
>> index 4257fdb03778..d33d0ba6f3bd 100644
>> --- a/fs/xfs/xfs_trans.c
>> +++ b/fs/xfs/xfs_trans.c
>> @@ -1050,7 +1050,7 @@ xfs_trans_cancel(
>> struct xfs_log_item *lip;
>>
>> list_for_each_entry(lip, &tp->t_items, li_trans)
>> - ASSERT(!(lip->li_type == XFS_LI_EFD));
>> + ASSERT(!(lip->li_ops->flags & XFS_ITEM_LOG_DONE));
>> }
>> #endif
>> xfs_trans_unreserve_and_mod_sb(tp);
>> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
>> index 7fb82eb92e65..b92138b13c40 100644
>> --- a/fs/xfs/xfs_trans.h
>> +++ b/fs/xfs/xfs_trans.h
>> @@ -85,6 +85,10 @@ struct xfs_item_ops {
>> * intents that never need to be written back in place.
>> */
>> #define XFS_ITEM_RELEASE_WHEN_COMMITTED (1 << 0)
>> +#define XFS_ITEM_LOG_DONE (1 << 1) /* log done item */
>> +
>> +#define XFS_ITEM_LOG_DONE_FLAG (XFS_ITEM_RELEASE_WHEN_COMMITTED | \
>> + XFS_ITEM_LOG_DONE)
>
> Please don't go adding more flags for a debugging check.
>
> You /could/ just detect intent-done items by the fact that their item
> ops don't have unpin or push methods, kind of like what we do for
> detecting intent items in log recovery.
>
> Oh wait, you mowed /that/ down too.
Yes, this patch and the next patch add two flags XFS_ITEM_LOG_INTENT and
XFS_ITEM_LOG_DONE to mark the log item is log intent item or log done item.
By now that we can use the struct xfs_item_ops methods to find the log
intent items and log done items, but I'm not sure if we will use these special
methods in other log items, for example, we use the iop_recover method in a log
item that is not a log intent item for a special purpose... Anyway, it's just
my half-thoughts:)
I will use the special methods to detect the log done items in the next version .
Thanks,
Kaixu
>
> --D
>
>>
>> void xfs_log_item_init(struct xfs_mount *mp, struct xfs_log_item *item,
>> int type, const struct xfs_item_ops *ops);
>> --
>> 2.20.0
>>
--
kaixuxia
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xfs: do the assert for all the log done items in xfs_trans_cancel
2020-09-17 0:10 ` Darrick J. Wong
2020-09-17 8:06 ` kaixuxia
@ 2020-09-17 8:08 ` Christoph Hellwig
1 sibling, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2020-09-17 8:08 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: xiakaixu1987, linux-xfs, Kaixu Xia
On Wed, Sep 16, 2020 at 05:10:42PM -0700, Darrick J. Wong wrote:
> Please don't go adding more flags for a debugging check.
>
> You /could/ just detect intent-done items by the fact that their item
> ops don't have unpin or push methods, kind of like what we do for
> detecting intent items in log recovery.
>
> Oh wait, you mowed /that/ down too.
Yes, I'd much rather change based on method presence instead of creating
redundant flags.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] xfs: add the XFS_ITEM_LOG_INTENT flag to mark the log intent item
2020-09-16 11:19 [RFC PATCH 0/9]xfs: random fixes and code cleanup xiakaixu1987
` (3 preceding siblings ...)
2020-09-16 11:19 ` [PATCH] xfs: do the assert for all the log done items in xfs_trans_cancel xiakaixu1987
@ 2020-09-16 11:19 ` xiakaixu1987
2020-09-16 11:19 ` [PATCH] xfs: remove the unnecessary variable error in xfs_trans_unreserve_and_mod_sb xiakaixu1987
` (3 subsequent siblings)
8 siblings, 0 replies; 26+ messages in thread
From: xiakaixu1987 @ 2020-09-16 11:19 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
This patch add the XFS_ITEM_LOG_INTENT flag to mark the log intent
item, so maybe we can simplify the judgement of log intent item.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
fs/xfs/xfs_bmap_item.c | 1 +
fs/xfs/xfs_extfree_item.c | 1 +
fs/xfs/xfs_log_recover.c | 15 ++++-----------
fs/xfs/xfs_refcount_item.c | 1 +
fs/xfs/xfs_rmap_item.c | 1 +
fs/xfs/xfs_trans.h | 3 ++-
6 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 2e49f48666f1..86485dcc9813 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -560,6 +560,7 @@ xfs_bui_item_match(
}
static const struct xfs_item_ops xfs_bui_item_ops = {
+ .flags = XFS_ITEM_LOG_INTENT,
.iop_size = xfs_bui_item_size,
.iop_format = xfs_bui_item_format,
.iop_unpin = xfs_bui_item_unpin,
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index f2c6cb67262e..44242b2ac8c7 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -650,6 +650,7 @@ xfs_efi_item_match(
}
static const struct xfs_item_ops xfs_efi_item_ops = {
+ .flags = XFS_ITEM_LOG_INTENT,
.iop_size = xfs_efi_item_size,
.iop_format = xfs_efi_item_format,
.iop_unpin = xfs_efi_item_unpin,
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 1ca35b53d3b9..32eeed4f8661 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2475,13 +2475,6 @@ xlog_finish_defer_ops(
return xfs_trans_commit(tp);
}
-/* Is this log item a deferred action intent? */
-static inline bool xlog_item_is_intent(struct xfs_log_item *lip)
-{
- return lip->li_ops->iop_recover != NULL &&
- lip->li_ops->iop_match != NULL;
-}
-
/*
* When this is called, all of the log intent items which did not have
* corresponding log done items should be in the AIL. What we do now
@@ -2535,10 +2528,10 @@ xlog_recover_process_intents(
* We're done when we see something other than an intent.
* There should be no intents left in the AIL now.
*/
- if (!xlog_item_is_intent(lip)) {
+ if (!(lip->li_ops->flags & XFS_ITEM_LOG_INTENT)) {
#ifdef DEBUG
for (; lip; lip = xfs_trans_ail_cursor_next(ailp, &cur))
- ASSERT(!xlog_item_is_intent(lip));
+ ASSERT(!(lip->li_ops->flags & XFS_ITEM_LOG_INTENT));
#endif
break;
}
@@ -2595,10 +2588,10 @@ xlog_recover_cancel_intents(
* We're done when we see something other than an intent.
* There should be no intents left in the AIL now.
*/
- if (!xlog_item_is_intent(lip)) {
+ if (!(lip->li_ops->flags & XFS_ITEM_LOG_INTENT)) {
#ifdef DEBUG
for (; lip; lip = xfs_trans_ail_cursor_next(ailp, &cur))
- ASSERT(!xlog_item_is_intent(lip));
+ ASSERT(!(lip->li_ops->flags & XFS_ITEM_LOG_INTENT));
#endif
break;
}
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 551bcc93acdd..a4d470af5f7b 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -575,6 +575,7 @@ xfs_cui_item_match(
}
static const struct xfs_item_ops xfs_cui_item_ops = {
+ .flags = XFS_ITEM_LOG_INTENT,
.iop_size = xfs_cui_item_size,
.iop_format = xfs_cui_item_format,
.iop_unpin = xfs_cui_item_unpin,
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index 427f90ef4509..335c76307383 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -591,6 +591,7 @@ xfs_rui_item_match(
}
static const struct xfs_item_ops xfs_rui_item_ops = {
+ .flags = XFS_ITEM_LOG_INTENT,
.iop_size = xfs_rui_item_size,
.iop_format = xfs_rui_item_format,
.iop_unpin = xfs_rui_item_unpin,
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index b92138b13c40..53cb5c5eda02 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -85,7 +85,8 @@ struct xfs_item_ops {
* intents that never need to be written back in place.
*/
#define XFS_ITEM_RELEASE_WHEN_COMMITTED (1 << 0)
-#define XFS_ITEM_LOG_DONE (1 << 1) /* log done item */
+#define XFS_ITEM_LOG_INTENT (1 << 1) /* log intent item */
+#define XFS_ITEM_LOG_DONE (1 << 2) /* log done item */
#define XFS_ITEM_LOG_DONE_FLAG (XFS_ITEM_RELEASE_WHEN_COMMITTED | \
XFS_ITEM_LOG_DONE)
--
2.20.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH] xfs: remove the unnecessary variable error in xfs_trans_unreserve_and_mod_sb
2020-09-16 11:19 [RFC PATCH 0/9]xfs: random fixes and code cleanup xiakaixu1987
` (4 preceding siblings ...)
2020-09-16 11:19 ` [PATCH] xfs: add the XFS_ITEM_LOG_INTENT flag to mark the log intent item xiakaixu1987
@ 2020-09-16 11:19 ` xiakaixu1987
2020-09-16 16:45 ` Darrick J. Wong
2020-09-16 11:19 ` [PATCH] xfs: remove the repeated crc verification in xfs_attr3_rmt_verify xiakaixu1987
` (2 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: xiakaixu1987 @ 2020-09-16 11:19 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
We can do the assert directly for the return value of xfs_mod_fdblocks()
function, and the variable error is unnecessary, so remove it.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
fs/xfs/xfs_trans.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index d33d0ba6f3bd..caa207220e2c 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -573,7 +573,6 @@ xfs_trans_unreserve_and_mod_sb(
int64_t rtxdelta = 0;
int64_t idelta = 0;
int64_t ifreedelta = 0;
- int error;
/* calculate deltas */
if (tp->t_blk_res > 0)
@@ -596,10 +595,8 @@ xfs_trans_unreserve_and_mod_sb(
}
/* apply the per-cpu counters */
- if (blkdelta) {
- error = xfs_mod_fdblocks(mp, blkdelta, rsvd);
- ASSERT(!error);
- }
+ if (blkdelta)
+ ASSERT(!xfs_mod_fdblocks(mp, blkdelta, rsvd));
if (idelta) {
percpu_counter_add_batch(&mp->m_icount, idelta,
--
2.20.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH] xfs: remove the unnecessary variable error in xfs_trans_unreserve_and_mod_sb
2020-09-16 11:19 ` [PATCH] xfs: remove the unnecessary variable error in xfs_trans_unreserve_and_mod_sb xiakaixu1987
@ 2020-09-16 16:45 ` Darrick J. Wong
2020-09-17 6:47 ` kaixuxia
0 siblings, 1 reply; 26+ messages in thread
From: Darrick J. Wong @ 2020-09-16 16:45 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
On Wed, Sep 16, 2020 at 07:19:09PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> We can do the assert directly for the return value of xfs_mod_fdblocks()
> function, and the variable error is unnecessary, so remove it.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> ---
> fs/xfs/xfs_trans.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index d33d0ba6f3bd..caa207220e2c 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -573,7 +573,6 @@ xfs_trans_unreserve_and_mod_sb(
> int64_t rtxdelta = 0;
> int64_t idelta = 0;
> int64_t ifreedelta = 0;
> - int error;
>
> /* calculate deltas */
> if (tp->t_blk_res > 0)
> @@ -596,10 +595,8 @@ xfs_trans_unreserve_and_mod_sb(
> }
>
> /* apply the per-cpu counters */
> - if (blkdelta) {
> - error = xfs_mod_fdblocks(mp, blkdelta, rsvd);
> - ASSERT(!error);
> - }
> + if (blkdelta)
> + ASSERT(!xfs_mod_fdblocks(mp, blkdelta, rsvd));
Um.... did you test this with ASSERTs disabled? Because this compiles
the free block counter update out of the function on non-debug kernels,
which (AFAICT) will cause fs corruption...
--D
>
> if (idelta) {
> percpu_counter_add_batch(&mp->m_icount, idelta,
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xfs: remove the unnecessary variable error in xfs_trans_unreserve_and_mod_sb
2020-09-16 16:45 ` Darrick J. Wong
@ 2020-09-17 6:47 ` kaixuxia
0 siblings, 0 replies; 26+ messages in thread
From: kaixuxia @ 2020-09-17 6:47 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, Kaixu Xia
Hi Darrick,
On 2020/9/17 0:45, Darrick J. Wong wrote:
> On Wed, Sep 16, 2020 at 07:19:09PM +0800, xiakaixu1987@gmail.com wrote:
>> From: Kaixu Xia <kaixuxia@tencent.com>
>>
>> We can do the assert directly for the return value of xfs_mod_fdblocks()
>> function, and the variable error is unnecessary, so remove it.
>>
>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
>> ---
>> fs/xfs/xfs_trans.c | 7 ++-----
>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
>> index d33d0ba6f3bd..caa207220e2c 100644
>> --- a/fs/xfs/xfs_trans.c
>> +++ b/fs/xfs/xfs_trans.c
>> @@ -573,7 +573,6 @@ xfs_trans_unreserve_and_mod_sb(
>> int64_t rtxdelta = 0;
>> int64_t idelta = 0;
>> int64_t ifreedelta = 0;
>> - int error;
>>
>> /* calculate deltas */
>> if (tp->t_blk_res > 0)
>> @@ -596,10 +595,8 @@ xfs_trans_unreserve_and_mod_sb(
>> }
>>
>> /* apply the per-cpu counters */
>> - if (blkdelta) {
>> - error = xfs_mod_fdblocks(mp, blkdelta, rsvd);
>> - ASSERT(!error);
>> - }
>> + if (blkdelta)
>> + ASSERT(!xfs_mod_fdblocks(mp, blkdelta, rsvd));
>
> Um.... did you test this with ASSERTs disabled? Because this compiles
> the free block counter update out of the function on non-debug kernels,
> which (AFAICT) will cause fs corruption...
Yes,thanks for pointing out... My fault...
>
> --D
>
>>
>> if (idelta) {
>> percpu_counter_add_batch(&mp->m_icount, idelta,
>> --
>> 2.20.0
>>
--
kaixuxia
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] xfs: remove the repeated crc verification in xfs_attr3_rmt_verify
2020-09-16 11:19 [RFC PATCH 0/9]xfs: random fixes and code cleanup xiakaixu1987
` (5 preceding siblings ...)
2020-09-16 11:19 ` [PATCH] xfs: remove the unnecessary variable error in xfs_trans_unreserve_and_mod_sb xiakaixu1987
@ 2020-09-16 11:19 ` xiakaixu1987
2020-09-16 18:45 ` Darrick J. Wong
2020-09-16 11:19 ` [PATCH] xfs: cleanup the useless backslash in xfs_attr_leaf_entsize_remote xiakaixu1987
2020-09-16 11:19 ` [PATCH] xfs: fix some comments xiakaixu1987
8 siblings, 1 reply; 26+ messages in thread
From: xiakaixu1987 @ 2020-09-16 11:19 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
We already do the crc verification before calling the xfs_attr3_rmt_verify()
function, and just return directly for non-crc buffers, so don't need
to do the repeated crc verification in xfs_attr3_rmt_verify().
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
fs/xfs/libxfs/xfs_attr_remote.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 3f80cede7406..48d8e9caf86f 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -96,8 +96,6 @@ xfs_attr3_rmt_verify(
{
struct xfs_attr3_rmt_hdr *rmt = ptr;
- if (!xfs_sb_version_hascrc(&mp->m_sb))
- return __this_address;
if (!xfs_verify_magic(bp, rmt->rm_magic))
return __this_address;
if (!uuid_equal(&rmt->rm_uuid, &mp->m_sb.sb_meta_uuid))
--
2.20.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH] xfs: remove the repeated crc verification in xfs_attr3_rmt_verify
2020-09-16 11:19 ` [PATCH] xfs: remove the repeated crc verification in xfs_attr3_rmt_verify xiakaixu1987
@ 2020-09-16 18:45 ` Darrick J. Wong
2020-09-17 7:03 ` kaixuxia
0 siblings, 1 reply; 26+ messages in thread
From: Darrick J. Wong @ 2020-09-16 18:45 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
On Wed, Sep 16, 2020 at 07:19:10PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> We already do the crc verification before calling the xfs_attr3_rmt_verify()
Nit: The function call you're removing does not itself do crc
verification; it merely checks that the *crc feature* is set. This
commit message needs to make this distinction, because at first I
thought "Why would you remove crc verification?"
IOWs...
"We already check that the crc feature is enabled before calling
xfs_attr3_rmt_verify(), so remove the redundant feature check in that
function."
--D
> function, and just return directly for non-crc buffers, so don't need
> to do the repeated crc verification in xfs_attr3_rmt_verify().
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> ---
> fs/xfs/libxfs/xfs_attr_remote.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> index 3f80cede7406..48d8e9caf86f 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> @@ -96,8 +96,6 @@ xfs_attr3_rmt_verify(
> {
> struct xfs_attr3_rmt_hdr *rmt = ptr;
>
> - if (!xfs_sb_version_hascrc(&mp->m_sb))
> - return __this_address;
> if (!xfs_verify_magic(bp, rmt->rm_magic))
> return __this_address;
> if (!uuid_equal(&rmt->rm_uuid, &mp->m_sb.sb_meta_uuid))
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xfs: remove the repeated crc verification in xfs_attr3_rmt_verify
2020-09-16 18:45 ` Darrick J. Wong
@ 2020-09-17 7:03 ` kaixuxia
0 siblings, 0 replies; 26+ messages in thread
From: kaixuxia @ 2020-09-17 7:03 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, Kaixu Xia
On 2020/9/17 2:45, Darrick J. Wong wrote:
> On Wed, Sep 16, 2020 at 07:19:10PM +0800, xiakaixu1987@gmail.com wrote:
>> From: Kaixu Xia <kaixuxia@tencent.com>
>>
>> We already do the crc verification before calling the xfs_attr3_rmt_verify()
>
> Nit: The function call you're removing does not itself do crc
> verification; it merely checks that the *crc feature* is set. This
> commit message needs to make this distinction, because at first I
> thought "Why would you remove crc verification?"
>
> IOWs...
>
> "We already check that the crc feature is enabled before calling
> xfs_attr3_rmt_verify(), so remove the redundant feature check in that
> function."
Yes, right! The original commit message is confusing, will update it
in the next version.
Thanks,
Kaixu
>
> --D
>
>> function, and just return directly for non-crc buffers, so don't need
>> to do the repeated crc verification in xfs_attr3_rmt_verify().
>>
>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
>> ---
>> fs/xfs/libxfs/xfs_attr_remote.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
>> index 3f80cede7406..48d8e9caf86f 100644
>> --- a/fs/xfs/libxfs/xfs_attr_remote.c
>> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
>> @@ -96,8 +96,6 @@ xfs_attr3_rmt_verify(
>> {
>> struct xfs_attr3_rmt_hdr *rmt = ptr;
>>
>> - if (!xfs_sb_version_hascrc(&mp->m_sb))
>> - return __this_address;
>> if (!xfs_verify_magic(bp, rmt->rm_magic))
>> return __this_address;
>> if (!uuid_equal(&rmt->rm_uuid, &mp->m_sb.sb_meta_uuid))
>> --
>> 2.20.0
>>
--
kaixuxia
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] xfs: cleanup the useless backslash in xfs_attr_leaf_entsize_remote
2020-09-16 11:19 [RFC PATCH 0/9]xfs: random fixes and code cleanup xiakaixu1987
` (6 preceding siblings ...)
2020-09-16 11:19 ` [PATCH] xfs: remove the repeated crc verification in xfs_attr3_rmt_verify xiakaixu1987
@ 2020-09-16 11:19 ` xiakaixu1987
2020-09-16 18:50 ` Darrick J. Wong
2020-09-16 11:19 ` [PATCH] xfs: fix some comments xiakaixu1987
8 siblings, 1 reply; 26+ messages in thread
From: xiakaixu1987 @ 2020-09-16 11:19 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
The backslash is useless and remove it.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
fs/xfs/libxfs/xfs_da_format.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
index b40a4e80f5ee..4fe974773d85 100644
--- a/fs/xfs/libxfs/xfs_da_format.h
+++ b/fs/xfs/libxfs/xfs_da_format.h
@@ -746,7 +746,7 @@ xfs_attr3_leaf_name_local(xfs_attr_leafblock_t *leafp, int idx)
*/
static inline int xfs_attr_leaf_entsize_remote(int nlen)
{
- return ((uint)sizeof(xfs_attr_leaf_name_remote_t) - 1 + (nlen) + \
+ return ((uint)sizeof(xfs_attr_leaf_name_remote_t) - 1 + (nlen) +
XFS_ATTR_LEAF_NAME_ALIGN - 1) & ~(XFS_ATTR_LEAF_NAME_ALIGN - 1);
}
--
2.20.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH] xfs: cleanup the useless backslash in xfs_attr_leaf_entsize_remote
2020-09-16 11:19 ` [PATCH] xfs: cleanup the useless backslash in xfs_attr_leaf_entsize_remote xiakaixu1987
@ 2020-09-16 18:50 ` Darrick J. Wong
2020-09-17 7:17 ` kaixuxia
0 siblings, 1 reply; 26+ messages in thread
From: Darrick J. Wong @ 2020-09-16 18:50 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
On Wed, Sep 16, 2020 at 07:19:11PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> The backslash is useless and remove it.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> ---
> fs/xfs/libxfs/xfs_da_format.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> index b40a4e80f5ee..4fe974773d85 100644
> --- a/fs/xfs/libxfs/xfs_da_format.h
> +++ b/fs/xfs/libxfs/xfs_da_format.h
> @@ -746,7 +746,7 @@ xfs_attr3_leaf_name_local(xfs_attr_leafblock_t *leafp, int idx)
> */
> static inline int xfs_attr_leaf_entsize_remote(int nlen)
> {
> - return ((uint)sizeof(xfs_attr_leaf_name_remote_t) - 1 + (nlen) + \
> + return ((uint)sizeof(xfs_attr_leaf_name_remote_t) - 1 + (nlen) +
> XFS_ATTR_LEAF_NAME_ALIGN - 1) & ~(XFS_ATTR_LEAF_NAME_ALIGN - 1);
If you're going to go mess with these, you might as well clean up the
typedef usage, the unnecessary parentheses, and the open-coded round_up
call:
static inline int xfs_attr_leaf_entsize_remote(int nlen)
{
return round_up(sizeof(struct xfs_attr_leaf_name_remote) - 1 + nlen,
XFS_ATTR_LEAF_NAME_ALIGN);
}
(and similar for xfs_attr_leaf_entsize_local().)
--D
> }
>
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xfs: cleanup the useless backslash in xfs_attr_leaf_entsize_remote
2020-09-16 18:50 ` Darrick J. Wong
@ 2020-09-17 7:17 ` kaixuxia
0 siblings, 0 replies; 26+ messages in thread
From: kaixuxia @ 2020-09-17 7:17 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, Kaixu Xia
On 2020/9/17 2:50, Darrick J. Wong wrote:
> On Wed, Sep 16, 2020 at 07:19:11PM +0800, xiakaixu1987@gmail.com wrote:
>> From: Kaixu Xia <kaixuxia@tencent.com>
>>
>> The backslash is useless and remove it.
>>
>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
>> ---
>> fs/xfs/libxfs/xfs_da_format.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
>> index b40a4e80f5ee..4fe974773d85 100644
>> --- a/fs/xfs/libxfs/xfs_da_format.h
>> +++ b/fs/xfs/libxfs/xfs_da_format.h
>> @@ -746,7 +746,7 @@ xfs_attr3_leaf_name_local(xfs_attr_leafblock_t *leafp, int idx)
>> */
>> static inline int xfs_attr_leaf_entsize_remote(int nlen)
>> {
>> - return ((uint)sizeof(xfs_attr_leaf_name_remote_t) - 1 + (nlen) + \
>> + return ((uint)sizeof(xfs_attr_leaf_name_remote_t) - 1 + (nlen) +
>> XFS_ATTR_LEAF_NAME_ALIGN - 1) & ~(XFS_ATTR_LEAF_NAME_ALIGN - 1);
>
> If you're going to go mess with these, you might as well clean up the
> typedef usage, the unnecessary parentheses, and the open-coded round_up
> call:
>
> static inline int xfs_attr_leaf_entsize_remote(int nlen)
> {
> return round_up(sizeof(struct xfs_attr_leaf_name_remote) - 1 + nlen,
> XFS_ATTR_LEAF_NAME_ALIGN);
> }
>
> (and similar for xfs_attr_leaf_entsize_local().)
>
Okay, will do these code cleanups in the next version:)
Thanks,
Kaixu
> --D
>
>> }
>>
>> --
>> 2.20.0
>>
--
kaixuxia
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] xfs: fix some comments
2020-09-16 11:19 [RFC PATCH 0/9]xfs: random fixes and code cleanup xiakaixu1987
` (7 preceding siblings ...)
2020-09-16 11:19 ` [PATCH] xfs: cleanup the useless backslash in xfs_attr_leaf_entsize_remote xiakaixu1987
@ 2020-09-16 11:19 ` xiakaixu1987
2020-09-16 21:31 ` Darrick J. Wong
8 siblings, 1 reply; 26+ messages in thread
From: xiakaixu1987 @ 2020-09-16 11:19 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Kaixu Xia
From: Kaixu Xia <kaixuxia@tencent.com>
Fix the comments to help people understand the code.
Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
fs/xfs/libxfs/xfs_da_format.h | 4 ++--
fs/xfs/xfs_dquot.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
index 4fe974773d85..b58943f2eb41 100644
--- a/fs/xfs/libxfs/xfs_da_format.h
+++ b/fs/xfs/libxfs/xfs_da_format.h
@@ -35,8 +35,8 @@ typedef struct xfs_da_blkinfo {
*/
#define XFS_DA3_NODE_MAGIC 0x3ebe /* magic number: non-leaf blocks */
#define XFS_ATTR3_LEAF_MAGIC 0x3bee /* magic number: attribute leaf blks */
-#define XFS_DIR3_LEAF1_MAGIC 0x3df1 /* magic number: v2 dirlf single blks */
-#define XFS_DIR3_LEAFN_MAGIC 0x3dff /* magic number: v2 dirlf multi blks */
+#define XFS_DIR3_LEAF1_MAGIC 0x3df1 /* magic number: v3 dirlf single blks */
+#define XFS_DIR3_LEAFN_MAGIC 0x3dff /* magic number: v3 dirlf multi blks */
struct xfs_da3_blkinfo {
/*
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 3072814e407d..1d95ed387d66 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -831,8 +831,8 @@ xfs_qm_dqget_checks(
}
/*
- * Given the file system, id, and type (UDQUOT/GDQUOT), return a locked
- * dquot, doing an allocation (if requested) as needed.
+ * Given the file system, id, and type (UDQUOT/GDQUOT/PDQUOT), return a
+ * locked dquot, doing an allocation (if requested) as needed.
*/
int
xfs_qm_dqget(
--
2.20.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH] xfs: fix some comments
2020-09-16 11:19 ` [PATCH] xfs: fix some comments xiakaixu1987
@ 2020-09-16 21:31 ` Darrick J. Wong
0 siblings, 0 replies; 26+ messages in thread
From: Darrick J. Wong @ 2020-09-16 21:31 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, Kaixu Xia
On Wed, Sep 16, 2020 at 07:19:12PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
>
> Fix the comments to help people understand the code.
>
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/libxfs/xfs_da_format.h | 4 ++--
> fs/xfs/xfs_dquot.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> index 4fe974773d85..b58943f2eb41 100644
> --- a/fs/xfs/libxfs/xfs_da_format.h
> +++ b/fs/xfs/libxfs/xfs_da_format.h
> @@ -35,8 +35,8 @@ typedef struct xfs_da_blkinfo {
> */
> #define XFS_DA3_NODE_MAGIC 0x3ebe /* magic number: non-leaf blocks */
> #define XFS_ATTR3_LEAF_MAGIC 0x3bee /* magic number: attribute leaf blks */
> -#define XFS_DIR3_LEAF1_MAGIC 0x3df1 /* magic number: v2 dirlf single blks */
> -#define XFS_DIR3_LEAFN_MAGIC 0x3dff /* magic number: v2 dirlf multi blks */
> +#define XFS_DIR3_LEAF1_MAGIC 0x3df1 /* magic number: v3 dirlf single blks */
> +#define XFS_DIR3_LEAFN_MAGIC 0x3dff /* magic number: v3 dirlf multi blks */
>
> struct xfs_da3_blkinfo {
> /*
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 3072814e407d..1d95ed387d66 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -831,8 +831,8 @@ xfs_qm_dqget_checks(
> }
>
> /*
> - * Given the file system, id, and type (UDQUOT/GDQUOT), return a locked
> - * dquot, doing an allocation (if requested) as needed.
> + * Given the file system, id, and type (UDQUOT/GDQUOT/PDQUOT), return a
> + * locked dquot, doing an allocation (if requested) as needed.
> */
> int
> xfs_qm_dqget(
> --
> 2.20.0
>
^ permalink raw reply [flat|nested] 26+ messages in thread