* [PATCH 1/5] xfs: remove unnecessary parameter from xfs_iext_inc_seq
2019-08-26 21:49 [PATCH 0/5] xfs: remove unnecessary parameters and returns Darrick J. Wong
@ 2019-08-26 21:49 ` Darrick J. Wong
2019-08-26 23:16 ` Dave Chinner
2019-08-29 7:25 ` Christoph Hellwig
2019-08-26 21:49 ` [PATCH 2/5] xfs: remove unnecessary int returns from deferred rmap functions Darrick J. Wong
` (3 subsequent siblings)
4 siblings, 2 replies; 18+ messages in thread
From: Darrick J. Wong @ 2019-08-26 21:49 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
This function doesn't use the @state parameter, so get rid of it.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_iext_tree.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_iext_tree.c b/fs/xfs/libxfs/xfs_iext_tree.c
index 27aa3f2bc4bc..7bc87408f1a0 100644
--- a/fs/xfs/libxfs/xfs_iext_tree.c
+++ b/fs/xfs/libxfs/xfs_iext_tree.c
@@ -616,7 +616,7 @@ xfs_iext_realloc_root(
* sequence counter is seen before the modifications to the extent tree itself
* take effect.
*/
-static inline void xfs_iext_inc_seq(struct xfs_ifork *ifp, int state)
+static inline void xfs_iext_inc_seq(struct xfs_ifork *ifp)
{
WRITE_ONCE(ifp->if_seq, READ_ONCE(ifp->if_seq) + 1);
}
@@ -633,7 +633,7 @@ xfs_iext_insert(
struct xfs_iext_leaf *new = NULL;
int nr_entries, i;
- xfs_iext_inc_seq(ifp, state);
+ xfs_iext_inc_seq(ifp);
if (ifp->if_height == 0)
xfs_iext_alloc_root(ifp, cur);
@@ -875,7 +875,7 @@ xfs_iext_remove(
ASSERT(ifp->if_u1.if_root != NULL);
ASSERT(xfs_iext_valid(ifp, cur));
- xfs_iext_inc_seq(ifp, state);
+ xfs_iext_inc_seq(ifp);
nr_entries = xfs_iext_leaf_nr_entries(ifp, leaf, cur->pos) - 1;
for (i = cur->pos; i < nr_entries; i++)
@@ -983,7 +983,7 @@ xfs_iext_update_extent(
{
struct xfs_ifork *ifp = xfs_iext_state_to_fork(ip, state);
- xfs_iext_inc_seq(ifp, state);
+ xfs_iext_inc_seq(ifp);
if (cur->pos == 0) {
struct xfs_bmbt_irec old;
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] xfs: remove unnecessary parameter from xfs_iext_inc_seq
2019-08-26 21:49 ` [PATCH 1/5] xfs: remove unnecessary parameter from xfs_iext_inc_seq Darrick J. Wong
@ 2019-08-26 23:16 ` Dave Chinner
2019-08-29 7:25 ` Christoph Hellwig
1 sibling, 0 replies; 18+ messages in thread
From: Dave Chinner @ 2019-08-26 23:16 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Mon, Aug 26, 2019 at 02:49:19PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> This function doesn't use the @state parameter, so get rid of it.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Good little cleanup.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] xfs: remove unnecessary parameter from xfs_iext_inc_seq
2019-08-26 21:49 ` [PATCH 1/5] xfs: remove unnecessary parameter from xfs_iext_inc_seq Darrick J. Wong
2019-08-26 23:16 ` Dave Chinner
@ 2019-08-29 7:25 ` Christoph Hellwig
1 sibling, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2019-08-29 7:25 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Mon, Aug 26, 2019 at 02:49:19PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> This function doesn't use the @state parameter, so get rid of it.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/5] xfs: remove unnecessary int returns from deferred rmap functions
2019-08-26 21:49 [PATCH 0/5] xfs: remove unnecessary parameters and returns Darrick J. Wong
2019-08-26 21:49 ` [PATCH 1/5] xfs: remove unnecessary parameter from xfs_iext_inc_seq Darrick J. Wong
@ 2019-08-26 21:49 ` Darrick J. Wong
2019-08-26 23:22 ` Dave Chinner
2019-08-29 7:26 ` Christoph Hellwig
2019-08-26 21:49 ` [PATCH 3/5] xfs: remove unnecessary int returns from deferred refcount functions Darrick J. Wong
` (2 subsequent siblings)
4 siblings, 2 replies; 18+ messages in thread
From: Darrick J. Wong @ 2019-08-26 21:49 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
Remove the return value from the functions that schedule deferred rmap
operations since they never fail and do not return status.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_bmap.c | 36 ++++++++++++------------------------
fs/xfs/libxfs/xfs_refcount.c | 9 +++------
fs/xfs/libxfs/xfs_rmap.c | 33 ++++++++++++++++-----------------
fs/xfs/libxfs/xfs_rmap.h | 10 +++++-----
4 files changed, 36 insertions(+), 52 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 07aad70f3931..9842064de80c 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1985,11 +1985,8 @@ xfs_bmap_add_extent_delay_real(
}
/* add reverse mapping unless caller opted out */
- if (!(bma->flags & XFS_BMAPI_NORMAP)) {
- error = xfs_rmap_map_extent(bma->tp, bma->ip, whichfork, new);
- if (error)
- goto done;
- }
+ if (!(bma->flags & XFS_BMAPI_NORMAP))
+ xfs_rmap_map_extent(bma->tp, bma->ip, whichfork, new);
/* convert to a btree if necessary */
if (xfs_bmap_needs_btree(bma->ip, whichfork)) {
@@ -2471,9 +2468,7 @@ xfs_bmap_add_extent_unwritten_real(
}
/* update reverse mappings */
- error = xfs_rmap_convert_extent(mp, tp, ip, whichfork, new);
- if (error)
- goto done;
+ xfs_rmap_convert_extent(mp, tp, ip, whichfork, new);
/* convert to a btree if necessary */
if (xfs_bmap_needs_btree(ip, whichfork)) {
@@ -2832,11 +2827,8 @@ xfs_bmap_add_extent_hole_real(
}
/* add reverse mapping unless caller opted out */
- if (!(flags & XFS_BMAPI_NORMAP)) {
- error = xfs_rmap_map_extent(tp, ip, whichfork, new);
- if (error)
- goto done;
- }
+ if (!(flags & XFS_BMAPI_NORMAP))
+ xfs_rmap_map_extent(tp, ip, whichfork, new);
/* convert to a btree if necessary */
if (xfs_bmap_needs_btree(ip, whichfork)) {
@@ -5149,9 +5141,7 @@ xfs_bmap_del_extent_real(
}
/* remove reverse mapping */
- error = xfs_rmap_unmap_extent(tp, ip, whichfork, del);
- if (error)
- goto done;
+ xfs_rmap_unmap_extent(tp, ip, whichfork, del);
/*
* If we need to, add to list of extents to delete.
@@ -5651,12 +5641,11 @@ xfs_bmse_merge(
&new);
/* update reverse mapping. rmap functions merge the rmaps for us */
- error = xfs_rmap_unmap_extent(tp, ip, whichfork, got);
- if (error)
- return error;
+ xfs_rmap_unmap_extent(tp, ip, whichfork, got);
memcpy(&new, got, sizeof(new));
new.br_startoff = left->br_startoff + left->br_blockcount;
- return xfs_rmap_map_extent(tp, ip, whichfork, &new);
+ xfs_rmap_map_extent(tp, ip, whichfork, &new);
+ return 0;
}
static int
@@ -5695,10 +5684,9 @@ xfs_bmap_shift_update_extent(
got);
/* update reverse mapping */
- error = xfs_rmap_unmap_extent(tp, ip, whichfork, &prev);
- if (error)
- return error;
- return xfs_rmap_map_extent(tp, ip, whichfork, got);
+ xfs_rmap_unmap_extent(tp, ip, whichfork, &prev);
+ xfs_rmap_map_extent(tp, ip, whichfork, got);
+ return 0;
}
int
diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index 51bb9bdb0e84..f4e17a1f4b27 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -1558,8 +1558,9 @@ xfs_refcount_alloc_cow_extent(
return error;
/* Add rmap entry */
- return xfs_rmap_alloc_extent(tp, XFS_FSB_TO_AGNO(mp, fsb),
+ xfs_rmap_alloc_extent(tp, XFS_FSB_TO_AGNO(mp, fsb),
XFS_FSB_TO_AGBNO(mp, fsb), len, XFS_RMAP_OWN_COW);
+ return 0;
}
/* Forget a CoW staging event in the refcount btree. */
@@ -1570,17 +1571,13 @@ xfs_refcount_free_cow_extent(
xfs_extlen_t len)
{
struct xfs_mount *mp = tp->t_mountp;
- int error;
if (!xfs_sb_version_hasreflink(&mp->m_sb))
return 0;
/* Remove rmap entry */
- error = xfs_rmap_free_extent(tp, XFS_FSB_TO_AGNO(mp, fsb),
+ xfs_rmap_free_extent(tp, XFS_FSB_TO_AGNO(mp, fsb),
XFS_FSB_TO_AGBNO(mp, fsb), len, XFS_RMAP_OWN_COW);
- if (error)
- return error;
-
return __xfs_refcount_add(tp, XFS_REFCOUNT_FREE_COW, fsb, len);
}
diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
index 819693ef852c..56769826a5ca 100644
--- a/fs/xfs/libxfs/xfs_rmap.c
+++ b/fs/xfs/libxfs/xfs_rmap.c
@@ -2268,7 +2268,7 @@ xfs_rmap_update_is_needed(
* Record a rmap intent; the list is kept sorted first by AG and then by
* increasing age.
*/
-static int
+static void
__xfs_rmap_add(
struct xfs_trans *tp,
enum xfs_rmap_intent_type type,
@@ -2295,11 +2295,10 @@ __xfs_rmap_add(
ri->ri_bmap = *bmap;
xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_RMAP, &ri->ri_list);
- return 0;
}
/* Map an extent into a file. */
-int
+void
xfs_rmap_map_extent(
struct xfs_trans *tp,
struct xfs_inode *ip,
@@ -2307,15 +2306,15 @@ xfs_rmap_map_extent(
struct xfs_bmbt_irec *PREV)
{
if (!xfs_rmap_update_is_needed(tp->t_mountp, whichfork))
- return 0;
+ return;
- return __xfs_rmap_add(tp, xfs_is_reflink_inode(ip) ?
+ __xfs_rmap_add(tp, xfs_is_reflink_inode(ip) ?
XFS_RMAP_MAP_SHARED : XFS_RMAP_MAP, ip->i_ino,
whichfork, PREV);
}
/* Unmap an extent out of a file. */
-int
+void
xfs_rmap_unmap_extent(
struct xfs_trans *tp,
struct xfs_inode *ip,
@@ -2323,9 +2322,9 @@ xfs_rmap_unmap_extent(
struct xfs_bmbt_irec *PREV)
{
if (!xfs_rmap_update_is_needed(tp->t_mountp, whichfork))
- return 0;
+ return;
- return __xfs_rmap_add(tp, xfs_is_reflink_inode(ip) ?
+ __xfs_rmap_add(tp, xfs_is_reflink_inode(ip) ?
XFS_RMAP_UNMAP_SHARED : XFS_RMAP_UNMAP, ip->i_ino,
whichfork, PREV);
}
@@ -2336,7 +2335,7 @@ xfs_rmap_unmap_extent(
* Note that tp can be NULL here as no transaction is used for COW fork
* unwritten conversion.
*/
-int
+void
xfs_rmap_convert_extent(
struct xfs_mount *mp,
struct xfs_trans *tp,
@@ -2345,15 +2344,15 @@ xfs_rmap_convert_extent(
struct xfs_bmbt_irec *PREV)
{
if (!xfs_rmap_update_is_needed(mp, whichfork))
- return 0;
+ return;
- return __xfs_rmap_add(tp, xfs_is_reflink_inode(ip) ?
+ __xfs_rmap_add(tp, xfs_is_reflink_inode(ip) ?
XFS_RMAP_CONVERT_SHARED : XFS_RMAP_CONVERT, ip->i_ino,
whichfork, PREV);
}
/* Schedule the creation of an rmap for non-file data. */
-int
+void
xfs_rmap_alloc_extent(
struct xfs_trans *tp,
xfs_agnumber_t agno,
@@ -2364,18 +2363,18 @@ xfs_rmap_alloc_extent(
struct xfs_bmbt_irec bmap;
if (!xfs_rmap_update_is_needed(tp->t_mountp, XFS_DATA_FORK))
- return 0;
+ return;
bmap.br_startblock = XFS_AGB_TO_FSB(tp->t_mountp, agno, bno);
bmap.br_blockcount = len;
bmap.br_startoff = 0;
bmap.br_state = XFS_EXT_NORM;
- return __xfs_rmap_add(tp, XFS_RMAP_ALLOC, owner, XFS_DATA_FORK, &bmap);
+ __xfs_rmap_add(tp, XFS_RMAP_ALLOC, owner, XFS_DATA_FORK, &bmap);
}
/* Schedule the deletion of an rmap for non-file data. */
-int
+void
xfs_rmap_free_extent(
struct xfs_trans *tp,
xfs_agnumber_t agno,
@@ -2386,14 +2385,14 @@ xfs_rmap_free_extent(
struct xfs_bmbt_irec bmap;
if (!xfs_rmap_update_is_needed(tp->t_mountp, XFS_DATA_FORK))
- return 0;
+ return;
bmap.br_startblock = XFS_AGB_TO_FSB(tp->t_mountp, agno, bno);
bmap.br_blockcount = len;
bmap.br_startoff = 0;
bmap.br_state = XFS_EXT_NORM;
- return __xfs_rmap_add(tp, XFS_RMAP_FREE, owner, XFS_DATA_FORK, &bmap);
+ __xfs_rmap_add(tp, XFS_RMAP_FREE, owner, XFS_DATA_FORK, &bmap);
}
/* Compare rmap records. Returns -1 if a < b, 1 if a > b, and 0 if equal. */
diff --git a/fs/xfs/libxfs/xfs_rmap.h b/fs/xfs/libxfs/xfs_rmap.h
index e21ed0294e5c..0c2c3cb73429 100644
--- a/fs/xfs/libxfs/xfs_rmap.h
+++ b/fs/xfs/libxfs/xfs_rmap.h
@@ -161,16 +161,16 @@ struct xfs_rmap_intent {
};
/* functions for updating the rmapbt based on bmbt map/unmap operations */
-int xfs_rmap_map_extent(struct xfs_trans *tp, struct xfs_inode *ip,
+void xfs_rmap_map_extent(struct xfs_trans *tp, struct xfs_inode *ip,
int whichfork, struct xfs_bmbt_irec *imap);
-int xfs_rmap_unmap_extent(struct xfs_trans *tp, struct xfs_inode *ip,
+void xfs_rmap_unmap_extent(struct xfs_trans *tp, struct xfs_inode *ip,
int whichfork, struct xfs_bmbt_irec *imap);
-int xfs_rmap_convert_extent(struct xfs_mount *mp, struct xfs_trans *tp,
+void xfs_rmap_convert_extent(struct xfs_mount *mp, struct xfs_trans *tp,
struct xfs_inode *ip, int whichfork,
struct xfs_bmbt_irec *imap);
-int xfs_rmap_alloc_extent(struct xfs_trans *tp, xfs_agnumber_t agno,
+void xfs_rmap_alloc_extent(struct xfs_trans *tp, xfs_agnumber_t agno,
xfs_agblock_t bno, xfs_extlen_t len, uint64_t owner);
-int xfs_rmap_free_extent(struct xfs_trans *tp, xfs_agnumber_t agno,
+void xfs_rmap_free_extent(struct xfs_trans *tp, xfs_agnumber_t agno,
xfs_agblock_t bno, xfs_extlen_t len, uint64_t owner);
void xfs_rmap_finish_one_cleanup(struct xfs_trans *tp,
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/5] xfs: remove unnecessary int returns from deferred rmap functions
2019-08-26 21:49 ` [PATCH 2/5] xfs: remove unnecessary int returns from deferred rmap functions Darrick J. Wong
@ 2019-08-26 23:22 ` Dave Chinner
2019-08-29 7:26 ` Christoph Hellwig
1 sibling, 0 replies; 18+ messages in thread
From: Dave Chinner @ 2019-08-26 23:22 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Mon, Aug 26, 2019 at 02:49:27PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Remove the return value from the functions that schedule deferred rmap
> operations since they never fail and do not return status.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 36 ++++++++++++------------------------
> fs/xfs/libxfs/xfs_refcount.c | 9 +++------
> fs/xfs/libxfs/xfs_rmap.c | 33 ++++++++++++++++-----------------
> fs/xfs/libxfs/xfs_rmap.h | 10 +++++-----
> 4 files changed, 36 insertions(+), 52 deletions(-)
Amazing how much gunk a bit of unnecessary error checking adds...
Looks good.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/5] xfs: remove unnecessary int returns from deferred rmap functions
2019-08-26 21:49 ` [PATCH 2/5] xfs: remove unnecessary int returns from deferred rmap functions Darrick J. Wong
2019-08-26 23:22 ` Dave Chinner
@ 2019-08-29 7:26 ` Christoph Hellwig
1 sibling, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2019-08-29 7:26 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Mon, Aug 26, 2019 at 02:49:27PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Remove the return value from the functions that schedule deferred rmap
> operations since they never fail and do not return status.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/5] xfs: remove unnecessary int returns from deferred refcount functions
2019-08-26 21:49 [PATCH 0/5] xfs: remove unnecessary parameters and returns Darrick J. Wong
2019-08-26 21:49 ` [PATCH 1/5] xfs: remove unnecessary parameter from xfs_iext_inc_seq Darrick J. Wong
2019-08-26 21:49 ` [PATCH 2/5] xfs: remove unnecessary int returns from deferred rmap functions Darrick J. Wong
@ 2019-08-26 21:49 ` Darrick J. Wong
2019-08-26 23:24 ` Dave Chinner
2019-08-29 7:26 ` Christoph Hellwig
2019-08-26 21:49 ` [PATCH 4/5] xfs: remove unnecessary int returns from deferred bmap functions Darrick J. Wong
2019-08-26 21:49 ` [PATCH 5/5] xfs: reinitialize rm_flags when unpacking an offset into an rmap irec Darrick J. Wong
4 siblings, 2 replies; 18+ messages in thread
From: Darrick J. Wong @ 2019-08-26 21:49 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
Remove the return value from the functions that schedule deferred
refcount operations since they never fail and do not return status.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_bmap.c | 21 ++++++---------------
fs/xfs/libxfs/xfs_refcount.c | 39 ++++++++++++++++-----------------------
fs/xfs/libxfs/xfs_refcount.h | 12 ++++++------
fs/xfs/xfs_refcount_item.c | 10 ++++------
fs/xfs/xfs_reflink.c | 15 ++++-----------
5 files changed, 36 insertions(+), 61 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 9842064de80c..e0cbe73ebf04 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -4393,12 +4393,9 @@ xfs_bmapi_write(
* If this is a CoW allocation, record the data in
* the refcount btree for orphan recovery.
*/
- if (whichfork == XFS_COW_FORK) {
- error = xfs_refcount_alloc_cow_extent(tp,
- bma.blkno, bma.length);
- if (error)
- goto error0;
- }
+ if (whichfork == XFS_COW_FORK)
+ xfs_refcount_alloc_cow_extent(tp, bma.blkno,
+ bma.length);
}
/* Deal with the allocated space we found. */
@@ -4532,12 +4529,8 @@ xfs_bmapi_convert_delalloc(
*imap = bma.got;
*seq = READ_ONCE(ifp->if_seq);
- if (whichfork == XFS_COW_FORK) {
- error = xfs_refcount_alloc_cow_extent(tp, bma.blkno,
- bma.length);
- if (error)
- goto out_finish;
- }
+ if (whichfork == XFS_COW_FORK)
+ xfs_refcount_alloc_cow_extent(tp, bma.blkno, bma.length);
error = xfs_bmap_btree_to_extents(tp, ip, bma.cur, &bma.logflags,
whichfork);
@@ -5148,9 +5141,7 @@ xfs_bmap_del_extent_real(
*/
if (do_fx && !(bflags & XFS_BMAPI_REMAP)) {
if (xfs_is_reflink_inode(ip) && whichfork == XFS_DATA_FORK) {
- error = xfs_refcount_decrease_extent(tp, del);
- if (error)
- goto done;
+ xfs_refcount_decrease_extent(tp, del);
} else {
__xfs_bmap_add_free(tp, del->br_startblock,
del->br_blockcount, NULL,
diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index f4e17a1f4b27..8e0a50214d94 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -1174,7 +1174,7 @@ xfs_refcount_finish_one(
/*
* Record a refcount intent for later processing.
*/
-static int
+static void
__xfs_refcount_add(
struct xfs_trans *tp,
enum xfs_refcount_intent_type type,
@@ -1196,37 +1196,36 @@ __xfs_refcount_add(
ri->ri_blockcount = blockcount;
xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_REFCOUNT, &ri->ri_list);
- return 0;
}
/*
* Increase the reference count of the blocks backing a file's extent.
*/
-int
+void
xfs_refcount_increase_extent(
struct xfs_trans *tp,
struct xfs_bmbt_irec *PREV)
{
if (!xfs_sb_version_hasreflink(&tp->t_mountp->m_sb))
- return 0;
+ return;
- return __xfs_refcount_add(tp, XFS_REFCOUNT_INCREASE,
- PREV->br_startblock, PREV->br_blockcount);
+ __xfs_refcount_add(tp, XFS_REFCOUNT_INCREASE, PREV->br_startblock,
+ PREV->br_blockcount);
}
/*
* Decrease the reference count of the blocks backing a file's extent.
*/
-int
+void
xfs_refcount_decrease_extent(
struct xfs_trans *tp,
struct xfs_bmbt_irec *PREV)
{
if (!xfs_sb_version_hasreflink(&tp->t_mountp->m_sb))
- return 0;
+ return;
- return __xfs_refcount_add(tp, XFS_REFCOUNT_DECREASE,
- PREV->br_startblock, PREV->br_blockcount);
+ __xfs_refcount_add(tp, XFS_REFCOUNT_DECREASE, PREV->br_startblock,
+ PREV->br_blockcount);
}
/*
@@ -1541,30 +1540,26 @@ __xfs_refcount_cow_free(
}
/* Record a CoW staging extent in the refcount btree. */
-int
+void
xfs_refcount_alloc_cow_extent(
struct xfs_trans *tp,
xfs_fsblock_t fsb,
xfs_extlen_t len)
{
struct xfs_mount *mp = tp->t_mountp;
- int error;
if (!xfs_sb_version_hasreflink(&mp->m_sb))
- return 0;
+ return;
- error = __xfs_refcount_add(tp, XFS_REFCOUNT_ALLOC_COW, fsb, len);
- if (error)
- return error;
+ __xfs_refcount_add(tp, XFS_REFCOUNT_ALLOC_COW, fsb, len);
/* Add rmap entry */
xfs_rmap_alloc_extent(tp, XFS_FSB_TO_AGNO(mp, fsb),
XFS_FSB_TO_AGBNO(mp, fsb), len, XFS_RMAP_OWN_COW);
- return 0;
}
/* Forget a CoW staging event in the refcount btree. */
-int
+void
xfs_refcount_free_cow_extent(
struct xfs_trans *tp,
xfs_fsblock_t fsb,
@@ -1573,12 +1568,12 @@ xfs_refcount_free_cow_extent(
struct xfs_mount *mp = tp->t_mountp;
if (!xfs_sb_version_hasreflink(&mp->m_sb))
- return 0;
+ return;
/* Remove rmap entry */
xfs_rmap_free_extent(tp, XFS_FSB_TO_AGNO(mp, fsb),
XFS_FSB_TO_AGBNO(mp, fsb), len, XFS_RMAP_OWN_COW);
- return __xfs_refcount_add(tp, XFS_REFCOUNT_FREE_COW, fsb, len);
+ __xfs_refcount_add(tp, XFS_REFCOUNT_FREE_COW, fsb, len);
}
struct xfs_refcount_recovery {
@@ -1676,10 +1671,8 @@ xfs_refcount_recover_cow_leftovers(
/* Free the orphan record */
agbno = rr->rr_rrec.rc_startblock - XFS_REFC_COW_START;
fsb = XFS_AGB_TO_FSB(mp, agno, agbno);
- error = xfs_refcount_free_cow_extent(tp, fsb,
+ xfs_refcount_free_cow_extent(tp, fsb,
rr->rr_rrec.rc_blockcount);
- if (error)
- goto out_trans;
/* Free the block. */
xfs_bmap_add_free(tp, fsb, rr->rr_rrec.rc_blockcount, NULL);
diff --git a/fs/xfs/libxfs/xfs_refcount.h b/fs/xfs/libxfs/xfs_refcount.h
index 1d9c518575e7..209795539c8d 100644
--- a/fs/xfs/libxfs/xfs_refcount.h
+++ b/fs/xfs/libxfs/xfs_refcount.h
@@ -29,9 +29,9 @@ struct xfs_refcount_intent {
xfs_extlen_t ri_blockcount;
};
-extern int xfs_refcount_increase_extent(struct xfs_trans *tp,
+void xfs_refcount_increase_extent(struct xfs_trans *tp,
struct xfs_bmbt_irec *irec);
-extern int xfs_refcount_decrease_extent(struct xfs_trans *tp,
+void xfs_refcount_decrease_extent(struct xfs_trans *tp,
struct xfs_bmbt_irec *irec);
extern void xfs_refcount_finish_one_cleanup(struct xfs_trans *tp,
@@ -45,10 +45,10 @@ extern int xfs_refcount_find_shared(struct xfs_btree_cur *cur,
xfs_agblock_t agbno, xfs_extlen_t aglen, xfs_agblock_t *fbno,
xfs_extlen_t *flen, bool find_end_of_shared);
-extern int xfs_refcount_alloc_cow_extent(struct xfs_trans *tp,
- xfs_fsblock_t fsb, xfs_extlen_t len);
-extern int xfs_refcount_free_cow_extent(struct xfs_trans *tp,
- xfs_fsblock_t fsb, xfs_extlen_t len);
+void xfs_refcount_alloc_cow_extent(struct xfs_trans *tp, xfs_fsblock_t fsb,
+ xfs_extlen_t len);
+void xfs_refcount_free_cow_extent(struct xfs_trans *tp, xfs_fsblock_t fsb,
+ xfs_extlen_t len);
extern int xfs_refcount_recover_cow_leftovers(struct xfs_mount *mp,
xfs_agnumber_t agno);
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index d8288aa0670a..bc3db2be4194 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -555,26 +555,24 @@ xfs_cui_recover(
irec.br_blockcount = new_len;
switch (type) {
case XFS_REFCOUNT_INCREASE:
- error = xfs_refcount_increase_extent(tp, &irec);
+ xfs_refcount_increase_extent(tp, &irec);
break;
case XFS_REFCOUNT_DECREASE:
- error = xfs_refcount_decrease_extent(tp, &irec);
+ xfs_refcount_decrease_extent(tp, &irec);
break;
case XFS_REFCOUNT_ALLOC_COW:
- error = xfs_refcount_alloc_cow_extent(tp,
+ xfs_refcount_alloc_cow_extent(tp,
irec.br_startblock,
irec.br_blockcount);
break;
case XFS_REFCOUNT_FREE_COW:
- error = xfs_refcount_free_cow_extent(tp,
+ xfs_refcount_free_cow_extent(tp,
irec.br_startblock,
irec.br_blockcount);
break;
default:
ASSERT(0);
}
- if (error)
- goto abort_error;
requeue_only = true;
}
}
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index edbe37b7f636..eae128ea0c12 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -495,10 +495,8 @@ xfs_reflink_cancel_cow_blocks(
ASSERT((*tpp)->t_firstblock == NULLFSBLOCK);
/* Free the CoW orphan record. */
- error = xfs_refcount_free_cow_extent(*tpp,
- del.br_startblock, del.br_blockcount);
- if (error)
- break;
+ xfs_refcount_free_cow_extent(*tpp, del.br_startblock,
+ del.br_blockcount);
xfs_bmap_add_free(*tpp, del.br_startblock,
del.br_blockcount, NULL);
@@ -675,10 +673,7 @@ xfs_reflink_end_cow_extent(
trace_xfs_reflink_cow_remap(ip, &del);
/* Free the CoW orphan record. */
- error = xfs_refcount_free_cow_extent(tp, del.br_startblock,
- del.br_blockcount);
- if (error)
- goto out_cancel;
+ xfs_refcount_free_cow_extent(tp, del.br_startblock, del.br_blockcount);
/* Map the new blocks into the data fork. */
error = xfs_bmap_map_extent(tp, ip, &del);
@@ -1070,9 +1065,7 @@ xfs_reflink_remap_extent(
uirec.br_blockcount, uirec.br_startblock);
/* Update the refcount tree */
- error = xfs_refcount_increase_extent(tp, &uirec);
- if (error)
- goto out_cancel;
+ xfs_refcount_increase_extent(tp, &uirec);
/* Map the new blocks into the data fork. */
error = xfs_bmap_map_extent(tp, ip, &uirec);
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/5] xfs: remove unnecessary int returns from deferred refcount functions
2019-08-26 21:49 ` [PATCH 3/5] xfs: remove unnecessary int returns from deferred refcount functions Darrick J. Wong
@ 2019-08-26 23:24 ` Dave Chinner
2019-08-29 7:26 ` Christoph Hellwig
1 sibling, 0 replies; 18+ messages in thread
From: Dave Chinner @ 2019-08-26 23:24 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Mon, Aug 26, 2019 at 02:49:33PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Remove the return value from the functions that schedule deferred
> refcount operations since they never fail and do not return status.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 21 ++++++---------------
> fs/xfs/libxfs/xfs_refcount.c | 39 ++++++++++++++++-----------------------
> fs/xfs/libxfs/xfs_refcount.h | 12 ++++++------
> fs/xfs/xfs_refcount_item.c | 10 ++++------
> fs/xfs/xfs_reflink.c | 15 ++++-----------
> 5 files changed, 36 insertions(+), 61 deletions(-)
looks good.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/5] xfs: remove unnecessary int returns from deferred refcount functions
2019-08-26 21:49 ` [PATCH 3/5] xfs: remove unnecessary int returns from deferred refcount functions Darrick J. Wong
2019-08-26 23:24 ` Dave Chinner
@ 2019-08-29 7:26 ` Christoph Hellwig
1 sibling, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2019-08-29 7:26 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Mon, Aug 26, 2019 at 02:49:33PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Remove the return value from the functions that schedule deferred
> refcount operations since they never fail and do not return status.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/5] xfs: remove unnecessary int returns from deferred bmap functions
2019-08-26 21:49 [PATCH 0/5] xfs: remove unnecessary parameters and returns Darrick J. Wong
` (2 preceding siblings ...)
2019-08-26 21:49 ` [PATCH 3/5] xfs: remove unnecessary int returns from deferred refcount functions Darrick J. Wong
@ 2019-08-26 21:49 ` Darrick J. Wong
2019-08-26 23:25 ` Dave Chinner
2019-08-29 7:27 ` Christoph Hellwig
2019-08-26 21:49 ` [PATCH 5/5] xfs: reinitialize rm_flags when unpacking an offset into an rmap irec Darrick J. Wong
4 siblings, 2 replies; 18+ messages in thread
From: Darrick J. Wong @ 2019-08-26 21:49 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
Remove the return value from the functions that schedule deferred bmap
operations since they never fail and do not return status.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_bmap.c | 12 ++++++------
fs/xfs/libxfs/xfs_bmap.h | 4 ++--
fs/xfs/xfs_bmap_item.c | 4 +---
fs/xfs/xfs_bmap_util.c | 16 ++++------------
fs/xfs/xfs_reflink.c | 8 ++------
5 files changed, 15 insertions(+), 29 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index e0cbe73ebf04..fb0da409f143 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -6085,29 +6085,29 @@ __xfs_bmap_add(
}
/* Map an extent into a file. */
-int
+void
xfs_bmap_map_extent(
struct xfs_trans *tp,
struct xfs_inode *ip,
struct xfs_bmbt_irec *PREV)
{
if (!xfs_bmap_is_update_needed(PREV))
- return 0;
+ return;
- return __xfs_bmap_add(tp, XFS_BMAP_MAP, ip, XFS_DATA_FORK, PREV);
+ __xfs_bmap_add(tp, XFS_BMAP_MAP, ip, XFS_DATA_FORK, PREV);
}
/* Unmap an extent out of a file. */
-int
+void
xfs_bmap_unmap_extent(
struct xfs_trans *tp,
struct xfs_inode *ip,
struct xfs_bmbt_irec *PREV)
{
if (!xfs_bmap_is_update_needed(PREV))
- return 0;
+ return;
- return __xfs_bmap_add(tp, XFS_BMAP_UNMAP, ip, XFS_DATA_FORK, PREV);
+ __xfs_bmap_add(tp, XFS_BMAP_UNMAP, ip, XFS_DATA_FORK, PREV);
}
/*
diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 8f597f9abdbe..c409871a096e 100644
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -254,9 +254,9 @@ int xfs_bmap_finish_one(struct xfs_trans *tp, struct xfs_inode *ip,
enum xfs_bmap_intent_type type, int whichfork,
xfs_fileoff_t startoff, xfs_fsblock_t startblock,
xfs_filblks_t *blockcount, xfs_exntst_t state);
-int xfs_bmap_map_extent(struct xfs_trans *tp, struct xfs_inode *ip,
+void xfs_bmap_map_extent(struct xfs_trans *tp, struct xfs_inode *ip,
struct xfs_bmbt_irec *imap);
-int xfs_bmap_unmap_extent(struct xfs_trans *tp, struct xfs_inode *ip,
+void xfs_bmap_unmap_extent(struct xfs_trans *tp, struct xfs_inode *ip,
struct xfs_bmbt_irec *imap);
static inline int xfs_bmap_fork_to_state(int whichfork)
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 9fa4a7ee8cfc..c9d1903aee3e 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -542,9 +542,7 @@ xfs_bui_recover(
irec.br_blockcount = count;
irec.br_startoff = bmap->me_startoff;
irec.br_state = state;
- error = xfs_bmap_unmap_extent(tp, ip, &irec);
- if (error)
- goto err_inode;
+ xfs_bmap_unmap_extent(tp, ip, &irec);
}
set_bit(XFS_BUI_RECOVERED, &buip->bui_flags);
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 98c6a7a71427..e12f0ba7f2eb 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1532,24 +1532,16 @@ xfs_swap_extent_rmap(
trace_xfs_swap_extent_rmap_remap_piece(tip, &uirec);
/* Remove the mapping from the donor file. */
- error = xfs_bmap_unmap_extent(tp, tip, &uirec);
- if (error)
- goto out;
+ xfs_bmap_unmap_extent(tp, tip, &uirec);
/* Remove the mapping from the source file. */
- error = xfs_bmap_unmap_extent(tp, ip, &irec);
- if (error)
- goto out;
+ xfs_bmap_unmap_extent(tp, ip, &irec);
/* Map the donor file's blocks into the source file. */
- error = xfs_bmap_map_extent(tp, ip, &uirec);
- if (error)
- goto out;
+ xfs_bmap_map_extent(tp, ip, &uirec);
/* Map the source file's blocks into the donor file. */
- error = xfs_bmap_map_extent(tp, tip, &irec);
- if (error)
- goto out;
+ xfs_bmap_map_extent(tp, tip, &irec);
error = xfs_defer_finish(tpp);
tp = *tpp;
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index eae128ea0c12..0f08153b4994 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -676,9 +676,7 @@ xfs_reflink_end_cow_extent(
xfs_refcount_free_cow_extent(tp, del.br_startblock, del.br_blockcount);
/* Map the new blocks into the data fork. */
- error = xfs_bmap_map_extent(tp, ip, &del);
- if (error)
- goto out_cancel;
+ xfs_bmap_map_extent(tp, ip, &del);
/* Charge this new data fork mapping to the on-disk quota. */
xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_DELBCOUNT,
@@ -1068,9 +1066,7 @@ xfs_reflink_remap_extent(
xfs_refcount_increase_extent(tp, &uirec);
/* Map the new blocks into the data fork. */
- error = xfs_bmap_map_extent(tp, ip, &uirec);
- if (error)
- goto out_cancel;
+ xfs_bmap_map_extent(tp, ip, &uirec);
/* Update quota accounting. */
xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT,
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 4/5] xfs: remove unnecessary int returns from deferred bmap functions
2019-08-26 21:49 ` [PATCH 4/5] xfs: remove unnecessary int returns from deferred bmap functions Darrick J. Wong
@ 2019-08-26 23:25 ` Dave Chinner
2019-08-29 7:27 ` Christoph Hellwig
1 sibling, 0 replies; 18+ messages in thread
From: Dave Chinner @ 2019-08-26 23:25 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Mon, Aug 26, 2019 at 02:49:40PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Remove the return value from the functions that schedule deferred bmap
> operations since they never fail and do not return status.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 12 ++++++------
> fs/xfs/libxfs/xfs_bmap.h | 4 ++--
> fs/xfs/xfs_bmap_item.c | 4 +---
> fs/xfs/xfs_bmap_util.c | 16 ++++------------
> fs/xfs/xfs_reflink.c | 8 ++------
> 5 files changed, 15 insertions(+), 29 deletions(-)
And another :)
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/5] xfs: remove unnecessary int returns from deferred bmap functions
2019-08-26 21:49 ` [PATCH 4/5] xfs: remove unnecessary int returns from deferred bmap functions Darrick J. Wong
2019-08-26 23:25 ` Dave Chinner
@ 2019-08-29 7:27 ` Christoph Hellwig
1 sibling, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2019-08-29 7:27 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Mon, Aug 26, 2019 at 02:49:40PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Remove the return value from the functions that schedule deferred bmap
> operations since they never fail and do not return status.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 5/5] xfs: reinitialize rm_flags when unpacking an offset into an rmap irec
2019-08-26 21:49 [PATCH 0/5] xfs: remove unnecessary parameters and returns Darrick J. Wong
` (3 preceding siblings ...)
2019-08-26 21:49 ` [PATCH 4/5] xfs: remove unnecessary int returns from deferred bmap functions Darrick J. Wong
@ 2019-08-26 21:49 ` Darrick J. Wong
2019-08-26 23:26 ` Dave Chinner
2019-08-29 7:29 ` Christoph Hellwig
4 siblings, 2 replies; 18+ messages in thread
From: Darrick J. Wong @ 2019-08-26 21:49 UTC (permalink / raw)
To: darrick.wong; +Cc: linux-xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
In xfs_rmap_irec_offset_unpack, we should always clear the contents of
rm_flags before we begin unpacking the encoded (ondisk) offset into the
incore rm_offset and incore rm_flags fields. Remove the open-coded
field zeroing as this encourages api misuse.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_rmap.c | 1 -
fs/xfs/libxfs/xfs_rmap.h | 1 +
2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
index 56769826a5ca..424c5f7343e1 100644
--- a/fs/xfs/libxfs/xfs_rmap.c
+++ b/fs/xfs/libxfs/xfs_rmap.c
@@ -168,7 +168,6 @@ xfs_rmap_btrec_to_irec(
union xfs_btree_rec *rec,
struct xfs_rmap_irec *irec)
{
- irec->rm_flags = 0;
irec->rm_startblock = be32_to_cpu(rec->rmap.rm_startblock);
irec->rm_blockcount = be32_to_cpu(rec->rmap.rm_blockcount);
irec->rm_owner = be64_to_cpu(rec->rmap.rm_owner);
diff --git a/fs/xfs/libxfs/xfs_rmap.h b/fs/xfs/libxfs/xfs_rmap.h
index 0c2c3cb73429..abe633403fd1 100644
--- a/fs/xfs/libxfs/xfs_rmap.h
+++ b/fs/xfs/libxfs/xfs_rmap.h
@@ -68,6 +68,7 @@ xfs_rmap_irec_offset_unpack(
if (offset & ~(XFS_RMAP_OFF_MASK | XFS_RMAP_OFF_FLAGS))
return -EFSCORRUPTED;
irec->rm_offset = XFS_RMAP_OFF(offset);
+ irec->rm_flags = 0;
if (offset & XFS_RMAP_OFF_ATTR_FORK)
irec->rm_flags |= XFS_RMAP_ATTR_FORK;
if (offset & XFS_RMAP_OFF_BMBT_BLOCK)
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 5/5] xfs: reinitialize rm_flags when unpacking an offset into an rmap irec
2019-08-26 21:49 ` [PATCH 5/5] xfs: reinitialize rm_flags when unpacking an offset into an rmap irec Darrick J. Wong
@ 2019-08-26 23:26 ` Dave Chinner
2019-08-29 7:29 ` Christoph Hellwig
1 sibling, 0 replies; 18+ messages in thread
From: Dave Chinner @ 2019-08-26 23:26 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Mon, Aug 26, 2019 at 02:49:46PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> In xfs_rmap_irec_offset_unpack, we should always clear the contents of
> rm_flags before we begin unpacking the encoded (ondisk) offset into the
> incore rm_offset and incore rm_flags fields. Remove the open-coded
> field zeroing as this encourages api misuse.
Makes sense. Flags are meaningless until they've been extracted.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 5/5] xfs: reinitialize rm_flags when unpacking an offset into an rmap irec
2019-08-26 21:49 ` [PATCH 5/5] xfs: reinitialize rm_flags when unpacking an offset into an rmap irec Darrick J. Wong
2019-08-26 23:26 ` Dave Chinner
@ 2019-08-29 7:29 ` Christoph Hellwig
2019-08-29 16:01 ` Darrick J. Wong
1 sibling, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2019-08-29 7:29 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Mon, Aug 26, 2019 at 02:49:46PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> In xfs_rmap_irec_offset_unpack, we should always clear the contents of
> rm_flags before we begin unpacking the encoded (ondisk) offset into the
> incore rm_offset and incore rm_flags fields. Remove the open-coded
> field zeroing as this encourages api misuse.
This one doesn't fit the series' theme, does it? :)
> +++ b/fs/xfs/libxfs/xfs_rmap.c
> @@ -168,7 +168,6 @@ xfs_rmap_btrec_to_irec(
> union xfs_btree_rec *rec,
> struct xfs_rmap_irec *irec)
> {
> - irec->rm_flags = 0;
> irec->rm_startblock = be32_to_cpu(rec->rmap.rm_startblock);
> irec->rm_blockcount = be32_to_cpu(rec->rmap.rm_blockcount);
> irec->rm_owner = be64_to_cpu(rec->rmap.rm_owner);
> diff --git a/fs/xfs/libxfs/xfs_rmap.h b/fs/xfs/libxfs/xfs_rmap.h
> index 0c2c3cb73429..abe633403fd1 100644
> --- a/fs/xfs/libxfs/xfs_rmap.h
> +++ b/fs/xfs/libxfs/xfs_rmap.h
> @@ -68,6 +68,7 @@ xfs_rmap_irec_offset_unpack(
> if (offset & ~(XFS_RMAP_OFF_MASK | XFS_RMAP_OFF_FLAGS))
> return -EFSCORRUPTED;
> irec->rm_offset = XFS_RMAP_OFF(offset);
> + irec->rm_flags = 0;
The change looks sensible-ish. But why do we even have a separate
xfs_rmap_irec_offset_unpack with a single caller nd out of the
way in a header? Wouldn't it make sense to just merge the two
functions?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 5/5] xfs: reinitialize rm_flags when unpacking an offset into an rmap irec
2019-08-29 7:29 ` Christoph Hellwig
@ 2019-08-29 16:01 ` Darrick J. Wong
2019-08-30 15:31 ` Christoph Hellwig
0 siblings, 1 reply; 18+ messages in thread
From: Darrick J. Wong @ 2019-08-29 16:01 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Thu, Aug 29, 2019 at 12:29:57AM -0700, Christoph Hellwig wrote:
> On Mon, Aug 26, 2019 at 02:49:46PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > In xfs_rmap_irec_offset_unpack, we should always clear the contents of
> > rm_flags before we begin unpacking the encoded (ondisk) offset into the
> > incore rm_offset and incore rm_flags fields. Remove the open-coded
> > field zeroing as this encourages api misuse.
>
> This one doesn't fit the series' theme, does it? :)
Nope, there's always one cling-on patch. :/
> > +++ b/fs/xfs/libxfs/xfs_rmap.c
> > @@ -168,7 +168,6 @@ xfs_rmap_btrec_to_irec(
> > union xfs_btree_rec *rec,
> > struct xfs_rmap_irec *irec)
> > {
> > - irec->rm_flags = 0;
> > irec->rm_startblock = be32_to_cpu(rec->rmap.rm_startblock);
> > irec->rm_blockcount = be32_to_cpu(rec->rmap.rm_blockcount);
> > irec->rm_owner = be64_to_cpu(rec->rmap.rm_owner);
> > diff --git a/fs/xfs/libxfs/xfs_rmap.h b/fs/xfs/libxfs/xfs_rmap.h
> > index 0c2c3cb73429..abe633403fd1 100644
> > --- a/fs/xfs/libxfs/xfs_rmap.h
> > +++ b/fs/xfs/libxfs/xfs_rmap.h
> > @@ -68,6 +68,7 @@ xfs_rmap_irec_offset_unpack(
> > if (offset & ~(XFS_RMAP_OFF_MASK | XFS_RMAP_OFF_FLAGS))
> > return -EFSCORRUPTED;
> > irec->rm_offset = XFS_RMAP_OFF(offset);
> > + irec->rm_flags = 0;
>
> The change looks sensible-ish. But why do we even have a separate
> xfs_rmap_irec_offset_unpack with a single caller nd out of the
> way in a header? Wouldn't it make sense to just merge the two
> functions?
xfs_repair uses libxfs_rmap_irec_offset_unpack, which is why it's a
separate function.
--D
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 5/5] xfs: reinitialize rm_flags when unpacking an offset into an rmap irec
2019-08-29 16:01 ` Darrick J. Wong
@ 2019-08-30 15:31 ` Christoph Hellwig
0 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2019-08-30 15:31 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs
On Thu, Aug 29, 2019 at 09:01:18AM -0700, Darrick J. Wong wrote:
> > > irec->rm_owner = be64_to_cpu(rec->rmap.rm_owner);
> > > diff --git a/fs/xfs/libxfs/xfs_rmap.h b/fs/xfs/libxfs/xfs_rmap.h
> > > index 0c2c3cb73429..abe633403fd1 100644
> > > --- a/fs/xfs/libxfs/xfs_rmap.h
> > > +++ b/fs/xfs/libxfs/xfs_rmap.h
> > > @@ -68,6 +68,7 @@ xfs_rmap_irec_offset_unpack(
> > > if (offset & ~(XFS_RMAP_OFF_MASK | XFS_RMAP_OFF_FLAGS))
> > > return -EFSCORRUPTED;
> > > irec->rm_offset = XFS_RMAP_OFF(offset);
> > > + irec->rm_flags = 0;
> >
> > The change looks sensible-ish. But why do we even have a separate
> > xfs_rmap_irec_offset_unpack with a single caller nd out of the
> > way in a header? Wouldn't it make sense to just merge the two
> > functions?
>
> xfs_repair uses libxfs_rmap_irec_offset_unpack, which is why it's a
> separate function.
True. Athough repair would also benefit a lot from a version of
xfs_rmap_btrec_to_irec that just takes a bmbt_key instead, but that is
for another time.
^ permalink raw reply [flat|nested] 18+ messages in thread