| Message ID | 20250320075221.1505190-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] xfs: remove the leftover xfs_{set,clear}_li_failed infrastructure | expand |
On Thu, Mar 20, 2025 at 08:52:13AM +0100, Christoph Hellwig wrote: > Marking a log item as failed kept a buffer reference around for > resubmission of inode and dquote items. > > For inode items commit 298f7bec503f3 ("xfs: pin inode backing buffer to > the inode log item") started pinning the inode item buffers > unconditionally and removed the need for this. Later commit acc8f8628c37 > ("xfs: attach dquot buffer to dquot log item buffer") did the same for > dquot items but didn't fully clean up the xfs_clear_li_failed side > for them. Stop adding the extra pin for dquot items and remove the > helpers. > > This happens to fix a call to xfs_buf_free with the AIL lock held, > which would be incorrect for the unlikely case freeing the buffer > ends up calling vfree. > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > fs/xfs/xfs_dquot.c | 3 +-- > fs/xfs/xfs_inode_item.c | 6 ------ > fs/xfs/xfs_trans_ail.c | 5 ++--- > fs/xfs/xfs_trans_priv.h | 28 ---------------------------- > 4 files changed, 3 insertions(+), 39 deletions(-) > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > index edbc521870a1..b4e32f0860b7 100644 > --- a/fs/xfs/xfs_dquot.c > +++ b/fs/xfs/xfs_dquot.c > @@ -1186,9 +1186,8 @@ xfs_qm_dqflush_done( > if (test_bit(XFS_LI_IN_AIL, &lip->li_flags) && > (lip->li_lsn == qlip->qli_flush_lsn || > test_bit(XFS_LI_FAILED, &lip->li_flags))) { > - > spin_lock(&ailp->ail_lock); > - xfs_clear_li_failed(lip); > + clear_bit(XFS_LI_FAILED, &lip->li_flags); > if (lip->li_lsn == qlip->qli_flush_lsn) { > /* xfs_ail_update_finish() drops the AIL lock */ > tail_lsn = xfs_ail_delete_one(ailp, lip); > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c > index 40fc1bf900af..c6cb0b6b9e46 100644 > --- a/fs/xfs/xfs_inode_item.c > +++ b/fs/xfs/xfs_inode_item.c > @@ -1089,13 +1089,7 @@ xfs_iflush_abort( > * state. Whilst the inode is in the AIL, it should have a valid buffer > * pointer for push operations to access - it is only safe to remove the > * inode from the buffer once it has been removed from the AIL. > - * > - * We also clear the failed bit before removing the item from the AIL > - * as xfs_trans_ail_delete()->xfs_clear_li_failed() will release buffer > - * references the inode item owns and needs to hold until we've fully > - * aborted the inode log item and detached it from the buffer. > */ > - clear_bit(XFS_LI_FAILED, &iip->ili_item.li_flags); > xfs_trans_ail_delete(&iip->ili_item, 0); > > /* > diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c > index 0fcb1828e598..85a649fec6ac 100644 > --- a/fs/xfs/xfs_trans_ail.c > +++ b/fs/xfs/xfs_trans_ail.c > @@ -909,10 +909,9 @@ xfs_trans_ail_delete( > return; > } > > - /* xfs_ail_update_finish() drops the AIL lock */ > - xfs_clear_li_failed(lip); > + clear_bit(XFS_LI_FAILED, &lip->li_flags); > tail_lsn = xfs_ail_delete_one(ailp, lip); > - xfs_ail_update_finish(ailp, tail_lsn); > + xfs_ail_update_finish(ailp, tail_lsn); /* drops the AIL lock */ > } > > int > diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h > index bd841df93021..f945f0450b16 100644 > --- a/fs/xfs/xfs_trans_priv.h > +++ b/fs/xfs/xfs_trans_priv.h > @@ -167,32 +167,4 @@ xfs_trans_ail_copy_lsn( > } > #endif > > -static inline void > -xfs_clear_li_failed( > - struct xfs_log_item *lip) > -{ > - struct xfs_buf *bp = lip->li_buf; > - > - ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags)); > - lockdep_assert_held(&lip->li_ailp->ail_lock); > - > - if (test_and_clear_bit(XFS_LI_FAILED, &lip->li_flags)) { > - lip->li_buf = NULL; > - xfs_buf_rele(bp); > - } > -} > - > -static inline void > -xfs_set_li_failed( > - struct xfs_log_item *lip, > - struct xfs_buf *bp) > -{ > - lockdep_assert_held(&lip->li_ailp->ail_lock); > - > - if (!test_and_set_bit(XFS_LI_FAILED, &lip->li_flags)) { > - xfs_buf_hold(bp); > - lip->li_buf = bp; > - } > -} > - > #endif /* __XFS_TRANS_PRIV_H__ */ > -- > 2.45.2 >
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c index edbc521870a1..b4e32f0860b7 100644 --- a/fs/xfs/xfs_dquot.c +++ b/fs/xfs/xfs_dquot.c @@ -1186,9 +1186,8 @@ xfs_qm_dqflush_done( if (test_bit(XFS_LI_IN_AIL, &lip->li_flags) && (lip->li_lsn == qlip->qli_flush_lsn || test_bit(XFS_LI_FAILED, &lip->li_flags))) { - spin_lock(&ailp->ail_lock); - xfs_clear_li_failed(lip); + clear_bit(XFS_LI_FAILED, &lip->li_flags); if (lip->li_lsn == qlip->qli_flush_lsn) { /* xfs_ail_update_finish() drops the AIL lock */ tail_lsn = xfs_ail_delete_one(ailp, lip); diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c index 40fc1bf900af..c6cb0b6b9e46 100644 --- a/fs/xfs/xfs_inode_item.c +++ b/fs/xfs/xfs_inode_item.c @@ -1089,13 +1089,7 @@ xfs_iflush_abort( * state. Whilst the inode is in the AIL, it should have a valid buffer * pointer for push operations to access - it is only safe to remove the * inode from the buffer once it has been removed from the AIL. - * - * We also clear the failed bit before removing the item from the AIL - * as xfs_trans_ail_delete()->xfs_clear_li_failed() will release buffer - * references the inode item owns and needs to hold until we've fully - * aborted the inode log item and detached it from the buffer. */ - clear_bit(XFS_LI_FAILED, &iip->ili_item.li_flags); xfs_trans_ail_delete(&iip->ili_item, 0); /* diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c index 0fcb1828e598..85a649fec6ac 100644 --- a/fs/xfs/xfs_trans_ail.c +++ b/fs/xfs/xfs_trans_ail.c @@ -909,10 +909,9 @@ xfs_trans_ail_delete( return; } - /* xfs_ail_update_finish() drops the AIL lock */ - xfs_clear_li_failed(lip); + clear_bit(XFS_LI_FAILED, &lip->li_flags); tail_lsn = xfs_ail_delete_one(ailp, lip); - xfs_ail_update_finish(ailp, tail_lsn); + xfs_ail_update_finish(ailp, tail_lsn); /* drops the AIL lock */ } int diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h index bd841df93021..f945f0450b16 100644 --- a/fs/xfs/xfs_trans_priv.h +++ b/fs/xfs/xfs_trans_priv.h @@ -167,32 +167,4 @@ xfs_trans_ail_copy_lsn( } #endif -static inline void -xfs_clear_li_failed( - struct xfs_log_item *lip) -{ - struct xfs_buf *bp = lip->li_buf; - - ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags)); - lockdep_assert_held(&lip->li_ailp->ail_lock); - - if (test_and_clear_bit(XFS_LI_FAILED, &lip->li_flags)) { - lip->li_buf = NULL; - xfs_buf_rele(bp); - } -} - -static inline void -xfs_set_li_failed( - struct xfs_log_item *lip, - struct xfs_buf *bp) -{ - lockdep_assert_held(&lip->li_ailp->ail_lock); - - if (!test_and_set_bit(XFS_LI_FAILED, &lip->li_flags)) { - xfs_buf_hold(bp); - lip->li_buf = bp; - } -} - #endif /* __XFS_TRANS_PRIV_H__ */
Marking a log item as failed kept a buffer reference around for resubmission of inode and dquote items. For inode items commit 298f7bec503f3 ("xfs: pin inode backing buffer to the inode log item") started pinning the inode item buffers unconditionally and removed the need for this. Later commit acc8f8628c37 ("xfs: attach dquot buffer to dquot log item buffer") did the same for dquot items but didn't fully clean up the xfs_clear_li_failed side for them. Stop adding the extra pin for dquot items and remove the helpers. This happens to fix a call to xfs_buf_free with the AIL lock held, which would be incorrect for the unlikely case freeing the buffer ends up calling vfree. Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_dquot.c | 3 +-- fs/xfs/xfs_inode_item.c | 6 ------ fs/xfs/xfs_trans_ail.c | 5 ++--- fs/xfs/xfs_trans_priv.h | 28 ---------------------------- 4 files changed, 3 insertions(+), 39 deletions(-)