[PATCH 0/7] decouple the in-memory from the on-disk log format

[PATCH 0/7] decouple the in-memory from the on-disk log format

[Christoph Hellwig]

Since the introduction of the CIL we already have a layer of indirection
between the physical log format and the data structure tracking the
changes in memory.  But due to the way iop_format works we are still
forced to keep a copy of everything that goes out to the log in memory
even before copying it into the CIL.

The first patch in this series changes iop_format so that the log items
are free to store their in-memory data however they want before formatting
them into the CIL, and the other patches take advantage of that by not
keeping most log formats in memory all the time.  Especially the EFI and
EFD related ones at the end start to show the benefit.

What's missing from this series are larger changes to the in-core inode
layout.  No needing the full struct icdinode at all times will be the
biggest benefit of this change, but it will be large enough series of it's
own.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/7] xfs: let iop_format write directly into the linear buffer

[Christoph Hellwig]

[-- Attachment #1: 0001-xfs-let-iop_format-write-directly-into-the-linear-bu.patch --]
[-- Type: text/plain, Size: 26219 bytes --]

Instead of setting up pointers to memory locations in iop_format which then
get copied into the CIL linear buffer after return move the copy into
the individual inode items.  This avoids the need to always have a memory
block in the exact same layout that gets written into the log around, and
allow the log items to be much more flexible in their in-memory layouts.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf_item.c     |   89 ++++++++++++-------
 fs/xfs/xfs_dquot_item.c   |   35 +++++---
 fs/xfs/xfs_extfree_item.c |   33 ++++---
 fs/xfs/xfs_icreate_item.c |   12 ++-
 fs/xfs/xfs_inode_fork.c   |   15 ++--
 fs/xfs/xfs_inode_item.c   |  209 ++++++++++++++++-----------------------------
 fs/xfs/xfs_inode_item.h   |    4 -
 fs/xfs/xfs_log.h          |   30 +++++++
 fs/xfs/xfs_log_cil.c      |   35 +-------
 fs/xfs/xfs_trans.h        |    2 +-
 10 files changed, 222 insertions(+), 242 deletions(-)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index a64f67b..067a840 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -182,10 +182,44 @@ xfs_buf_item_size(
 	trace_xfs_buf_item_size(bip);
 }
 
+static inline struct xfs_log_iovec *
+xfs_buf_item_set_iovec(
+	struct xfs_log_vec	*lv,
+	struct xfs_log_iovec	*vec,
+	struct xfs_buf		*bp,
+	uint			offset,
+	int			first_bit,
+	uint			len)
+{
+	vec = xlog_next_iovec(lv, vec);
+
+	offset += first_bit * XFS_BLF_CHUNK;
+	len *= XFS_BLF_CHUNK;
+
+	memcpy(vec->i_addr, xfs_buf_offset(bp, offset), len);
+	vec->i_len = len;
+	vec->i_type = XLOG_REG_TYPE_BCHUNK;
+
+	return vec;
+}
+
+static inline bool
+xfs_buf_item_straddle(
+	struct xfs_buf		*bp,
+	uint			offset,
+	int			next_bit,
+	int			last_bit)
+{
+	return xfs_buf_offset(bp, offset + (next_bit << XFS_BLF_SHIFT)) !=
+		(xfs_buf_offset(bp, offset + (last_bit << XFS_BLF_SHIFT)) +
+		 XFS_BLF_CHUNK);
+}
+
 static struct xfs_log_iovec *
 xfs_buf_item_format_segment(
 	struct xfs_buf_log_item	*bip,
-	struct xfs_log_iovec	*vecp,
+	struct xfs_log_vec	*lv,
+	struct xfs_log_iovec	*vec,
 	uint			offset,
 	struct xfs_buf_log_format *blfp)
 {
@@ -196,7 +230,6 @@ xfs_buf_item_format_segment(
 	int		last_bit;
 	int		next_bit;
 	uint		nbits;
-	uint		buffer_offset;
 
 	/* copy the flags across from the base format item */
 	blfp->blf_flags = bip->__bli_format.blf_flags;
@@ -218,10 +251,14 @@ xfs_buf_item_format_segment(
 		goto out;
 	}
 
-	vecp->i_addr = blfp;
-	vecp->i_len = base_size;
-	vecp->i_type = XLOG_REG_TYPE_BFORMAT;
-	vecp++;
+	if (vec)
+		vec = xlog_next_iovec(lv, vec);
+	else
+		vec = xlog_first_iovec(lv);
+
+	blfp = memcpy(vec->i_addr, blfp, base_size);
+	vec->i_len = base_size;
+	vec->i_type = XLOG_REG_TYPE_BFORMAT;
 	nvecs = 1;
 
 	if (bip->bli_flags & XFS_BLI_STALE) {
@@ -261,33 +298,16 @@ xfs_buf_item_format_segment(
 		 * keep counting and scanning.
 		 */
 		if (next_bit == -1) {
-			buffer_offset = offset + first_bit * XFS_BLF_CHUNK;
-			vecp->i_addr = xfs_buf_offset(bp, buffer_offset);
-			vecp->i_len = nbits * XFS_BLF_CHUNK;
-			vecp->i_type = XLOG_REG_TYPE_BCHUNK;
+			vec = xfs_buf_item_set_iovec(lv, vec, bp, offset,
+						     first_bit, nbits);
 			nvecs++;
 			break;
-		} else if (next_bit != last_bit + 1) {
-			buffer_offset = offset + first_bit * XFS_BLF_CHUNK;
-			vecp->i_addr = xfs_buf_offset(bp, buffer_offset);
-			vecp->i_len = nbits * XFS_BLF_CHUNK;
-			vecp->i_type = XLOG_REG_TYPE_BCHUNK;
+		} else if (next_bit != last_bit + 1 ||
+		           xfs_buf_item_straddle(bp, offset, next_bit, last_bit)) {
+			vec = xfs_buf_item_set_iovec(lv, vec, bp, offset,
+						     first_bit, nbits);
 			nvecs++;
-			vecp++;
-			first_bit = next_bit;
-			last_bit = next_bit;
-			nbits = 1;
-		} else if (xfs_buf_offset(bp, offset +
-					      (next_bit << XFS_BLF_SHIFT)) !=
-			   (xfs_buf_offset(bp, offset +
-					       (last_bit << XFS_BLF_SHIFT)) +
-			    XFS_BLF_CHUNK)) {
-			buffer_offset = offset + first_bit * XFS_BLF_CHUNK;
-			vecp->i_addr = xfs_buf_offset(bp, buffer_offset);
-			vecp->i_len = nbits * XFS_BLF_CHUNK;
-			vecp->i_type = XLOG_REG_TYPE_BCHUNK;
-			nvecs++;
-			vecp++;
+
 			first_bit = next_bit;
 			last_bit = next_bit;
 			nbits = 1;
@@ -298,7 +318,7 @@ xfs_buf_item_format_segment(
 	}
 out:
 	blfp->blf_size = nvecs;
-	return vecp;
+	return vec;
 }
 
 /*
@@ -310,10 +330,11 @@ out:
 STATIC void
 xfs_buf_item_format(
 	struct xfs_log_item	*lip,
-	struct xfs_log_iovec	*vecp)
+	struct xfs_log_vec	*lv)
 {
 	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
 	struct xfs_buf		*bp = bip->bli_buf;
+	struct xfs_log_iovec	*vec = NULL;
 	uint			offset = 0;
 	int			i;
 
@@ -354,11 +375,13 @@ xfs_buf_item_format(
 	}
 
 	for (i = 0; i < bip->bli_format_count; i++) {
-		vecp = xfs_buf_item_format_segment(bip, vecp, offset,
+		vec = xfs_buf_item_format_segment(bip, lv, vec, offset,
 						&bip->bli_formats[i]);
 		offset += bp->b_maps[i].bm_len;
 	}
 
+	xlog_last_iovec(lv, vec);
+
 	/*
 	 * Check to make sure everything is consistent.
 	 */
diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index 92e5f62..5f446a7 100644
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -57,20 +57,24 @@ xfs_qm_dquot_logitem_size(
 STATIC void
 xfs_qm_dquot_logitem_format(
 	struct xfs_log_item	*lip,
-	struct xfs_log_iovec	*logvec)
+	struct xfs_log_vec	*lv)
 {
 	struct xfs_dq_logitem	*qlip = DQUOT_ITEM(lip);
-
-	logvec->i_addr = &qlip->qli_format;
-	logvec->i_len  = sizeof(xfs_dq_logformat_t);
-	logvec->i_type = XLOG_REG_TYPE_QFORMAT;
-	logvec++;
-	logvec->i_addr = &qlip->qli_dquot->q_core;
-	logvec->i_len  = sizeof(xfs_disk_dquot_t);
-	logvec->i_type = XLOG_REG_TYPE_DQUOT;
+	struct xfs_log_iovec	*vec;
 
 	qlip->qli_format.qlf_size = 2;
 
+	vec = xlog_first_iovec(lv);
+	memcpy(vec->i_addr, &qlip->qli_format, sizeof(xfs_dq_logformat_t));
+	vec->i_len = sizeof(xfs_dq_logformat_t);
+	vec->i_type = XLOG_REG_TYPE_QFORMAT;
+
+	vec = xlog_next_iovec(lv, vec);
+	memcpy(vec->i_addr, &qlip->qli_dquot->q_core, sizeof(xfs_disk_dquot_t));
+	vec->i_len = sizeof(xfs_disk_dquot_t);
+	vec->i_type = XLOG_REG_TYPE_DQUOT;
+
+	xlog_last_iovec(lv, vec);
 }
 
 /*
@@ -304,16 +308,21 @@ xfs_qm_qoff_logitem_size(
 STATIC void
 xfs_qm_qoff_logitem_format(
 	struct xfs_log_item	*lip,
-	struct xfs_log_iovec	*log_vector)
+	struct xfs_log_vec	*lv)
 {
 	struct xfs_qoff_logitem	*qflip = QOFF_ITEM(lip);
+	struct xfs_log_iovec *vec;
 
 	ASSERT(qflip->qql_format.qf_type == XFS_LI_QUOTAOFF);
 
-	log_vector->i_addr = &qflip->qql_format;
-	log_vector->i_len = sizeof(xfs_qoff_logitem_t);
-	log_vector->i_type = XLOG_REG_TYPE_QUOTAOFF;
 	qflip->qql_format.qf_size = 1;
+
+	vec = xlog_first_iovec(lv);
+	memcpy(vec->i_addr, &qflip->qql_format, sizeof(xfs_qoff_logitem_t));
+	vec->i_len = sizeof(xfs_qoff_logitem_t);
+	vec->i_type = XLOG_REG_TYPE_QUOTAOFF;
+
+	xlog_last_iovec(lv, vec);
 }
 
 /*
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 3680d04..94bf5e7 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -26,6 +26,7 @@
 #include "xfs_trans_priv.h"
 #include "xfs_buf_item.h"
 #include "xfs_extfree_item.h"
+#include "xfs_log.h"
 
 
 kmem_zone_t	*xfs_efi_zone;
@@ -101,9 +102,11 @@ xfs_efi_item_size(
 STATIC void
 xfs_efi_item_format(
 	struct xfs_log_item	*lip,
-	struct xfs_log_iovec	*log_vector)
+	struct xfs_log_vec	*lv)
 {
 	struct xfs_efi_log_item	*efip = EFI_ITEM(lip);
+	struct xfs_log_iovec	*vec;
+	int			size;
 
 	ASSERT(atomic_read(&efip->efi_next_extent) ==
 				efip->efi_format.efi_nextents);
@@ -111,12 +114,16 @@ xfs_efi_item_format(
 	efip->efi_format.efi_type = XFS_LI_EFI;
 	efip->efi_format.efi_size = 1;
 
-	log_vector->i_addr = &efip->efi_format;
-	log_vector->i_len = xfs_efi_item_sizeof(efip);
-	log_vector->i_type = XLOG_REG_TYPE_EFI_FORMAT;
-	ASSERT(log_vector->i_len >= sizeof(xfs_efi_log_format_t));
-}
+	size = xfs_efi_item_sizeof(efip);
+	ASSERT(size >= sizeof(xfs_efi_log_format_t));
+
+	vec = xlog_first_iovec(lv);
+	memcpy(vec->i_addr, &efip->efi_format, size);
+	vec->i_len = size;
+	vec->i_type = XLOG_REG_TYPE_EFI_FORMAT;
 
+	xlog_last_iovec(lv, vec);
+}
 
 /*
  * Pinning has no meaning for an efi item, so just return.
@@ -368,19 +375,23 @@ xfs_efd_item_size(
 STATIC void
 xfs_efd_item_format(
 	struct xfs_log_item	*lip,
-	struct xfs_log_iovec	*log_vector)
+	struct xfs_log_vec	*lv)
 {
 	struct xfs_efd_log_item	*efdp = EFD_ITEM(lip);
+	struct xfs_log_iovec	*vec;
 
 	ASSERT(efdp->efd_next_extent == efdp->efd_format.efd_nextents);
 
 	efdp->efd_format.efd_type = XFS_LI_EFD;
 	efdp->efd_format.efd_size = 1;
 
-	log_vector->i_addr = &efdp->efd_format;
-	log_vector->i_len = xfs_efd_item_sizeof(efdp);
-	log_vector->i_type = XLOG_REG_TYPE_EFD_FORMAT;
-	ASSERT(log_vector->i_len >= sizeof(xfs_efd_log_format_t));
+	vec = xlog_first_iovec(lv);
+	memcpy(vec->i_addr, &efdp->efd_format, xfs_efd_item_sizeof(efdp));
+	vec->i_len = xfs_efd_item_sizeof(efdp);
+	vec->i_type = XLOG_REG_TYPE_EFD_FORMAT;
+	ASSERT(vec->i_len >= sizeof(xfs_efd_log_format_t));
+
+	xlog_last_iovec(lv, vec);
 }
 
 /*
diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c
index d2eaccf..f6115c3 100644
--- a/fs/xfs/xfs_icreate_item.c
+++ b/fs/xfs/xfs_icreate_item.c
@@ -28,6 +28,7 @@
 #include "xfs_trans_priv.h"
 #include "xfs_error.h"
 #include "xfs_icreate_item.h"
+#include "xfs_log.h"
 
 kmem_zone_t	*xfs_icreate_zone;		/* inode create item zone */
 
@@ -58,13 +59,16 @@ xfs_icreate_item_size(
 STATIC void
 xfs_icreate_item_format(
 	struct xfs_log_item	*lip,
-	struct xfs_log_iovec	*log_vector)
+	struct xfs_log_vec	*lv)
 {
 	struct xfs_icreate_item	*icp = ICR_ITEM(lip);
+	struct xfs_log_iovec *vec;
 
-	log_vector->i_addr = (xfs_caddr_t)&icp->ic_format;
-	log_vector->i_len  = sizeof(struct xfs_icreate_log);
-	log_vector->i_type = XLOG_REG_TYPE_ICREATE;
+	vec = xlog_first_iovec(lv);
+	memcpy(vec->i_addr, &icp->ic_format, sizeof(struct xfs_icreate_log));
+	vec->i_len  = sizeof(struct xfs_icreate_log);
+	vec->i_type = XLOG_REG_TYPE_ICREATE;
+	xlog_last_iovec(lv, vec);
 }
 
 
diff --git a/fs/xfs/xfs_inode_fork.c b/fs/xfs/xfs_inode_fork.c
index cfee14a..06abaee 100644
--- a/fs/xfs/xfs_inode_fork.c
+++ b/fs/xfs/xfs_inode_fork.c
@@ -721,15 +721,16 @@ xfs_idestroy_fork(
 }
 
 /*
- * xfs_iextents_copy()
+ * Convert in-core extents to on-disk form
  *
- * This is called to copy the REAL extents (as opposed to the delayed
- * allocation extents) from the inode into the given buffer.  It
- * returns the number of bytes copied into the buffer.
+ * For either the data or attr fork in extent format, we need to endian convert
+ * the in-core extent as we place them into the on-disk inode.
  *
- * If there are no delayed allocation extents, then we can just
- * memcpy() the extents into the buffer.  Otherwise, we need to
- * examine each extent in turn and skip those which are delayed.
+ * In the case of the data fork, the in-core and on-disk fork sizes can be
+ * different due to delayed allocation extents. We only copy on-disk extents
+ * here, so callers must always use the physical fork size to determine the
+ * size of the buffer passed to this routine.  We will return the size actually
+ * used.
  */
 int
 xfs_iextents_copy(
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 7c0d391f..fbdcde1 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -30,6 +30,7 @@
 #include "xfs_trace.h"
 #include "xfs_trans_priv.h"
 #include "xfs_dinode.h"
+#include "xfs_log.h"
 
 
 kmem_zone_t	*xfs_ili_zone;		/* inode log item zone */
@@ -137,41 +138,6 @@ xfs_inode_item_size(
 }
 
 /*
- * xfs_inode_item_format_extents - convert in-core extents to on-disk form
- *
- * For either the data or attr fork in extent format, we need to endian convert
- * the in-core extent as we place them into the on-disk inode. In this case, we
- * need to do this conversion before we write the extents into the log. Because
- * we don't have the disk inode to write into here, we allocate a buffer and
- * format the extents into it via xfs_iextents_copy(). We free the buffer in
- * the unlock routine after the copy for the log has been made.
- *
- * In the case of the data fork, the in-core and on-disk fork sizes can be
- * different due to delayed allocation extents. We only log on-disk extents
- * here, so always use the physical fork size to determine the size of the
- * buffer we need to allocate.
- */
-STATIC void
-xfs_inode_item_format_extents(
-	struct xfs_inode	*ip,
-	struct xfs_log_iovec	*vecp,
-	int			whichfork,
-	int			type)
-{
-	xfs_bmbt_rec_t		*ext_buffer;
-
-	ext_buffer = kmem_alloc(XFS_IFORK_SIZE(ip, whichfork), KM_SLEEP);
-	if (whichfork == XFS_DATA_FORK)
-		ip->i_itemp->ili_extents_buf = ext_buffer;
-	else
-		ip->i_itemp->ili_aextents_buf = ext_buffer;
-
-	vecp->i_addr = ext_buffer;
-	vecp->i_len = xfs_iextents_copy(ip, ext_buffer, whichfork);
-	vecp->i_type = type;
-}
-
-/*
  * This is called to fill in the vector of log iovecs for the
  * given inode log item.  It fills the first item with an inode
  * log format structure, the second with the on-disk inode structure,
@@ -181,24 +147,29 @@ xfs_inode_item_format_extents(
 STATIC void
 xfs_inode_item_format(
 	struct xfs_log_item	*lip,
-	struct xfs_log_iovec	*vecp)
+	struct xfs_log_vec	*lv)
 {
 	struct xfs_inode_log_item *iip = INODE_ITEM(lip);
 	struct xfs_inode	*ip = iip->ili_inode;
+	struct xfs_log_iovec	*vec;
+	struct xfs_inode_log_format *ilf;
+	uint			size;
 	uint			nvecs;
 	size_t			data_bytes;
 	xfs_mount_t		*mp;
 
-	vecp->i_addr = &iip->ili_format;
-	vecp->i_len  = sizeof(xfs_inode_log_format_t);
-	vecp->i_type = XLOG_REG_TYPE_IFORMAT;
-	vecp++;
-	nvecs	     = 1;
-
-	vecp->i_addr = &ip->i_d;
-	vecp->i_len  = xfs_icdinode_size(ip->i_d.di_version);
-	vecp->i_type = XLOG_REG_TYPE_ICORE;
-	vecp++;
+	vec = xlog_first_iovec(lv);
+	ilf = memcpy(vec->i_addr, &iip->ili_format,
+		     sizeof(struct xfs_inode_log_format));
+	vec->i_len = sizeof(struct xfs_inode_log_format);
+	vec->i_type = XLOG_REG_TYPE_IFORMAT;
+	nvecs = 1;
+
+	vec = xlog_next_iovec(lv, vec);
+	size = xfs_icdinode_size(ip->i_d.di_version);
+	memcpy(vec->i_addr, &ip->i_d, size);
+	vec->i_len = size;
+	vec->i_type = XLOG_REG_TYPE_ICORE;
 	nvecs++;
 
 	/*
@@ -210,9 +181,10 @@ xfs_inode_item_format(
 	 * has a new version number, then we don't bother converting back.
 	 */
 	mp = ip->i_mount;
-	ASSERT(ip->i_d.di_version == 1 || xfs_sb_version_hasnlink(&mp->m_sb));
+	ASSERT(ip->i_d.di_version == 1 ||
+	       xfs_sb_version_hasnlink(&ip->i_mount->m_sb));
 	if (ip->i_d.di_version == 1) {
-		if (!xfs_sb_version_hasnlink(&mp->m_sb)) {
+		if (!xfs_sb_version_hasnlink(&ip->i_mount->m_sb)) {
 			/*
 			 * Convert it back.
 			 */
@@ -241,29 +213,15 @@ xfs_inode_item_format(
 		    ip->i_df.if_bytes > 0) {
 			ASSERT(ip->i_df.if_u1.if_extents != NULL);
 			ASSERT(ip->i_df.if_bytes / sizeof(xfs_bmbt_rec_t) > 0);
-			ASSERT(iip->ili_extents_buf == NULL);
-
-#ifdef XFS_NATIVE_HOST
-                       if (ip->i_d.di_nextents == ip->i_df.if_bytes /
-                                               (uint)sizeof(xfs_bmbt_rec_t)) {
-				/*
-				 * There are no delayed allocation
-				 * extents, so just point to the
-				 * real extents array.
-				 */
-				vecp->i_addr = ip->i_df.if_u1.if_extents;
-				vecp->i_len = ip->i_df.if_bytes;
-				vecp->i_type = XLOG_REG_TYPE_IEXT;
-			} else
-#endif
-			{
-				xfs_inode_item_format_extents(ip, vecp,
-					XFS_DATA_FORK, XLOG_REG_TYPE_IEXT);
-			}
-			ASSERT(vecp->i_len <= ip->i_df.if_bytes);
-			iip->ili_format.ilf_dsize = vecp->i_len;
-			vecp++;
+
+			vec = xlog_next_iovec(lv, vec);
+			vec->i_len = xfs_iextents_copy(ip, vec->i_addr,
+						       XFS_DATA_FORK);
+			vec->i_type = XLOG_REG_TYPE_IEXT;
 			nvecs++;
+
+			ASSERT(vec->i_len <= ip->i_df.if_bytes);
+			ilf->ilf_dsize = vec->i_len;
 		} else {
 			iip->ili_fields &= ~XFS_ILOG_DEXT;
 		}
@@ -277,12 +235,15 @@ xfs_inode_item_format(
 		if ((iip->ili_fields & XFS_ILOG_DBROOT) &&
 		    ip->i_df.if_broot_bytes > 0) {
 			ASSERT(ip->i_df.if_broot != NULL);
-			vecp->i_addr = ip->i_df.if_broot;
-			vecp->i_len = ip->i_df.if_broot_bytes;
-			vecp->i_type = XLOG_REG_TYPE_IBROOT;
-			vecp++;
+
+			vec = xlog_next_iovec(lv, vec);
+			memcpy(vec->i_addr, ip->i_df.if_broot,
+			       ip->i_df.if_broot_bytes);
+			vec->i_len = ip->i_df.if_broot_bytes;
+			vec->i_type = XLOG_REG_TYPE_IBROOT;
 			nvecs++;
-			iip->ili_format.ilf_dsize = ip->i_df.if_broot_bytes;
+
+			ilf->ilf_dsize = ip->i_df.if_broot_bytes;
 		} else {
 			ASSERT(!(iip->ili_fields &
 				 XFS_ILOG_DBROOT));
@@ -299,7 +260,6 @@ xfs_inode_item_format(
 			ASSERT(ip->i_df.if_u1.if_data != NULL);
 			ASSERT(ip->i_d.di_size > 0);
 
-			vecp->i_addr = ip->i_df.if_u1.if_data;
 			/*
 			 * Round i_bytes up to a word boundary.
 			 * The underlying memory is guaranteed to
@@ -308,11 +268,14 @@ xfs_inode_item_format(
 			data_bytes = roundup(ip->i_df.if_bytes, 4);
 			ASSERT((ip->i_df.if_real_bytes == 0) ||
 			       (ip->i_df.if_real_bytes == data_bytes));
-			vecp->i_len = (int)data_bytes;
-			vecp->i_type = XLOG_REG_TYPE_ILOCAL;
-			vecp++;
+
+			vec = xlog_next_iovec(lv, vec);
+			memcpy(vec->i_addr, ip->i_df.if_u1.if_data, data_bytes);
+			vec->i_len = data_bytes;
+			vec->i_type = XLOG_REG_TYPE_ILOCAL;
 			nvecs++;
-			iip->ili_format.ilf_dsize = (unsigned)data_bytes;
+
+			ilf->ilf_dsize = (unsigned)data_bytes;
 		} else {
 			iip->ili_fields &= ~XFS_ILOG_DDATA;
 		}
@@ -322,20 +285,16 @@ xfs_inode_item_format(
 		iip->ili_fields &=
 			~(XFS_ILOG_DDATA | XFS_ILOG_DBROOT |
 			  XFS_ILOG_DEXT | XFS_ILOG_UUID);
-		if (iip->ili_fields & XFS_ILOG_DEV) {
-			iip->ili_format.ilf_u.ilfu_rdev =
-				ip->i_df.if_u2.if_rdev;
-		}
+		if (iip->ili_fields & XFS_ILOG_DEV)
+			ilf->ilf_u.ilfu_rdev = ip->i_df.if_u2.if_rdev;
 		break;
 
 	case XFS_DINODE_FMT_UUID:
 		iip->ili_fields &=
 			~(XFS_ILOG_DDATA | XFS_ILOG_DBROOT |
 			  XFS_ILOG_DEXT | XFS_ILOG_DEV);
-		if (iip->ili_fields & XFS_ILOG_UUID) {
-			iip->ili_format.ilf_u.ilfu_uuid =
-				ip->i_df.if_u2.if_uuid;
-		}
+		if (iip->ili_fields & XFS_ILOG_UUID)
+			ilf->ilf_u.ilfu_uuid = ip->i_df.if_u2.if_uuid;
 		break;
 
 	default:
@@ -363,22 +322,14 @@ xfs_inode_item_format(
 			ASSERT(ip->i_afp->if_bytes / sizeof(xfs_bmbt_rec_t) ==
 				ip->i_d.di_anextents);
 			ASSERT(ip->i_afp->if_u1.if_extents != NULL);
-#ifdef XFS_NATIVE_HOST
-			/*
-			 * There are not delayed allocation extents
-			 * for attributes, so just point at the array.
-			 */
-			vecp->i_addr = ip->i_afp->if_u1.if_extents;
-			vecp->i_len = ip->i_afp->if_bytes;
-			vecp->i_type = XLOG_REG_TYPE_IATTR_EXT;
-#else
-			ASSERT(iip->ili_aextents_buf == NULL);
-			xfs_inode_item_format_extents(ip, vecp,
-					XFS_ATTR_FORK, XLOG_REG_TYPE_IATTR_EXT);
-#endif
-			iip->ili_format.ilf_asize = vecp->i_len;
-			vecp++;
+
+			vec = xlog_next_iovec(lv, vec);
+			vec->i_len = xfs_iextents_copy(ip, vec->i_addr,
+						       XFS_ATTR_FORK);
+			vec->i_type = XLOG_REG_TYPE_IATTR_EXT;
 			nvecs++;
+
+			ilf->ilf_asize = vec->i_len;
 		} else {
 			iip->ili_fields &= ~XFS_ILOG_AEXT;
 		}
@@ -392,12 +343,14 @@ xfs_inode_item_format(
 		    ip->i_afp->if_broot_bytes > 0) {
 			ASSERT(ip->i_afp->if_broot != NULL);
 
-			vecp->i_addr = ip->i_afp->if_broot;
-			vecp->i_len = ip->i_afp->if_broot_bytes;
-			vecp->i_type = XLOG_REG_TYPE_IATTR_BROOT;
-			vecp++;
+			vec = xlog_next_iovec(lv, vec);
+			memcpy(vec->i_addr, ip->i_afp->if_broot,
+			       ip->i_afp->if_broot_bytes);
+			vec->i_len = ip->i_afp->if_broot_bytes;
+			vec->i_type = XLOG_REG_TYPE_IATTR_BROOT;
 			nvecs++;
-			iip->ili_format.ilf_asize = ip->i_afp->if_broot_bytes;
+
+			ilf->ilf_asize = ip->i_afp->if_broot_bytes;
 		} else {
 			iip->ili_fields &= ~XFS_ILOG_ABROOT;
 		}
@@ -411,7 +364,6 @@ xfs_inode_item_format(
 		    ip->i_afp->if_bytes > 0) {
 			ASSERT(ip->i_afp->if_u1.if_data != NULL);
 
-			vecp->i_addr = ip->i_afp->if_u1.if_data;
 			/*
 			 * Round i_bytes up to a word boundary.
 			 * The underlying memory is guaranteed to
@@ -420,11 +372,15 @@ xfs_inode_item_format(
 			data_bytes = roundup(ip->i_afp->if_bytes, 4);
 			ASSERT((ip->i_afp->if_real_bytes == 0) ||
 			       (ip->i_afp->if_real_bytes == data_bytes));
-			vecp->i_len = (int)data_bytes;
-			vecp->i_type = XLOG_REG_TYPE_IATTR_LOCAL;
-			vecp++;
+
+			vec = xlog_next_iovec(lv, vec);
+			memcpy(vec->i_addr, ip->i_afp->if_u1.if_data,
+			       data_bytes);
+			vec->i_len = data_bytes;
+			vec->i_type = XLOG_REG_TYPE_IATTR_LOCAL;
 			nvecs++;
-			iip->ili_format.ilf_asize = (unsigned)data_bytes;
+
+			ilf->ilf_asize = (unsigned)data_bytes;
 		} else {
 			iip->ili_fields &= ~XFS_ILOG_ADATA;
 		}
@@ -436,18 +392,18 @@ xfs_inode_item_format(
 	}
 
 out:
+	xlog_last_iovec(lv, vec);
 	/*
 	 * Now update the log format that goes out to disk from the in-core
 	 * values.  We always write the inode core to make the arithmetic
 	 * games in recovery easier, which isn't a big deal as just about any
 	 * transaction would dirty it anyway.
 	 */
-	iip->ili_format.ilf_fields = XFS_ILOG_CORE |
+	ilf->ilf_fields = XFS_ILOG_CORE |
 		(iip->ili_fields & ~XFS_ILOG_TIMESTAMP);
-	iip->ili_format.ilf_size = nvecs;
+	ilf->ilf_size = nvecs;
 }
 
-
 /*
  * This is called to pin the inode associated with the inode log
  * item in memory so it cannot be written out.
@@ -563,27 +519,6 @@ xfs_inode_item_unlock(
 	ASSERT(ip->i_itemp != NULL);
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
-	/*
-	 * If the inode needed a separate buffer with which to log
-	 * its extents, then free it now.
-	 */
-	if (iip->ili_extents_buf != NULL) {
-		ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_EXTENTS);
-		ASSERT(ip->i_d.di_nextents > 0);
-		ASSERT(iip->ili_fields & XFS_ILOG_DEXT);
-		ASSERT(ip->i_df.if_bytes > 0);
-		kmem_free(iip->ili_extents_buf);
-		iip->ili_extents_buf = NULL;
-	}
-	if (iip->ili_aextents_buf != NULL) {
-		ASSERT(ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS);
-		ASSERT(ip->i_d.di_anextents > 0);
-		ASSERT(iip->ili_fields & XFS_ILOG_AEXT);
-		ASSERT(ip->i_afp->if_bytes > 0);
-		kmem_free(iip->ili_aextents_buf);
-		iip->ili_aextents_buf = NULL;
-	}
-
 	lock_flags = iip->ili_lock_flags;
 	iip->ili_lock_flags = 0;
 	if (lock_flags)
diff --git a/fs/xfs/xfs_inode_item.h b/fs/xfs/xfs_inode_item.h
index dce4d65..29b5f2b 100644
--- a/fs/xfs/xfs_inode_item.h
+++ b/fs/xfs/xfs_inode_item.h
@@ -34,10 +34,6 @@ typedef struct xfs_inode_log_item {
 	unsigned short		ili_logged;	   /* flushed logged data */
 	unsigned int		ili_last_fields;   /* fields when flushed */
 	unsigned int		ili_fields;	   /* fields to be logged */
-	struct xfs_bmbt_rec	*ili_extents_buf;  /* array of logged
-						      data exts */
-	struct xfs_bmbt_rec	*ili_aextents_buf; /* array of logged
-						      attr exts */
 	xfs_inode_log_format_t	ili_format;	   /* logged structure */
 } xfs_inode_log_item_t;
 
diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
index e148719..3769830 100644
--- a/fs/xfs/xfs_log.h
+++ b/fs/xfs/xfs_log.h
@@ -30,6 +30,36 @@ struct xfs_log_vec {
 
 #define XFS_LOG_VEC_ORDERED	(-1)
 
+static inline struct xfs_log_iovec *
+xlog_first_iovec(struct xfs_log_vec *lv)
+{
+	struct xfs_log_iovec *vec = &lv->lv_iovecp[0];
+
+	vec->i_addr = lv->lv_buf;
+	lv->lv_buf_len = 0;
+	return vec;
+}
+
+static inline struct xfs_log_iovec *
+xlog_next_iovec(struct xfs_log_vec *lv, struct xfs_log_iovec *prev)
+{
+	struct xfs_log_iovec *vec;
+
+	ASSERT(prev - lv->lv_iovecp < lv->lv_niovecs);
+
+	vec = prev + 1;
+	vec->i_addr = prev->i_addr + prev->i_len;
+	lv->lv_buf_len += prev->i_len;
+	return vec;
+}
+
+static inline void
+xlog_last_iovec(struct xfs_log_vec *lv, struct xfs_log_iovec *prev)
+{
+	ASSERT(prev - lv->lv_iovecp < lv->lv_niovecs);
+	lv->lv_buf_len += prev->i_len;
+}
+
 /*
  * Structure used to pass callback function and the function's argument
  * to the log manager.
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 5eb51fc..8d5f2ea 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -82,36 +82,6 @@ xlog_cil_init_post_recovery(
 								log->l_curr_block);
 }
 
-STATIC int
-xlog_cil_lv_item_format(
-	struct xfs_log_item	*lip,
-	struct xfs_log_vec	*lv)
-{
-	int	index;
-	char	*ptr;
-
-	/* format new vectors into array */
-	lip->li_ops->iop_format(lip, lv->lv_iovecp);
-
-	/* copy data into existing array */
-	ptr = lv->lv_buf;
-	for (index = 0; index < lv->lv_niovecs; index++) {
-		struct xfs_log_iovec *vec = &lv->lv_iovecp[index];
-
-		memcpy(ptr, vec->i_addr, vec->i_len);
-		vec->i_addr = ptr;
-		ptr += vec->i_len;
-	}
-
-	/*
-	 * some size calculations for log vectors over-estimate, so the caller
-	 * doesn't know the amount of space actually used by the item. Return
-	 * the byte count to the caller so they can check and store it
-	 * appropriately.
-	 */
-	return ptr - lv->lv_buf;
-}
-
 /*
  * Prepare the log item for insertion into the CIL. Calculate the difference in
  * log space and vectors it will consume, and if it is a new item pin it as
@@ -259,7 +229,7 @@ xlog_cil_insert_format_items(
 			lv->lv_niovecs = niovecs;
 			lv->lv_buf = (char *)lv + buf_size - nbytes;
 
-			lv->lv_buf_len = xlog_cil_lv_item_format(lip, lv);
+			lip->li_ops->iop_format(lip, lv);
 			goto insert;
 		}
 
@@ -281,7 +251,8 @@ xlog_cil_insert_format_items(
 		/* The allocated data region lies beyond the iovec region */
 		lv->lv_buf = (char *)lv + buf_size - nbytes;
 
-		lv->lv_buf_len = xlog_cil_lv_item_format(lip, lv);
+		/* Format new vectors into array */
+		lip->li_ops->iop_format(lip, lv);
 insert:
 		ASSERT(lv->lv_buf_len <= nbytes);
 		xfs_cil_prepare_item(log, lv, old_lv, diff_len, diff_iovecs);
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 9b96d35..b5bc1ab 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -64,7 +64,7 @@ typedef struct xfs_log_item {
 
 struct xfs_item_ops {
 	void (*iop_size)(xfs_log_item_t *, int *, int *);
-	void (*iop_format)(xfs_log_item_t *, struct xfs_log_iovec *);
+	void (*iop_format)(xfs_log_item_t *, struct xfs_log_vec *);
 	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 *);
-- 
1.7.10.4


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/7] xfs: let iop_format write directly into the linear buffer

[Dave Chinner]

On Sat, Nov 23, 2013 at 07:11:52AM -0800, Christoph Hellwig wrote:
> Instead of setting up pointers to memory locations in iop_format which then
> get copied into the CIL linear buffer after return move the copy into
> the individual inode items.  This avoids the need to always have a memory
> block in the exact same layout that gets written into the log around, and
> allow the log items to be much more flexible in their in-memory layouts.

There's a lot of fair intricate manipulations in this patch that
could be separated and hence easier to understand. I think you could
break this up in several patches:

	- xfs_buf_item_straddle() factoring
	- removal of the special cases for no endian swapping around
	  xfs_inode_item_format_extents()
	- a separate patch to introduce xlog_first/next/last_iovec(),
	  as I had to find those first to understand how the new
	  code worked
	- a new xlog_copy_iovec() function instead of open coding
	  the same 3 lines of code in 14 different places:

static inline void
xlog_copy_iovec(
	struct xfs_log_iovec	*vec,
	void			*src,
	int			len,
	int			type)
{
	memcpy(vec->i_addr, src_ptr, len);
	vec->i_len = len;
	vec->i_type = type;
}

	- and finally all the conversions. The addition of an
	  xlog_copy_vec() helper will make the final patch that does
	  the switchover much smaller and easier to verify....


Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/7] xfs: let iop_format write directly into the linear buffer

[Christoph Hellwig]

On Mon, Nov 25, 2013 at 08:15:27PM +1100, Dave Chinner wrote:
> 	- xfs_buf_item_straddle() factoring

This could probably be split out.

> 	- removal of the special cases for no endian swapping around
> 	  xfs_inode_item_format_extents()

This can only be done after we have the new iop_format, or rather must
be because the old way doesn't work really well.  I'll have to see if
it can be a separate one, but it would have to be after the actual
iop_format change.

> 	- a separate patch to introduce xlog_first/next/last_iovec(),
> 	  as I had to find those first to understand how the new
> 	  code worked

What's the point of a separate patch if we don't make use of it yet?

> 	- a new xlog_copy_iovec() function instead of open coding
> 	  the same 3 lines of code in 14 different places:
> 
> static inline void
> xlog_copy_iovec(
> 	struct xfs_log_iovec	*vec,
> 	void			*src,
> 	int			len,
> 	int			type)
> {
> 	memcpy(vec->i_addr, src_ptr, len);
> 	vec->i_len = len;
> 	vec->i_type = type;
> }

I went forth and back between having this a couple of times and found
having the helper more confusing than not.  If there's enough strong
opinion to have it I can add it back.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/7] xfs: let iop_format write directly into the linear buffer

[Dave Chinner]

On Mon, Nov 25, 2013 at 05:37:55AM -0800, Christoph Hellwig wrote:
> On Mon, Nov 25, 2013 at 08:15:27PM +1100, Dave Chinner wrote:
> > 	- xfs_buf_item_straddle() factoring
> 
> This could probably be split out.
> 
> > 	- removal of the special cases for no endian swapping around
> > 	  xfs_inode_item_format_extents()
> 
> This can only be done after we have the new iop_format, or rather must
> be because the old way doesn't work really well.  I'll have to see if
> it can be a separate one, but it would have to be after the actual
> iop_format change.

I think the current code could be changed first, just to remove the
special cases (i.e. the ifdef NATIVE_HOST/else conditionals) by
always calling xfs_inode_item_format_extents(). That's easy enough
to do and then the iop_format change can simple change it to calling
xfs_iextent_copy() directly...

> > 	- a separate patch to introduce xlog_first/next/last_iovec(),
> > 	  as I had to find those first to understand how the new
> > 	  code worked
> 
> What's the point of a separate patch if we don't make use of it yet?

The problem I had looking at the patch was that the actual
implementation of critical infrastructure the entire patch relies on
is the last thing in the patch.  I found myself constant scrolling
through the patch to check what the infrastructure changes did and
losing context because of this....

> > 	- a new xlog_copy_iovec() function instead of open coding
> > 	  the same 3 lines of code in 14 different places:
> > 
> > static inline void
> > xlog_copy_iovec(
> > 	struct xfs_log_iovec	*vec,
> > 	void			*src,
> > 	int			len,
> > 	int			type)
> > {
> > 	memcpy(vec->i_addr, src_ptr, len);
> > 	vec->i_len = len;
> > 	vec->i_type = type;
> > }
> 
> I went forth and back between having this a couple of times and found
> having the helper more confusing than not.  If there's enough strong
> opinion to have it I can add it back.

I'd prefer to have a helper than have the same boilerplate code
repeated 14 times purely from a maintenance POV. It's easy to find
all the callers, it's easy to check that they do the right thing,
and in future there's only one piece of code to modify for all the
simple log item formatting operations....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/7] xfs: let iop_format write directly into the linear buffer

[Christoph Hellwig]

On Tue, Nov 26, 2013 at 07:45:25AM +1100, Dave Chinner wrote:
> I think the current code could be changed first, just to remove the
> special cases (i.e. the ifdef NATIVE_HOST/else conditionals) by
> always calling xfs_inode_item_format_extents(). That's easy enough
> to do and then the iop_format change can simple change it to calling
> xfs_iextent_copy() directly...

Ok, I can do that.

> I'd prefer to have a helper than have the same boilerplate code
> repeated 14 times purely from a maintenance POV. It's easy to find
> all the callers, it's easy to check that they do the right thing,
> and in future there's only one piece of code to modify for all the
> simple log item formatting operations....

I'll resent with it re-added.  The reason it confused me is that only
the len argument ever got used twice in the helper, otherwise it's
just a bunch of assignment using a disjoint arguments.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/7] xfs: remove the inode log format from the inode log item

[Christoph Hellwig]

[-- Attachment #1: 0002-xfs-remove-the-inode-log-format-from-the-inode-log-i.patch --]
[-- Type: text/plain, Size: 4734 bytes --]

No need to keep the inode log format around all the time, we can easily
generate it at iop_format time.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_inode_item.c |   51 +++++++++++++++++++----------------------------
 fs/xfs/xfs_inode_item.h |    1 -
 2 files changed, 21 insertions(+), 31 deletions(-)

diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index fbdcde1..9ed1635 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -153,24 +153,28 @@ xfs_inode_item_format(
 	struct xfs_inode	*ip = iip->ili_inode;
 	struct xfs_log_iovec	*vec;
 	struct xfs_inode_log_format *ilf;
-	uint			size;
-	uint			nvecs;
 	size_t			data_bytes;
 	xfs_mount_t		*mp;
 
 	vec = xlog_first_iovec(lv);
-	ilf = memcpy(vec->i_addr, &iip->ili_format,
-		     sizeof(struct xfs_inode_log_format));
-	vec->i_len = sizeof(struct xfs_inode_log_format);
 	vec->i_type = XLOG_REG_TYPE_IFORMAT;
-	nvecs = 1;
+	vec->i_len = sizeof(struct xfs_inode_log_format);
+
+	ilf = vec->i_addr;
+	ilf->ilf_type = XFS_LI_INODE;
+	ilf->ilf_ino = ip->i_ino;
+	ilf->ilf_blkno = ip->i_imap.im_blkno;
+	ilf->ilf_len = ip->i_imap.im_len;
+	ilf->ilf_boffset = ip->i_imap.im_boffset;
+	ilf->ilf_fields = XFS_ILOG_CORE;
+	ilf->ilf_size = 1;
 
 	vec = xlog_next_iovec(lv, vec);
-	size = xfs_icdinode_size(ip->i_d.di_version);
-	memcpy(vec->i_addr, &ip->i_d, size);
-	vec->i_len = size;
 	vec->i_type = XLOG_REG_TYPE_ICORE;
-	nvecs++;
+	vec->i_len = xfs_icdinode_size(ip->i_d.di_version);
+	memcpy(vec->i_addr, &ip->i_d, vec->i_len);
+
+	ilf->ilf_size++;
 
 	/*
 	 * If this is really an old format inode, then we need to
@@ -218,7 +222,7 @@ xfs_inode_item_format(
 			vec->i_len = xfs_iextents_copy(ip, vec->i_addr,
 						       XFS_DATA_FORK);
 			vec->i_type = XLOG_REG_TYPE_IEXT;
-			nvecs++;
+			ilf->ilf_size++;
 
 			ASSERT(vec->i_len <= ip->i_df.if_bytes);
 			ilf->ilf_dsize = vec->i_len;
@@ -241,7 +245,7 @@ xfs_inode_item_format(
 			       ip->i_df.if_broot_bytes);
 			vec->i_len = ip->i_df.if_broot_bytes;
 			vec->i_type = XLOG_REG_TYPE_IBROOT;
-			nvecs++;
+			ilf->ilf_size++;
 
 			ilf->ilf_dsize = ip->i_df.if_broot_bytes;
 		} else {
@@ -273,7 +277,7 @@ xfs_inode_item_format(
 			memcpy(vec->i_addr, ip->i_df.if_u1.if_data, data_bytes);
 			vec->i_len = data_bytes;
 			vec->i_type = XLOG_REG_TYPE_ILOCAL;
-			nvecs++;
+			ilf->ilf_size++;
 
 			ilf->ilf_dsize = (unsigned)data_bytes;
 		} else {
@@ -327,7 +331,7 @@ xfs_inode_item_format(
 			vec->i_len = xfs_iextents_copy(ip, vec->i_addr,
 						       XFS_ATTR_FORK);
 			vec->i_type = XLOG_REG_TYPE_IATTR_EXT;
-			nvecs++;
+			ilf->ilf_size++;
 
 			ilf->ilf_asize = vec->i_len;
 		} else {
@@ -348,7 +352,7 @@ xfs_inode_item_format(
 			       ip->i_afp->if_broot_bytes);
 			vec->i_len = ip->i_afp->if_broot_bytes;
 			vec->i_type = XLOG_REG_TYPE_IATTR_BROOT;
-			nvecs++;
+			ilf->ilf_size++;
 
 			ilf->ilf_asize = ip->i_afp->if_broot_bytes;
 		} else {
@@ -378,7 +382,7 @@ xfs_inode_item_format(
 			       data_bytes);
 			vec->i_len = data_bytes;
 			vec->i_type = XLOG_REG_TYPE_IATTR_LOCAL;
-			nvecs++;
+			ilf->ilf_size++;
 
 			ilf->ilf_asize = (unsigned)data_bytes;
 		} else {
@@ -393,15 +397,7 @@ xfs_inode_item_format(
 
 out:
 	xlog_last_iovec(lv, vec);
-	/*
-	 * Now update the log format that goes out to disk from the in-core
-	 * values.  We always write the inode core to make the arithmetic
-	 * games in recovery easier, which isn't a big deal as just about any
-	 * transaction would dirty it anyway.
-	 */
-	ilf->ilf_fields = XFS_ILOG_CORE |
-		(iip->ili_fields & ~XFS_ILOG_TIMESTAMP);
-	ilf->ilf_size = nvecs;
+	ilf->ilf_fields |= (iip->ili_fields & ~XFS_ILOG_TIMESTAMP);
 }
 
 /*
@@ -605,11 +601,6 @@ xfs_inode_item_init(
 	iip->ili_inode = ip;
 	xfs_log_item_init(mp, &iip->ili_item, XFS_LI_INODE,
 						&xfs_inode_item_ops);
-	iip->ili_format.ilf_type = XFS_LI_INODE;
-	iip->ili_format.ilf_ino = ip->i_ino;
-	iip->ili_format.ilf_blkno = ip->i_imap.im_blkno;
-	iip->ili_format.ilf_len = ip->i_imap.im_len;
-	iip->ili_format.ilf_boffset = ip->i_imap.im_boffset;
 }
 
 /*
diff --git a/fs/xfs/xfs_inode_item.h b/fs/xfs/xfs_inode_item.h
index 29b5f2b..488d812 100644
--- a/fs/xfs/xfs_inode_item.h
+++ b/fs/xfs/xfs_inode_item.h
@@ -34,7 +34,6 @@ typedef struct xfs_inode_log_item {
 	unsigned short		ili_logged;	   /* flushed logged data */
 	unsigned int		ili_last_fields;   /* fields when flushed */
 	unsigned int		ili_fields;	   /* fields to be logged */
-	xfs_inode_log_format_t	ili_format;	   /* logged structure */
 } xfs_inode_log_item_t;
 
 static inline int xfs_inode_clean(xfs_inode_t *ip)
-- 
1.7.10.4


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/7] xfs: remove the dquot log format from the dquot log item

[Christoph Hellwig]

[-- Attachment #1: 0003-xfs-remove-the-dquot-log-format-from-the-dquot-log-i.patch --]
[-- Type: text/plain, Size: 2693 bytes --]

No need to keep the dquot log format around all the time, we can easily
generate it at iop_format time.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_dquot_item.c |   26 ++++++++++----------------
 fs/xfs/xfs_dquot_item.h |    1 -
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index 5f446a7..2df55fe 100644
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -60,15 +60,21 @@ xfs_qm_dquot_logitem_format(
 	struct xfs_log_vec	*lv)
 {
 	struct xfs_dq_logitem	*qlip = DQUOT_ITEM(lip);
+	struct xfs_dq_logformat	*qlf;
 	struct xfs_log_iovec	*vec;
 
-	qlip->qli_format.qlf_size = 2;
-
 	vec = xlog_first_iovec(lv);
-	memcpy(vec->i_addr, &qlip->qli_format, sizeof(xfs_dq_logformat_t));
-	vec->i_len = sizeof(xfs_dq_logformat_t);
+	vec->i_len = sizeof(struct xfs_dq_logformat);
 	vec->i_type = XLOG_REG_TYPE_QFORMAT;
 
+	qlf = vec->i_addr;
+	qlf->qlf_type = XFS_LI_DQUOT;
+	qlf->qlf_size = 2;
+	qlf->qlf_id = be32_to_cpu(qlip->qli_dquot->q_core.d_id);
+	qlf->qlf_blkno = qlip->qli_dquot->q_blkno;
+	qlf->qlf_len = 1;
+	qlf->qlf_boffset = qlip->qli_dquot->q_bufoffset;
+
 	vec = xlog_next_iovec(lv, vec);
 	memcpy(vec->i_addr, &qlip->qli_dquot->q_core, sizeof(xfs_disk_dquot_t));
 	vec->i_len = sizeof(xfs_disk_dquot_t);
@@ -261,18 +267,6 @@ xfs_qm_dquot_logitem_init(
 	xfs_log_item_init(dqp->q_mount, &lp->qli_item, XFS_LI_DQUOT,
 					&xfs_dquot_item_ops);
 	lp->qli_dquot = dqp;
-	lp->qli_format.qlf_type = XFS_LI_DQUOT;
-	lp->qli_format.qlf_id = be32_to_cpu(dqp->q_core.d_id);
-	lp->qli_format.qlf_blkno = dqp->q_blkno;
-	lp->qli_format.qlf_len = 1;
-	/*
-	 * This is just the offset of this dquot within its buffer
-	 * (which is currently 1 FSB and probably won't change).
-	 * Hence 32 bits for this offset should be just fine.
-	 * Alternatively, we can store (bufoffset / sizeof(xfs_dqblk_t))
-	 * here, and recompute it at recovery time.
-	 */
-	lp->qli_format.qlf_boffset = (__uint32_t)dqp->q_bufoffset;
 }
 
 /*------------------  QUOTAOFF LOG ITEMS  -------------------*/
diff --git a/fs/xfs/xfs_dquot_item.h b/fs/xfs/xfs_dquot_item.h
index 5acae2a..925cbe9 100644
--- a/fs/xfs/xfs_dquot_item.h
+++ b/fs/xfs/xfs_dquot_item.h
@@ -27,7 +27,6 @@ typedef struct xfs_dq_logitem {
 	xfs_log_item_t		 qli_item;	   /* common portion */
 	struct xfs_dquot	*qli_dquot;	   /* dquot ptr */
 	xfs_lsn_t		 qli_flush_lsn;	   /* lsn at last flush */
-	xfs_dq_logformat_t	 qli_format;	   /* logged structure */
 } xfs_dq_logitem_t;
 
 typedef struct xfs_qoff_logitem {
-- 
1.7.10.4


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 4/7] xfs: remove the quotaoff log format from the quotaoff log item

[Christoph Hellwig]

[-- Attachment #1: 0004-xfs-remove-the-quotaoff-log-format-from-the-quotaoff.patch --]
[-- Type: text/plain, Size: 2013 bytes --]

No quite as large as the previous ones, but easy enough to decouple.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_dquot_item.c |   16 ++++++++--------
 fs/xfs/xfs_dquot_item.h |    2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index 2df55fe..c1917fd 100644
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -306,16 +306,17 @@ xfs_qm_qoff_logitem_format(
 {
 	struct xfs_qoff_logitem	*qflip = QOFF_ITEM(lip);
 	struct xfs_log_iovec *vec;
-
-	ASSERT(qflip->qql_format.qf_type == XFS_LI_QUOTAOFF);
-
-	qflip->qql_format.qf_size = 1;
+	struct xfs_qoff_logformat *qlf;
 
 	vec = xlog_first_iovec(lv);
-	memcpy(vec->i_addr, &qflip->qql_format, sizeof(xfs_qoff_logitem_t));
-	vec->i_len = sizeof(xfs_qoff_logitem_t);
+	vec->i_len = sizeof(struct xfs_qoff_logitem);
 	vec->i_type = XLOG_REG_TYPE_QUOTAOFF;
 
+	qlf = vec->i_addr;
+	qlf->qf_type = XFS_LI_QUOTAOFF;
+	qlf->qf_size = 1;
+	qlf->qf_flags = qflip->qql_flags;
+
 	xlog_last_iovec(lv, vec);
 }
 
@@ -456,8 +457,7 @@ xfs_qm_qoff_logitem_init(
 	xfs_log_item_init(mp, &qf->qql_item, XFS_LI_QUOTAOFF, start ?
 			&xfs_qm_qoffend_logitem_ops : &xfs_qm_qoff_logitem_ops);
 	qf->qql_item.li_mountp = mp;
-	qf->qql_format.qf_type = XFS_LI_QUOTAOFF;
-	qf->qql_format.qf_flags = flags;
 	qf->qql_start_lip = start;
+	qf->qql_flags = flags;
 	return qf;
 }
diff --git a/fs/xfs/xfs_dquot_item.h b/fs/xfs/xfs_dquot_item.h
index 925cbe9..502e946 100644
--- a/fs/xfs/xfs_dquot_item.h
+++ b/fs/xfs/xfs_dquot_item.h
@@ -32,7 +32,7 @@ typedef struct xfs_dq_logitem {
 typedef struct xfs_qoff_logitem {
 	xfs_log_item_t		 qql_item;	/* common portion */
 	struct xfs_qoff_logitem *qql_start_lip; /* qoff-start logitem, if any */
-	xfs_qoff_logformat_t	 qql_format;	/* logged structure */
+	unsigned int		qql_flags;
 } xfs_qoff_logitem_t;
 
 
-- 
1.7.10.4


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 5/7] xfs: defer EFI and EFD log formatting until iop_format time

[Christoph Hellwig]

[-- Attachment #1: 0005-xfs-defer-EFI-and-EFD-log-formatting-until-iop_forma.patch --]
[-- Type: text/plain, Size: 30815 bytes --]

No need to allocate large chunks of memory to format each extent into
an array when logging the EFI or EFD items.  Instead just point to the
bmap free list and only generate the log format at iop_format time.

Also get rid of the now almost empty xfs_trans_extfree.c by merging it
into xfs_extfree_item.c.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/Makefile            |    1 -
 fs/xfs/xfs_bmap.c          |   51 ++++----
 fs/xfs/xfs_bmap.h          |    2 +
 fs/xfs/xfs_bmap_util.c     |   20 ++--
 fs/xfs/xfs_extfree_item.c  |  276 ++++++++++++++++++++++++--------------------
 fs/xfs/xfs_extfree_item.h  |   19 +--
 fs/xfs/xfs_log_recover.c   |   86 +++++++-------
 fs/xfs/xfs_super.c         |   10 +-
 fs/xfs/xfs_trans.h         |   18 +--
 fs/xfs/xfs_trans_extfree.c |  134 ---------------------
 10 files changed, 248 insertions(+), 369 deletions(-)
 delete mode 100644 fs/xfs/xfs_trans_extfree.c

diff --git a/fs/xfs/Makefile b/fs/xfs/Makefile
index c21f435..0555fb7 100644
--- a/fs/xfs/Makefile
+++ b/fs/xfs/Makefile
@@ -94,7 +94,6 @@ xfs-y				+= xfs_log.o \
 				   xfs_inode_item.o \
 				   xfs_trans_ail.o \
 				   xfs_trans_buf.o \
-				   xfs_trans_extfree.o \
 				   xfs_trans_inode.o \
 
 # optional features
diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index 3ef11b2..66bf92a 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -33,11 +33,11 @@
 #include "xfs_btree.h"
 #include "xfs_trans.h"
 #include "xfs_inode_item.h"
-#include "xfs_extfree_item.h"
 #include "xfs_alloc.h"
 #include "xfs_bmap.h"
 #include "xfs_bmap_util.h"
 #include "xfs_bmap_btree.h"
+#include "xfs_extfree_item.h"
 #include "xfs_rtalloc.h"
 #include "xfs_error.h"
 #include "xfs_quota.h"
@@ -591,6 +591,31 @@ xfs_bmap_validate_ret(
  * bmap free list manipulation functions
  */
 
+void
+_xfs_bmap_add_free(
+	xfs_fsblock_t		bno,		/* fs block number of extent */
+	xfs_filblks_t		len,		/* length of extent */
+	struct xfs_bmap_free	*flist)		/* list of extents */
+{
+	struct xfs_bmap_free_item *cur, *new, *prev;
+
+	new = kmem_zone_alloc(xfs_bmap_free_item_zone, KM_SLEEP);
+	new->xbfi_startblock = bno;
+	new->xbfi_blockcount = (xfs_extlen_t)len;
+	for (prev = NULL, cur = flist->xbf_first;
+	     cur != NULL;
+	     prev = cur, cur = cur->xbfi_next) {
+		if (cur->xbfi_startblock >= bno)
+			break;
+	}
+	if (prev)
+		prev->xbfi_next = new;
+	else
+		flist->xbf_first = new;
+	new->xbfi_next = cur;
+	flist->xbf_count++;
+}
+
 /*
  * Add the extent to the list of extents to be free at transaction end.
  * The list is maintained sorted (by block number).
@@ -599,12 +624,9 @@ void
 xfs_bmap_add_free(
 	xfs_fsblock_t		bno,		/* fs block number of extent */
 	xfs_filblks_t		len,		/* length of extent */
-	xfs_bmap_free_t		*flist,		/* list of extents */
-	xfs_mount_t		*mp)		/* mount point structure */
+	struct xfs_bmap_free	*flist,		/* list of extents */
+	struct xfs_mount	*mp)		/* mount point structure */
 {
-	xfs_bmap_free_item_t	*cur;		/* current (next) element */
-	xfs_bmap_free_item_t	*new;		/* new element */
-	xfs_bmap_free_item_t	*prev;		/* previous element */
 #ifdef DEBUG
 	xfs_agnumber_t		agno;
 	xfs_agblock_t		agbno;
@@ -620,22 +642,7 @@ xfs_bmap_add_free(
 	ASSERT(len < mp->m_sb.sb_agblocks);
 	ASSERT(agbno + len <= mp->m_sb.sb_agblocks);
 #endif
-	ASSERT(xfs_bmap_free_item_zone != NULL);
-	new = kmem_zone_alloc(xfs_bmap_free_item_zone, KM_SLEEP);
-	new->xbfi_startblock = bno;
-	new->xbfi_blockcount = (xfs_extlen_t)len;
-	for (prev = NULL, cur = flist->xbf_first;
-	     cur != NULL;
-	     prev = cur, cur = cur->xbfi_next) {
-		if (cur->xbfi_startblock >= bno)
-			break;
-	}
-	if (prev)
-		prev->xbfi_next = new;
-	else
-		flist->xbf_first = new;
-	new->xbfi_next = cur;
-	flist->xbf_count++;
+	return _xfs_bmap_add_free(bno, len, flist);
 }
 
 /*
diff --git a/fs/xfs/xfs_bmap.h b/fs/xfs/xfs_bmap.h
index 33b41f3..f24eb35 100644
--- a/fs/xfs/xfs_bmap.h
+++ b/fs/xfs/xfs_bmap.h
@@ -140,6 +140,8 @@ int	xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd);
 void	xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork);
 void	xfs_bmap_add_free(xfs_fsblock_t bno, xfs_filblks_t len,
 		struct xfs_bmap_free *flist, struct xfs_mount *mp);
+void	_xfs_bmap_add_free(xfs_fsblock_t bno, xfs_filblks_t len,
+		struct xfs_bmap_free *flist);
 void	xfs_bmap_cancel(struct xfs_bmap_free *flist);
 void	xfs_bmap_compute_maxlevels(struct xfs_mount *mp, int whichfork);
 int	xfs_bmap_first_unused(struct xfs_trans *tp, struct xfs_inode *ip,
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 5887e41..d90ce2c 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -30,11 +30,11 @@
 #include "xfs_inode.h"
 #include "xfs_btree.h"
 #include "xfs_trans.h"
-#include "xfs_extfree_item.h"
 #include "xfs_alloc.h"
 #include "xfs_bmap.h"
 #include "xfs_bmap_util.h"
 #include "xfs_bmap_btree.h"
+#include "xfs_extfree_item.h"
 #include "xfs_rtalloc.h"
 #include "xfs_error.h"
 #include "xfs_quota.h"
@@ -89,10 +89,8 @@ xfs_bmap_finish(
 		return 0;
 	}
 	ntp = *tp;
-	efi = xfs_trans_get_efi(ntp, flist->xbf_count);
-	for (free = flist->xbf_first; free; free = free->xbfi_next)
-		xfs_trans_log_efi_extent(ntp, efi, free->xbfi_startblock,
-			free->xbfi_blockcount);
+
+	efi = xfs_trans_log_efi(ntp, flist);
 
 	tres.tr_logres = ntp->t_log_res;
 	tres.tr_logcount = ntp->t_log_count;
@@ -117,8 +115,8 @@ xfs_bmap_finish(
 	error = xfs_trans_reserve(ntp, &tres, 0, 0);
 	if (error)
 		return error;
-	efd = xfs_trans_get_efd(ntp, efi, flist->xbf_count);
-	for (free = flist->xbf_first; free != NULL; free = next) {
+
+	for (free = flist->xbf_first; free != NULL; free = free->xbfi_next) {
 		next = free->xbfi_next;
 		if ((error = xfs_free_extent(ntp, free->xbfi_startblock,
 				free->xbfi_blockcount))) {
@@ -138,8 +136,12 @@ xfs_bmap_finish(
 						   SHUTDOWN_META_IO_ERROR);
 			return error;
 		}
-		xfs_trans_log_efd_extent(ntp, efd, free->xbfi_startblock,
-			free->xbfi_blockcount);
+	}
+
+	efd = xfs_trans_log_efd(ntp, efi, flist);
+
+	for (free = flist->xbf_first; free != NULL; free = next) {
+		next = free->xbfi_next;
 		xfs_bmap_del_free(flist, NULL, free);
 	}
 	return 0;
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 94bf5e7..7ed0e01 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -18,6 +18,7 @@
 #include "xfs.h"
 #include "xfs_fs.h"
 #include "xfs_log_format.h"
+#include "xfs_shared.h"
 #include "xfs_trans_resv.h"
 #include "xfs_sb.h"
 #include "xfs_ag.h"
@@ -25,6 +26,7 @@
 #include "xfs_trans.h"
 #include "xfs_trans_priv.h"
 #include "xfs_buf_item.h"
+#include "xfs_bmap.h"
 #include "xfs_extfree_item.h"
 #include "xfs_log.h"
 
@@ -41,10 +43,7 @@ void
 xfs_efi_item_free(
 	struct xfs_efi_log_item	*efip)
 {
-	if (efip->efi_format.efi_nextents > XFS_EFI_MAX_FAST_EXTENTS)
-		kmem_free(efip);
-	else
-		kmem_zone_free(xfs_efi_zone, efip);
+	kmem_zone_free(xfs_efi_zone, efip);
 }
 
 /*
@@ -79,7 +78,7 @@ xfs_efi_item_sizeof(
 	struct xfs_efi_log_item *efip)
 {
 	return sizeof(struct xfs_efi_log_format) +
-	       (efip->efi_format.efi_nextents - 1) * sizeof(xfs_extent_t);
+	       (efip->efi_flist.xbf_count - 1) * sizeof(xfs_extent_t);
 }
 
 STATIC void
@@ -105,22 +104,28 @@ xfs_efi_item_format(
 	struct xfs_log_vec	*lv)
 {
 	struct xfs_efi_log_item	*efip = EFI_ITEM(lip);
+	struct xfs_efi_log_format *elf;
+	struct xfs_bmap_free_item *free;
 	struct xfs_log_iovec	*vec;
-	int			size;
+	int			e = 0;
 
-	ASSERT(atomic_read(&efip->efi_next_extent) ==
-				efip->efi_format.efi_nextents);
+	vec = xlog_first_iovec(lv);
+	vec->i_len = xfs_efi_item_sizeof(efip);
+	vec->i_type = XLOG_REG_TYPE_EFI_FORMAT;
 
-	efip->efi_format.efi_type = XFS_LI_EFI;
-	efip->efi_format.efi_size = 1;
+	elf = vec->i_addr;
+	elf->efi_type = XFS_LI_EFI;
+	elf->efi_size = 1;
+	elf->efi_nextents = efip->efi_flist.xbf_count;
+	elf->efi_id = efip->efi_id;
 
-	size = xfs_efi_item_sizeof(efip);
-	ASSERT(size >= sizeof(xfs_efi_log_format_t));
+	for (free = efip->efi_flist.xbf_first; free; free = free->xbfi_next) {
+		atomic_inc(&efip->efi_next_extent);
 
-	vec = xlog_first_iovec(lv);
-	memcpy(vec->i_addr, &efip->efi_format, size);
-	vec->i_len = size;
-	vec->i_type = XLOG_REG_TYPE_EFI_FORMAT;
+		elf->efi_extents[e].ext_start = free->xbfi_startblock;
+		elf->efi_extents[e].ext_len = free->xbfi_blockcount;
+		e++;
+	}
 
 	xlog_last_iovec(lv, vec);
 }
@@ -222,91 +227,118 @@ static const struct xfs_item_ops xfs_efi_item_ops = {
 	.iop_committing = xfs_efi_item_committing
 };
 
-
 /*
  * Allocate and initialize an efi item with the given number of extents.
  */
-struct xfs_efi_log_item *
+STATIC struct xfs_efi_log_item *
 xfs_efi_init(
-	struct xfs_mount	*mp,
-	uint			nextents)
+	struct xfs_mount	*mp)
 
 {
 	struct xfs_efi_log_item	*efip;
-	uint			size;
-
-	ASSERT(nextents > 0);
-	if (nextents > XFS_EFI_MAX_FAST_EXTENTS) {
-		size = (uint)(sizeof(xfs_efi_log_item_t) +
-			((nextents - 1) * sizeof(xfs_extent_t)));
-		efip = kmem_zalloc(size, KM_SLEEP);
-	} else {
-		efip = kmem_zone_zalloc(xfs_efi_zone, KM_SLEEP);
-	}
 
+	efip = kmem_zone_zalloc(xfs_efi_zone, KM_SLEEP);
 	xfs_log_item_init(mp, &efip->efi_item, XFS_LI_EFI, &xfs_efi_item_ops);
-	efip->efi_format.efi_nextents = nextents;
-	efip->efi_format.efi_id = (__psint_t)(void*)efip;
+
 	atomic_set(&efip->efi_next_extent, 0);
 	atomic_set(&efip->efi_refcount, 2);
+	efip->efi_id = (unsigned long)efip;
 
 	return efip;
 }
 
 /*
- * Copy an EFI format buffer from the given buf, and into the destination
- * EFI format structure.
+ * Signal the intent to free all extents contained on the passed freelist.
+ */
+struct xfs_efi_log_item *
+xfs_trans_log_efi(
+	struct xfs_trans		*tp,
+	struct xfs_bmap_free		*flist)
+{
+	struct xfs_efi_log_item		*efi;
+
+	ASSERT(flist->xbf_count > 0);
+
+	efi = xfs_efi_init(tp->t_mountp);
+	xfs_trans_add_item(tp, &efi->efi_item);
+
+	tp->t_flags |= XFS_TRANS_DIRTY;
+	efi->efi_item.li_desc->lid_flags |= XFS_LID_DIRTY;
+
+	/*
+	 * We use a structure assignment here to make life easier for the log
+	 * recovery code.  The structure must not be modified in the log item
+	 * code.
+	 */
+	efi->efi_flist = *flist;
+	return efi;
+}
+
+
+static inline int
+xfs_efi_format32_sizeof(
+	struct xfs_efi_log_format	*elf)
+{
+	return sizeof(struct xfs_efi_log_format_32) + 
+		(elf->efi_nextents - 1) * sizeof(struct xfs_extent_32);
+}
+
+static inline int
+xfs_efi_format64_sizeof(
+	struct xfs_efi_log_format	*elf)
+{
+	return sizeof(struct xfs_efi_log_format_64) + 
+		(elf->efi_nextents - 1) * sizeof(struct xfs_extent_64);
+}
+
+/*
+ * Create and EFI from a given buffer recovered from disk.
+ *
  * The given buffer can be in 32 bit or 64 bit form (which has different padding),
  * one of which will be the native format for this kernel.
- * It will handle the conversion of formats if necessary.
  */
 int
-xfs_efi_copy_format(xfs_log_iovec_t *buf, xfs_efi_log_format_t *dst_efi_fmt)
+xfs_efi_item_from_disk(
+	struct xfs_mount		*mp,
+	struct xfs_log_iovec		*vec,
+	struct xfs_efi_log_item		**efipp)
 {
-	xfs_efi_log_format_t *src_efi_fmt = buf->i_addr;
-	uint i;
-	uint len = sizeof(xfs_efi_log_format_t) + 
-		(src_efi_fmt->efi_nextents - 1) * sizeof(xfs_extent_t);  
-	uint len32 = sizeof(xfs_efi_log_format_32_t) + 
-		(src_efi_fmt->efi_nextents - 1) * sizeof(xfs_extent_32_t);  
-	uint len64 = sizeof(xfs_efi_log_format_64_t) + 
-		(src_efi_fmt->efi_nextents - 1) * sizeof(xfs_extent_64_t);  
-
-	if (buf->i_len == len) {
-		memcpy((char *)dst_efi_fmt, (char*)src_efi_fmt, len);
-		return 0;
-	} else if (buf->i_len == len32) {
-		xfs_efi_log_format_32_t *src_efi_fmt_32 = buf->i_addr;
-
-		dst_efi_fmt->efi_type     = src_efi_fmt_32->efi_type;
-		dst_efi_fmt->efi_size     = src_efi_fmt_32->efi_size;
-		dst_efi_fmt->efi_nextents = src_efi_fmt_32->efi_nextents;
-		dst_efi_fmt->efi_id       = src_efi_fmt_32->efi_id;
-		for (i = 0; i < dst_efi_fmt->efi_nextents; i++) {
-			dst_efi_fmt->efi_extents[i].ext_start =
-				src_efi_fmt_32->efi_extents[i].ext_start;
-			dst_efi_fmt->efi_extents[i].ext_len =
-				src_efi_fmt_32->efi_extents[i].ext_len;
+	struct xfs_efi_log_item		*efip;
+	int				e;
+
+	efip = xfs_efi_init(mp);
+
+	if (vec->i_len == xfs_efi_format32_sizeof(vec->i_addr)) {
+		struct xfs_efi_log_format_32	*elf32 = vec->i_addr;
+
+		for (e = 0; e < elf32->efi_nextents; e++) {
+			_xfs_bmap_add_free(elf32->efi_extents[e].ext_start,
+					   elf32->efi_extents[e].ext_len,
+					   &efip->efi_flist);
 		}
-		return 0;
-	} else if (buf->i_len == len64) {
-		xfs_efi_log_format_64_t *src_efi_fmt_64 = buf->i_addr;
-
-		dst_efi_fmt->efi_type     = src_efi_fmt_64->efi_type;
-		dst_efi_fmt->efi_size     = src_efi_fmt_64->efi_size;
-		dst_efi_fmt->efi_nextents = src_efi_fmt_64->efi_nextents;
-		dst_efi_fmt->efi_id       = src_efi_fmt_64->efi_id;
-		for (i = 0; i < dst_efi_fmt->efi_nextents; i++) {
-			dst_efi_fmt->efi_extents[i].ext_start =
-				src_efi_fmt_64->efi_extents[i].ext_start;
-			dst_efi_fmt->efi_extents[i].ext_len =
-				src_efi_fmt_64->efi_extents[i].ext_len;
+		efip->efi_id = elf32->efi_id;
+	} else if (vec->i_len == xfs_efi_format64_sizeof(vec->i_addr)) {
+		struct xfs_efi_log_format_64	*elf64 = vec->i_addr;
+
+		for (e = 0; e < elf64->efi_nextents; e++) {
+			_xfs_bmap_add_free(elf64->efi_extents[e].ext_start,
+					   elf64->efi_extents[e].ext_len,
+					   &efip->efi_flist);
 		}
-		return 0;
+		efip->efi_id = elf64->efi_id;
+	} else {
+		xfs_warn(mp, "invalid inode free log item (size = %d)\n",
+			     vec->i_len);
+		xfs_efi_item_free(efip);
+		return EFSCORRUPTED;
 	}
-	return EFSCORRUPTED;
+
+	atomic_set(&efip->efi_next_extent, efip->efi_flist.xbf_count);
+	*efipp = efip;
+	return 0;
 }
 
+
 /*
  * This is called by the efd item code below to release references to the given
  * efi item.  Each efd calls this with the number of extents that it has
@@ -318,14 +350,8 @@ xfs_efi_release(xfs_efi_log_item_t	*efip,
 		uint			nextents)
 {
 	ASSERT(atomic_read(&efip->efi_next_extent) >= nextents);
-	if (atomic_sub_and_test(nextents, &efip->efi_next_extent)) {
-		/* recovery needs us to drop the EFI reference, too */
-		if (test_bit(XFS_EFI_RECOVERED, &efip->efi_flags))
-			__xfs_efi_release(efip);
-
+	if (atomic_sub_and_test(nextents, &efip->efi_next_extent))
 		__xfs_efi_release(efip);
-		/* efip may now have been freed, do not reference it again. */
-	}
 }
 
 static inline struct xfs_efd_log_item *EFD_ITEM(struct xfs_log_item *lip)
@@ -333,15 +359,6 @@ static inline struct xfs_efd_log_item *EFD_ITEM(struct xfs_log_item *lip)
 	return container_of(lip, struct xfs_efd_log_item, efd_item);
 }
 
-STATIC void
-xfs_efd_item_free(struct xfs_efd_log_item *efdp)
-{
-	if (efdp->efd_format.efd_nextents > XFS_EFD_MAX_FAST_EXTENTS)
-		kmem_free(efdp);
-	else
-		kmem_zone_free(xfs_efd_zone, efdp);
-}
-
 /*
  * This returns the number of iovecs needed to log the given efd item.
  * We only need 1 iovec for an efd item.  It just logs the efd_log_format
@@ -352,7 +369,7 @@ xfs_efd_item_sizeof(
 	struct xfs_efd_log_item *efdp)
 {
 	return sizeof(xfs_efd_log_format_t) +
-	       (efdp->efd_format.efd_nextents - 1) * sizeof(xfs_extent_t);
+	       (efdp->efd_flist.xbf_count - 1) * sizeof(xfs_extent_t);
 }
 
 STATIC void
@@ -378,18 +395,27 @@ xfs_efd_item_format(
 	struct xfs_log_vec	*lv)
 {
 	struct xfs_efd_log_item	*efdp = EFD_ITEM(lip);
+	struct xfs_efd_log_format *efd;
 	struct xfs_log_iovec	*vec;
-
-	ASSERT(efdp->efd_next_extent == efdp->efd_format.efd_nextents);
-
-	efdp->efd_format.efd_type = XFS_LI_EFD;
-	efdp->efd_format.efd_size = 1;
+	struct xfs_bmap_free_item *free;
+	int			e = 0;
 
 	vec = xlog_first_iovec(lv);
-	memcpy(vec->i_addr, &efdp->efd_format, xfs_efd_item_sizeof(efdp));
-	vec->i_len = xfs_efd_item_sizeof(efdp);
 	vec->i_type = XLOG_REG_TYPE_EFD_FORMAT;
-	ASSERT(vec->i_len >= sizeof(xfs_efd_log_format_t));
+	vec->i_len = xfs_efd_item_sizeof(efdp);
+
+	efd = vec->i_addr;
+	efd->efd_type = XFS_LI_EFD;
+	efd->efd_size = 1;
+	efd->efd_efi_id = (unsigned long)efdp->efd_efip;
+
+	efd->efd_nextents = efdp->efd_flist.xbf_count;
+
+	for (free = efdp->efd_flist.xbf_first; free; free = free->xbfi_next) {
+		efd->efd_extents[e].ext_start = free->xbfi_startblock;
+		efd->efd_extents[e].ext_len = free->xbfi_blockcount;
+		e++;
+	}
 
 	xlog_last_iovec(lv, vec);
 }
@@ -431,7 +457,7 @@ xfs_efd_item_unlock(
 	struct xfs_log_item	*lip)
 {
 	if (lip->li_flags & XFS_LI_ABORTED)
-		xfs_efd_item_free(EFD_ITEM(lip));
+		kmem_zone_free(xfs_efd_zone, EFD_ITEM(lip));
 }
 
 /*
@@ -453,9 +479,9 @@ xfs_efd_item_committed(
 	 * EFI got unpinned and freed before the EFD got aborted.
 	 */
 	if (!(lip->li_flags & XFS_LI_ABORTED))
-		xfs_efi_release(efdp->efd_efip, efdp->efd_format.efd_nextents);
+		xfs_efi_release(efdp->efd_efip, efdp->efd_flist.xbf_count);
 
-	xfs_efd_item_free(efdp);
+	kmem_zone_free(xfs_efd_zone, efdp);
 	return (xfs_lsn_t)-1;
 }
 
@@ -487,32 +513,32 @@ static const struct xfs_item_ops xfs_efd_item_ops = {
 	.iop_committing = xfs_efd_item_committing
 };
 
-/*
- * Allocate and initialize an efd item with the given number of extents.
- */
 struct xfs_efd_log_item *
-xfs_efd_init(
-	struct xfs_mount	*mp,
-	struct xfs_efi_log_item	*efip,
-	uint			nextents)
-
+xfs_trans_log_efd(
+	struct xfs_trans		*tp,
+	struct xfs_efi_log_item		*efip,
+	struct xfs_bmap_free		*flist)
 {
-	struct xfs_efd_log_item	*efdp;
-	uint			size;
-
-	ASSERT(nextents > 0);
-	if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
-		size = (uint)(sizeof(xfs_efd_log_item_t) +
-			((nextents - 1) * sizeof(xfs_extent_t)));
-		efdp = kmem_zalloc(size, KM_SLEEP);
-	} else {
-		efdp = kmem_zone_zalloc(xfs_efd_zone, KM_SLEEP);
-	}
+	struct xfs_efd_log_item		*efdp;
+
+	ASSERT(flist->xbf_count > 0);
+
+	efdp = kmem_zone_zalloc(xfs_efd_zone, KM_SLEEP);
+
+	xfs_log_item_init(tp->t_mountp, &efdp->efd_item, XFS_LI_EFD,
+			  &xfs_efd_item_ops);
+	xfs_trans_add_item(tp, &efdp->efd_item);
+
+	tp->t_flags |= XFS_TRANS_DIRTY;
+	efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;
 
-	xfs_log_item_init(mp, &efdp->efd_item, XFS_LI_EFD, &xfs_efd_item_ops);
 	efdp->efd_efip = efip;
-	efdp->efd_format.efd_nextents = nextents;
-	efdp->efd_format.efd_efi_id = efip->efi_format.efi_id;
 
+	/*
+	 * We use a structure assignment here to make life easier for the log
+	 * recovery code.  The structure must not be modified in the log item
+	 * code.
+	 */
+	efdp->efd_flist = *flist;
 	return efdp;
 }
diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
index 0ffbce3..724a7e4 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -48,7 +48,8 @@ typedef struct xfs_efi_log_item {
 	atomic_t		efi_refcount;
 	atomic_t		efi_next_extent;
 	unsigned long		efi_flags;	/* misc flags */
-	xfs_efi_log_format_t	efi_format;
+	struct xfs_bmap_free	efi_flist;
+	unsigned long		efi_id;
 } xfs_efi_log_item_t;
 
 /*
@@ -59,23 +60,15 @@ typedef struct xfs_efi_log_item {
 typedef struct xfs_efd_log_item {
 	xfs_log_item_t		efd_item;
 	xfs_efi_log_item_t	*efd_efip;
-	uint			efd_next_extent;
-	xfs_efd_log_format_t	efd_format;
+	struct xfs_bmap_free	efd_flist;
 } xfs_efd_log_item_t;
 
-/*
- * Max number of extents in fast allocation path.
- */
-#define	XFS_EFD_MAX_FAST_EXTENTS	16
-
 extern struct kmem_zone	*xfs_efi_zone;
 extern struct kmem_zone	*xfs_efd_zone;
 
-xfs_efi_log_item_t	*xfs_efi_init(struct xfs_mount *, uint);
-xfs_efd_log_item_t	*xfs_efd_init(struct xfs_mount *, xfs_efi_log_item_t *,
-				      uint);
-int			xfs_efi_copy_format(xfs_log_iovec_t *buf,
-					    xfs_efi_log_format_t *dst_efi_fmt);
 void			xfs_efi_item_free(xfs_efi_log_item_t *);
+int			xfs_efi_item_from_disk(struct xfs_mount *,
+					       struct xfs_log_iovec *,
+					       struct xfs_efi_log_item **);
 
 #endif	/* __XFS_EXTFREE_ITEM_H__ */
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index b6b669d..61e8587 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -33,7 +33,6 @@
 #include "xfs_log_priv.h"
 #include "xfs_log_recover.h"
 #include "xfs_inode_item.h"
-#include "xfs_extfree_item.h"
 #include "xfs_trans_priv.h"
 #include "xfs_alloc.h"
 #include "xfs_ialloc.h"
@@ -42,6 +41,9 @@
 #include "xfs_trace.h"
 #include "xfs_icache.h"
 #include "xfs_bmap_btree.h"
+#include "xfs_bmap.h"
+#include "xfs_bmap_util.h"
+#include "xfs_extfree_item.h"
 #include "xfs_dinode.h"
 #include "xfs_error.h"
 #include "xfs_dir2.h"
@@ -3059,30 +3061,17 @@ xlog_recover_efi_pass2(
 	struct xlog_recover_item	*item,
 	xfs_lsn_t			lsn)
 {
-	int			error;
-	xfs_mount_t		*mp = log->l_mp;
-	xfs_efi_log_item_t	*efip;
-	xfs_efi_log_format_t	*efi_formatp;
-
-	efi_formatp = item->ri_buf[0].i_addr;
+	struct xfs_efi_log_item		*efip;
+	int				error;
 
-	efip = xfs_efi_init(mp, efi_formatp->efi_nextents);
-	if ((error = xfs_efi_copy_format(&(item->ri_buf[0]),
-					 &(efip->efi_format)))) {
-		xfs_efi_item_free(efip);
-		return error;
+	error = xfs_efi_item_from_disk(log->l_mp, &item->ri_buf[0], &efip);
+	if (!error) {
+		spin_lock(&log->l_ailp->xa_lock);
+		xfs_trans_ail_update(log->l_ailp, &efip->efi_item, lsn);
 	}
-	atomic_set(&efip->efi_next_extent, efi_formatp->efi_nextents);
-
-	spin_lock(&log->l_ailp->xa_lock);
-	/*
-	 * xfs_trans_ail_update() drops the AIL lock.
-	 */
-	xfs_trans_ail_update(log->l_ailp, &efip->efi_item, lsn);
-	return 0;
+	return error;
 }
 
-
 /*
  * This routine is called when an efd format structure is found in
  * a committed transaction in the log.  It's purpose is to cancel
@@ -3119,7 +3108,7 @@ xlog_recover_efd_pass2(
 	while (lip != NULL) {
 		if (lip->li_type == XFS_LI_EFI) {
 			efip = (xfs_efi_log_item_t *)lip;
-			if (efip->efi_format.efi_id == efi_id) {
+			if (efip->efi_id == efi_id) {
 				/*
 				 * xfs_trans_ail_delete() drops the
 				 * AIL lock.
@@ -3626,34 +3615,33 @@ xlog_recover_process_efi(
 	xfs_mount_t		*mp,
 	xfs_efi_log_item_t	*efip)
 {
-	xfs_efd_log_item_t	*efdp;
-	xfs_trans_t		*tp;
-	int			i;
-	int			error = 0;
-	xfs_extent_t		*extp;
+	struct xfs_trans	*tp;
+	struct xfs_efd_log_item *efdp;
+	struct xfs_bmap_free_item *free, *next;
 	xfs_fsblock_t		startblock_fsb;
+	int			error = 0;
 
 	ASSERT(!test_bit(XFS_EFI_RECOVERED, &efip->efi_flags));
 
 	/*
-	 * First check the validity of the extents described by the
-	 * EFI.  If any are bad, then assume that all are bad and
-	 * just toss the EFI.
+	 * First check the validity of the extents described by the EFI.  If
+	 * any are bad, then assume that all are bad and just toss the EFI.
 	 */
-	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
-		extp = &(efip->efi_format.efi_extents[i]);
-		startblock_fsb = XFS_BB_TO_FSB(mp,
-				   XFS_FSB_TO_DADDR(mp, extp->ext_start));
-		if ((startblock_fsb == 0) ||
-		    (extp->ext_len == 0) ||
-		    (startblock_fsb >= mp->m_sb.sb_dblocks) ||
-		    (extp->ext_len >= mp->m_sb.sb_agblocks)) {
+	for (free = efip->efi_flist.xbf_first; free; free = free->xbfi_next) {
+		startblock_fsb =
+			XFS_BB_TO_FSB(mp,
+				XFS_FSB_TO_DADDR(mp, free->xbfi_startblock));
+
+		if (startblock_fsb == 0 ||
+		    free->xbfi_blockcount == 0 ||
+		    startblock_fsb >= mp->m_sb.sb_dblocks ||
+		    free->xbfi_blockcount >= mp->m_sb.sb_agblocks) {
 			/*
-			 * This will pull the EFI from the AIL and
-			 * free the memory associated with it.
+			 * This will pull the EFI from the AIL and free the
+			 * memory associated with it.
 			 */
 			set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
-			xfs_efi_release(efip, efip->efi_format.efi_nextents);
+			xfs_efi_release(efip, efip->efi_flist.xbf_count);
 			return XFS_ERROR(EIO);
 		}
 	}
@@ -3662,15 +3650,19 @@ xlog_recover_process_efi(
 	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_itruncate, 0, 0);
 	if (error)
 		goto abort_error;
-	efdp = xfs_trans_get_efd(tp, efip, efip->efi_format.efi_nextents);
 
-	for (i = 0; i < efip->efi_format.efi_nextents; i++) {
-		extp = &(efip->efi_format.efi_extents[i]);
-		error = xfs_free_extent(tp, extp->ext_start, extp->ext_len);
+	for (free = efip->efi_flist.xbf_first; free; free = free->xbfi_next) {
+		error = xfs_free_extent(tp, free->xbfi_startblock,
+					    free->xbfi_blockcount);
 		if (error)
 			goto abort_error;
-		xfs_trans_log_efd_extent(tp, efdp, extp->ext_start,
-					 extp->ext_len);
+	}
+
+	efdp = xfs_trans_log_efd(tp, efip, &efip->efi_flist);
+
+	for (free = efip->efi_flist.xbf_first; free != NULL; free = next) {
+		next = free->xbfi_next;
+		xfs_bmap_del_free(&efip->efi_flist, NULL, free);
 	}
 
 	set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index d971f49..368232e 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1620,15 +1620,13 @@ xfs_init_zones(void)
 	if (!xfs_buf_item_zone)
 		goto out_destroy_log_item_desc_zone;
 
-	xfs_efd_zone = kmem_zone_init((sizeof(xfs_efd_log_item_t) +
-			((XFS_EFD_MAX_FAST_EXTENTS - 1) *
-				 sizeof(xfs_extent_t))), "xfs_efd_item");
+	xfs_efd_zone =
+		kmem_zone_init(sizeof(struct xfs_efd_log_item), "xfs_efd_item");
 	if (!xfs_efd_zone)
 		goto out_destroy_buf_item_zone;
 
-	xfs_efi_zone = kmem_zone_init((sizeof(xfs_efi_log_item_t) +
-			((XFS_EFI_MAX_FAST_EXTENTS - 1) *
-				sizeof(xfs_extent_t))), "xfs_efi_item");
+	xfs_efi_zone =
+		kmem_zone_init(sizeof(struct xfs_efi_log_item), "xfs_efi_item");
 	if (!xfs_efi_zone)
 		goto out_destroy_efd_zone;
 
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index b5bc1ab..f6b4cf0 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -20,6 +20,7 @@
 
 /* kernel only transaction subsystem defines */
 
+struct xfs_bmap_free;
 struct xfs_buf;
 struct xfs_buftarg;
 struct xfs_efd_log_item;
@@ -215,19 +216,12 @@ void		xfs_trans_ichgtime(struct xfs_trans *, struct xfs_inode *, int);
 void		xfs_trans_ijoin(struct xfs_trans *, struct xfs_inode *, uint);
 void		xfs_trans_log_buf(xfs_trans_t *, struct xfs_buf *, uint, uint);
 void		xfs_trans_log_inode(xfs_trans_t *, struct xfs_inode *, uint);
-struct xfs_efi_log_item	*xfs_trans_get_efi(xfs_trans_t *, uint);
 void		xfs_efi_release(struct xfs_efi_log_item *, uint);
-void		xfs_trans_log_efi_extent(xfs_trans_t *,
-					 struct xfs_efi_log_item *,
-					 xfs_fsblock_t,
-					 xfs_extlen_t);
-struct xfs_efd_log_item	*xfs_trans_get_efd(xfs_trans_t *,
-				  struct xfs_efi_log_item *,
-				  uint);
-void		xfs_trans_log_efd_extent(xfs_trans_t *,
-					 struct xfs_efd_log_item *,
-					 xfs_fsblock_t,
-					 xfs_extlen_t);
+struct xfs_efi_log_item *xfs_trans_log_efi(struct xfs_trans *,
+			struct xfs_bmap_free *);
+struct xfs_efd_log_item	*xfs_trans_log_efd(struct xfs_trans *,
+			struct xfs_efi_log_item *,
+			struct xfs_bmap_free *);
 int		xfs_trans_commit(xfs_trans_t *, uint flags);
 int		xfs_trans_roll(struct xfs_trans **, struct xfs_inode *);
 void		xfs_trans_cancel(xfs_trans_t *, int);
diff --git a/fs/xfs/xfs_trans_extfree.c b/fs/xfs/xfs_trans_extfree.c
deleted file mode 100644
index 47978ba..0000000
--- a/fs/xfs/xfs_trans_extfree.c
+++ /dev/null
@@ -1,134 +0,0 @@
-/*
- * Copyright (c) 2000,2005 Silicon Graphics, Inc.
- * All Rights Reserved.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write the Free Software Foundation,
- * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
- */
-#include "xfs.h"
-#include "xfs_fs.h"
-#include "xfs_shared.h"
-#include "xfs_log_format.h"
-#include "xfs_trans_resv.h"
-#include "xfs_sb.h"
-#include "xfs_ag.h"
-#include "xfs_mount.h"
-#include "xfs_trans.h"
-#include "xfs_trans_priv.h"
-#include "xfs_extfree_item.h"
-
-/*
- * This routine is called to allocate an "extent free intention"
- * log item that will hold nextents worth of extents.  The
- * caller must use all nextents extents, because we are not
- * flexible about this at all.
- */
-xfs_efi_log_item_t *
-xfs_trans_get_efi(xfs_trans_t	*tp,
-		  uint		nextents)
-{
-	xfs_efi_log_item_t	*efip;
-
-	ASSERT(tp != NULL);
-	ASSERT(nextents > 0);
-
-	efip = xfs_efi_init(tp->t_mountp, nextents);
-	ASSERT(efip != NULL);
-
-	/*
-	 * Get a log_item_desc to point at the new item.
-	 */
-	xfs_trans_add_item(tp, &efip->efi_item);
-	return efip;
-}
-
-/*
- * This routine is called to indicate that the described
- * extent is to be logged as needing to be freed.  It should
- * be called once for each extent to be freed.
- */
-void
-xfs_trans_log_efi_extent(xfs_trans_t		*tp,
-			 xfs_efi_log_item_t	*efip,
-			 xfs_fsblock_t		start_block,
-			 xfs_extlen_t		ext_len)
-{
-	uint			next_extent;
-	xfs_extent_t		*extp;
-
-	tp->t_flags |= XFS_TRANS_DIRTY;
-	efip->efi_item.li_desc->lid_flags |= XFS_LID_DIRTY;
-
-	/*
-	 * atomic_inc_return gives us the value after the increment;
-	 * we want to use it as an array index so we need to subtract 1 from
-	 * it.
-	 */
-	next_extent = atomic_inc_return(&efip->efi_next_extent) - 1;
-	ASSERT(next_extent < efip->efi_format.efi_nextents);
-	extp = &(efip->efi_format.efi_extents[next_extent]);
-	extp->ext_start = start_block;
-	extp->ext_len = ext_len;
-}
-
-
-/*
- * This routine is called to allocate an "extent free done"
- * log item that will hold nextents worth of extents.  The
- * caller must use all nextents extents, because we are not
- * flexible about this at all.
- */
-xfs_efd_log_item_t *
-xfs_trans_get_efd(xfs_trans_t		*tp,
-		  xfs_efi_log_item_t	*efip,
-		  uint			nextents)
-{
-	xfs_efd_log_item_t	*efdp;
-
-	ASSERT(tp != NULL);
-	ASSERT(nextents > 0);
-
-	efdp = xfs_efd_init(tp->t_mountp, efip, nextents);
-	ASSERT(efdp != NULL);
-
-	/*
-	 * Get a log_item_desc to point at the new item.
-	 */
-	xfs_trans_add_item(tp, &efdp->efd_item);
-	return efdp;
-}
-
-/*
- * This routine is called to indicate that the described
- * extent is to be logged as having been freed.  It should
- * be called once for each extent freed.
- */
-void
-xfs_trans_log_efd_extent(xfs_trans_t		*tp,
-			 xfs_efd_log_item_t	*efdp,
-			 xfs_fsblock_t		start_block,
-			 xfs_extlen_t		ext_len)
-{
-	uint			next_extent;
-	xfs_extent_t		*extp;
-
-	tp->t_flags |= XFS_TRANS_DIRTY;
-	efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;
-
-	next_extent = efdp->efd_next_extent;
-	ASSERT(next_extent < efdp->efd_format.efd_nextents);
-	extp = &(efdp->efd_format.efd_extents[next_extent]);
-	extp->ext_start = start_block;
-	extp->ext_len = ext_len;
-	efdp->efd_next_extent++;
-}
-- 
1.7.10.4


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/7] xfs: defer EFI and EFD log formatting until iop_format time

[Christoph Hellwig]

On Sat, Nov 23, 2013 at 07:11:56AM -0800, Christoph Hellwig wrote:
> No need to allocate large chunks of memory to format each extent into
> an array when logging the EFI or EFD items.  Instead just point to the
> bmap free list and only generate the log format at iop_format time.
> 
> Also get rid of the now almost empty xfs_trans_extfree.c by merging it
> into xfs_extfree_item.c.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Turns out this version can fairly easily cause use after frees, so it'll
need a bit of an overhaul to get the lifetime rules right.  As I have
other things in that area in the queue consider patches 5 to 7 withdrawn
for now, they will be submitted as a separate series once ready.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/7] xfs: defer EFI and EFD log formatting until iop_format time

[Dave Chinner]

On Sun, Nov 24, 2013 at 01:18:30AM -0800, Christoph Hellwig wrote:
> On Sat, Nov 23, 2013 at 07:11:56AM -0800, Christoph Hellwig wrote:
> > No need to allocate large chunks of memory to format each extent into
> > an array when logging the EFI or EFD items.  Instead just point to the
> > bmap free list and only generate the log format at iop_format time.
> > 
> > Also get rid of the now almost empty xfs_trans_extfree.c by merging it
> > into xfs_extfree_item.c.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Turns out this version can fairly easily cause use after frees, so it'll
> need a bit of an overhaul to get the lifetime rules right.

Yeah, you can't use the freelist structure like that - it's a
linked, which you copy the freelist structure when logging the
EFI/EFD, and then free the items on the linked list. Then when
formatting the structure, you walk the list attached to the copy of
the freelist structure, which has alreayd been freed.

Basically, we've got a bunch of nasty life cycle issues around the
EFI/EFD that need to be fixed. Firstly, the EFD code assumes that
the EFI always outlives it, but we don't take a reference when we
connect the EFD to the EFI - the EFI is created with the reference
for the EFD already added to it. Then in abort cases we simply free
the EFI, even though there may be an EFD that still references it...

So I think that this needs to be fixed up before you can even
consider sharing something like a reference counted freelist
structure between the EFI/EFD structures....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/7] xfs: defer EFI and EFD log formatting until iop_format time

[Christoph Hellwig]

On Mon, Nov 25, 2013 at 07:50:49PM +1100, Dave Chinner wrote:
> Yeah, you can't use the freelist structure like that - it's a
> linked, which you copy the freelist structure when logging the
> EFI/EFD, and then free the items on the linked list. Then when
> formatting the structure, you walk the list attached to the copy of
> the freelist structure, which has alreayd been freed.
> 
> Basically, we've got a bunch of nasty life cycle issues around the
> EFI/EFD that need to be fixed. Firstly, the EFD code assumes that
> the EFI always outlives it, but we don't take a reference when we
> connect the EFD to the EFI - the EFI is created with the reference
> for the EFD already added to it. Then in abort cases we simply free
> the EFI, even though there may be an EFD that still references it...
> 
> So I think that this needs to be fixed up before you can even
> consider sharing something like a reference counted freelist
> structure between the EFI/EFD structures....

In fact I was pondering having just a single refcounted structured for
the EFI and EFD, and have two xfs_log_item structures embedded into it,
with the ops modifying the common refcount.  This isn't quite ready
yet but looks feasible.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 6/7] xfs: remove efi_next_extent

[Christoph Hellwig]

[-- Attachment #1: 0006-xfs-remove-efi_next_extent.patch --]
[-- Type: text/plain, Size: 5279 bytes --]

As the new code shows much more clearly we always have a 1:1 relationship
between EFI and EFD items, and thus don't need to count the number of
extents that and EFD cancels out.  Thus we can remove the efi_next_extent
field and all logic related to it.

Also this makes clear that the only field we actually use in the EFD is
the efi_id, thus there's no need to even bother formatting the extents
into the EFD.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_extfree_item.c |   37 ++++++++++---------------------------
 fs/xfs/xfs_extfree_item.h |    1 -
 fs/xfs/xfs_log_recover.c  |    2 +-
 fs/xfs/xfs_trans.h        |    2 +-
 4 files changed, 12 insertions(+), 30 deletions(-)

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 7ed0e01..da0ac7c 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -47,14 +47,17 @@ xfs_efi_item_free(
 }
 
 /*
+ * This is called by the efd item code below to release references to the given
+ * efi item.
+ *
  * Freeing the efi requires that we remove it from the AIL if it has already
  * been placed there. However, the EFI may not yet have been placed in the AIL
- * when called by xfs_efi_release() from EFD processing due to the ordering of
- * committed vs unpin operations in bulk insert operations. Hence the reference
- * count to ensure only the last caller frees the EFI.
+ * when called by xfs_efd_item_committed due to the ordering of committed vs
+ * unpin operations in bulk insert operations. Hence the reference count to
+ * ensure only the last caller frees the EFI.
  */
-STATIC void
-__xfs_efi_release(
+void
+xfs_efi_release(
 	struct xfs_efi_log_item	*efip)
 {
 	struct xfs_ail		*ailp = efip->efi_item.li_ailp;
@@ -120,8 +123,6 @@ xfs_efi_item_format(
 	elf->efi_id = efip->efi_id;
 
 	for (free = efip->efi_flist.xbf_first; free; free = free->xbfi_next) {
-		atomic_inc(&efip->efi_next_extent);
-
 		elf->efi_extents[e].ext_start = free->xbfi_startblock;
 		elf->efi_extents[e].ext_len = free->xbfi_blockcount;
 		e++;
@@ -161,7 +162,7 @@ xfs_efi_item_unpin(
 		xfs_efi_item_free(efip);
 		return;
 	}
-	__xfs_efi_release(efip);
+	xfs_efi_release(efip);
 }
 
 /*
@@ -240,7 +241,6 @@ xfs_efi_init(
 	efip = kmem_zone_zalloc(xfs_efi_zone, KM_SLEEP);
 	xfs_log_item_init(mp, &efip->efi_item, XFS_LI_EFI, &xfs_efi_item_ops);
 
-	atomic_set(&efip->efi_next_extent, 0);
 	atomic_set(&efip->efi_refcount, 2);
 	efip->efi_id = (unsigned long)efip;
 
@@ -333,27 +333,10 @@ xfs_efi_item_from_disk(
 		return EFSCORRUPTED;
 	}
 
-	atomic_set(&efip->efi_next_extent, efip->efi_flist.xbf_count);
 	*efipp = efip;
 	return 0;
 }
 
-
-/*
- * This is called by the efd item code below to release references to the given
- * efi item.  Each efd calls this with the number of extents that it has
- * logged, and when the sum of these reaches the total number of extents logged
- * by this efi item we can free the efi item.
- */
-void
-xfs_efi_release(xfs_efi_log_item_t	*efip,
-		uint			nextents)
-{
-	ASSERT(atomic_read(&efip->efi_next_extent) >= nextents);
-	if (atomic_sub_and_test(nextents, &efip->efi_next_extent))
-		__xfs_efi_release(efip);
-}
-
 static inline struct xfs_efd_log_item *EFD_ITEM(struct xfs_log_item *lip)
 {
 	return container_of(lip, struct xfs_efd_log_item, efd_item);
@@ -479,7 +462,7 @@ xfs_efd_item_committed(
 	 * EFI got unpinned and freed before the EFD got aborted.
 	 */
 	if (!(lip->li_flags & XFS_LI_ABORTED))
-		xfs_efi_release(efdp->efd_efip, efdp->efd_flist.xbf_count);
+		xfs_efi_release(efdp->efd_efip);
 
 	kmem_zone_free(xfs_efd_zone, efdp);
 	return (xfs_lsn_t)-1;
diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
index 724a7e4..e658d42 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -46,7 +46,6 @@ struct kmem_zone;
 typedef struct xfs_efi_log_item {
 	xfs_log_item_t		efi_item;
 	atomic_t		efi_refcount;
-	atomic_t		efi_next_extent;
 	unsigned long		efi_flags;	/* misc flags */
 	struct xfs_bmap_free	efi_flist;
 	unsigned long		efi_id;
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 61e8587..b679a0b 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3641,7 +3641,7 @@ xlog_recover_process_efi(
 			 * memory associated with it.
 			 */
 			set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
-			xfs_efi_release(efip, efip->efi_flist.xbf_count);
+			xfs_efi_release(efip);
 			return XFS_ERROR(EIO);
 		}
 	}
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index f6b4cf0..6723157 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -216,7 +216,7 @@ void		xfs_trans_ichgtime(struct xfs_trans *, struct xfs_inode *, int);
 void		xfs_trans_ijoin(struct xfs_trans *, struct xfs_inode *, uint);
 void		xfs_trans_log_buf(xfs_trans_t *, struct xfs_buf *, uint, uint);
 void		xfs_trans_log_inode(xfs_trans_t *, struct xfs_inode *, uint);
-void		xfs_efi_release(struct xfs_efi_log_item *, uint);
+void		xfs_efi_release(struct xfs_efi_log_item *);
 struct xfs_efi_log_item *xfs_trans_log_efi(struct xfs_trans *,
 			struct xfs_bmap_free *);
 struct xfs_efd_log_item	*xfs_trans_log_efd(struct xfs_trans *,
-- 
1.7.10.4


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 7/7] xfs: remove opencoded versions of xfs_bmap_cancel

[Christoph Hellwig]

[-- Attachment #1: 0007-xfs-remove-opencoded-versions-of-xfs_bmap_cancel.patch --]
[-- Type: text/plain, Size: 3736 bytes --]

And fold the now otherwise unused xfs_bmap_del_free into xfs_bmap_cancel.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_bmap.c        |   33 +++++++++------------------------
 fs/xfs/xfs_bmap_util.c   |    6 +-----
 fs/xfs/xfs_bmap_util.h   |    3 ---
 fs/xfs/xfs_log_recover.c |    8 ++------
 4 files changed, 12 insertions(+), 38 deletions(-)

diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index 66bf92a..3a5383f 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -646,39 +646,24 @@ xfs_bmap_add_free(
 }
 
 /*
- * Remove the entry "free" from the free item list.  Prev points to the
- * previous entry, unless "free" is the head of the list.
- */
-void
-xfs_bmap_del_free(
-	xfs_bmap_free_t		*flist,	/* free item list header */
-	xfs_bmap_free_item_t	*prev,	/* previous item on list, if any */
-	xfs_bmap_free_item_t	*free)	/* list item to be freed */
-{
-	if (prev)
-		prev->xbfi_next = free->xbfi_next;
-	else
-		flist->xbf_first = free->xbfi_next;
-	flist->xbf_count--;
-	kmem_zone_free(xfs_bmap_free_item_zone, free);
-}
-
-/*
  * Free up any items left in the list.
  */
 void
 xfs_bmap_cancel(
-	xfs_bmap_free_t		*flist)	/* list of bmap_free_items */
+	struct xfs_bmap_free	*flist)
 {
-	xfs_bmap_free_item_t	*free;	/* free list item */
-	xfs_bmap_free_item_t	*next;
+	struct xfs_bmap_free_item *free;
 
 	if (flist->xbf_count == 0)
 		return;
+
 	ASSERT(flist->xbf_first != NULL);
-	for (free = flist->xbf_first; free; free = next) {
-		next = free->xbfi_next;
-		xfs_bmap_del_free(flist, NULL, free);
+	while ((free = flist->xbf_first)) {
+		flist->xbf_first = free->xbfi_next;
+		flist->xbf_count--;
+
+		kmem_zone_free(xfs_bmap_free_item_zone, free);
+
 	}
 	ASSERT(flist->xbf_count == 0);
 }
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index d90ce2c..268dbcf 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -139,11 +139,7 @@ xfs_bmap_finish(
 	}
 
 	efd = xfs_trans_log_efd(ntp, efi, flist);
-
-	for (free = flist->xbf_first; free != NULL; free = next) {
-		next = free->xbfi_next;
-		xfs_bmap_del_free(flist, NULL, free);
-	}
+	xfs_bmap_cancel(flist);
 	return 0;
 }
 
diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
index 900747b..54f81f8 100644
--- a/fs/xfs/xfs_bmap_util.h
+++ b/fs/xfs/xfs_bmap_util.h
@@ -80,9 +80,6 @@ int	xfs_getbmap(struct xfs_inode *ip, struct getbmapx *bmv,
 		xfs_bmap_format_t formatter, void *arg);
 
 /* functions in xfs_bmap.c that are only needed by xfs_bmap_util.c */
-void	xfs_bmap_del_free(struct xfs_bmap_free *flist,
-			  struct xfs_bmap_free_item *prev,
-			  struct xfs_bmap_free_item *free);
 int	xfs_bmap_extsize_align(struct xfs_mount *mp, struct xfs_bmbt_irec *gotp,
 			       struct xfs_bmbt_irec *prevp, xfs_extlen_t extsz,
 			       int rt, int eof, int delay, int convert,
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index b679a0b..e4b2de0 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3617,7 +3617,7 @@ xlog_recover_process_efi(
 {
 	struct xfs_trans	*tp;
 	struct xfs_efd_log_item *efdp;
-	struct xfs_bmap_free_item *free, *next;
+	struct xfs_bmap_free_item *free;
 	xfs_fsblock_t		startblock_fsb;
 	int			error = 0;
 
@@ -3659,11 +3659,7 @@ xlog_recover_process_efi(
 	}
 
 	efdp = xfs_trans_log_efd(tp, efip, &efip->efi_flist);
-
-	for (free = efip->efi_flist.xbf_first; free != NULL; free = next) {
-		next = free->xbfi_next;
-		xfs_bmap_del_free(&efip->efi_flist, NULL, free);
-	}
+	xfs_bmap_cancel(&efip->efi_flist);
 
 	set_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
 	error = xfs_trans_commit(tp, 0);
-- 
1.7.10.4


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/7] decouple the in-memory from the on-disk log format

[Dave Chinner]

On Sat, Nov 23, 2013 at 07:11:51AM -0800, Christoph Hellwig wrote:
> Since the introduction of the CIL we already have a layer of indirection
> between the physical log format and the data structure tracking the
> changes in memory.  But due to the way iop_format works we are still
> forced to keep a copy of everything that goes out to the log in memory
> even before copying it into the CIL.
> 
> The first patch in this series changes iop_format so that the log items
> are free to store their in-memory data however they want before formatting
> them into the CIL, and the other patches take advantage of that by not
> keeping most log formats in memory all the time.  Especially the EFI and
> EFD related ones at the end start to show the benefit.

OK, it makes sense from that perspective - it should reduce the
memory footprint and simplify the code, if nothing else...

> What's missing from this series are larger changes to the in-core inode
> layout.  No needing the full struct icdinode at all times will be the
> biggest benefit of this change, but it will be large enough series of it's
> own.

Can you give us an outline of where you are taking this code and
what problems you are trying to solve? I'm missing the big picture
view that is driving this work - I think I know where you are going
to take it (i.e. closer integration with the vfs struct inode to
remove a bunch of duplicate information), but I'd like to know for
sure where this is going before looking at the code in real depth...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/7] decouple the in-memory from the on-disk log format

[Christoph Hellwig]

On Mon, Nov 25, 2013 at 07:54:15PM +1100, Dave Chinner wrote:
> Can you give us an outline of where you are taking this code and
> what problems you are trying to solve? I'm missing the big picture
> view that is driving this work - I think I know where you are going
> to take it (i.e. closer integration with the vfs struct inode to
> remove a bunch of duplicate information), but I'd like to know for
> sure where this is going before looking at the code in real depth...

There's no problem per se, it's just the requiring the in-memory copy
of the logged data is hugely inefficient.  We already see that with the
three patches dropping the trivial log format, and it will be even
bigger for the inode.  Basically the next step is to get rid of
struct icdinode - the structure will still exist sa struct xfs_log_inode
or similar, but only inside the log item.  The other fields will get
merged into struct xfs_inode.  Most of them will then go away in favour
of using exist VFS inode fields or moving into struct xfs_ifork so
that a lot of data vs attribute fork special casing can go away.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs