[RFC, PATCH 0/4] xfs: clean up xlog_recover_process_data

[RFC, PATCH 0/4] xfs: clean up xlog_recover_process_data

[Dave Chinner]

Hi folks,

The log recovery use-after-free that Brian posted a patch for has
had several previous attempts sent and none have completed review.
So let's put this one to bed for good.

This patch set addresses the previous review feedback for fixing
this problem. It factors xlog_recover_process_data() to make it
cleaner and isolate the context of the transaction recvoery
structure that is causing problems. It fixes a leak of the structure
in an error condition that I noticed while factoring, as well as the
double free that Brian and others have identified and tried to fix
in the past. It then re-arranges the recovery item management code
to put it all in one place, rather than in 3 separate parts of the
file separated by several hundred lines of unrelated code.

Comments?

-Dave.

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

[PATCH 1/4] xfs: refactor xlog_recover_process_data()

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Clean up xlog_recover_process_data() structure in preparation for
fixing the allocationa nd freeing context of the transaction being
recovered.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_recover.c | 151 ++++++++++++++++++++++++++---------------------
 1 file changed, 84 insertions(+), 67 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 01becbb..1970732f 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3531,12 +3531,78 @@ out:
 }
 
 STATIC int
-xlog_recover_unmount_trans(
-	struct xlog		*log)
+xlog_recovery_process_ophdr(
+	struct xlog		*log,
+	struct hlist_head	rhash[],
+	struct xlog_rec_header	*rhead,
+	struct xlog_op_header	*ohead,
+	xfs_caddr_t		dp,
+	xfs_caddr_t		lp,
+	int			pass)
 {
-	/* Do nothing now */
-	xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
-	return 0;
+	struct xlog_recover	*trans;
+	xlog_tid_t		tid;
+	int			error;
+	unsigned long		hash;
+	uint			flags;
+	unsigned int		hlen;
+
+	hlen = be32_to_cpu(ohead->oh_len);
+	tid = be32_to_cpu(ohead->oh_tid);
+	hash = XLOG_RHASH(tid);
+	trans = xlog_recover_find_tid(&rhash[hash], tid);
+	if (!trans) {
+		/* add new tid if this is a new transaction */
+		if (ohead->oh_flags & XLOG_START_TRANS) {
+			xlog_recover_new_tid(&rhash[hash], tid,
+					     be64_to_cpu(rhead->h_lsn));
+		}
+		return 0;
+	}
+
+	error = -EIO;
+	if (dp + hlen > lp) {
+		xfs_warn(log->l_mp, "%s: bad length 0x%x", __func__, hlen);
+		WARN_ON(1);
+		goto out_free;
+	}
+
+	flags = ohead->oh_flags & ~XLOG_END_TRANS;
+	if (flags & XLOG_WAS_CONT_TRANS)
+		flags &= ~XLOG_CONTINUE_TRANS;
+
+	switch (flags) {
+	/* expected flag values */
+	case 0:
+	case XLOG_CONTINUE_TRANS:
+		error = xlog_recover_add_to_trans(log, trans, dp, hlen);
+		break;
+	case XLOG_WAS_CONT_TRANS:
+		error = xlog_recover_add_to_cont_trans(log, trans, dp, hlen);
+		break;
+	case XLOG_COMMIT_TRANS:
+		error = xlog_recover_commit_trans(log, trans, pass);
+		break;
+
+	/* unexpected flag values */
+	case XLOG_UNMOUNT_TRANS:
+		xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
+		error = 0;
+		break;
+	case XLOG_START_TRANS:
+		xfs_warn(log->l_mp, "%s: bad transaction 0x%x", __func__, tid);
+		ASSERT(0);
+		break;
+	default:
+		xfs_warn(log->l_mp, "%s: bad flag 0x%x", __func__, flags);
+		ASSERT(0);
+		break;
+	}
+
+out_free:
+	if (error)
+		xlog_recover_free_trans(trans);
+	return error;
 }
 
 /*
@@ -3556,14 +3622,10 @@ xlog_recover_process_data(
 	xfs_caddr_t		dp,
 	int			pass)
 {
+	struct xlog_op_header	*ohead;
 	xfs_caddr_t		lp;
 	int			num_logops;
-	xlog_op_header_t	*ohead;
-	xlog_recover_t		*trans;
-	xlog_tid_t		tid;
 	int			error;
-	unsigned long		hash;
-	uint			flags;
 
 	lp = dp + be32_to_cpu(rhead->h_len);
 	num_logops = be32_to_cpu(rhead->h_num_logops);
@@ -3573,69 +3635,24 @@ xlog_recover_process_data(
 		return -EIO;
 
 	while ((dp < lp) && num_logops) {
-		ASSERT(dp + sizeof(xlog_op_header_t) <= lp);
-		ohead = (xlog_op_header_t *)dp;
-		dp += sizeof(xlog_op_header_t);
+		ASSERT(dp + sizeof(struct xlog_op_header) <= lp);
+
+		ohead = (struct xlog_op_header *)dp;
+		dp += sizeof(*ohead);
+
 		if (ohead->oh_clientid != XFS_TRANSACTION &&
 		    ohead->oh_clientid != XFS_LOG) {
 			xfs_warn(log->l_mp, "%s: bad clientid 0x%x",
-					__func__, ohead->oh_clientid);
+				__func__, ohead->oh_clientid);
 			ASSERT(0);
 			return -EIO;
 		}
-		tid = be32_to_cpu(ohead->oh_tid);
-		hash = XLOG_RHASH(tid);
-		trans = xlog_recover_find_tid(&rhash[hash], tid);
-		if (trans == NULL) {		   /* not found; add new tid */
-			if (ohead->oh_flags & XLOG_START_TRANS)
-				xlog_recover_new_tid(&rhash[hash], tid,
-					be64_to_cpu(rhead->h_lsn));
-		} else {
-			if (dp + be32_to_cpu(ohead->oh_len) > lp) {
-				xfs_warn(log->l_mp, "%s: bad length 0x%x",
-					__func__, be32_to_cpu(ohead->oh_len));
-				WARN_ON(1);
-				return -EIO;
-			}
-			flags = ohead->oh_flags & ~XLOG_END_TRANS;
-			if (flags & XLOG_WAS_CONT_TRANS)
-				flags &= ~XLOG_CONTINUE_TRANS;
-			switch (flags) {
-			case XLOG_COMMIT_TRANS:
-				error = xlog_recover_commit_trans(log,
-								trans, pass);
-				break;
-			case XLOG_UNMOUNT_TRANS:
-				error = xlog_recover_unmount_trans(log);
-				break;
-			case XLOG_WAS_CONT_TRANS:
-				error = xlog_recover_add_to_cont_trans(log,
-						trans, dp,
-						be32_to_cpu(ohead->oh_len));
-				break;
-			case XLOG_START_TRANS:
-				xfs_warn(log->l_mp, "%s: bad transaction",
-					__func__);
-				ASSERT(0);
-				error = -EIO;
-				break;
-			case 0:
-			case XLOG_CONTINUE_TRANS:
-				error = xlog_recover_add_to_trans(log, trans,
-						dp, be32_to_cpu(ohead->oh_len));
-				break;
-			default:
-				xfs_warn(log->l_mp, "%s: bad flag 0x%x",
-					__func__, flags);
-				ASSERT(0);
-				error = -EIO;
-				break;
-			}
-			if (error) {
-				xlog_recover_free_trans(trans);
-				return error;
-			}
-		}
+
+		error = xlog_recovery_process_ophdr(log, rhash, rhead, ohead,
+						    dp, lp, pass);
+		if (error)
+			return error;
+
 		dp += be32_to_cpu(ohead->oh_len);
 		num_logops--;
 	}
