xfs
[Top] [All Lists]

Re: [PATCH 5/9] xfs: split out iclog writing from xfs_trans_commit()

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 5/9] xfs: split out iclog writing from xfs_trans_commit()
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Sat, 6 Mar 2010 06:08:33 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1267840284-4652-6-git-send-email-david@xxxxxxxxxxxxx>
References: <1267840284-4652-1-git-send-email-david@xxxxxxxxxxxxx> <1267840284-4652-6-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.19 (2009-01-05)
On Sat, Mar 06, 2010 at 12:51:20PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> Split the the part of xfs_trans_commit() that deals with writing the
> transaction into the iclog into a separate function. This isolates the
> physical commit process from the logical commit operation and makes
> it easier to insert different transaction commit paths without affecting
> the existing algorithm adversely.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>

Looks good, but some comments below:

> +        if (nvec > XFS_TRANS_LOGVEC_COUNT)
> +                kmem_free(log_vector);

These two lines are indented with whitespaces instead of tabs.

> +shut_us_down:
> +     shutdown = XFS_FORCED_SHUTDOWN(mp) ? EIO : 0;
> +     if (!(tp->t_flags & XFS_TRANS_DIRTY) || shutdown) {
> +             xfs_trans_unreserve_and_mod_sb(tp);
> +             /*

This whole area in _xfs_trans_commit is still a complete mess.


What about throwing the patch below in once you're rewriting it anyway?


Index: xfs/fs/xfs/xfs_trans.c
===================================================================
--- xfs.orig/fs/xfs/xfs_trans.c 2010-03-06 12:03:05.567023865 +0100
+++ xfs/fs/xfs/xfs_trans.c      2010-03-06 12:04:47.610255671 +0100
@@ -1020,19 +1020,17 @@ xfs_trans_commit_iclog(
  * have already been unlocked as if the commit had succeeded.
  * Do not reference the transaction structure after this call.
  */
- /*ARGSUSED*/
 int
 _xfs_trans_commit(
-       xfs_trans_t     *tp,
-       uint            flags,
-       int             *log_flushed)
+       struct xfs_trans        *tp,
+       uint                    flags,
+       int                     *log_flushed)
 {
-       xfs_mount_t             *mp = tp->t_mountp;
+       struct xfs_mount        *mp = tp->t_mountp;
        xfs_lsn_t               commit_lsn = -1;
-       int                     error;
+       int                     error = 0;
        int                     log_flags = 0;
        int                     sync = tp->t_flags & XFS_TRANS_SYNC;
-       int                     shutdown;
 
        /*
         * Determine whether this commit is releasing a permanent
@@ -1050,30 +1048,14 @@ _xfs_trans_commit(
         * Also make sure to return any reserved blocks to
         * the free pool.
         */
-shut_us_down:
-       shutdown = XFS_FORCED_SHUTDOWN(mp) ? EIO : 0;
-       if (!(tp->t_flags & XFS_TRANS_DIRTY) || shutdown) {
-               xfs_trans_unreserve_and_mod_sb(tp);
-               /*
-                * It is indeed possible for the transaction to be
-                * not dirty but the dqinfo portion to be. All that
-                * means is that we have some (non-persistent) quota
-                * reservations that need to be unreserved.
-                */
-               xfs_trans_unreserve_and_mod_dquots(tp);
-               if (tp->t_ticket) {
-                       commit_lsn = xfs_log_done(mp, tp->t_ticket,
-                                                       NULL, log_flags);
-                       if (commit_lsn == -1 && !shutdown)
-                               shutdown = XFS_ERROR(EIO);
-               }
-               current_restore_flags_nested(&tp->t_pflags, PF_FSTRANS);
-               xfs_trans_free_items(tp, shutdown? XFS_TRANS_ABORT : 0);
-               xfs_trans_free_busy(tp);
-               xfs_trans_free(tp);
-               XFS_STATS_INC(xs_trans_empty);
-               return (shutdown);
+       if (!(tp->t_flags & XFS_TRANS_DIRTY))
+               goto out_unreserve;
+
+       if (XFS_FORCED_SHUTDOWN(mp)) {
+               error = EIO;
+               goto out_unreserve;
        }
+
        ASSERT(tp->t_ticket != NULL);
 
        /*
@@ -1086,7 +1068,8 @@ shut_us_down:
        error = xfs_trans_commit_iclog(mp, tp, &commit_lsn, flags);
        if (error == ENOMEM) {
                xfs_force_shutdown(mp, SHUTDOWN_LOG_IO_ERROR);
-               goto shut_us_down;
+               error = EIO;
+               goto out_unreserve;
        }
 
        /*
@@ -1103,7 +1086,29 @@ shut_us_down:
                XFS_STATS_INC(xs_trans_async);
        }
 
-       return (error);
+       return error;
+
+out_unreserve:
+       xfs_trans_unreserve_and_mod_sb(tp);
+
+       /*
+        * It is indeed possible for the transaction to be not dirty but
+        * the dqinfo portion to be.  All that means is that we have some
+        * (non-persistent) quota reservations that need to be unreserved.
+        */
+       xfs_trans_unreserve_and_mod_dquots(tp);
+       if (tp->t_ticket) {
+               commit_lsn = xfs_log_done(mp, tp->t_ticket, NULL, log_flags);
+               if (commit_lsn == -1 && !error)
+                       error = XFS_ERROR(EIO);
+       }
+       current_restore_flags_nested(&tp->t_pflags, PF_FSTRANS);
+       xfs_trans_free_items(tp, error ? XFS_TRANS_ABORT : 0);
+       xfs_trans_free_busy(tp);
+       xfs_trans_free(tp);
+
+       XFS_STATS_INC(xs_trans_empty);
+       return error;
 }
 
 /*

<Prev in Thread] Current Thread [Next in Thread>