* two small log recovery cleanups @ 2020-04-27 13:52 Christoph Hellwig 2020-04-27 13:52 ` [PATCH 1/2] xfs: refactor the buffer cancellation table helpers Christoph Hellwig ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Christoph Hellwig @ 2020-04-27 13:52 UTC (permalink / raw) To: linux-xfs Hi all, two small log recovery cleanups based on my review of the refactoring series from Darrick. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] xfs: refactor the buffer cancellation table helpers 2020-04-27 13:52 two small log recovery cleanups Christoph Hellwig @ 2020-04-27 13:52 ` Christoph Hellwig 2020-04-27 18:12 ` Darrick J. Wong 2020-04-28 15:10 ` Brian Foster 2020-04-27 13:52 ` [PATCH 2/2] xfs: rename inode_list xlog_recover_reorder_trans Christoph Hellwig ` (2 subsequent siblings) 3 siblings, 2 replies; 11+ messages in thread From: Christoph Hellwig @ 2020-04-27 13:52 UTC (permalink / raw) To: linux-xfs Replace the somewhat convoluted use of xlog_peek_buffer_cancelled and xlog_check_buffer_cancelled with two obvious helpers: xlog_is_buffer_cancelled, which returns true if there is a buffer in the cancellation table, and xlog_put_buffer_cancelled, which also decrements the reference count of the buffer cancellation table. Both share a little helper to look up the entry. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_log_recover.c | 109 ++++++++++++++++++--------------------- 1 file changed, 50 insertions(+), 59 deletions(-) diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 11c3502b07b13..750a81b941ea4 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -1972,26 +1972,17 @@ xlog_recover_buffer_pass1( return 0; } -/* - * Check to see whether the buffer being recovered has a corresponding - * entry in the buffer cancel record table. If it is, return the cancel - * buffer structure to the caller. - */ -STATIC struct xfs_buf_cancel * -xlog_peek_buffer_cancelled( +static struct xfs_buf_cancel * +xlog_find_buffer_cancelled( struct xlog *log, xfs_daddr_t blkno, - uint len, - unsigned short flags) + uint len) { struct list_head *bucket; struct xfs_buf_cancel *bcp; - if (!log->l_buf_cancel_table) { - /* empty table means no cancelled buffers in the log */ - ASSERT(!(flags & XFS_BLF_CANCEL)); + if (!log->l_buf_cancel_table) return NULL; - } bucket = XLOG_BUF_CANCEL_BUCKET(log, blkno); list_for_each_entry(bcp, bucket, bc_list) { @@ -1999,50 +1990,48 @@ xlog_peek_buffer_cancelled( return bcp; } - /* - * We didn't find a corresponding entry in the table, so return 0 so - * that the buffer is NOT cancelled. - */ - ASSERT(!(flags & XFS_BLF_CANCEL)); return NULL; } /* - * If the buffer is being cancelled then return 1 so that it will be cancelled, - * otherwise return 0. If the buffer is actually a buffer cancel item - * (XFS_BLF_CANCEL is set), then decrement the refcount on the entry in the - * table and remove it from the table if this is the last reference. + * Check if there is and entry for blkno, len in the buffer cancel record table. + */ +static bool +xlog_is_buffer_cancelled( + struct xlog *log, + xfs_daddr_t blkno, + uint len) +{ + return xlog_find_buffer_cancelled(log, blkno, len) != NULL; +} + +/* + * Check if there is and entry for blkno, len in the buffer cancel record table, + * and decremented the reference count on it if there is one. * - * We remove the cancel record from the table when we encounter its last - * occurrence in the log so that if the same buffer is re-used again after its - * last cancellation we actually replay the changes made at that point. + * Remove the cancel record once the refcount hits zero, so that if the same + * buffer is re-used again after its last cancellation we actually replay the + * changes made at that point. */ -STATIC int -xlog_check_buffer_cancelled( +static bool +xlog_put_buffer_cancelled( struct xlog *log, xfs_daddr_t blkno, - uint len, - unsigned short flags) + uint len) { struct xfs_buf_cancel *bcp; - bcp = xlog_peek_buffer_cancelled(log, blkno, len, flags); - if (!bcp) - return 0; + bcp = xlog_find_buffer_cancelled(log, blkno, len); + if (!bcp) { + ASSERT(0); + return false; + } - /* - * We've go a match, so return 1 so that the recovery of this buffer - * is cancelled. If this buffer is actually a buffer cancel log - * item, then decrement the refcount on the one in the table and - * remove it if this is the last reference. - */ - if (flags & XFS_BLF_CANCEL) { - if (--bcp->bc_refcount == 0) { - list_del(&bcp->bc_list); - kmem_free(bcp); - } + if (--bcp->bc_refcount == 0) { + list_del(&bcp->bc_list); + kmem_free(bcp); } - return 1; + return true; } /* @@ -2733,10 +2722,15 @@ xlog_recover_buffer_pass2( * In this pass we only want to recover all the buffers which have * not been cancelled and are not cancellation buffers themselves. */ - if (xlog_check_buffer_cancelled(log, buf_f->blf_blkno, - buf_f->blf_len, buf_f->blf_flags)) { - trace_xfs_log_recover_buf_cancel(log, buf_f); - return 0; + if (buf_f->blf_flags & XFS_BLF_CANCEL) { + if (xlog_put_buffer_cancelled(log, buf_f->blf_blkno, + buf_f->blf_len)) + goto cancelled; + } else { + + if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno, + buf_f->blf_len)) + goto cancelled; } trace_xfs_log_recover_buf_recover(log, buf_f); @@ -2820,6 +2814,9 @@ xlog_recover_buffer_pass2( out_release: xfs_buf_relse(bp); return error; +cancelled: + trace_xfs_log_recover_buf_cancel(log, buf_f); + return 0; } /* @@ -2937,8 +2934,7 @@ xlog_recover_inode_pass2( * Inode buffers can be freed, look out for it, * and do not replay the inode. */ - if (xlog_check_buffer_cancelled(log, in_f->ilf_blkno, - in_f->ilf_len, 0)) { + if (xlog_is_buffer_cancelled(log, in_f->ilf_blkno, in_f->ilf_len)) { error = 0; trace_xfs_log_recover_inode_cancel(log, in_f); goto error; @@ -3840,7 +3836,7 @@ xlog_recover_do_icreate_pass2( daddr = XFS_AGB_TO_DADDR(mp, agno, agbno + i * igeo->blocks_per_cluster); - if (xlog_check_buffer_cancelled(log, daddr, bb_per_cluster, 0)) + if (xlog_is_buffer_cancelled(log, daddr, bb_per_cluster)) cancel_count++; } @@ -3876,11 +3872,8 @@ xlog_recover_buffer_ra_pass2( struct xfs_buf_log_format *buf_f = item->ri_buf[0].i_addr; struct xfs_mount *mp = log->l_mp; - if (xlog_peek_buffer_cancelled(log, buf_f->blf_blkno, - buf_f->blf_len, buf_f->blf_flags)) { + if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno, buf_f->blf_len)) return; - } - xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno, buf_f->blf_len, NULL); } @@ -3905,9 +3898,8 @@ xlog_recover_inode_ra_pass2( return; } - if (xlog_peek_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len, 0)) + if (xlog_is_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len)) return; - xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno, ilfp->ilf_len, &xfs_inode_buf_ra_ops); } @@ -3943,9 +3935,8 @@ xlog_recover_dquot_ra_pass2( ASSERT(dq_f->qlf_len == 1); len = XFS_FSB_TO_BB(mp, dq_f->qlf_len); - if (xlog_peek_buffer_cancelled(log, dq_f->qlf_blkno, len, 0)) + if (xlog_is_buffer_cancelled(log, dq_f->qlf_blkno, len)) return; - xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno, len, &xfs_dquot_buf_ra_ops); } -- 2.26.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] xfs: refactor the buffer cancellation table helpers 2020-04-27 13:52 ` [PATCH 1/2] xfs: refactor the buffer cancellation table helpers Christoph Hellwig @ 2020-04-27 18:12 ` Darrick J. Wong 2020-04-28 15:10 ` Brian Foster 1 sibling, 0 replies; 11+ messages in thread From: Darrick J. Wong @ 2020-04-27 18:12 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs On Mon, Apr 27, 2020 at 03:52:28PM +0200, Christoph Hellwig wrote: > Replace the somewhat convoluted use of xlog_peek_buffer_cancelled and > xlog_check_buffer_cancelled with two obvious helpers: > > xlog_is_buffer_cancelled, which returns true if there is a buffer in > the cancellation table, and > xlog_put_buffer_cancelled, which also decrements the reference count > of the buffer cancellation table. > > Both share a little helper to look up the entry. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Looks ok, though it took me a while to wrap my head around the convolution. Will give all of these a spin. Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > fs/xfs/xfs_log_recover.c | 109 ++++++++++++++++++--------------------- > 1 file changed, 50 insertions(+), 59 deletions(-) > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 11c3502b07b13..750a81b941ea4 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -1972,26 +1972,17 @@ xlog_recover_buffer_pass1( > return 0; > } > > -/* > - * Check to see whether the buffer being recovered has a corresponding > - * entry in the buffer cancel record table. If it is, return the cancel > - * buffer structure to the caller. > - */ > -STATIC struct xfs_buf_cancel * > -xlog_peek_buffer_cancelled( > +static struct xfs_buf_cancel * > +xlog_find_buffer_cancelled( > struct xlog *log, > xfs_daddr_t blkno, > - uint len, > - unsigned short flags) > + uint len) > { > struct list_head *bucket; > struct xfs_buf_cancel *bcp; > > - if (!log->l_buf_cancel_table) { > - /* empty table means no cancelled buffers in the log */ > - ASSERT(!(flags & XFS_BLF_CANCEL)); > + if (!log->l_buf_cancel_table) > return NULL; > - } > > bucket = XLOG_BUF_CANCEL_BUCKET(log, blkno); > list_for_each_entry(bcp, bucket, bc_list) { > @@ -1999,50 +1990,48 @@ xlog_peek_buffer_cancelled( > return bcp; > } > > - /* > - * We didn't find a corresponding entry in the table, so return 0 so > - * that the buffer is NOT cancelled. > - */ > - ASSERT(!(flags & XFS_BLF_CANCEL)); > return NULL; > } > > /* > - * If the buffer is being cancelled then return 1 so that it will be cancelled, > - * otherwise return 0. If the buffer is actually a buffer cancel item > - * (XFS_BLF_CANCEL is set), then decrement the refcount on the entry in the > - * table and remove it from the table if this is the last reference. > + * Check if there is and entry for blkno, len in the buffer cancel record table. > + */ > +static bool > +xlog_is_buffer_cancelled( > + struct xlog *log, > + xfs_daddr_t blkno, > + uint len) > +{ > + return xlog_find_buffer_cancelled(log, blkno, len) != NULL; > +} > + > +/* > + * Check if there is and entry for blkno, len in the buffer cancel record table, > + * and decremented the reference count on it if there is one. > * > - * We remove the cancel record from the table when we encounter its last > - * occurrence in the log so that if the same buffer is re-used again after its > - * last cancellation we actually replay the changes made at that point. > + * Remove the cancel record once the refcount hits zero, so that if the same > + * buffer is re-used again after its last cancellation we actually replay the > + * changes made at that point. > */ > -STATIC int > -xlog_check_buffer_cancelled( > +static bool > +xlog_put_buffer_cancelled( > struct xlog *log, > xfs_daddr_t blkno, > - uint len, > - unsigned short flags) > + uint len) > { > struct xfs_buf_cancel *bcp; > > - bcp = xlog_peek_buffer_cancelled(log, blkno, len, flags); > - if (!bcp) > - return 0; > + bcp = xlog_find_buffer_cancelled(log, blkno, len); > + if (!bcp) { > + ASSERT(0); > + return false; > + } > > - /* > - * We've go a match, so return 1 so that the recovery of this buffer > - * is cancelled. If this buffer is actually a buffer cancel log > - * item, then decrement the refcount on the one in the table and > - * remove it if this is the last reference. > - */ > - if (flags & XFS_BLF_CANCEL) { > - if (--bcp->bc_refcount == 0) { > - list_del(&bcp->bc_list); > - kmem_free(bcp); > - } > + if (--bcp->bc_refcount == 0) { > + list_del(&bcp->bc_list); > + kmem_free(bcp); > } > - return 1; > + return true; > } > > /* > @@ -2733,10 +2722,15 @@ xlog_recover_buffer_pass2( > * In this pass we only want to recover all the buffers which have > * not been cancelled and are not cancellation buffers themselves. > */ > - if (xlog_check_buffer_cancelled(log, buf_f->blf_blkno, > - buf_f->blf_len, buf_f->blf_flags)) { > - trace_xfs_log_recover_buf_cancel(log, buf_f); > - return 0; > + if (buf_f->blf_flags & XFS_BLF_CANCEL) { > + if (xlog_put_buffer_cancelled(log, buf_f->blf_blkno, > + buf_f->blf_len)) > + goto cancelled; > + } else { > + > + if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno, > + buf_f->blf_len)) > + goto cancelled; > } > > trace_xfs_log_recover_buf_recover(log, buf_f); > @@ -2820,6 +2814,9 @@ xlog_recover_buffer_pass2( > out_release: > xfs_buf_relse(bp); > return error; > +cancelled: > + trace_xfs_log_recover_buf_cancel(log, buf_f); > + return 0; > } > > /* > @@ -2937,8 +2934,7 @@ xlog_recover_inode_pass2( > * Inode buffers can be freed, look out for it, > * and do not replay the inode. > */ > - if (xlog_check_buffer_cancelled(log, in_f->ilf_blkno, > - in_f->ilf_len, 0)) { > + if (xlog_is_buffer_cancelled(log, in_f->ilf_blkno, in_f->ilf_len)) { > error = 0; > trace_xfs_log_recover_inode_cancel(log, in_f); > goto error; > @@ -3840,7 +3836,7 @@ xlog_recover_do_icreate_pass2( > > daddr = XFS_AGB_TO_DADDR(mp, agno, > agbno + i * igeo->blocks_per_cluster); > - if (xlog_check_buffer_cancelled(log, daddr, bb_per_cluster, 0)) > + if (xlog_is_buffer_cancelled(log, daddr, bb_per_cluster)) > cancel_count++; > } > > @@ -3876,11 +3872,8 @@ xlog_recover_buffer_ra_pass2( > struct xfs_buf_log_format *buf_f = item->ri_buf[0].i_addr; > struct xfs_mount *mp = log->l_mp; > > - if (xlog_peek_buffer_cancelled(log, buf_f->blf_blkno, > - buf_f->blf_len, buf_f->blf_flags)) { > + if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno, buf_f->blf_len)) > return; > - } > - > xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno, > buf_f->blf_len, NULL); > } > @@ -3905,9 +3898,8 @@ xlog_recover_inode_ra_pass2( > return; > } > > - if (xlog_peek_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len, 0)) > + if (xlog_is_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len)) > return; > - > xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno, > ilfp->ilf_len, &xfs_inode_buf_ra_ops); > } > @@ -3943,9 +3935,8 @@ xlog_recover_dquot_ra_pass2( > ASSERT(dq_f->qlf_len == 1); > > len = XFS_FSB_TO_BB(mp, dq_f->qlf_len); > - if (xlog_peek_buffer_cancelled(log, dq_f->qlf_blkno, len, 0)) > + if (xlog_is_buffer_cancelled(log, dq_f->qlf_blkno, len)) > return; > - > xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno, len, > &xfs_dquot_buf_ra_ops); > } > -- > 2.26.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] xfs: refactor the buffer cancellation table helpers 2020-04-27 13:52 ` [PATCH 1/2] xfs: refactor the buffer cancellation table helpers Christoph Hellwig 2020-04-27 18:12 ` Darrick J. Wong @ 2020-04-28 15:10 ` Brian Foster 1 sibling, 0 replies; 11+ messages in thread From: Brian Foster @ 2020-04-28 15:10 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs On Mon, Apr 27, 2020 at 03:52:28PM +0200, Christoph Hellwig wrote: > Replace the somewhat convoluted use of xlog_peek_buffer_cancelled and > xlog_check_buffer_cancelled with two obvious helpers: > > xlog_is_buffer_cancelled, which returns true if there is a buffer in > the cancellation table, and > xlog_put_buffer_cancelled, which also decrements the reference count > of the buffer cancellation table. > > Both share a little helper to look up the entry. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_log_recover.c | 109 ++++++++++++++++++--------------------- > 1 file changed, 50 insertions(+), 59 deletions(-) > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 11c3502b07b13..750a81b941ea4 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -1972,26 +1972,17 @@ xlog_recover_buffer_pass1( > return 0; > } > ... > /* > - * If the buffer is being cancelled then return 1 so that it will be cancelled, > - * otherwise return 0. If the buffer is actually a buffer cancel item > - * (XFS_BLF_CANCEL is set), then decrement the refcount on the entry in the > - * table and remove it from the table if this is the last reference. > + * Check if there is and entry for blkno, len in the buffer cancel record table. > + */ > +static bool > +xlog_is_buffer_cancelled( > + struct xlog *log, > + xfs_daddr_t blkno, > + uint len) > +{ > + return xlog_find_buffer_cancelled(log, blkno, len) != NULL; > +} > + > +/* > + * Check if there is and entry for blkno, len in the buffer cancel record table, Nit: an > + * and decremented the reference count on it if there is one. Nit: decrement ... > @@ -2733,10 +2722,15 @@ xlog_recover_buffer_pass2( > * In this pass we only want to recover all the buffers which have > * not been cancelled and are not cancellation buffers themselves. > */ > - if (xlog_check_buffer_cancelled(log, buf_f->blf_blkno, > - buf_f->blf_len, buf_f->blf_flags)) { > - trace_xfs_log_recover_buf_cancel(log, buf_f); > - return 0; > + if (buf_f->blf_flags & XFS_BLF_CANCEL) { > + if (xlog_put_buffer_cancelled(log, buf_f->blf_blkno, > + buf_f->blf_len)) > + goto cancelled; > + } else { > + Extra whitespace above. With those nits fixed: Reviewed-by: Brian Foster <bfoster@redhat.com> > + if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno, > + buf_f->blf_len)) > + goto cancelled; > } > > trace_xfs_log_recover_buf_recover(log, buf_f); > @@ -2820,6 +2814,9 @@ xlog_recover_buffer_pass2( > out_release: > xfs_buf_relse(bp); > return error; > +cancelled: > + trace_xfs_log_recover_buf_cancel(log, buf_f); > + return 0; > } > > /* > @@ -2937,8 +2934,7 @@ xlog_recover_inode_pass2( > * Inode buffers can be freed, look out for it, > * and do not replay the inode. > */ > - if (xlog_check_buffer_cancelled(log, in_f->ilf_blkno, > - in_f->ilf_len, 0)) { > + if (xlog_is_buffer_cancelled(log, in_f->ilf_blkno, in_f->ilf_len)) { > error = 0; > trace_xfs_log_recover_inode_cancel(log, in_f); > goto error; > @@ -3840,7 +3836,7 @@ xlog_recover_do_icreate_pass2( > > daddr = XFS_AGB_TO_DADDR(mp, agno, > agbno + i * igeo->blocks_per_cluster); > - if (xlog_check_buffer_cancelled(log, daddr, bb_per_cluster, 0)) > + if (xlog_is_buffer_cancelled(log, daddr, bb_per_cluster)) > cancel_count++; > } > > @@ -3876,11 +3872,8 @@ xlog_recover_buffer_ra_pass2( > struct xfs_buf_log_format *buf_f = item->ri_buf[0].i_addr; > struct xfs_mount *mp = log->l_mp; > > - if (xlog_peek_buffer_cancelled(log, buf_f->blf_blkno, > - buf_f->blf_len, buf_f->blf_flags)) { > + if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno, buf_f->blf_len)) > return; > - } > - > xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno, > buf_f->blf_len, NULL); > } > @@ -3905,9 +3898,8 @@ xlog_recover_inode_ra_pass2( > return; > } > > - if (xlog_peek_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len, 0)) > + if (xlog_is_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len)) > return; > - > xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno, > ilfp->ilf_len, &xfs_inode_buf_ra_ops); > } > @@ -3943,9 +3935,8 @@ xlog_recover_dquot_ra_pass2( > ASSERT(dq_f->qlf_len == 1); > > len = XFS_FSB_TO_BB(mp, dq_f->qlf_len); > - if (xlog_peek_buffer_cancelled(log, dq_f->qlf_blkno, len, 0)) > + if (xlog_is_buffer_cancelled(log, dq_f->qlf_blkno, len)) > return; > - > xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno, len, > &xfs_dquot_buf_ra_ops); > } > -- > 2.26.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] xfs: rename inode_list xlog_recover_reorder_trans 2020-04-27 13:52 two small log recovery cleanups Christoph Hellwig 2020-04-27 13:52 ` [PATCH 1/2] xfs: refactor the buffer cancellation table helpers Christoph Hellwig @ 2020-04-27 13:52 ` Christoph Hellwig 2020-04-27 18:05 ` Darrick J. Wong 2020-04-28 15:10 ` Brian Foster 2020-04-27 19:33 ` [PATCH 3/2] xfs: factor out a xlog_buf_readahead helper Christoph Hellwig 2020-04-27 19:33 ` [PATCH 4/2] xfs: simplify xlog_recover_inode_ra_pass2 Christoph Hellwig 3 siblings, 2 replies; 11+ messages in thread From: Christoph Hellwig @ 2020-04-27 13:52 UTC (permalink / raw) To: linux-xfs This list contains pretty much everything that is not a buffer. The comment calls it item_list, which is a much better name than inode list, so switch the actual variable name to that as well. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_log_recover.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 750a81b941ea4..33cac61570abe 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -1847,7 +1847,7 @@ xlog_recover_reorder_trans( LIST_HEAD(cancel_list); LIST_HEAD(buffer_list); LIST_HEAD(inode_buffer_list); - LIST_HEAD(inode_list); + LIST_HEAD(item_list); list_splice_init(&trans->r_itemq, &sort_list); list_for_each_entry_safe(item, n, &sort_list, ri_list) { @@ -1883,7 +1883,7 @@ xlog_recover_reorder_trans( case XFS_LI_BUD: trace_xfs_log_recover_item_reorder_tail(log, trans, item, pass); - list_move_tail(&item->ri_list, &inode_list); + list_move_tail(&item->ri_list, &item_list); break; default: xfs_warn(log->l_mp, @@ -1904,8 +1904,8 @@ xlog_recover_reorder_trans( ASSERT(list_empty(&sort_list)); if (!list_empty(&buffer_list)) list_splice(&buffer_list, &trans->r_itemq); - if (!list_empty(&inode_list)) - list_splice_tail(&inode_list, &trans->r_itemq); + if (!list_empty(&item_list)) + list_splice_tail(&item_list, &trans->r_itemq); if (!list_empty(&inode_buffer_list)) list_splice_tail(&inode_buffer_list, &trans->r_itemq); if (!list_empty(&cancel_list)) -- 2.26.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: rename inode_list xlog_recover_reorder_trans 2020-04-27 13:52 ` [PATCH 2/2] xfs: rename inode_list xlog_recover_reorder_trans Christoph Hellwig @ 2020-04-27 18:05 ` Darrick J. Wong 2020-04-28 15:10 ` Brian Foster 1 sibling, 0 replies; 11+ messages in thread From: Darrick J. Wong @ 2020-04-27 18:05 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs On Mon, Apr 27, 2020 at 03:52:29PM +0200, Christoph Hellwig wrote: > This list contains pretty much everything that is not a buffer. The > comment calls it item_list, which is a much better name than inode > list, so switch the actual variable name to that as well. > > Signed-off-by: Christoph Hellwig <hch@lst.de> LGTM, who doesn't know where the "inode_list" name came from... :) Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > fs/xfs/xfs_log_recover.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 750a81b941ea4..33cac61570abe 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -1847,7 +1847,7 @@ xlog_recover_reorder_trans( > LIST_HEAD(cancel_list); > LIST_HEAD(buffer_list); > LIST_HEAD(inode_buffer_list); > - LIST_HEAD(inode_list); > + LIST_HEAD(item_list); > > list_splice_init(&trans->r_itemq, &sort_list); > list_for_each_entry_safe(item, n, &sort_list, ri_list) { > @@ -1883,7 +1883,7 @@ xlog_recover_reorder_trans( > case XFS_LI_BUD: > trace_xfs_log_recover_item_reorder_tail(log, > trans, item, pass); > - list_move_tail(&item->ri_list, &inode_list); > + list_move_tail(&item->ri_list, &item_list); > break; > default: > xfs_warn(log->l_mp, > @@ -1904,8 +1904,8 @@ xlog_recover_reorder_trans( > ASSERT(list_empty(&sort_list)); > if (!list_empty(&buffer_list)) > list_splice(&buffer_list, &trans->r_itemq); > - if (!list_empty(&inode_list)) > - list_splice_tail(&inode_list, &trans->r_itemq); > + if (!list_empty(&item_list)) > + list_splice_tail(&item_list, &trans->r_itemq); > if (!list_empty(&inode_buffer_list)) > list_splice_tail(&inode_buffer_list, &trans->r_itemq); > if (!list_empty(&cancel_list)) > -- > 2.26.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: rename inode_list xlog_recover_reorder_trans 2020-04-27 13:52 ` [PATCH 2/2] xfs: rename inode_list xlog_recover_reorder_trans Christoph Hellwig 2020-04-27 18:05 ` Darrick J. Wong @ 2020-04-28 15:10 ` Brian Foster 1 sibling, 0 replies; 11+ messages in thread From: Brian Foster @ 2020-04-28 15:10 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs On Mon, Apr 27, 2020 at 03:52:29PM +0200, Christoph Hellwig wrote: > This list contains pretty much everything that is not a buffer. The > comment calls it item_list, which is a much better name than inode > list, so switch the actual variable name to that as well. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > fs/xfs/xfs_log_recover.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 750a81b941ea4..33cac61570abe 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -1847,7 +1847,7 @@ xlog_recover_reorder_trans( > LIST_HEAD(cancel_list); > LIST_HEAD(buffer_list); > LIST_HEAD(inode_buffer_list); > - LIST_HEAD(inode_list); > + LIST_HEAD(item_list); > > list_splice_init(&trans->r_itemq, &sort_list); > list_for_each_entry_safe(item, n, &sort_list, ri_list) { > @@ -1883,7 +1883,7 @@ xlog_recover_reorder_trans( > case XFS_LI_BUD: > trace_xfs_log_recover_item_reorder_tail(log, > trans, item, pass); > - list_move_tail(&item->ri_list, &inode_list); > + list_move_tail(&item->ri_list, &item_list); > break; > default: > xfs_warn(log->l_mp, > @@ -1904,8 +1904,8 @@ xlog_recover_reorder_trans( > ASSERT(list_empty(&sort_list)); > if (!list_empty(&buffer_list)) > list_splice(&buffer_list, &trans->r_itemq); > - if (!list_empty(&inode_list)) > - list_splice_tail(&inode_list, &trans->r_itemq); > + if (!list_empty(&item_list)) > + list_splice_tail(&item_list, &trans->r_itemq); > if (!list_empty(&inode_buffer_list)) > list_splice_tail(&inode_buffer_list, &trans->r_itemq); > if (!list_empty(&cancel_list)) > -- > 2.26.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/2] xfs: factor out a xlog_buf_readahead helper 2020-04-27 13:52 two small log recovery cleanups Christoph Hellwig 2020-04-27 13:52 ` [PATCH 1/2] xfs: refactor the buffer cancellation table helpers Christoph Hellwig 2020-04-27 13:52 ` [PATCH 2/2] xfs: rename inode_list xlog_recover_reorder_trans Christoph Hellwig @ 2020-04-27 19:33 ` Christoph Hellwig 2020-04-28 15:10 ` Brian Foster 2020-04-27 19:33 ` [PATCH 4/2] xfs: simplify xlog_recover_inode_ra_pass2 Christoph Hellwig 3 siblings, 1 reply; 11+ messages in thread From: Christoph Hellwig @ 2020-04-27 19:33 UTC (permalink / raw) To: linux-xfs Add a little helper to readahead a buffer if it hasn't been cancelled. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_log_recover.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 33cac61570abe..4cb8f24f3aa63 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -2034,6 +2034,17 @@ xlog_put_buffer_cancelled( return true; } +static void +xlog_buf_readahead( + struct xlog *log, + xfs_daddr_t blkno, + uint len, + const struct xfs_buf_ops *ops) +{ + if (!xlog_is_buffer_cancelled(log, blkno, len)) + xfs_buf_readahead(log->l_mp->m_ddev_targp, blkno, len, ops); +} + /* * Perform recovery for a buffer full of inodes. In these buffers, the only * data which should be recovered is that which corresponds to the @@ -3870,12 +3881,8 @@ xlog_recover_buffer_ra_pass2( struct xlog_recover_item *item) { struct xfs_buf_log_format *buf_f = item->ri_buf[0].i_addr; - struct xfs_mount *mp = log->l_mp; - if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno, buf_f->blf_len)) - return; - xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno, - buf_f->blf_len, NULL); + xlog_buf_readahead(log, buf_f->blf_blkno, buf_f->blf_len, NULL); } STATIC void @@ -3885,7 +3892,6 @@ xlog_recover_inode_ra_pass2( { struct xfs_inode_log_format ilf_buf; struct xfs_inode_log_format *ilfp; - struct xfs_mount *mp = log->l_mp; int error; if (item->ri_buf[0].i_len == sizeof(struct xfs_inode_log_format)) { @@ -3898,10 +3904,8 @@ xlog_recover_inode_ra_pass2( return; } - if (xlog_is_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len)) - return; - xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno, - ilfp->ilf_len, &xfs_inode_buf_ra_ops); + xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len, + &xfs_inode_buf_ra_ops); } STATIC void @@ -3913,8 +3917,6 @@ xlog_recover_dquot_ra_pass2( struct xfs_disk_dquot *recddq; struct xfs_dq_logformat *dq_f; uint type; - int len; - if (mp->m_qflags == 0) return; @@ -3934,11 +3936,9 @@ xlog_recover_dquot_ra_pass2( ASSERT(dq_f); ASSERT(dq_f->qlf_len == 1); - len = XFS_FSB_TO_BB(mp, dq_f->qlf_len); - if (xlog_is_buffer_cancelled(log, dq_f->qlf_blkno, len)) - return; - xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno, len, - &xfs_dquot_buf_ra_ops); + xlog_buf_readahead(log, dq_f->qlf_blkno, + XFS_FSB_TO_BB(mp, dq_f->qlf_len), + &xfs_dquot_buf_ra_ops); } STATIC void -- 2.26.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/2] xfs: factor out a xlog_buf_readahead helper 2020-04-27 19:33 ` [PATCH 3/2] xfs: factor out a xlog_buf_readahead helper Christoph Hellwig @ 2020-04-28 15:10 ` Brian Foster 0 siblings, 0 replies; 11+ messages in thread From: Brian Foster @ 2020-04-28 15:10 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs On Mon, Apr 27, 2020 at 09:33:32PM +0200, Christoph Hellwig wrote: > Add a little helper to readahead a buffer if it hasn't been cancelled. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > fs/xfs/xfs_log_recover.c | 34 +++++++++++++++++----------------- > 1 file changed, 17 insertions(+), 17 deletions(-) > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 33cac61570abe..4cb8f24f3aa63 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -2034,6 +2034,17 @@ xlog_put_buffer_cancelled( > return true; > } > > +static void > +xlog_buf_readahead( > + struct xlog *log, > + xfs_daddr_t blkno, > + uint len, > + const struct xfs_buf_ops *ops) > +{ > + if (!xlog_is_buffer_cancelled(log, blkno, len)) > + xfs_buf_readahead(log->l_mp->m_ddev_targp, blkno, len, ops); > +} > + > /* > * Perform recovery for a buffer full of inodes. In these buffers, the only > * data which should be recovered is that which corresponds to the > @@ -3870,12 +3881,8 @@ xlog_recover_buffer_ra_pass2( > struct xlog_recover_item *item) > { > struct xfs_buf_log_format *buf_f = item->ri_buf[0].i_addr; > - struct xfs_mount *mp = log->l_mp; > > - if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno, buf_f->blf_len)) > - return; > - xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno, > - buf_f->blf_len, NULL); > + xlog_buf_readahead(log, buf_f->blf_blkno, buf_f->blf_len, NULL); > } > > STATIC void > @@ -3885,7 +3892,6 @@ xlog_recover_inode_ra_pass2( > { > struct xfs_inode_log_format ilf_buf; > struct xfs_inode_log_format *ilfp; > - struct xfs_mount *mp = log->l_mp; > int error; > > if (item->ri_buf[0].i_len == sizeof(struct xfs_inode_log_format)) { > @@ -3898,10 +3904,8 @@ xlog_recover_inode_ra_pass2( > return; > } > > - if (xlog_is_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len)) > - return; > - xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno, > - ilfp->ilf_len, &xfs_inode_buf_ra_ops); > + xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len, > + &xfs_inode_buf_ra_ops); > } > > STATIC void > @@ -3913,8 +3917,6 @@ xlog_recover_dquot_ra_pass2( > struct xfs_disk_dquot *recddq; > struct xfs_dq_logformat *dq_f; > uint type; > - int len; > - > > if (mp->m_qflags == 0) > return; > @@ -3934,11 +3936,9 @@ xlog_recover_dquot_ra_pass2( > ASSERT(dq_f); > ASSERT(dq_f->qlf_len == 1); > > - len = XFS_FSB_TO_BB(mp, dq_f->qlf_len); > - if (xlog_is_buffer_cancelled(log, dq_f->qlf_blkno, len)) > - return; > - xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno, len, > - &xfs_dquot_buf_ra_ops); > + xlog_buf_readahead(log, dq_f->qlf_blkno, > + XFS_FSB_TO_BB(mp, dq_f->qlf_len), > + &xfs_dquot_buf_ra_ops); > } > > STATIC void > -- > 2.26.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/2] xfs: simplify xlog_recover_inode_ra_pass2 2020-04-27 13:52 two small log recovery cleanups Christoph Hellwig ` (2 preceding siblings ...) 2020-04-27 19:33 ` [PATCH 3/2] xfs: factor out a xlog_buf_readahead helper Christoph Hellwig @ 2020-04-27 19:33 ` Christoph Hellwig 2020-04-28 15:11 ` Brian Foster 3 siblings, 1 reply; 11+ messages in thread From: Christoph Hellwig @ 2020-04-27 19:33 UTC (permalink / raw) To: linux-xfs Don't bother to allocate memory and convert the log item when we only need the block number and the length. Just extract them directly and call xlog_buf_readahead separately in each branch. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_log_recover.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 4cb8f24f3aa63..fe4dad5b77a95 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -3890,22 +3890,17 @@ xlog_recover_inode_ra_pass2( struct xlog *log, struct xlog_recover_item *item) { - struct xfs_inode_log_format ilf_buf; - struct xfs_inode_log_format *ilfp; - int error; - if (item->ri_buf[0].i_len == sizeof(struct xfs_inode_log_format)) { - ilfp = item->ri_buf[0].i_addr; + struct xfs_inode_log_format *ilfp = item->ri_buf[0].i_addr; + + xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len, + &xfs_inode_buf_ra_ops); } else { - ilfp = &ilf_buf; - memset(ilfp, 0, sizeof(*ilfp)); - error = xfs_inode_item_format_convert(&item->ri_buf[0], ilfp); - if (error) - return; - } + struct xfs_inode_log_format_32 *ilfp = item->ri_buf[0].i_addr; - xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len, - &xfs_inode_buf_ra_ops); + xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len, + &xfs_inode_buf_ra_ops); + } } STATIC void -- 2.26.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 4/2] xfs: simplify xlog_recover_inode_ra_pass2 2020-04-27 19:33 ` [PATCH 4/2] xfs: simplify xlog_recover_inode_ra_pass2 Christoph Hellwig @ 2020-04-28 15:11 ` Brian Foster 0 siblings, 0 replies; 11+ messages in thread From: Brian Foster @ 2020-04-28 15:11 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs On Mon, Apr 27, 2020 at 09:33:49PM +0200, Christoph Hellwig wrote: > Don't bother to allocate memory and convert the log item when we > only need the block number and the length. Just extract them directly > and call xlog_buf_readahead separately in each branch. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > fs/xfs/xfs_log_recover.c | 21 ++++++++------------- > 1 file changed, 8 insertions(+), 13 deletions(-) > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 4cb8f24f3aa63..fe4dad5b77a95 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -3890,22 +3890,17 @@ xlog_recover_inode_ra_pass2( > struct xlog *log, > struct xlog_recover_item *item) > { > - struct xfs_inode_log_format ilf_buf; > - struct xfs_inode_log_format *ilfp; > - int error; > - > if (item->ri_buf[0].i_len == sizeof(struct xfs_inode_log_format)) { > - ilfp = item->ri_buf[0].i_addr; > + struct xfs_inode_log_format *ilfp = item->ri_buf[0].i_addr; > + > + xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len, > + &xfs_inode_buf_ra_ops); > } else { > - ilfp = &ilf_buf; > - memset(ilfp, 0, sizeof(*ilfp)); > - error = xfs_inode_item_format_convert(&item->ri_buf[0], ilfp); > - if (error) > - return; > - } > + struct xfs_inode_log_format_32 *ilfp = item->ri_buf[0].i_addr; > > - xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len, > - &xfs_inode_buf_ra_ops); > + xlog_buf_readahead(log, ilfp->ilf_blkno, ilfp->ilf_len, > + &xfs_inode_buf_ra_ops); > + } > } > > STATIC void > -- > 2.26.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-04-28 15:11 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-27 13:52 two small log recovery cleanups Christoph Hellwig 2020-04-27 13:52 ` [PATCH 1/2] xfs: refactor the buffer cancellation table helpers Christoph Hellwig 2020-04-27 18:12 ` Darrick J. Wong 2020-04-28 15:10 ` Brian Foster 2020-04-27 13:52 ` [PATCH 2/2] xfs: rename inode_list xlog_recover_reorder_trans Christoph Hellwig 2020-04-27 18:05 ` Darrick J. Wong 2020-04-28 15:10 ` Brian Foster 2020-04-27 19:33 ` [PATCH 3/2] xfs: factor out a xlog_buf_readahead helper Christoph Hellwig 2020-04-28 15:10 ` Brian Foster 2020-04-27 19:33 ` [PATCH 4/2] xfs: simplify xlog_recover_inode_ra_pass2 Christoph Hellwig 2020-04-28 15:11 ` Brian Foster
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.