-- 
2.0.0

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

[PATCH 2/4] xfs: recovery of XLOG_UNMOUNT_TRANS leaks memory

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

It aborts recovery without freeing the current trans structure that
we are decoding.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_recover.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 1970732f..460cf98 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3587,8 +3587,9 @@ xlog_recovery_process_ophdr(
 	/* unexpected flag values */
 	case XLOG_UNMOUNT_TRANS:
 		xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
-		error = 0;
-		break;
+		xlog_recover_free_trans(trans);
+		return 0;
+
 	case XLOG_START_TRANS:
 		xfs_warn(log->l_mp, "%s: bad transaction 0x%x", __func__, tid);
 		ASSERT(0);
-- 
2.0.0

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

[PATCH 3/4] xfs: fix double free in xlog_recover_commit_trans

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

When an error occurs during buffer submission in
xlog_recover_commit_trans(), we free the trans structure twice. Fix
it by only freeing the structure in the caller regardless of the
success or failure of the function.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_recover.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 460cf98..23895d5 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3524,8 +3524,6 @@ out:
 	if (!list_empty(&done_list))
 		list_splice_init(&done_list, &trans->r_itemq);
 
-	xlog_recover_free_trans(trans);
-
 	error2 = xfs_buf_delwri_submit(&buffer_list);
 	return error ? error : error2;
 }
