| Message ID | 20200504141154.55887-11-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: flush related error handling cleanups | expand |
On 5/4/20 7:11 AM, Brian Foster wrote: > Several callers acquire the lock just prior to the call. Callers > that require ->ail_lock for other purposes already check IN_AIL > state and thus don't require the additional shutdown check in the > helper. Push the lock down into xfs_trans_ail_delete(), open code > the instances that still acquire it, and remove the unnecessary ailp > parameter. > > Signed-off-by: Brian Foster <bfoster@redhat.com> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> Ok, followed it through and I think it makes sense: Reviewed-by: Allison Collins <allison.henderson@oracle.com> > --- > fs/xfs/xfs_buf_item.c | 27 +++++++++++---------------- > fs/xfs/xfs_dquot.c | 6 ++++-- > fs/xfs/xfs_trans_ail.c | 3 ++- > fs/xfs/xfs_trans_priv.h | 14 ++++++++------ > 4 files changed, 25 insertions(+), 25 deletions(-) > > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c > index 1f7acffc99ba..06e306b49283 100644 > --- a/fs/xfs/xfs_buf_item.c > +++ b/fs/xfs/xfs_buf_item.c > @@ -410,7 +410,6 @@ xfs_buf_item_unpin( > { > struct xfs_buf_log_item *bip = BUF_ITEM(lip); > xfs_buf_t *bp = bip->bli_buf; > - struct xfs_ail *ailp = lip->li_ailp; > int stale = bip->bli_flags & XFS_BLI_STALE; > int freed; > > @@ -452,10 +451,10 @@ xfs_buf_item_unpin( > } > > /* > - * If we get called here because of an IO error, we may > - * or may not have the item on the AIL. xfs_trans_ail_delete() > - * will take care of that situation. > - * xfs_trans_ail_delete() drops the AIL lock. > + * If we get called here because of an IO error, we may or may > + * not have the item on the AIL. xfs_trans_ail_delete() will > + * take care of that situation. xfs_trans_ail_delete() drops > + * the AIL lock. > */ > if (bip->bli_flags & XFS_BLI_STALE_INODE) { > xfs_buf_do_callbacks(bp); > @@ -463,8 +462,7 @@ xfs_buf_item_unpin( > list_del_init(&bp->b_li_list); > bp->b_iodone = NULL; > } else { > - spin_lock(&ailp->ail_lock); > - xfs_trans_ail_delete(ailp, lip, SHUTDOWN_LOG_IO_ERROR); > + xfs_trans_ail_delete(lip, SHUTDOWN_LOG_IO_ERROR); > xfs_buf_item_relse(bp); > ASSERT(bp->b_log_item == NULL); > } > @@ -1205,22 +1203,19 @@ xfs_buf_iodone( > struct xfs_buf *bp, > struct xfs_log_item *lip) > { > - struct xfs_ail *ailp = lip->li_ailp; > - > ASSERT(BUF_ITEM(lip)->bli_buf == bp); > > xfs_buf_rele(bp); > > /* > - * If we are forcibly shutting down, this may well be > - * off the AIL already. That's because we simulate the > - * log-committed callbacks to unpin these buffers. Or we may never > - * have put this item on AIL because of the transaction was > - * aborted forcibly. xfs_trans_ail_delete() takes care of these. > + * If we are forcibly shutting down, this may well be off the AIL > + * already. That's because we simulate the log-committed callbacks to > + * unpin these buffers. Or we may never have put this item on AIL > + * because of the transaction was aborted forcibly. > + * xfs_trans_ail_delete() takes care of these. > * > * Either way, AIL is useless if we're forcing a shutdown. > */ > - spin_lock(&ailp->ail_lock); > - xfs_trans_ail_delete(ailp, lip, SHUTDOWN_CORRUPT_INCORE); > + xfs_trans_ail_delete(lip, SHUTDOWN_CORRUPT_INCORE); > xfs_buf_item_free(BUF_ITEM(lip)); > } > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > index ffe607733c50..5fb65f43b980 100644 > --- a/fs/xfs/xfs_dquot.c > +++ b/fs/xfs/xfs_dquot.c > @@ -1021,6 +1021,7 @@ xfs_qm_dqflush_done( > struct xfs_dq_logitem *qip = (struct xfs_dq_logitem *)lip; > struct xfs_dquot *dqp = qip->qli_dquot; > struct xfs_ail *ailp = lip->li_ailp; > + xfs_lsn_t tail_lsn; > > /* > * We only want to pull the item from the AIL if its > @@ -1034,10 +1035,11 @@ xfs_qm_dqflush_done( > ((lip->li_lsn == qip->qli_flush_lsn) || > test_bit(XFS_LI_FAILED, &lip->li_flags))) { > > - /* xfs_trans_ail_delete() drops the AIL lock. */ > spin_lock(&ailp->ail_lock); > if (lip->li_lsn == qip->qli_flush_lsn) { > - xfs_trans_ail_delete(ailp, lip, SHUTDOWN_CORRUPT_INCORE); > + /* xfs_ail_update_finish() drops the AIL lock */ > + tail_lsn = xfs_ail_delete_one(ailp, lip); > + xfs_ail_update_finish(ailp, tail_lsn); > } else { > /* > * Clear the failed state since we are about to drop the > diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c > index 2574d01e4a83..cfba691664c7 100644 > --- a/fs/xfs/xfs_trans_ail.c > +++ b/fs/xfs/xfs_trans_ail.c > @@ -864,13 +864,14 @@ xfs_ail_delete_one( > */ > void > xfs_trans_ail_delete( > - struct xfs_ail *ailp, > struct xfs_log_item *lip, > int shutdown_type) > { > + struct xfs_ail *ailp = lip->li_ailp; > struct xfs_mount *mp = ailp->ail_mount; > xfs_lsn_t tail_lsn; > > + spin_lock(&ailp->ail_lock); > if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) { > spin_unlock(&ailp->ail_lock); > if (!XFS_FORCED_SHUTDOWN(mp)) { > diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h > index 35655eac01a6..e4362fb8d483 100644 > --- a/fs/xfs/xfs_trans_priv.h > +++ b/fs/xfs/xfs_trans_priv.h > @@ -94,8 +94,7 @@ xfs_trans_ail_update( > xfs_lsn_t xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip); > void xfs_ail_update_finish(struct xfs_ail *ailp, xfs_lsn_t old_lsn) > __releases(ailp->ail_lock); > -void xfs_trans_ail_delete(struct xfs_ail *ailp, struct xfs_log_item *lip, > - int shutdown_type); > +void xfs_trans_ail_delete(struct xfs_log_item *lip, int shutdown_type); > > static inline void > xfs_trans_ail_remove( > @@ -103,13 +102,16 @@ xfs_trans_ail_remove( > int shutdown_type) > { > struct xfs_ail *ailp = lip->li_ailp; > + xfs_lsn_t tail_lsn; > > spin_lock(&ailp->ail_lock); > - /* xfs_trans_ail_delete() drops the AIL lock */ > - if (test_bit(XFS_LI_IN_AIL, &lip->li_flags)) > - xfs_trans_ail_delete(ailp, lip, shutdown_type); > - else > + /* xfs_ail_update_finish() drops the AIL lock */ > + if (test_bit(XFS_LI_IN_AIL, &lip->li_flags)) { > + tail_lsn = xfs_ail_delete_one(ailp, lip); > + xfs_ail_update_finish(ailp, tail_lsn); > + } else { > spin_unlock(&ailp->ail_lock); > + } > } > > void xfs_ail_push(struct xfs_ail *, xfs_lsn_t); >
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c index 1f7acffc99ba..06e306b49283 100644 --- a/fs/xfs/xfs_buf_item.c +++ b/fs/xfs/xfs_buf_item.c @@ -410,7 +410,6 @@ xfs_buf_item_unpin( { struct xfs_buf_log_item *bip = BUF_ITEM(lip); xfs_buf_t *bp = bip->bli_buf; - struct xfs_ail *ailp = lip->li_ailp; int stale = bip->bli_flags & XFS_BLI_STALE; int freed; @@ -452,10 +451,10 @@ xfs_buf_item_unpin( } /* - * If we get called here because of an IO error, we may - * or may not have the item on the AIL. xfs_trans_ail_delete() - * will take care of that situation. - * xfs_trans_ail_delete() drops the AIL lock. + * If we get called here because of an IO error, we may or may + * not have the item on the AIL. xfs_trans_ail_delete() will + * take care of that situation. xfs_trans_ail_delete() drops + * the AIL lock. */ if (bip->bli_flags & XFS_BLI_STALE_INODE) { xfs_buf_do_callbacks(bp); @@ -463,8 +462,7 @@ xfs_buf_item_unpin( list_del_init(&bp->b_li_list); bp->b_iodone = NULL; } else { - spin_lock(&ailp->ail_lock); - xfs_trans_ail_delete(ailp, lip, SHUTDOWN_LOG_IO_ERROR); + xfs_trans_ail_delete(lip, SHUTDOWN_LOG_IO_ERROR); xfs_buf_item_relse(bp); ASSERT(bp->b_log_item == NULL); } @@ -1205,22 +1203,19 @@ xfs_buf_iodone( struct xfs_buf *bp, struct xfs_log_item *lip) { - struct xfs_ail *ailp = lip->li_ailp; - ASSERT(BUF_ITEM(lip)->bli_buf == bp); xfs_buf_rele(bp); /* - * If we are forcibly shutting down, this may well be - * off the AIL already. That's because we simulate the - * log-committed callbacks to unpin these buffers. Or we may never - * have put this item on AIL because of the transaction was - * aborted forcibly. xfs_trans_ail_delete() takes care of these. + * If we are forcibly shutting down, this may well be off the AIL + * already. That's because we simulate the log-committed callbacks to + * unpin these buffers. Or we may never have put this item on AIL + * because of the transaction was aborted forcibly. + * xfs_trans_ail_delete() takes care of these. * * Either way, AIL is useless if we're forcing a shutdown. */ - spin_lock(&ailp->ail_lock); - xfs_trans_ail_delete(ailp, lip, SHUTDOWN_CORRUPT_INCORE); + xfs_trans_ail_delete(lip, SHUTDOWN_CORRUPT_INCORE); xfs_buf_item_free(BUF_ITEM(lip)); } diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c index ffe607733c50..5fb65f43b980 100644 --- a/fs/xfs/xfs_dquot.c +++ b/fs/xfs/xfs_dquot.c @@ -1021,6 +1021,7 @@ xfs_qm_dqflush_done( struct xfs_dq_logitem *qip = (struct xfs_dq_logitem *)lip; struct xfs_dquot *dqp = qip->qli_dquot; struct xfs_ail *ailp = lip->li_ailp; + xfs_lsn_t tail_lsn; /* * We only want to pull the item from the AIL if its @@ -1034,10 +1035,11 @@ xfs_qm_dqflush_done( ((lip->li_lsn == qip->qli_flush_lsn) || test_bit(XFS_LI_FAILED, &lip->li_flags))) { - /* xfs_trans_ail_delete() drops the AIL lock. */ spin_lock(&ailp->ail_lock); if (lip->li_lsn == qip->qli_flush_lsn) { - xfs_trans_ail_delete(ailp, lip, SHUTDOWN_CORRUPT_INCORE); + /* xfs_ail_update_finish() drops the AIL lock */ + tail_lsn = xfs_ail_delete_one(ailp, lip); + xfs_ail_update_finish(ailp, tail_lsn); } else { /* * Clear the failed state since we are about to drop the diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c index 2574d01e4a83..cfba691664c7 100644 --- a/fs/xfs/xfs_trans_ail.c +++ b/fs/xfs/xfs_trans_ail.c @@ -864,13 +864,14 @@ xfs_ail_delete_one( */ void xfs_trans_ail_delete( - struct xfs_ail *ailp, struct xfs_log_item *lip, int shutdown_type) { + struct xfs_ail *ailp = lip->li_ailp; struct xfs_mount *mp = ailp->ail_mount; xfs_lsn_t tail_lsn; + spin_lock(&ailp->ail_lock); if (!test_bit(XFS_LI_IN_AIL, &lip->li_flags)) { spin_unlock(&ailp->ail_lock); if (!XFS_FORCED_SHUTDOWN(mp)) { diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h index 35655eac01a6..e4362fb8d483 100644 --- a/fs/xfs/xfs_trans_priv.h +++ b/fs/xfs/xfs_trans_priv.h @@ -94,8 +94,7 @@ xfs_trans_ail_update( xfs_lsn_t xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip); void xfs_ail_update_finish(struct xfs_ail *ailp, xfs_lsn_t old_lsn) __releases(ailp->ail_lock); -void xfs_trans_ail_delete(struct xfs_ail *ailp, struct xfs_log_item *lip, - int shutdown_type); +void xfs_trans_ail_delete(struct xfs_log_item *lip, int shutdown_type); static inline void xfs_trans_ail_remove( @@ -103,13 +102,16 @@ xfs_trans_ail_remove( int shutdown_type) { struct xfs_ail *ailp = lip->li_ailp; + xfs_lsn_t tail_lsn; spin_lock(&ailp->ail_lock); - /* xfs_trans_ail_delete() drops the AIL lock */ - if (test_bit(XFS_LI_IN_AIL, &lip->li_flags)) - xfs_trans_ail_delete(ailp, lip, shutdown_type); - else + /* xfs_ail_update_finish() drops the AIL lock */ + if (test_bit(XFS_LI_IN_AIL, &lip->li_flags)) { + tail_lsn = xfs_ail_delete_one(ailp, lip); + xfs_ail_update_finish(ailp, tail_lsn); + } else { spin_unlock(&ailp->ail_lock); + } } void xfs_ail_push(struct xfs_ail *, xfs_lsn_t);