xfs: split iop_unlock

The iop_unlock method is called when comitting or cancelling a
transaction.  In the latter case, the transaction may or may not be
aborted.  While there is no known problem with the current code in
practice, this implementation is limited in that any log item
implementation that might want to differentiate between a commit and a
cancellation must rely on the aborted state.  The aborted bit is only
set when the cancelled transaction is dirty, however.  This means that
there is no way to distinguish between a commit and a clean transaction
cancellation.

For example, intent log items currently rely on this distinction.  The
log item is either transferred to the CIL on commit or released on
transaction cancel. There is currently no possibility for a clean intent
log item in a transaction, but if that state is ever introduced a cancel
of such a transaction will immediately result in memory leaks of the
associated log item(s).  This is an interface deficiency and landmine.

To clean this up, replace the iop_unlock method with an iop_release
method that is specific to transaction cancel.  The existing
iop_committing method occurs at the same time as iop_unlock in the
commit path and there is no need for two separate callbacks here.
Overload the iop_committing method with the current commit time
iop_unlock implementations to eliminate the need for the latter and
further simplify the interface.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index b2395cb1674cdd4e15d03de6940795b98e427da8..bb5fff120ef529d8c2f183f71eda0430c6457538 100644 (file)
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -120,11 +120,10 @@ xfs_bui_item_unpin(
  * constructed and thus we free the BUI here directly.
  */
 STATIC void
-xfs_bui_item_unlock(
+xfs_bui_item_release(
        struct xfs_log_item     *lip)
 {
-       if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
-               xfs_bui_release(BUI_ITEM(lip));
+       xfs_bui_release(BUI_ITEM(lip));
 }
 
 /*
@@ -134,7 +133,7 @@ static const struct xfs_item_ops xfs_bui_item_ops = {
        .iop_size       = xfs_bui_item_size,
        .iop_format     = xfs_bui_item_format,
        .iop_unpin      = xfs_bui_item_unpin,
-       .iop_unlock     = xfs_bui_item_unlock,
+       .iop_release    = xfs_bui_item_release,
 };
 
 /*
@@ -201,15 +200,13 @@ xfs_bud_item_format(
  * BUD.
  */
 STATIC void
-xfs_bud_item_unlock(
+xfs_bud_item_release(
        struct xfs_log_item     *lip)
 {
        struct xfs_bud_log_item *budp = BUD_ITEM(lip);
 
-       if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
-               xfs_bui_release(budp->bud_buip);
-               kmem_zone_free(xfs_bud_zone, budp);
-       }
+       xfs_bui_release(budp->bud_buip);
+       kmem_zone_free(xfs_bud_zone, budp);
 }
 
 /*
@@ -243,7 +240,7 @@ xfs_bud_item_committed(
 static const struct xfs_item_ops xfs_bud_item_ops = {
        .iop_size       = xfs_bud_item_size,
        .iop_format     = xfs_bud_item_format,
-       .iop_unlock     = xfs_bud_item_unlock,
+       .iop_release    = xfs_bud_item_release,
        .iop_committed  = xfs_bud_item_committed,
 };
 

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index b6c684ea56c151b29ca4999519aebab925035fa3..e9da8274bf31bc3ba2a8f086a3795bdd1c02d121 100644 (file)
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -595,7 +595,7 @@ xfs_buf_item_put(
  * free the item.
  */
 STATIC void
-xfs_buf_item_unlock(
+xfs_buf_item_release(
        struct xfs_log_item     *lip)
 {
        struct xfs_buf_log_item *bip = BUF_ITEM(lip);
@@ -610,7 +610,7 @@ xfs_buf_item_unlock(
                                                   &lip->li_flags);
 #endif
 
-       trace_xfs_buf_item_unlock(bip);
+       trace_xfs_buf_item_release(bip);
 
        /*
         * The bli dirty state should match whether the blf has logged segments
@@ -640,6 +640,14 @@ xfs_buf_item_unlock(
        xfs_buf_relse(bp);
 }
 
+STATIC void
+xfs_buf_item_committing(
+       struct xfs_log_item     *lip,
+       xfs_lsn_t               commit_lsn)
+{
+       return xfs_buf_item_release(lip);
+}
+
 /*
  * This is called to find out where the oldest active copy of the
  * buf log item in the on disk log resides now that the last log
@@ -680,7 +688,8 @@ static const struct xfs_item_ops xfs_buf_item_ops = {
        .iop_format     = xfs_buf_item_format,
        .iop_pin        = xfs_buf_item_pin,
        .iop_unpin      = xfs_buf_item_unpin,
-       .iop_unlock     = xfs_buf_item_unlock,
+       .iop_release    = xfs_buf_item_release,
+       .iop_committing = xfs_buf_item_committing,
        .iop_committed  = xfs_buf_item_committed,
        .iop_push       = xfs_buf_item_push,
 };

diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index 98b0d732ef534afbf9cb651bd0e8bcfe7fa80124..334f6ca1cefccdf608ac89a381df4f05695b32ea 100644 (file)
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -198,14 +198,8 @@ out_unlock:
        return rval;
 }
 
-/*
- * Unlock the dquot associated with the log item.
- * Clear the fields of the dquot and dquot log item that
- * are specific to the current transaction.  If the
- * hold flags is set, do not unlock the dquot.
- */
 STATIC void
-xfs_qm_dquot_logitem_unlock(
+xfs_qm_dquot_logitem_release(
        struct xfs_log_item     *lip)
 {
        struct xfs_dquot        *dqp = DQUOT_ITEM(lip)->qli_dquot;
@@ -221,6 +215,14 @@ xfs_qm_dquot_logitem_unlock(
        xfs_dqunlock(dqp);
 }
 
+STATIC void
+xfs_qm_dquot_logitem_committing(
+       struct xfs_log_item     *lip,
+       xfs_lsn_t               commit_lsn)
+{
+       return xfs_qm_dquot_logitem_release(lip);
+}
+
 /*
  * This is the ops vector for dquots
  */
@@ -229,7 +231,8 @@ static const struct xfs_item_ops xfs_dquot_item_ops = {
        .iop_format     = xfs_qm_dquot_logitem_format,
        .iop_pin        = xfs_qm_dquot_logitem_pin,
        .iop_unpin      = xfs_qm_dquot_logitem_unpin,
-       .iop_unlock     = xfs_qm_dquot_logitem_unlock,
+       .iop_release    = xfs_qm_dquot_logitem_release,
+       .iop_committing = xfs_qm_dquot_logitem_committing,
        .iop_push       = xfs_qm_dquot_logitem_push,
        .iop_error      = xfs_dquot_item_error
 };

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index c1ab3a056bb85eac123eb9cbf2161778b989ae69..70df190079950d962de4f540cc6c86b2f629f4a1 100644 (file)
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -130,11 +130,10 @@ xfs_efi_item_unpin(
  * constructed and thus we free the EFI here directly.
  */
 STATIC void
-xfs_efi_item_unlock(
+xfs_efi_item_release(
        struct xfs_log_item     *lip)
 {
-       if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
-               xfs_efi_release(EFI_ITEM(lip));
+       xfs_efi_release(EFI_ITEM(lip));
 }
 
 /*
@@ -144,7 +143,7 @@ static const struct xfs_item_ops xfs_efi_item_ops = {
        .iop_size       = xfs_efi_item_size,
        .iop_format     = xfs_efi_item_format,
        .iop_unpin      = xfs_efi_item_unpin,
-       .iop_unlock     = xfs_efi_item_unlock,
+       .iop_release    = xfs_efi_item_release,
 };
 
 
@@ -300,15 +299,13 @@ xfs_efd_item_format(
  * the transaction is cancelled, drop our reference to the EFI and free the EFD.
  */
 STATIC void
-xfs_efd_item_unlock(
+xfs_efd_item_release(
        struct xfs_log_item     *lip)
 {
        struct xfs_efd_log_item *efdp = EFD_ITEM(lip);
 
-       if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
-               xfs_efi_release(efdp->efd_efip);
-               xfs_efd_item_free(efdp);
-       }
+       xfs_efi_release(efdp->efd_efip);
+       xfs_efd_item_free(efdp);
 }
 
 /*
@@ -342,7 +339,7 @@ xfs_efd_item_committed(
 static const struct xfs_item_ops xfs_efd_item_ops = {
        .iop_size       = xfs_efd_item_size,
        .iop_format     = xfs_efd_item_format,
-       .iop_unlock     = xfs_efd_item_unlock,
+       .iop_release    = xfs_efd_item_release,
        .iop_committed  = xfs_efd_item_committed,
 };
 

diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c
index cbaabc55f0c934c8696712c4755a48e761a5ee95..9aceb35dce24687a0d2d5d461510547e22187f38 100644 (file)
--- a/fs/xfs/xfs_icreate_item.c
+++ b/fs/xfs/xfs_icreate_item.c
@@ -57,14 +57,10 @@ xfs_icreate_item_format(
 }
 
 STATIC void
-xfs_icreate_item_unlock(
+xfs_icreate_item_release(
        struct xfs_log_item     *lip)
 {
-       struct xfs_icreate_item *icp = ICR_ITEM(lip);
-
-       if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
-               kmem_zone_free(xfs_icreate_zone, icp);
-       return;
+       kmem_zone_free(xfs_icreate_zone, ICR_ITEM(lip));
 }
 
 /*
@@ -89,7 +85,7 @@ xfs_icreate_item_committed(
 static const struct xfs_item_ops xfs_icreate_item_ops = {
        .iop_size       = xfs_icreate_item_size,
        .iop_format     = xfs_icreate_item_format,
-       .iop_unlock     = xfs_icreate_item_unlock,
+       .iop_release    = xfs_icreate_item_release,
        .iop_committed  = xfs_icreate_item_committed,
 };
 

diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 957f3dca0d7a5d55eb28a2cce6cf6078bb03efe5..e152b3bb32725c9da69bc5909bb4cacf2f81e3c9 100644 (file)
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -566,7 +566,7 @@ out_unlock:
  * Unlock the inode associated with the inode log item.
  */
 STATIC void
-xfs_inode_item_unlock(
+xfs_inode_item_release(
        struct xfs_log_item     *lip)
 {
        struct xfs_inode_log_item *iip = INODE_ITEM(lip);
@@ -622,9 +622,10 @@ xfs_inode_item_committed(
 STATIC void
 xfs_inode_item_committing(
        struct xfs_log_item     *lip,
-       xfs_lsn_t               lsn)
+       xfs_lsn_t               commit_lsn)
 {
-       INODE_ITEM(lip)->ili_last_lsn = lsn;
+       INODE_ITEM(lip)->ili_last_lsn = commit_lsn;
+       return xfs_inode_item_release(lip);
 }
 
 /*
@@ -635,10 +636,10 @@ static const struct xfs_item_ops xfs_inode_item_ops = {
        .iop_format     = xfs_inode_item_format,
        .iop_pin        = xfs_inode_item_pin,
        .iop_unpin      = xfs_inode_item_unpin,
-       .iop_unlock     = xfs_inode_item_unlock,
+       .iop_release    = xfs_inode_item_release,
        .iop_committed  = xfs_inode_item_committed,
        .iop_push       = xfs_inode_item_push,
-       .iop_committing = xfs_inode_item_committing,
+       .iop_committing = xfs_inode_item_committing,
        .iop_error      = xfs_inode_item_error
 };
 

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index c856bfce5bf2077fad0efa94673ffd70bf9ffd38..4cb459f21ad41f49b3489c5cc43d75c632c011e3 100644 (file)
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -1024,8 +1024,6 @@ xfs_log_commit_cil(
                xfs_trans_del_item(lip);
                if (lip->li_ops->iop_committing)
                        lip->li_ops->iop_committing(lip, xc_commit_lsn);
-               if (lip->li_ops->iop_unlock)
-                       lip->li_ops->iop_unlock(lip);
        }
        xlog_cil_push_background(log);
 

diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 03a61886fe2aba6f24255655f5ce35117f1b882e..9f8fb23dcc8105e83ab3b805c24588790f39fedd 100644 (file)
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -118,11 +118,10 @@ xfs_cui_item_unpin(
  * constructed and thus we free the CUI here directly.
  */
 STATIC void
-xfs_cui_item_unlock(
+xfs_cui_item_release(
        struct xfs_log_item     *lip)
 {
-       if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
-               xfs_cui_release(CUI_ITEM(lip));
+       xfs_cui_release(CUI_ITEM(lip));
 }
 
 /*
@@ -132,7 +131,7 @@ static const struct xfs_item_ops xfs_cui_item_ops = {
        .iop_size       = xfs_cui_item_size,
        .iop_format     = xfs_cui_item_format,
        .iop_unpin      = xfs_cui_item_unpin,
-       .iop_unlock     = xfs_cui_item_unlock,
+       .iop_release    = xfs_cui_item_release,
 };
 
 /*
@@ -205,15 +204,13 @@ xfs_cud_item_format(
  * CUD.
  */
 STATIC void
-xfs_cud_item_unlock(
+xfs_cud_item_release(
        struct xfs_log_item     *lip)
 {
        struct xfs_cud_log_item *cudp = CUD_ITEM(lip);
 
-       if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
-               xfs_cui_release(cudp->cud_cuip);
-               kmem_zone_free(xfs_cud_zone, cudp);
-       }
+       xfs_cui_release(cudp->cud_cuip);
+       kmem_zone_free(xfs_cud_zone, cudp);
 }
 
 /*
@@ -247,7 +244,7 @@ xfs_cud_item_committed(
 static const struct xfs_item_ops xfs_cud_item_ops = {
        .iop_size       = xfs_cud_item_size,
        .iop_format     = xfs_cud_item_format,
-       .iop_unlock     = xfs_cud_item_unlock,
+       .iop_release    = xfs_cud_item_release,
        .iop_committed  = xfs_cud_item_committed,
 };
 

diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index df9f2505c5f330828fdf91ee6f21c17f92f0c02d..e907bd169de5731ce759bcee8789d6f3a708fa65 100644 (file)
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -117,11 +117,10 @@ xfs_rui_item_unpin(
  * constructed and thus we free the RUI here directly.
  */
 STATIC void
-xfs_rui_item_unlock(
+xfs_rui_item_release(
        struct xfs_log_item     *lip)
 {
-       if (test_bit(XFS_LI_ABORTED, &lip->li_flags))
-               xfs_rui_release(RUI_ITEM(lip));
+       xfs_rui_release(RUI_ITEM(lip));
 }
 
 /*
@@ -131,7 +130,7 @@ static const struct xfs_item_ops xfs_rui_item_ops = {
        .iop_size       = xfs_rui_item_size,
        .iop_format     = xfs_rui_item_format,
        .iop_unpin      = xfs_rui_item_unpin,
-       .iop_unlock     = xfs_rui_item_unlock,
+       .iop_release    = xfs_rui_item_release,
 };
 
 /*
@@ -226,15 +225,13 @@ xfs_rud_item_format(
  * RUD.
  */
 STATIC void
-xfs_rud_item_unlock(
+xfs_rud_item_release(
        struct xfs_log_item     *lip)
 {
        struct xfs_rud_log_item *rudp = RUD_ITEM(lip);
 
-       if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
-               xfs_rui_release(rudp->rud_ruip);
-               kmem_zone_free(xfs_rud_zone, rudp);
-       }
+       xfs_rui_release(rudp->rud_ruip);
+       kmem_zone_free(xfs_rud_zone, rudp);
 }
 
 /*
@@ -268,7 +265,7 @@ xfs_rud_item_committed(
 static const struct xfs_item_ops xfs_rud_item_ops = {
        .iop_size       = xfs_rud_item_size,
        .iop_format     = xfs_rud_item_format,
-       .iop_unlock     = xfs_rud_item_unlock,
+       .iop_release    = xfs_rud_item_release,
        .iop_committed  = xfs_rud_item_committed,
 };
 

diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 195a9cdb954e314d91bd0d270e3d1102f05da6e2..65c920554b96a01476d89df843430013594f05b6 100644 (file)
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -475,7 +475,7 @@ DEFINE_BUF_ITEM_EVENT(xfs_buf_item_ordered);
 DEFINE_BUF_ITEM_EVENT(xfs_buf_item_pin);
 DEFINE_BUF_ITEM_EVENT(xfs_buf_item_unpin);
 DEFINE_BUF_ITEM_EVENT(xfs_buf_item_unpin_stale);
-DEFINE_BUF_ITEM_EVENT(xfs_buf_item_unlock);
+DEFINE_BUF_ITEM_EVENT(xfs_buf_item_release);
 DEFINE_BUF_ITEM_EVENT(xfs_buf_item_committed);
 DEFINE_BUF_ITEM_EVENT(xfs_buf_item_push);
 DEFINE_BUF_ITEM_EVENT(xfs_trans_get_buf);

diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 003d916904d296e1debf128fbea7a6b5ed789126..261c51486d7abb2a2aae3e9151bf98c2ea6cb274 100644 (file)
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -780,9 +780,8 @@ xfs_trans_free_items(
                xfs_trans_del_item(lip);
                if (abort)
                        set_bit(XFS_LI_ABORTED, &lip->li_flags);
-
-               if (lip->li_ops->iop_unlock)
-                       lip->li_ops->iop_unlock(lip);
+               if (lip->li_ops->iop_release)
+                       lip->li_ops->iop_release(lip);
        }
 }
 
@@ -815,7 +814,7 @@ xfs_log_item_batch_insert(
  *
  * If we are called with the aborted flag set, it is because a log write during
  * a CIL checkpoint commit has failed. In this case, all the items in the
- * checkpoint have already gone through iop_committed and iop_unlock, which
+ * checkpoint have already gone through iop_committed and iop_committing, which
  * means that checkpoint commit abort handling is treated exactly the same
  * as an iclog write error even though we haven't started any IO yet. Hence in
  * this case all we need to do is iop_committed processing, followed by an

diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index fd35da161a2b16a9cd45078fabc99dedf01d3e37..522c1d565aa412c152a2afb837d5892a1740ab7d 100644 (file)
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -72,9 +72,9 @@ struct xfs_item_ops {
        void (*iop_pin)(xfs_log_item_t *);
        void (*iop_unpin)(xfs_log_item_t *, int remove);
        uint (*iop_push)(struct xfs_log_item *, struct list_head *);
-       void (*iop_unlock)(xfs_log_item_t *);
+       void (*iop_committing)(struct xfs_log_item *, xfs_lsn_t commit_lsn);
+       void (*iop_release)(struct xfs_log_item *);
        xfs_lsn_t (*iop_committed)(xfs_log_item_t *, xfs_lsn_t);
-       void (*iop_committing)(xfs_log_item_t *, xfs_lsn_t);
        void (*iop_error)(xfs_log_item_t *, xfs_buf_t *);
 };
 

diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index a1764a1dbd996f896f315f61567b5ca5bfdbc30a..0cdd8a314d85bc9f0902da5569ed617098dd1975 100644 (file)
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -427,7 +427,7 @@ xfs_trans_brelse(
 
 /*
  * Mark the buffer as not needing to be unlocked when the buf item's
- * iop_unlock() routine is called.  The buffer must already be locked
+ * iop_committing() routine is called.  The buffer must already be locked
  * and associated with the given transaction.
  */
 /* ARGSUSED */