@@ -3571,6 +3569,11 @@ xlog_recovery_process_ophdr(
 	if (flags & XLOG_WAS_CONT_TRANS)
 		flags &= ~XLOG_CONTINUE_TRANS;
 
+	/*
+	 * Callees must not free the trans structure. We own it, so we'll decide
+	 * if we need to free it or not based on the operation being done and
+	 * it's result.
+	 */
 	switch (flags) {
 	/* expected flag values */
 	case 0:
@@ -3582,7 +3585,8 @@ xlog_recovery_process_ophdr(
 		break;
 	case XLOG_COMMIT_TRANS:
 		error = xlog_recover_commit_trans(log, trans, pass);
-		break;
+		xlog_recover_free_trans(trans);
+		return error;
 
 	/* unexpected flag values */
 	case XLOG_UNMOUNT_TRANS:
-- 
2.0.0

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

[PATCH 4/4] xfs: reorganise transaction recovery item code

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The code for managing transactions anf the items for recovery is
spread across 3 different locations in the file. Move them all
together so that it is easy to read the code without needing to jump
long distances in the file.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_recover.c | 358 +++++++++++++++++++++++------------------------
 1 file changed, 179 insertions(+), 179 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 23895d5..b36d96a 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1441,160 +1441,6 @@ xlog_clear_stale_blocks(
  ******************************************************************************
  */
 
-STATIC xlog_recover_t *
-xlog_recover_find_tid(
-	struct hlist_head	*head,
-	xlog_tid_t		tid)
-{
-	xlog_recover_t		*trans;
-
-	hlist_for_each_entry(trans, head, r_list) {
-		if (trans->r_log_tid == tid)
-			return trans;
-	}
-	return NULL;
-}
-
-STATIC void
-xlog_recover_new_tid(
-	struct hlist_head	*head,
-	xlog_tid_t		tid,
-	xfs_lsn_t		lsn)
-{
-	xlog_recover_t		*trans;
-
-	trans = kmem_zalloc(sizeof(xlog_recover_t), KM_SLEEP);
-	trans->r_log_tid   = tid;
-	trans->r_lsn	   = lsn;
-	INIT_LIST_HEAD(&trans->r_itemq);
-
-	INIT_HLIST_NODE(&trans->r_list);
-	hlist_add_head(&trans->r_list, head);
-}
-
-STATIC void
-xlog_recover_add_item(
-	struct list_head	*head)
-{
-	xlog_recover_item_t	*item;
-
-	item = kmem_zalloc(sizeof(xlog_recover_item_t), KM_SLEEP);
-	INIT_LIST_HEAD(&item->ri_list);
-	list_add_tail(&item->ri_list, head);
-}
-
-STATIC int
-xlog_recover_add_to_cont_trans(
-	struct xlog		*log,
-	struct xlog_recover	*trans,
-	xfs_caddr_t		dp,
-	int			len)
-{
-	xlog_recover_item_t	*item;
-	xfs_caddr_t		ptr, old_ptr;
-	int			old_len;
-
-	if (list_empty(&trans->r_itemq)) {
-		/* finish copying rest of trans header */
-		xlog_recover_add_item(&trans->r_itemq);
-		ptr = (xfs_caddr_t) &trans->r_theader +
-				sizeof(xfs_trans_header_t) - len;
-		memcpy(ptr, dp, len); /* d, s, l */
-		return 0;
-	}
-	/* take the tail entry */
-	item = list_entry(trans->r_itemq.prev, xlog_recover_item_t, ri_list);
-
-	old_ptr = item->ri_buf[item->ri_cnt-1].i_addr;
-	old_len = item->ri_buf[item->ri_cnt-1].i_len;
-
-	ptr = kmem_realloc(old_ptr, len+old_len, old_len, KM_SLEEP);
-	memcpy(&ptr[old_len], dp, len); /* d, s, l */
-	item->ri_buf[item->ri_cnt-1].i_len += len;
-	item->ri_buf[item->ri_cnt-1].i_addr = ptr;
-	trace_xfs_log_recover_item_add_cont(log, trans, item, 0);
-	return 0;
-}
-
-/*
- * The next region to add is the start of a new region.  It could be
- * a whole region or it could be the first part of a new region.  Because
- * of this, the assumption here is that the type and size fields of all
- * format structures fit into the first 32 bits of the structure.
- *
- * This works because all regions must be 32 bit aligned.  Therefore, we
- * either have both fields or we have neither field.  In the case we have
- * neither field, the data part of the region is zero length.  We only have
- * a log_op_header and can throw away the header since a new one will appear
- * later.  If we have at least 4 bytes, then we can determine how many regions
- * will appear in the current log item.
- */
-STATIC int
-xlog_recover_add_to_trans(
-	struct xlog		*log,
-	struct xlog_recover	*trans,
-	xfs_caddr_t		dp,
-	int			len)
-{
-	xfs_inode_log_format_t	*in_f;			/* any will do */
-	xlog_recover_item_t	*item;
-	xfs_caddr_t		ptr;
-
-	if (!len)
-		return 0;
-	if (list_empty(&trans->r_itemq)) {
-		/* we need to catch log corruptions here */
-		if (*(uint *)dp != XFS_TRANS_HEADER_MAGIC) {
-			xfs_warn(log->l_mp, "%s: bad header magic number",
-				__func__);
-			ASSERT(0);
-			return -EIO;
-		}
-		if (len == sizeof(xfs_trans_header_t))
-			xlog_recover_add_item(&trans->r_itemq);
-		memcpy(&trans->r_theader, dp, len); /* d, s, l */
-		return 0;
-	}
-
-	ptr = kmem_alloc(len, KM_SLEEP);
-	memcpy(ptr, dp, len);
-	in_f = (xfs_inode_log_format_t *)ptr;
-
-	/* take the tail entry */
-	item = list_entry(trans->r_itemq.prev, xlog_recover_item_t, ri_list);
-	if (item->ri_total != 0 &&
-	     item->ri_total == item->ri_cnt) {
-		/* tail item is in use, get a new one */
-		xlog_recover_add_item(&trans->r_itemq);
-		item = list_entry(trans->r_itemq.prev,
-					xlog_recover_item_t, ri_list);
-	}
-
-	if (item->ri_total == 0) {		/* first region to be added */
-		if (in_f->ilf_size == 0 ||
-		    in_f->ilf_size > XLOG_MAX_REGIONS_IN_ITEM) {
-			xfs_warn(log->l_mp,
-		"bad number of regions (%d) in inode log format",
-				  in_f->ilf_size);
-			ASSERT(0);
-			kmem_free(ptr);
-			return -EIO;
-		}
-
-		item->ri_total = in_f->ilf_size;
-		item->ri_buf =
-			kmem_zalloc(item->ri_total * sizeof(xfs_log_iovec_t),
-				    KM_SLEEP);
-	}
-	ASSERT(item->ri_total > item->ri_cnt);
-	/* Description region is ri_buf[0] */
-	item->ri_buf[item->ri_cnt].i_addr = ptr;
-	item->ri_buf[item->ri_cnt].i_len  = len;
-	item->ri_cnt++;
-	trace_xfs_log_recover_item_add(log, trans, item, 0);
-	return 0;
-}
-
 /*
  * Sort the log items in the transaction.
  *
@@ -3250,31 +3096,6 @@ xlog_recover_do_icreate_pass2(
 	return 0;
 }
 
-/*
- * Free up any resources allocated by the transaction
- *
- * Remember that EFIs, EFDs, and IUNLINKs are handled later.
- */
-STATIC void
-xlog_recover_free_trans(
-	struct xlog_recover	*trans)
-{
-	xlog_recover_item_t	*item, *n;
-	int			i;
-
-	list_for_each_entry_safe(item, n, &trans->r_itemq, ri_list) {
-		/* Free the regions in the item. */
-		list_del(&item->ri_list);
-		for (i = 0; i < item->ri_cnt; i++)
-			kmem_free(item->ri_buf[i].i_addr);
-		/* Free the item itself */
-		kmem_free(item->ri_buf);
-		kmem_free(item);
-	}
-	/* Free the transaction recover structure */
-	kmem_free(trans);
-}
-
 STATIC void
 xlog_recover_buffer_ra_pass2(
 	struct xlog                     *log,
@@ -3528,6 +3349,185 @@ out:
 	return error ? error : error2;
 }
 
+
+STATIC xlog_recover_t *
+xlog_recover_find_tid(
+	struct hlist_head	*head,
+	xlog_tid_t		tid)
+{
+	xlog_recover_t		*trans;
+
+	hlist_for_each_entry(trans, head, r_list) {
+		if (trans->r_log_tid == tid)
+			return trans;
+	}
+	return NULL;
+}
+
+STATIC void
+xlog_recover_new_tid(
+	struct hlist_head	*head,
+	xlog_tid_t		tid,
+	xfs_lsn_t		lsn)
+{
+	xlog_recover_t		*trans;
+
+	trans = kmem_zalloc(sizeof(xlog_recover_t), KM_SLEEP);
+	trans->r_log_tid   = tid;
+	trans->r_lsn	   = lsn;
+	INIT_LIST_HEAD(&trans->r_itemq);
+
+	INIT_HLIST_NODE(&trans->r_list);
+	hlist_add_head(&trans->r_list, head);
+}
+
+STATIC void
+xlog_recover_add_item(
+	struct list_head	*head)
+{
+	xlog_recover_item_t	*item;
+
+	item = kmem_zalloc(sizeof(xlog_recover_item_t), KM_SLEEP);
+	INIT_LIST_HEAD(&item->ri_list);
+	list_add_tail(&item->ri_list, head);
+}
+
+STATIC int
+xlog_recover_add_to_cont_trans(
+	struct xlog		*log,
+	struct xlog_recover	*trans,
+	xfs_caddr_t		dp,
+	int			len)
+{
+	xlog_recover_item_t	*item;
+	xfs_caddr_t		ptr, old_ptr;
+	int			old_len;
+
+	if (list_empty(&trans->r_itemq)) {
+		/* finish copying rest of trans header */
+		xlog_recover_add_item(&trans->r_itemq);
+		ptr = (xfs_caddr_t) &trans->r_theader +
+				sizeof(xfs_trans_header_t) - len;
+		memcpy(ptr, dp, len);
+		return 0;
+	}
+	/* take the tail entry */
+	item = list_entry(trans->r_itemq.prev, xlog_recover_item_t, ri_list);
+
+	old_ptr = item->ri_buf[item->ri_cnt-1].i_addr;
+	old_len = item->ri_buf[item->ri_cnt-1].i_len;
+
+	ptr = kmem_realloc(old_ptr, len+old_len, old_len, KM_SLEEP);
+	memcpy(&ptr[old_len], dp, len);
+	item->ri_buf[item->ri_cnt-1].i_len += len;
+	item->ri_buf[item->ri_cnt-1].i_addr = ptr;
+	trace_xfs_log_recover_item_add_cont(log, trans, item, 0);
+	return 0;
+}
+
+/*
+ * The next region to add is the start of a new region.  It could be
+ * a whole region or it could be the first part of a new region.  Because
+ * of this, the assumption here is that the type and size fields of all
+ * format structures fit into the first 32 bits of the structure.
+ *
+ * This works because all regions must be 32 bit aligned.  Therefore, we
+ * either have both fields or we have neither field.  In the case we have
+ * neither field, the data part of the region is zero length.  We only have
+ * a log_op_header and can throw away the header since a new one will appear
+ * later.  If we have at least 4 bytes, then we can determine how many regions
+ * will appear in the current log item.
+ */
+STATIC int
+xlog_recover_add_to_trans(
+	struct xlog		*log,
+	struct xlog_recover	*trans,
+	xfs_caddr_t		dp,
+	int			len)
+{
+	xfs_inode_log_format_t	*in_f;			/* any will do */
+	xlog_recover_item_t	*item;
+	xfs_caddr_t		ptr;
+
+	if (!len)
+		return 0;
+	if (list_empty(&trans->r_itemq)) {
+		/* we need to catch log corruptions here */
+		if (*(uint *)dp != XFS_TRANS_HEADER_MAGIC) {
+			xfs_warn(log->l_mp, "%s: bad header magic number",
+				__func__);
+			ASSERT(0);
+			return -EIO;
+		}
+		if (len == sizeof(xfs_trans_header_t))
+			xlog_recover_add_item(&trans->r_itemq);
+		memcpy(&trans->r_theader, dp, len);
+		return 0;
+	}
+
+	ptr = kmem_alloc(len, KM_SLEEP);
+	memcpy(ptr, dp, len);
+	in_f = (xfs_inode_log_format_t *)ptr;
+
+	/* take the tail entry */
+	item = list_entry(trans->r_itemq.prev, xlog_recover_item_t, ri_list);
+	if (item->ri_total != 0 &&
+	     item->ri_total == item->ri_cnt) {
+		/* tail item is in use, get a new one */
+		xlog_recover_add_item(&trans->r_itemq);
+		item = list_entry(trans->r_itemq.prev,
+					xlog_recover_item_t, ri_list);
+	}
+
+	if (item->ri_total == 0) {		/* first region to be added */
+		if (in_f->ilf_size == 0 ||
+		    in_f->ilf_size > XLOG_MAX_REGIONS_IN_ITEM) {
+			xfs_warn(log->l_mp,
+		"bad number of regions (%d) in inode log format",
+				  in_f->ilf_size);
+			ASSERT(0);
+			kmem_free(ptr);
+			return -EIO;
+		}
+
+		item->ri_total = in_f->ilf_size;
+		item->ri_buf =
+			kmem_zalloc(item->ri_total * sizeof(xfs_log_iovec_t),
+				    KM_SLEEP);
+	}
+	ASSERT(item->ri_total > item->ri_cnt);
+	/* Description region is ri_buf[0] */
+	item->ri_buf[item->ri_cnt].i_addr = ptr;
+	item->ri_buf[item->ri_cnt].i_len  = len;
+	item->ri_cnt++;
+	trace_xfs_log_recover_item_add(log, trans, item, 0);
+	return 0;
+}
+/*
+ * Free up any resources allocated by the transaction
+ *
+ * Remember that EFIs, EFDs, and IUNLINKs are handled later.
+ */
+STATIC void
+xlog_recover_free_trans(
+	struct xlog_recover	*trans)
+{
+	xlog_recover_item_t	*item, *n;
+	int			i;
+
+	list_for_each_entry_safe(item, n, &trans->r_itemq, ri_list) {
+		/* Free the regions in the item. */
+		list_del(&item->ri_list);
+		for (i = 0; i < item->ri_cnt; i++)
+			kmem_free(item->ri_buf[i].i_addr);
+		/* Free the item itself */
+		kmem_free(item->ri_buf);
+		kmem_free(item);
+	}
+	/* Free the transaction recover structure */
+	kmem_free(trans);
+}
+
 STATIC int
 xlog_recovery_process_ophdr(
 	struct xlog		*log,
-- 
2.0.0

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

Re: [PATCH 1/4] xfs: refactor xlog_recover_process_data()

[Christoph Hellwig]

> @@ -3556,14 +3622,10 @@ xlog_recover_process_data(
>  	xfs_caddr_t		dp,
>  	int			pass)
>  {
> +	struct xlog_op_header	*ohead;
>  	xfs_caddr_t		lp;
>  	int			num_logops;
>  	int			error;
>  
>  	lp = dp + be32_to_cpu(rhead->h_len);
>  	num_logops = be32_to_cpu(rhead->h_num_logops);
> @@ -3573,69 +3635,24 @@ xlog_recover_process_data(
>  		return -EIO;
>  
>  	while ((dp < lp) && num_logops) {
> +		ASSERT(dp + sizeof(struct xlog_op_header) <= lp);
> +
> +		ohead = (struct xlog_op_header *)dp;
> +		dp += sizeof(*ohead);

Using sizeof type and sizeof variable for the same thing right next
to each other seems weird.  Also why duplicate the addition instead
of moving it below the assignment:

		ohead = (struct xlog_op_header *)dp;
		dp += sizeof(*ohead);

		ASSERT(dp <= lp);

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

Re: [PATCH 1/4] xfs: refactor xlog_recover_process_data()

[Dave Chinner]

On Mon, Aug 25, 2014 at 09:09:21PM -0700, Christoph Hellwig wrote:
> > @@ -3556,14 +3622,10 @@ xlog_recover_process_data(
> >  	xfs_caddr_t		dp,
> >  	int			pass)
> >  {
> > +	struct xlog_op_header	*ohead;
> >  	xfs_caddr_t		lp;
> >  	int			num_logops;
> >  	int			error;
> >  
> >  	lp = dp + be32_to_cpu(rhead->h_len);
> >  	num_logops = be32_to_cpu(rhead->h_num_logops);
> > @@ -3573,69 +3635,24 @@ xlog_recover_process_data(
> >  		return -EIO;
> >  
> >  	while ((dp < lp) && num_logops) {
> > +		ASSERT(dp + sizeof(struct xlog_op_header) <= lp);
> > +
> > +		ohead = (struct xlog_op_header *)dp;
> > +		dp += sizeof(*ohead);
> 
> Using sizeof type and sizeof variable for the same thing right next
> to each other seems weird.  Also why duplicate the addition instead
> of moving it below the assignment:

Oh, I missed converting the one in the ASSERT.

> 		ohead = (struct xlog_op_header *)dp;
> 		dp += sizeof(*ohead);
> 
> 		ASSERT(dp <= lp);

Yup, that makes sense.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [RFC, PATCH 0/4] xfs: clean up xlog_recover_process_data

[Brian Foster]

On Tue, Aug 26, 2014 at 11:21:37AM +1000, Dave Chinner wrote:
> Hi folks,
> 
> The log recovery use-after-free that Brian posted a patch for has
> had several previous attempts sent and none have completed review.
> So let's put this one to bed for good.
> 
> This patch set addresses the previous review feedback for fixing
> this problem. It factors xlog_recover_process_data() to make it
> cleaner and isolate the context of the transaction recvoery
> structure that is causing problems. It fixes a leak of the structure
> in an error condition that I noticed while factoring, as well as the
> double free that Brian and others have identified and tried to fix
> in the past. It then re-arranges the recovery item management code
> to put it all in one place, rather than in 3 separate parts of the
> file separated by several hundred lines of unrelated code.
> 
> Comments?
> 

This looks pretty good to me. I like the cleanups and it fixes the
problem. A few minor comments/questions to follow.

Brian

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

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

Re: [PATCH 1/4] xfs: refactor xlog_recover_process_data()

[Brian Foster]

On Tue, Aug 26, 2014 at 11:21:38AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Clean up xlog_recover_process_data() structure in preparation for
> fixing the allocationa nd freeing context of the transaction being
> recovered.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log_recover.c | 151 ++++++++++++++++++++++++++---------------------
>  1 file changed, 84 insertions(+), 67 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 01becbb..1970732f 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3531,12 +3531,78 @@ out:
>  }
>  
>  STATIC int
> -xlog_recover_unmount_trans(
> -	struct xlog		*log)
> +xlog_recovery_process_ophdr(
> +	struct xlog		*log,
> +	struct hlist_head	rhash[],
> +	struct xlog_rec_header	*rhead,
> +	struct xlog_op_header	*ohead,
> +	xfs_caddr_t		dp,
> +	xfs_caddr_t		lp,
> +	int			pass)
>  {
> -	/* Do nothing now */
> -	xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
> -	return 0;
> +	struct xlog_recover	*trans;
> +	xlog_tid_t		tid;
> +	int			error;
> +	unsigned long		hash;
> +	uint			flags;
> +	unsigned int		hlen;
> +
> +	hlen = be32_to_cpu(ohead->oh_len);
> +	tid = be32_to_cpu(ohead->oh_tid);
> +	hash = XLOG_RHASH(tid);
> +	trans = xlog_recover_find_tid(&rhash[hash], tid);
> +	if (!trans) {
> +		/* add new tid if this is a new transaction */
> +		if (ohead->oh_flags & XLOG_START_TRANS) {
> +			xlog_recover_new_tid(&rhash[hash], tid,
> +					     be64_to_cpu(rhead->h_lsn));
> +		}
> +		return 0;
> +	}
> +

Overall this looks pretty good to me. I wonder if we can clean this up
to separate state management from error detection while we're at it. I
don't see any reason this code to find trans has to be up here.

> +	error = -EIO;
> +	if (dp + hlen > lp) {
> +		xfs_warn(log->l_mp, "%s: bad length 0x%x", __func__, hlen);
> +		WARN_ON(1);
> +		goto out_free;
> +	}
> +
> +	flags = ohead->oh_flags & ~XLOG_END_TRANS;
> +	if (flags & XLOG_WAS_CONT_TRANS)
> +		flags &= ~XLOG_CONTINUE_TRANS;
> +

	/* we should find a trans for anything other than a start op */
	trans = xlog_recover_find_tid(&rhash[hash], tid);
	if (((ohead->oh_flags & XLOG_START_TRANS) && trans) ||
	    (!(ohead->oh_flags & XLOG_START_TRANS) && !trans)) {
		xfs_warn(log->l_mp, "%s: bad transaction 0x%x oh_flags 0x%x trans %p",
			 __func__, tid, ohead->oh_flags, trans);
		ASSERT(0);
		return -EIO;
	}

Maybe returning error here is not the right thing to do because we want
the recovery to proceed. We could still dump a warning and return 0
though.

> +	switch (flags) {
> +	/* expected flag values */
> +	case 0:
> +	case XLOG_CONTINUE_TRANS:
> +		error = xlog_recover_add_to_trans(log, trans, dp, hlen);
> +		break;
> +	case XLOG_WAS_CONT_TRANS:
> +		error = xlog_recover_add_to_cont_trans(log, trans, dp, hlen);
> +		break;
> +	case XLOG_COMMIT_TRANS:
> +		error = xlog_recover_commit_trans(log, trans, pass);
> +		break;
> +
> +	/* unexpected flag values */
> +	case XLOG_UNMOUNT_TRANS:
> +		xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
> +		error = 0;
> +		break;
> +	case XLOG_START_TRANS:
> +		xfs_warn(log->l_mp, "%s: bad transaction 0x%x", __func__, tid);
> +		ASSERT(0);
> +		break;

		xlog_recover_new_tid(&rhash[hash], tid, be64_to_cpu(rhead->h_lsn)
		error = 0;
		break;

Brian

> +	default:
> +		xfs_warn(log->l_mp, "%s: bad flag 0x%x", __func__, flags);
> +		ASSERT(0);
> +		break;
> +	}
> +
> +out_free:
> +	if (error)
> +		xlog_recover_free_trans(trans);
> +	return error;
>  }
>  
>  /*
> @@ -3556,14 +3622,10 @@ xlog_recover_process_data(
>  	xfs_caddr_t		dp,
>  	int			pass)
>  {
> +	struct xlog_op_header	*ohead;
>  	xfs_caddr_t		lp;
>  	int			num_logops;
> -	xlog_op_header_t	*ohead;
> -	xlog_recover_t		*trans;
> -	xlog_tid_t		tid;
>  	int			error;
> -	unsigned long		hash;
> -	uint			flags;
>  
>  	lp = dp + be32_to_cpu(rhead->h_len);
>  	num_logops = be32_to_cpu(rhead->h_num_logops);
> @@ -3573,69 +3635,24 @@ xlog_recover_process_data(
>  		return -EIO;
>  
>  	while ((dp < lp) && num_logops) {
> -		ASSERT(dp + sizeof(xlog_op_header_t) <= lp);
> -		ohead = (xlog_op_header_t *)dp;
> -		dp += sizeof(xlog_op_header_t);
> +		ASSERT(dp + sizeof(struct xlog_op_header) <= lp);
> +
> +		ohead = (struct xlog_op_header *)dp;
> +		dp += sizeof(*ohead);
> +
>  		if (ohead->oh_clientid != XFS_TRANSACTION &&
>  		    ohead->oh_clientid != XFS_LOG) {
>  			xfs_warn(log->l_mp, "%s: bad clientid 0x%x",
> -					__func__, ohead->oh_clientid);
> +				__func__, ohead->oh_clientid);
>  			ASSERT(0);
>  			return -EIO;
>  		}
> -		tid = be32_to_cpu(ohead->oh_tid);
> -		hash = XLOG_RHASH(tid);
> -		trans = xlog_recover_find_tid(&rhash[hash], tid);
> -		if (trans == NULL) {		   /* not found; add new tid */
> -			if (ohead->oh_flags & XLOG_START_TRANS)
> -				xlog_recover_new_tid(&rhash[hash], tid,
> -					be64_to_cpu(rhead->h_lsn));
> -		} else {
> -			if (dp + be32_to_cpu(ohead->oh_len) > lp) {
> -				xfs_warn(log->l_mp, "%s: bad length 0x%x",
> -					__func__, be32_to_cpu(ohead->oh_len));
> -				WARN_ON(1);
> -				return -EIO;
> -			}
> -			flags = ohead->oh_flags & ~XLOG_END_TRANS;
> -			if (flags & XLOG_WAS_CONT_TRANS)
> -				flags &= ~XLOG_CONTINUE_TRANS;
> -			switch (flags) {
> -			case XLOG_COMMIT_TRANS:
> -				error = xlog_recover_commit_trans(log,
> -								trans, pass);
> -				break;
> -			case XLOG_UNMOUNT_TRANS:
> -				error = xlog_recover_unmount_trans(log);
> -				break;
> -			case XLOG_WAS_CONT_TRANS:
> -				error = xlog_recover_add_to_cont_trans(log,
> -						trans, dp,
> -						be32_to_cpu(ohead->oh_len));
> -				break;
> -			case XLOG_START_TRANS:
> -				xfs_warn(log->l_mp, "%s: bad transaction",
> -					__func__);
> -				ASSERT(0);
> -				error = -EIO;
> -				break;
> -			case 0:
> -			case XLOG_CONTINUE_TRANS:
> -				error = xlog_recover_add_to_trans(log, trans,
> -						dp, be32_to_cpu(ohead->oh_len));
> -				break;
> -			default:
> -				xfs_warn(log->l_mp, "%s: bad flag 0x%x",
> -					__func__, flags);
> -				ASSERT(0);
> -				error = -EIO;
> -				break;
> -			}
> -			if (error) {
> -				xlog_recover_free_trans(trans);
> -				return error;
> -			}
> -		}
> +
> +		error = xlog_recovery_process_ophdr(log, rhash, rhead, ohead,
> +						    dp, lp, pass);
> +		if (error)
> +			return error;
> +
>  		dp += be32_to_cpu(ohead->oh_len);
>  		num_logops--;
>  	}
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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

Re: [PATCH 2/4] xfs: recovery of XLOG_UNMOUNT_TRANS leaks memory

[Brian Foster]

On Tue, Aug 26, 2014 at 11:21:39AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> It aborts recovery without freeing the current trans structure that
> we are decoding.
> 

What do you mean by "aborts recovery?" I don't see anything in the code
that reflects that behavior. Do you mean it's an on-disk marker for
completion?

> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log_recover.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 1970732f..460cf98 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3587,8 +3587,9 @@ xlog_recovery_process_ophdr(
>  	/* unexpected flag values */
>  	case XLOG_UNMOUNT_TRANS:
>  		xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
> -		error = 0;
> -		break;
> +		xlog_recover_free_trans(trans);
> +		return 0;
> +

The change to return here seems superfluous. It's fine, but just to
check, were you intending to alter behavior in some way (e.g., return
from xlog_recover_process_data())?

Brian

>  	case XLOG_START_TRANS:
>  		xfs_warn(log->l_mp, "%s: bad transaction 0x%x", __func__, tid);
>  		ASSERT(0);
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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

Re: [PATCH 3/4] xfs: fix double free in xlog_recover_commit_trans

[Brian Foster]

On Tue, Aug 26, 2014 at 11:21:40AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When an error occurs during buffer submission in
> xlog_recover_commit_trans(), we free the trans structure twice. Fix
> it by only freeing the structure in the caller regardless of the
> success or failure of the function.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_log_recover.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 460cf98..23895d5 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3524,8 +3524,6 @@ out:
>  	if (!list_empty(&done_list))
>  		list_splice_init(&done_list, &trans->r_itemq);
>  
> -	xlog_recover_free_trans(trans);
> -
>  	error2 = xfs_buf_delwri_submit(&buffer_list);
>  	return error ? error : error2;
>  }
> @@ -3571,6 +3569,11 @@ xlog_recovery_process_ophdr(
>  	if (flags & XLOG_WAS_CONT_TRANS)
>  		flags &= ~XLOG_CONTINUE_TRANS;
>  
> +	/*
> +	 * Callees must not free the trans structure. We own it, so we'll decide
> +	 * if we need to free it or not based on the operation being done and
> +	 * it's result.

	   its

> +	 */
>  	switch (flags) {
>  	/* expected flag values */
>  	case 0:
> @@ -3582,7 +3585,8 @@ xlog_recovery_process_ophdr(
>  		break;
>  	case XLOG_COMMIT_TRANS:
>  		error = xlog_recover_commit_trans(log, trans, pass);
> -		break;
> +		xlog_recover_free_trans(trans);
> +		return error;
>  
>  	/* unexpected flag values */
>  	case XLOG_UNMOUNT_TRANS:
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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

Re: [PATCH 1/4] xfs: refactor xlog_recover_process_data()

[Dave Chinner]

On Tue, Aug 26, 2014 at 08:41:13AM -0400, Brian Foster wrote:
> On Tue, Aug 26, 2014 at 11:21:38AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Clean up xlog_recover_process_data() structure in preparation for
> > fixing the allocationa nd freeing context of the transaction being
> > recovered.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_log_recover.c | 151 ++++++++++++++++++++++++++---------------------
> >  1 file changed, 84 insertions(+), 67 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 01becbb..1970732f 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -3531,12 +3531,78 @@ out:
> >  }
> >  
> >  STATIC int
> > -xlog_recover_unmount_trans(
> > -	struct xlog		*log)
> > +xlog_recovery_process_ophdr(
> > +	struct xlog		*log,
> > +	struct hlist_head	rhash[],
> > +	struct xlog_rec_header	*rhead,
> > +	struct xlog_op_header	*ohead,
> > +	xfs_caddr_t		dp,
> > +	xfs_caddr_t		lp,
> > +	int			pass)
> >  {
> > -	/* Do nothing now */
> > -	xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
> > -	return 0;
> > +	struct xlog_recover	*trans;
> > +	xlog_tid_t		tid;
> > +	int			error;
> > +	unsigned long		hash;
> > +	uint			flags;
> > +	unsigned int		hlen;
> > +
> > +	hlen = be32_to_cpu(ohead->oh_len);
> > +	tid = be32_to_cpu(ohead->oh_tid);
> > +	hash = XLOG_RHASH(tid);
> > +	trans = xlog_recover_find_tid(&rhash[hash], tid);
> > +	if (!trans) {
> > +		/* add new tid if this is a new transaction */
> > +		if (ohead->oh_flags & XLOG_START_TRANS) {
> > +			xlog_recover_new_tid(&rhash[hash], tid,
> > +					     be64_to_cpu(rhead->h_lsn));
> > +		}
> > +		return 0;
> > +	}
> > +
> 
> Overall this looks pretty good to me. I wonder if we can clean this up
> to separate state management from error detection while we're at it. I
> don't see any reason this code to find trans has to be up here.
> 
> > +	error = -EIO;
> > +	if (dp + hlen > lp) {
> > +		xfs_warn(log->l_mp, "%s: bad length 0x%x", __func__, hlen);
> > +		WARN_ON(1);
> > +		goto out_free;
> > +	}
> > +
> > +	flags = ohead->oh_flags & ~XLOG_END_TRANS;
> > +	if (flags & XLOG_WAS_CONT_TRANS)
> > +		flags &= ~XLOG_CONTINUE_TRANS;
> > +
> 
> 	/* we should find a trans for anything other than a start op */
> 	trans = xlog_recover_find_tid(&rhash[hash], tid);
> 	if (((ohead->oh_flags & XLOG_START_TRANS) && trans) ||
> 	    (!(ohead->oh_flags & XLOG_START_TRANS) && !trans)) {
> 		xfs_warn(log->l_mp, "%s: bad transaction 0x%x oh_flags 0x%x trans %p",
> 			 __func__, tid, ohead->oh_flags, trans);
> 		ASSERT(0);
> 		return -EIO;
> 	}
> 
> Maybe returning error here is not the right thing to do because we want
> the recovery to proceed. We could still dump a warning and return 0
> though.

Urk. Try understanding why that logic exists in a couple of years
time when you've forgetten all the context. :/

> > +	switch (flags) {
> > +	/* expected flag values */
> > +	case 0:
> > +	case XLOG_CONTINUE_TRANS:
> > +		error = xlog_recover_add_to_trans(log, trans, dp, hlen);
> > +		break;
> > +	case XLOG_WAS_CONT_TRANS:
> > +		error = xlog_recover_add_to_cont_trans(log, trans, dp, hlen);
> > +		break;
> > +	case XLOG_COMMIT_TRANS:
> > +		error = xlog_recover_commit_trans(log, trans, pass);
> > +		break;
> > +
> > +	/* unexpected flag values */
> > +	case XLOG_UNMOUNT_TRANS:
> > +		xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
> > +		error = 0;
> > +		break;
> > +	case XLOG_START_TRANS:
> > +		xfs_warn(log->l_mp, "%s: bad transaction 0x%x", __func__, tid);
> > +		ASSERT(0);
> > +		break;
> 
> 		xlog_recover_new_tid(&rhash[hash], tid, be64_to_cpu(rhead->h_lsn)
> 		error = 0;
> 		break;
> 

I like the idea, but I don't like the suggested implementation. I
was in two minds as to whether I should factor
xlog_recover_find_tid() further.  There's only one caller of it and
only one caller of xlog_recover_new_tid() and the happen within
three lines of each other. Hence I'm thinking that it makes more
sense to wrap the "find or allocate trans" code in a single helper
and lift all that logic clean out of this function. That helper can
handle all the XLOG_START_TRANS logic more cleanly, I think....

Actually, that makes the factoring I've already done a little
inconsistent. Let me rework this a bit.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 1/4] xfs: refactor xlog_recover_process_data()

[Brian Foster]

On Wed, Aug 27, 2014 at 08:34:07AM +1000, Dave Chinner wrote:
> On Tue, Aug 26, 2014 at 08:41:13AM -0400, Brian Foster wrote:
> > On Tue, Aug 26, 2014 at 11:21:38AM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Clean up xlog_recover_process_data() structure in preparation for
> > > fixing the allocationa nd freeing context of the transaction being
> > > recovered.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > ---
> > >  fs/xfs/xfs_log_recover.c | 151 ++++++++++++++++++++++++++---------------------
> > >  1 file changed, 84 insertions(+), 67 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > > index 01becbb..1970732f 100644
> > > --- a/fs/xfs/xfs_log_recover.c
> > > +++ b/fs/xfs/xfs_log_recover.c
> > > @@ -3531,12 +3531,78 @@ out:
> > >  }
> > >  
> > >  STATIC int
> > > -xlog_recover_unmount_trans(
> > > -	struct xlog		*log)
> > > +xlog_recovery_process_ophdr(
> > > +	struct xlog		*log,
> > > +	struct hlist_head	rhash[],
> > > +	struct xlog_rec_header	*rhead,
> > > +	struct xlog_op_header	*ohead,
> > > +	xfs_caddr_t		dp,
> > > +	xfs_caddr_t		lp,
> > > +	int			pass)
> > >  {
> > > -	/* Do nothing now */
> > > -	xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
> > > -	return 0;
> > > +	struct xlog_recover	*trans;
> > > +	xlog_tid_t		tid;
> > > +	int			error;
> > > +	unsigned long		hash;
> > > +	uint			flags;
> > > +	unsigned int		hlen;
> > > +
> > > +	hlen = be32_to_cpu(ohead->oh_len);
> > > +	tid = be32_to_cpu(ohead->oh_tid);
> > > +	hash = XLOG_RHASH(tid);
> > > +	trans = xlog_recover_find_tid(&rhash[hash], tid);
> > > +	if (!trans) {
> > > +		/* add new tid if this is a new transaction */
> > > +		if (ohead->oh_flags & XLOG_START_TRANS) {
> > > +			xlog_recover_new_tid(&rhash[hash], tid,
> > > +					     be64_to_cpu(rhead->h_lsn));
> > > +		}
> > > +		return 0;
> > > +	}
> > > +
> > 
> > Overall this looks pretty good to me. I wonder if we can clean this up
> > to separate state management from error detection while we're at it. I
> > don't see any reason this code to find trans has to be up here.
> > 
> > > +	error = -EIO;
> > > +	if (dp + hlen > lp) {
> > > +		xfs_warn(log->l_mp, "%s: bad length 0x%x", __func__, hlen);
> > > +		WARN_ON(1);
> > > +		goto out_free;
> > > +	}
> > > +
> > > +	flags = ohead->oh_flags & ~XLOG_END_TRANS;
> > > +	if (flags & XLOG_WAS_CONT_TRANS)
> > > +		flags &= ~XLOG_CONTINUE_TRANS;
> > > +
> > 
> > 	/* we should find a trans for anything other than a start op */
> > 	trans = xlog_recover_find_tid(&rhash[hash], tid);
> > 	if (((ohead->oh_flags & XLOG_START_TRANS) && trans) ||
> > 	    (!(ohead->oh_flags & XLOG_START_TRANS) && !trans)) {
> > 		xfs_warn(log->l_mp, "%s: bad transaction 0x%x oh_flags 0x%x trans %p",
> > 			 __func__, tid, ohead->oh_flags, trans);
> > 		ASSERT(0);
> > 		return -EIO;
> > 	}
> > 
> > Maybe returning error here is not the right thing to do because we want
> > the recovery to proceed. We could still dump a warning and return 0
> > though.
> 
> Urk. Try understanding why that logic exists in a couple of years
> time when you've forgetten all the context. :/
> 

Heh, that's the problem I had with the current code. The error checking
and state machine management is split between here and below. The above
is just an error check, and fwiw, it adds an extra check that doesn't
exist in the current code. Hide the flag busy-ness and effectively the
logic is:

	/* verify a tx is in progress or we're starting a new one */
	if (trans && is_start_header(ohead) ||
	    !trans && !is_start_header(ohead))
		return -EIO;

... which seems straightforward to me, but I'm sure there are other ways
to refactor things as well.

> > > +	switch (flags) {
> > > +	/* expected flag values */
> > > +	case 0:
> > > +	case XLOG_CONTINUE_TRANS:
> > > +		error = xlog_recover_add_to_trans(log, trans, dp, hlen);
> > > +		break;
> > > +	case XLOG_WAS_CONT_TRANS:
> > > +		error = xlog_recover_add_to_cont_trans(log, trans, dp, hlen);
> > > +		break;
> > > +	case XLOG_COMMIT_TRANS:
> > > +		error = xlog_recover_commit_trans(log, trans, pass);
> > > +		break;
> > > +
> > > +	/* unexpected flag values */
> > > +	case XLOG_UNMOUNT_TRANS:
> > > +		xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
> > > +		error = 0;
> > > +		break;
> > > +	case XLOG_START_TRANS:
> > > +		xfs_warn(log->l_mp, "%s: bad transaction 0x%x", __func__, tid);
> > > +		ASSERT(0);
> > > +		break;
> > 
> > 		xlog_recover_new_tid(&rhash[hash], tid, be64_to_cpu(rhead->h_lsn)
> > 		error = 0;
> > 		break;
> > 
> 
> I like the idea, but I don't like the suggested implementation. I
> was in two minds as to whether I should factor
> xlog_recover_find_tid() further.  There's only one caller of it and
> only one caller of xlog_recover_new_tid() and the happen within
> three lines of each other. Hence I'm thinking that it makes more
> sense to wrap the "find or allocate trans" code in a single helper
> and lift all that logic clean out of this function. That helper can
> handle all the XLOG_START_TRANS logic more cleanly, I think....
> 

Fair enough, that sounds reasonable so long as it isn't pulling the core
state management off into disparate functions. What I like about the
above (combined with the result of the rest of this series) is that the
lifecycle is obvious and contained in a single block of code.

Brian

> Actually, that makes the factoring I've already done a little
> inconsistent. Let me rework this a bit.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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

Re: [PATCH 1/4] xfs: refactor xlog_recover_process_data()

[Dave Chinner]

On Wed, Aug 27, 2014 at 07:19:10AM -0400, Brian Foster wrote:
> On Wed, Aug 27, 2014 at 08:34:07AM +1000, Dave Chinner wrote:
> > > > +	switch (flags) {
> > > > +	/* expected flag values */
> > > > +	case 0:
> > > > +	case XLOG_CONTINUE_TRANS:
> > > > +		error = xlog_recover_add_to_trans(log, trans, dp, hlen);
> > > > +		break;
> > > > +	case XLOG_WAS_CONT_TRANS:
> > > > +		error = xlog_recover_add_to_cont_trans(log, trans, dp, hlen);
> > > > +		break;
> > > > +	case XLOG_COMMIT_TRANS:
> > > > +		error = xlog_recover_commit_trans(log, trans, pass);
> > > > +		break;
> > > > +
> > > > +	/* unexpected flag values */
> > > > +	case XLOG_UNMOUNT_TRANS:
> > > > +		xfs_warn(log->l_mp, "%s: Unmount LR", __func__);
> > > > +		error = 0;
> > > > +		break;
> > > > +	case XLOG_START_TRANS:
> > > > +		xfs_warn(log->l_mp, "%s: bad transaction 0x%x", __func__, tid);
> > > > +		ASSERT(0);
> > > > +		break;
> > > 
> > > 		xlog_recover_new_tid(&rhash[hash], tid, be64_to_cpu(rhead->h_lsn)
> > > 		error = 0;
> > > 		break;
> > > 
> > 
> > I like the idea, but I don't like the suggested implementation. I
> > was in two minds as to whether I should factor
> > xlog_recover_find_tid() further.  There's only one caller of it and
> > only one caller of xlog_recover_new_tid() and the happen within
> > three lines of each other. Hence I'm thinking that it makes more
> > sense to wrap the "find or allocate trans" code in a single helper
> > and lift all that logic clean out of this function. That helper can
> > handle all the XLOG_START_TRANS logic more cleanly, I think....
> > 
> 
> Fair enough, that sounds reasonable so long as it isn't pulling the core
> state management off into disparate functions. What I like about the
> above (combined with the result of the rest of this series) is that the
> lifecycle is obvious and contained in a single block of code.

Well, it's not "state" as in "state machine". What we are doing is
decoding ophdrs, not walking through a state machine, and so I think
that the "start" ophdrs need to be treated differently because all
the other types of ophdrs have a dependency on the trans structure
existing.

Indeed, I suspect the correct thing to do is check for the start
flag *first*, before we do the lookup to find the trans structure,
because the start flag implies that we should not find an existing
trans structure for that tid. And once we've done that, we're
completely finished processing that ophdr, and hence should
return and not run any of the other code we run for all the
remaining ophdrs.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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