[PATCH 0/2] xfs: two small cleanup

[PATCH 0/2] xfs: two small cleanup

[Long Li]

This series contains two small cleanup patches. No functional changes.

Long Li (2):
  xfs: remove redundant update for t_curr_res in xfs_log_ticket_regrant
  xfs: remove bp->b_error check in xfs_attr3_root_inactive

 fs/xfs/xfs_attr_inactive.c | 5 -----
 fs/xfs/xfs_log.c           | 2 --
 2 files changed, 7 deletions(-)

-- 
2.39.2

[PATCH 1/2] xfs: remove redundant update for t_curr_res in xfs_log_ticket_regrant

[Long Li]

The current reservation of the log ticket has already been updated in
xfs_log_ticket_regrant(), so there is no need to update it again. This
is just a code cleanup with no functional changes.

Signed-off-by: Long Li <leo.lilong@huawei.com>
---
 fs/xfs/xfs_log.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 05daad8a8d34..f8851ff835de 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2744,8 +2744,6 @@ xfs_log_ticket_regrant(
 	if (!ticket->t_cnt) {
 		xlog_grant_add_space(&log->l_reserve_head, ticket->t_unit_res);
 		trace_xfs_log_ticket_regrant_exit(log, ticket);
-
-		ticket->t_curr_res = ticket->t_unit_res;
 	}
 
 	xfs_log_ticket_put(ticket);
-- 
2.39.2

Re: [PATCH 1/2] xfs: remove redundant update for t_curr_res in xfs_log_ticket_regrant

[Christoph Hellwig]

On Sat, Dec 21, 2024 at 02:30:42PM +0800, Long Li wrote:
> The current reservation of the log ticket has already been updated in
> xfs_log_ticket_regrant(),

Maybe say "a few lines above" as both calls are in
xfs_log_ticket_regrant?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2] xfs: remove redundant update for t_curr_res in xfs_log_ticket_regrant

[Long Li]

On Sat, Dec 21, 2024 at 12:05:58AM -0800, Christoph Hellwig wrote:
> On Sat, Dec 21, 2024 at 02:30:42PM +0800, Long Li wrote:
> > The current reservation of the log ticket has already been updated in
> > xfs_log_ticket_regrant(),
> 
> Maybe say "a few lines above" as both calls are in
> xfs_log_ticket_regrant?

Thanks for your review , it's a good suggestion for me, I'll update it
in next version. 

> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> 

[PATCH 2/2] xfs: remove bp->b_error check in xfs_attr3_root_inactive

[Long Li]

The xfs_da3_node_read earlier in the function will catch most cases of
incoming on-disk corruption, which makes this check mostly redundant,
unless someone corrupts the buffer and the AIL pushes it out to disk
while the buffer's unlocked.

In the first case we'll never reach this check, and in the second case
the AIL will shut down the log, at which point checking b_error becomes
meaningless. Remove the check to make the code consistent with most other
xfs_trans_get_buf() callers in XFS.

Signed-off-by: Long Li <leo.lilong@huawei.com>
---
 fs/xfs/xfs_attr_inactive.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
index 24fb12986a56..319004bf089f 100644
--- a/fs/xfs/xfs_attr_inactive.c
+++ b/fs/xfs/xfs_attr_inactive.c
@@ -305,11 +305,6 @@ xfs_attr3_root_inactive(
 			XFS_FSB_TO_BB(mp, mp->m_attr_geo->fsbcount), 0, &bp);
 	if (error)
 		return error;
-	error = bp->b_error;
-	if (error) {
-		xfs_trans_brelse(*trans, bp);
-		return error;
-	}
 	xfs_trans_binval(*trans, bp);	/* remove from cache */
 	/*
 	 * Commit the invalidate and start the next transaction.
-- 
2.39.2

Re: [PATCH 2/2] xfs: remove bp->b_error check in xfs_attr3_root_inactive

[Christoph Hellwig]

On Sat, Dec 21, 2024 at 02:30:43PM +0800, Long Li wrote:
> The xfs_da3_node_read earlier in the function will catch most cases of
> incoming on-disk corruption, which makes this check mostly redundant,
> unless someone corrupts the buffer and the AIL pushes it out to disk
> while the buffer's unlocked.
> 
> In the first case we'll never reach this check, and in the second case
> the AIL will shut down the log, at which point checking b_error becomes
> meaningless. Remove the check to make the code consistent with most other
> xfs_trans_get_buf() callers in XFS.

Hmm. I don't really understand the commit log.  The b_error check
is right after a call to xfs_trans_get_buf_map.  xfs_trans_get_buf_map
either reads the buffer from disk using xfs_buf_get_map which propagates
b_error, or finds it in the transaction, where whoever read it from
disk should have done the same.  So I think the change looks fine,
but I don't think the commit log really explains it very well.

Re: [PATCH 2/2] xfs: remove bp->b_error check in xfs_attr3_root_inactive

[Long Li]

On Sat, Dec 21, 2024 at 12:08:56AM -0800, Christoph Hellwig wrote:
> On Sat, Dec 21, 2024 at 02:30:43PM +0800, Long Li wrote:
> > The xfs_da3_node_read earlier in the function will catch most cases of
> > incoming on-disk corruption, which makes this check mostly redundant,
> > unless someone corrupts the buffer and the AIL pushes it out to disk
> > while the buffer's unlocked.
> > 
> > In the first case we'll never reach this check, and in the second case
> > the AIL will shut down the log, at which point checking b_error becomes
> > meaningless. Remove the check to make the code consistent with most other
> > xfs_trans_get_buf() callers in XFS.
> 
> Hmm. I don't really understand the commit log.  The b_error check
> is right after a call to xfs_trans_get_buf_map.  xfs_trans_get_buf_map
> either reads the buffer from disk using xfs_buf_get_map which propagates
> b_error, or finds it in the transaction, where whoever read it from
> disk should have done the same.  So I think the change looks fine,
> but I don't think the commit log really explains it very well.
> 

It's true that the commit message is not clearly explained, and I'll rewrite
it in the next version.

Thanks,
Long Li