[PATCH v3] xfs: introduce object readahead to log recovery

[PATCH v3] xfs: introduce object readahead to log recovery

[zwu.kernel]

From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

  It can take a long time to run log recovery operation because it is
single threaded and is bound by read latency. We can find that it took
most of the time to wait for the read IO to occur, so if one object
readahead is introduced to log recovery, it will obviously reduce the
log recovery time.

Log recovery time stat:

          w/o this patch        w/ this patch

real:        0m15.023s             0m7.802s
user:        0m0.001s              0m0.001s
sys:         0m0.246s              0m0.107s

Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
 fs/xfs/xfs_log_recover.c | 159 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 153 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 7681b19..ebb00bc 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3116,6 +3116,106 @@ xlog_recover_free_trans(
 	kmem_free(trans);
 }
 
+STATIC void
+xlog_recover_buffer_ra_pass2(
+	struct xlog                     *log,
+	struct xlog_recover_item        *item)
+{
+	struct xfs_buf_log_format	*buf_f = item->ri_buf[0].i_addr;
+	struct xfs_mount		*mp = log->l_mp;
+
+	if (xlog_check_buffer_cancelled(log, buf_f->blf_blkno,
+			buf_f->blf_len, buf_f->blf_flags)) {
+		return;
+	}
+
+	xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno,
+				buf_f->blf_len, NULL);
+}
+
+STATIC void
+xlog_recover_inode_ra_pass2(
+	struct xlog                     *log,
+	struct xlog_recover_item        *item)
+{
+	struct xfs_inode_log_format	ilf_buf;
+	struct xfs_inode_log_format	*ilfp;
+	struct xfs_mount		*mp = log->l_mp;
+	int			error;
+
+	if (item->ri_buf[0].i_len == sizeof(struct xfs_inode_log_format)) {
+		ilfp = item->ri_buf[0].i_addr;
+	} else {
+		ilfp = &ilf_buf;
+		memset(ilfp, 0, sizeof(*ilfp));
+		error = xfs_inode_item_format_convert(&item->ri_buf[0], ilfp);
+		if (error)
+			return;
+	}
+
+	if (xlog_check_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len, 0))
+		return;
+
+	xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno,
+				ilfp->ilf_len, &xfs_inode_buf_ops);
+}
+
+STATIC void
+xlog_recover_dquot_ra_pass2(
+	struct xlog			*log,
+	struct xlog_recover_item	*item)
+{
+	struct xfs_mount	*mp = log->l_mp;
+	struct xfs_disk_dquot	*recddq;
+	struct xfs_dq_logformat	*dq_f;
+	uint			type;
+
+
+	if (mp->m_qflags == 0)
+		return;
+
+	recddq = item->ri_buf[1].i_addr;
+	if (recddq == NULL)
+		return;
+	if (item->ri_buf[1].i_len < sizeof(struct xfs_disk_dquot))
+		return;
+
+	type = recddq->d_flags & (XFS_DQ_USER | XFS_DQ_PROJ | XFS_DQ_GROUP);
+	ASSERT(type);
+	if (log->l_quotaoffs_flag & type)
+		return;
+
+	dq_f = item->ri_buf[0].i_addr;
+	ASSERT(dq_f);
+	ASSERT(dq_f->qlf_len == 1);
+
+	xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno,
+				dq_f->qlf_len, NULL);
+}
+
+STATIC void
+xlog_recover_ra_pass2(
+	struct xlog			*log,
+	struct xlog_recover_item	*item)
+{
+	switch (ITEM_TYPE(item)) {
+	case XFS_LI_BUF:
+		xlog_recover_buffer_ra_pass2(log, item);
+		break;
+	case XFS_LI_INODE:
+		xlog_recover_inode_ra_pass2(log, item);
+		break;
+	case XFS_LI_DQUOT:
+		xlog_recover_dquot_ra_pass2(log, item);
+		break;
+	case XFS_LI_EFI:
+	case XFS_LI_EFD:
+	case XFS_LI_QUOTAOFF:
+	default:
+		break;
+	}
+}
+
 STATIC int
 xlog_recover_commit_pass1(
 	struct xlog			*log,
@@ -3177,6 +3277,26 @@ xlog_recover_commit_pass2(
 	}
 }
 
+STATIC int
+xlog_recover_items_pass2(
+	struct xlog                     *log,
+	struct xlog_recover             *trans,
+	struct list_head                *buffer_list,
+	struct list_head                *item_list)
+{
+	struct xlog_recover_item	*item;
+	int				error = 0;
+
+	list_for_each_entry(item, item_list, ri_list) {
+		error = xlog_recover_commit_pass2(log, trans,
+					  buffer_list, item);
+		if (error)
+			return error;
+	}
+
+	return error;
+}
+
 /*
  * Perform the transaction.
  *
@@ -3189,9 +3309,16 @@ xlog_recover_commit_trans(
 	struct xlog_recover	*trans,
 	int			pass)
 {
-	int			error = 0, error2;
-	xlog_recover_item_t	*item;
-	LIST_HEAD		(buffer_list);
+	int				error = 0;
+	int				error2;
+	int				items_queued = 0;
+	struct xlog_recover_item	*item;
+	struct xlog_recover_item	*next;
+	LIST_HEAD			(buffer_list);
+	LIST_HEAD			(ra_list);
+	LIST_HEAD			(done_list);
+
+	#define XLOG_RECOVER_COMMIT_QUEUE_MAX 100
 
 	hlist_del(&trans->r_list);
 
@@ -3199,14 +3326,22 @@ xlog_recover_commit_trans(
 	if (error)
 		return error;
 
-	list_for_each_entry(item, &trans->r_itemq, ri_list) {
+	list_for_each_entry_safe(item, next, &trans->r_itemq, ri_list) {
 		switch (pass) {
 		case XLOG_RECOVER_PASS1:
 			error = xlog_recover_commit_pass1(log, trans, item);
 			break;
 		case XLOG_RECOVER_PASS2:
-			error = xlog_recover_commit_pass2(log, trans,
-							  &buffer_list, item);
+			xlog_recover_ra_pass2(log, item);
+			list_move_tail(&item->ri_list, &ra_list);
+			items_queued++;
+			if (items_queued >= XLOG_RECOVER_COMMIT_QUEUE_MAX) {
+				error = xlog_recover_items_pass2(log, trans,
+						&buffer_list, &ra_list);
+				list_splice_tail_init(&ra_list, &done_list);
+				items_queued = 0;
+			}
+
 			break;
 		default:
 			ASSERT(0);
@@ -3216,6 +3351,18 @@ xlog_recover_commit_trans(
 			goto out;
 	}
 
+	if (!list_empty(&ra_list)) {
+		error = xlog_recover_items_pass2(log, trans,
+				&buffer_list, &ra_list);
+		if (error)
+			goto out;
+
+		list_splice_tail_init(&ra_list, &done_list);
+	}
+
+	if (!list_empty(&done_list))
+		list_splice_init(&done_list, &trans->r_itemq);
+
 	xlog_recover_free_trans(trans);
 
 out:
-- 
1.7.11.7

[PATCH v3] xfs: introduce object readahead to log recovery

[zwu.kernel]

From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

  It can take a long time to run log recovery operation because it is
single threaded and is bound by read latency. We can find that it took
most of the time to wait for the read IO to occur, so if one object
readahead is introduced to log recovery, it will obviously reduce the
log recovery time.

Log recovery time stat:

          w/o this patch        w/ this patch

real:        0m15.023s             0m7.802s
user:        0m0.001s              0m0.001s
sys:         0m0.246s              0m0.107s

Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
 fs/xfs/xfs_log_recover.c | 159 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 153 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 7681b19..ebb00bc 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3116,6 +3116,106 @@ xlog_recover_free_trans(
 	kmem_free(trans);
 }
 
+STATIC void
+xlog_recover_buffer_ra_pass2(
+	struct xlog                     *log,
+	struct xlog_recover_item        *item)
+{
+	struct xfs_buf_log_format	*buf_f = item->ri_buf[0].i_addr;
+	struct xfs_mount		*mp = log->l_mp;
+
+	if (xlog_check_buffer_cancelled(log, buf_f->blf_blkno,
+			buf_f->blf_len, buf_f->blf_flags)) {
+		return;
+	}
+
+	xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno,
+				buf_f->blf_len, NULL);
+}
+
+STATIC void
+xlog_recover_inode_ra_pass2(
+	struct xlog                     *log,
+	struct xlog_recover_item        *item)
+{
+	struct xfs_inode_log_format	ilf_buf;
+	struct xfs_inode_log_format	*ilfp;
+	struct xfs_mount		*mp = log->l_mp;
+	int			error;
+
+	if (item->ri_buf[0].i_len == sizeof(struct xfs_inode_log_format)) {
+		ilfp = item->ri_buf[0].i_addr;
+	} else {
+		ilfp = &ilf_buf;
+		memset(ilfp, 0, sizeof(*ilfp));
+		error = xfs_inode_item_format_convert(&item->ri_buf[0], ilfp);
+		if (error)
+			return;
+	}
+
+	if (xlog_check_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len, 0))
+		return;
+
+	xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno,
+				ilfp->ilf_len, &xfs_inode_buf_ops);
+}
+
+STATIC void
+xlog_recover_dquot_ra_pass2(
+	struct xlog			*log,
+	struct xlog_recover_item	*item)
+{
+	struct xfs_mount	*mp = log->l_mp;
+	struct xfs_disk_dquot	*recddq;
+	struct xfs_dq_logformat	*dq_f;
+	uint			type;
+
+
+	if (mp->m_qflags == 0)
+		return;
+
+	recddq = item->ri_buf[1].i_addr;
+	if (recddq == NULL)
+		return;
+	if (item->ri_buf[1].i_len < sizeof(struct xfs_disk_dquot))
+		return;
+
+	type = recddq->d_flags & (XFS_DQ_USER | XFS_DQ_PROJ | XFS_DQ_GROUP);
+	ASSERT(type);
+	if (log->l_quotaoffs_flag & type)
+		return;
+
+	dq_f = item->ri_buf[0].i_addr;
+	ASSERT(dq_f);
+	ASSERT(dq_f->qlf_len == 1);
+
+	xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno,
+				dq_f->qlf_len, NULL);
+}
+
+STATIC void
+xlog_recover_ra_pass2(
+	struct xlog			*log,
+	struct xlog_recover_item	*item)
+{
+	switch (ITEM_TYPE(item)) {
+	case XFS_LI_BUF:
+		xlog_recover_buffer_ra_pass2(log, item);
+		break;
+	case XFS_LI_INODE:
+		xlog_recover_inode_ra_pass2(log, item);
+		break;
+	case XFS_LI_DQUOT:
+		xlog_recover_dquot_ra_pass2(log, item);
+		break;
+	case XFS_LI_EFI:
+	case XFS_LI_EFD:
+	case XFS_LI_QUOTAOFF:
+	default:
+		break;
+	}
+}
+
 STATIC int
 xlog_recover_commit_pass1(
 	struct xlog			*log,
@@ -3177,6 +3277,26 @@ xlog_recover_commit_pass2(
 	}
 }
 
+STATIC int
+xlog_recover_items_pass2(
+	struct xlog                     *log,
+	struct xlog_recover             *trans,
+	struct list_head                *buffer_list,
+	struct list_head                *item_list)
+{
+	struct xlog_recover_item	*item;
+	int				error = 0;
+
+	list_for_each_entry(item, item_list, ri_list) {
+		error = xlog_recover_commit_pass2(log, trans,
+					  buffer_list, item);
+		if (error)
+			return error;
+	}
+
+	return error;
+}
+
 /*
  * Perform the transaction.
  *
@@ -3189,9 +3309,16 @@ xlog_recover_commit_trans(
 	struct xlog_recover	*trans,
 	int			pass)
 {
-	int			error = 0, error2;
-	xlog_recover_item_t	*item;
-	LIST_HEAD		(buffer_list);
+	int				error = 0;
+	int				error2;
+	int				items_queued = 0;
+	struct xlog_recover_item	*item;
+	struct xlog_recover_item	*next;
+	LIST_HEAD			(buffer_list);
+	LIST_HEAD			(ra_list);
+	LIST_HEAD			(done_list);
+
+	#define XLOG_RECOVER_COMMIT_QUEUE_MAX 100
 
 	hlist_del(&trans->r_list);
 
@@ -3199,14 +3326,22 @@ xlog_recover_commit_trans(
 	if (error)
 		return error;
 
-	list_for_each_entry(item, &trans->r_itemq, ri_list) {
+	list_for_each_entry_safe(item, next, &trans->r_itemq, ri_list) {
 		switch (pass) {
 		case XLOG_RECOVER_PASS1:
 			error = xlog_recover_commit_pass1(log, trans, item);
 			break;
 		case XLOG_RECOVER_PASS2:
-			error = xlog_recover_commit_pass2(log, trans,
-							  &buffer_list, item);
+			xlog_recover_ra_pass2(log, item);
+			list_move_tail(&item->ri_list, &ra_list);
+			items_queued++;
+			if (items_queued >= XLOG_RECOVER_COMMIT_QUEUE_MAX) {
+				error = xlog_recover_items_pass2(log, trans,
+						&buffer_list, &ra_list);
+				list_splice_tail_init(&ra_list, &done_list);
+				items_queued = 0;
+			}
+
 			break;
 		default:
 			ASSERT(0);
@@ -3216,6 +3351,18 @@ xlog_recover_commit_trans(
 			goto out;
 	}
 
+	if (!list_empty(&ra_list)) {
+		error = xlog_recover_items_pass2(log, trans,
+				&buffer_list, &ra_list);
+		if (error)
+			goto out;
+
+		list_splice_tail_init(&ra_list, &done_list);
+	}
+
+	if (!list_empty(&done_list))
+		list_splice_init(&done_list, &trans->r_itemq);
+
 	xlog_recover_free_trans(trans);
 
 out:
-- 
1.7.11.7

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

Re: [PATCH v3] xfs: introduce object readahead to log recovery

[Zhi Yong Wu]

HI, xfs maintainers,

any comments?

On Wed, Jul 31, 2013 at 4:42 PM,  <zwu.kernel@gmail.com> wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>
>   It can take a long time to run log recovery operation because it is
> single threaded and is bound by read latency. We can find that it took
> most of the time to wait for the read IO to occur, so if one object
> readahead is introduced to log recovery, it will obviously reduce the
> log recovery time.
>
> Log recovery time stat:
>
>           w/o this patch        w/ this patch
>
> real:        0m15.023s             0m7.802s
> user:        0m0.001s              0m0.001s
> sys:         0m0.246s              0m0.107s
>
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> ---
>  fs/xfs/xfs_log_recover.c | 159 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 153 insertions(+), 6 deletions(-)
>
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 7681b19..ebb00bc 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3116,6 +3116,106 @@ xlog_recover_free_trans(
>         kmem_free(trans);
>  }
>
> +STATIC void
> +xlog_recover_buffer_ra_pass2(
> +       struct xlog                     *log,
> +       struct xlog_recover_item        *item)
> +{
> +       struct xfs_buf_log_format       *buf_f = item->ri_buf[0].i_addr;
> +       struct xfs_mount                *mp = log->l_mp;
> +
> +       if (xlog_check_buffer_cancelled(log, buf_f->blf_blkno,
> +                       buf_f->blf_len, buf_f->blf_flags)) {
> +               return;
> +       }
> +
> +       xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno,
> +                               buf_f->blf_len, NULL);
> +}
> +
> +STATIC void
> +xlog_recover_inode_ra_pass2(
> +       struct xlog                     *log,
> +       struct xlog_recover_item        *item)
> +{
> +       struct xfs_inode_log_format     ilf_buf;
> +       struct xfs_inode_log_format     *ilfp;
> +       struct xfs_mount                *mp = log->l_mp;
> +       int                     error;
> +
> +       if (item->ri_buf[0].i_len == sizeof(struct xfs_inode_log_format)) {
> +               ilfp = item->ri_buf[0].i_addr;
> +       } else {
> +               ilfp = &ilf_buf;
> +               memset(ilfp, 0, sizeof(*ilfp));
> +               error = xfs_inode_item_format_convert(&item->ri_buf[0], ilfp);
> +               if (error)
> +                       return;
> +       }
> +
> +       if (xlog_check_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len, 0))
> +               return;
> +
> +       xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno,
> +                               ilfp->ilf_len, &xfs_inode_buf_ops);
> +}
> +
> +STATIC void
> +xlog_recover_dquot_ra_pass2(
> +       struct xlog                     *log,
> +       struct xlog_recover_item        *item)
> +{
> +       struct xfs_mount        *mp = log->l_mp;
> +       struct xfs_disk_dquot   *recddq;
> +       struct xfs_dq_logformat *dq_f;
> +       uint                    type;
> +
> +
> +       if (mp->m_qflags == 0)
> +               return;
> +
> +       recddq = item->ri_buf[1].i_addr;
> +       if (recddq == NULL)
> +               return;
> +       if (item->ri_buf[1].i_len < sizeof(struct xfs_disk_dquot))
> +               return;
> +
> +       type = recddq->d_flags & (XFS_DQ_USER | XFS_DQ_PROJ | XFS_DQ_GROUP);
> +       ASSERT(type);
> +       if (log->l_quotaoffs_flag & type)
> +               return;
> +
> +       dq_f = item->ri_buf[0].i_addr;
> +       ASSERT(dq_f);
> +       ASSERT(dq_f->qlf_len == 1);
> +
> +       xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno,
> +                               dq_f->qlf_len, NULL);
> +}
> +
> +STATIC void
> +xlog_recover_ra_pass2(
> +       struct xlog                     *log,
> +       struct xlog_recover_item        *item)
> +{
> +       switch (ITEM_TYPE(item)) {
> +       case XFS_LI_BUF:
> +               xlog_recover_buffer_ra_pass2(log, item);
> +               break;
> +       case XFS_LI_INODE:
> +               xlog_recover_inode_ra_pass2(log, item);
> +               break;
> +       case XFS_LI_DQUOT:
> +               xlog_recover_dquot_ra_pass2(log, item);
> +               break;
> +       case XFS_LI_EFI:
> +       case XFS_LI_EFD:
> +       case XFS_LI_QUOTAOFF:
> +       default:
> +               break;
> +       }
> +}
> +
>  STATIC int
>  xlog_recover_commit_pass1(
>         struct xlog                     *log,
> @@ -3177,6 +3277,26 @@ xlog_recover_commit_pass2(
>         }
>  }
>
> +STATIC int
> +xlog_recover_items_pass2(
> +       struct xlog                     *log,
> +       struct xlog_recover             *trans,
> +       struct list_head                *buffer_list,
> +       struct list_head                *item_list)
> +{
> +       struct xlog_recover_item        *item;
> +       int                             error = 0;
> +
> +       list_for_each_entry(item, item_list, ri_list) {
> +               error = xlog_recover_commit_pass2(log, trans,
> +                                         buffer_list, item);
> +               if (error)
> +                       return error;
> +       }
> +
> +       return error;
> +}
> +
>  /*
>   * Perform the transaction.
>   *
> @@ -3189,9 +3309,16 @@ xlog_recover_commit_trans(
>         struct xlog_recover     *trans,
>         int                     pass)
>  {
> -       int                     error = 0, error2;
> -       xlog_recover_item_t     *item;
> -       LIST_HEAD               (buffer_list);
> +       int                             error = 0;
> +       int                             error2;
> +       int                             items_queued = 0;
> +       struct xlog_recover_item        *item;
> +       struct xlog_recover_item        *next;
> +       LIST_HEAD                       (buffer_list);
> +       LIST_HEAD                       (ra_list);
> +       LIST_HEAD                       (done_list);
> +
> +       #define XLOG_RECOVER_COMMIT_QUEUE_MAX 100
>
>         hlist_del(&trans->r_list);
>
> @@ -3199,14 +3326,22 @@ xlog_recover_commit_trans(
>         if (error)
>                 return error;
>
> -       list_for_each_entry(item, &trans->r_itemq, ri_list) {
> +       list_for_each_entry_safe(item, next, &trans->r_itemq, ri_list) {
>                 switch (pass) {
>                 case XLOG_RECOVER_PASS1:
>                         error = xlog_recover_commit_pass1(log, trans, item);
>                         break;
>                 case XLOG_RECOVER_PASS2:
> -                       error = xlog_recover_commit_pass2(log, trans,
> -                                                         &buffer_list, item);
> +                       xlog_recover_ra_pass2(log, item);
> +                       list_move_tail(&item->ri_list, &ra_list);
> +                       items_queued++;
> +                       if (items_queued >= XLOG_RECOVER_COMMIT_QUEUE_MAX) {
> +                               error = xlog_recover_items_pass2(log, trans,
> +                                               &buffer_list, &ra_list);
> +                               list_splice_tail_init(&ra_list, &done_list);
> +                               items_queued = 0;
> +                       }
> +
>                         break;
>                 default:
>                         ASSERT(0);
> @@ -3216,6 +3351,18 @@ xlog_recover_commit_trans(
>                         goto out;
>         }
>
> +       if (!list_empty(&ra_list)) {
> +               error = xlog_recover_items_pass2(log, trans,
> +                               &buffer_list, &ra_list);
> +               if (error)
> +                       goto out;
> +
> +               list_splice_tail_init(&ra_list, &done_list);
> +       }
> +
> +       if (!list_empty(&done_list))
> +               list_splice_init(&done_list, &trans->r_itemq);
> +
>         xlog_recover_free_trans(trans);
>
>  out:
> --
> 1.7.11.7
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs



-- 
Regards,

Zhi Yong Wu

Re: [PATCH v3] xfs: introduce object readahead to log recovery

[Zhi Yong Wu]

HI, xfs maintainers,

any comments?

On Wed, Jul 31, 2013 at 4:42 PM,  <zwu.kernel@gmail.com> wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>
>   It can take a long time to run log recovery operation because it is
> single threaded and is bound by read latency. We can find that it took
> most of the time to wait for the read IO to occur, so if one object
> readahead is introduced to log recovery, it will obviously reduce the
> log recovery time.
>
> Log recovery time stat:
>
>           w/o this patch        w/ this patch
>
> real:        0m15.023s             0m7.802s
> user:        0m0.001s              0m0.001s
> sys:         0m0.246s              0m0.107s
>
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> ---
>  fs/xfs/xfs_log_recover.c | 159 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 153 insertions(+), 6 deletions(-)
>
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 7681b19..ebb00bc 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3116,6 +3116,106 @@ xlog_recover_free_trans(
>         kmem_free(trans);
>  }
>
> +STATIC void
> +xlog_recover_buffer_ra_pass2(
> +       struct xlog                     *log,
> +       struct xlog_recover_item        *item)
> +{
> +       struct xfs_buf_log_format       *buf_f = item->ri_buf[0].i_addr;
> +       struct xfs_mount                *mp = log->l_mp;
> +
> +       if (xlog_check_buffer_cancelled(log, buf_f->blf_blkno,
> +                       buf_f->blf_len, buf_f->blf_flags)) {
> +               return;
> +       }
> +
> +       xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno,
> +                               buf_f->blf_len, NULL);
> +}
> +
> +STATIC void
> +xlog_recover_inode_ra_pass2(
> +       struct xlog                     *log,
> +       struct xlog_recover_item        *item)
> +{
> +       struct xfs_inode_log_format     ilf_buf;
> +       struct xfs_inode_log_format     *ilfp;
> +       struct xfs_mount                *mp = log->l_mp;
> +       int                     error;
> +
> +       if (item->ri_buf[0].i_len == sizeof(struct xfs_inode_log_format)) {
> +               ilfp = item->ri_buf[0].i_addr;
> +       } else {
> +               ilfp = &ilf_buf;
> +               memset(ilfp, 0, sizeof(*ilfp));
> +               error = xfs_inode_item_format_convert(&item->ri_buf[0], ilfp);
> +               if (error)
> +                       return;
> +       }
> +
> +       if (xlog_check_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len, 0))
> +               return;
> +
> +       xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno,
> +                               ilfp->ilf_len, &xfs_inode_buf_ops);
> +}
> +
> +STATIC void
> +xlog_recover_dquot_ra_pass2(
> +       struct xlog                     *log,
> +       struct xlog_recover_item        *item)
> +{
> +       struct xfs_mount        *mp = log->l_mp;
> +       struct xfs_disk_dquot   *recddq;
> +       struct xfs_dq_logformat *dq_f;
> +       uint                    type;
> +
> +
> +       if (mp->m_qflags == 0)
> +               return;
> +
> +       recddq = item->ri_buf[1].i_addr;
> +       if (recddq == NULL)
> +               return;
> +       if (item->ri_buf[1].i_len < sizeof(struct xfs_disk_dquot))
> +               return;
> +
> +       type = recddq->d_flags & (XFS_DQ_USER | XFS_DQ_PROJ | XFS_DQ_GROUP);
> +       ASSERT(type);
> +       if (log->l_quotaoffs_flag & type)
> +               return;
> +
> +       dq_f = item->ri_buf[0].i_addr;
> +       ASSERT(dq_f);
> +       ASSERT(dq_f->qlf_len == 1);
> +
> +       xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno,
> +                               dq_f->qlf_len, NULL);
> +}
> +
> +STATIC void
> +xlog_recover_ra_pass2(
> +       struct xlog                     *log,
> +       struct xlog_recover_item        *item)
> +{
> +       switch (ITEM_TYPE(item)) {
> +       case XFS_LI_BUF:
> +               xlog_recover_buffer_ra_pass2(log, item);
> +               break;
> +       case XFS_LI_INODE:
> +               xlog_recover_inode_ra_pass2(log, item);
> +               break;
> +       case XFS_LI_DQUOT:
> +               xlog_recover_dquot_ra_pass2(log, item);
> +               break;
> +       case XFS_LI_EFI:
> +       case XFS_LI_EFD:
> +       case XFS_LI_QUOTAOFF:
> +       default:
> +               break;
> +       }
> +}
> +
>  STATIC int
>  xlog_recover_commit_pass1(
>         struct xlog                     *log,
> @@ -3177,6 +3277,26 @@ xlog_recover_commit_pass2(
>         }
>  }
>
> +STATIC int
> +xlog_recover_items_pass2(
> +       struct xlog                     *log,
> +       struct xlog_recover             *trans,
> +       struct list_head                *buffer_list,
> +       struct list_head                *item_list)
> +{
> +       struct xlog_recover_item        *item;
> +       int                             error = 0;
> +
> +       list_for_each_entry(item, item_list, ri_list) {
> +               error = xlog_recover_commit_pass2(log, trans,
> +                                         buffer_list, item);
> +               if (error)
> +                       return error;
> +       }
> +
> +       return error;
> +}
> +
>  /*
>   * Perform the transaction.
>   *
> @@ -3189,9 +3309,16 @@ xlog_recover_commit_trans(
>         struct xlog_recover     *trans,
>         int                     pass)
>  {
> -       int                     error = 0, error2;
> -       xlog_recover_item_t     *item;
> -       LIST_HEAD               (buffer_list);
> +       int                             error = 0;
> +       int                             error2;
> +       int                             items_queued = 0;
> +       struct xlog_recover_item        *item;
> +       struct xlog_recover_item        *next;
> +       LIST_HEAD                       (buffer_list);
> +       LIST_HEAD                       (ra_list);
> +       LIST_HEAD                       (done_list);
> +
> +       #define XLOG_RECOVER_COMMIT_QUEUE_MAX 100
>
>         hlist_del(&trans->r_list);
>
> @@ -3199,14 +3326,22 @@ xlog_recover_commit_trans(
>         if (error)
>                 return error;
>
> -       list_for_each_entry(item, &trans->r_itemq, ri_list) {
> +       list_for_each_entry_safe(item, next, &trans->r_itemq, ri_list) {
>                 switch (pass) {
>                 case XLOG_RECOVER_PASS1:
>                         error = xlog_recover_commit_pass1(log, trans, item);
>                         break;
>                 case XLOG_RECOVER_PASS2:
> -                       error = xlog_recover_commit_pass2(log, trans,
> -                                                         &buffer_list, item);
> +                       xlog_recover_ra_pass2(log, item);
> +                       list_move_tail(&item->ri_list, &ra_list);
> +                       items_queued++;
> +                       if (items_queued >= XLOG_RECOVER_COMMIT_QUEUE_MAX) {
> +                               error = xlog_recover_items_pass2(log, trans,
> +                                               &buffer_list, &ra_list);
> +                               list_splice_tail_init(&ra_list, &done_list);
> +                               items_queued = 0;
> +                       }
> +
>                         break;
>                 default:
>                         ASSERT(0);
> @@ -3216,6 +3351,18 @@ xlog_recover_commit_trans(
>                         goto out;
>         }
>
> +       if (!list_empty(&ra_list)) {
> +               error = xlog_recover_items_pass2(log, trans,
> +                               &buffer_list, &ra_list);
> +               if (error)
> +                       goto out;
> +
> +               list_splice_tail_init(&ra_list, &done_list);
> +       }
> +
> +       if (!list_empty(&done_list))
> +               list_splice_init(&done_list, &trans->r_itemq);
> +
>         xlog_recover_free_trans(trans);
>
>  out:
> --
> 1.7.11.7
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs



-- 
Regards,

Zhi Yong Wu

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

Re: [PATCH v3] xfs: introduce object readahead to log recovery

[Dave Chinner]

On Wed, Jul 31, 2013 at 04:42:45PM +0800, zwu.kernel@gmail.com wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
>   It can take a long time to run log recovery operation because it is
> single threaded and is bound by read latency. We can find that it took
> most of the time to wait for the read IO to occur, so if one object
> readahead is introduced to log recovery, it will obviously reduce the
> log recovery time.
> 
> Log recovery time stat:
> 
>           w/o this patch        w/ this patch
> 
> real:        0m15.023s             0m7.802s
> user:        0m0.001s              0m0.001s
> sys:         0m0.246s              0m0.107s

This version works as advertised as well.

> @@ -3216,6 +3351,18 @@ xlog_recover_commit_trans(
>  			goto out;
>  	}
>  
> +	if (!list_empty(&ra_list)) {
> +		error = xlog_recover_items_pass2(log, trans,
> +				&buffer_list, &ra_list);
> +		if (error)
> +			goto out;
> +
> +		list_splice_tail_init(&ra_list, &done_list);
> +	}
> +
> +	if (!list_empty(&done_list))
> +		list_splice_init(&done_list, &trans->r_itemq);
> +
>  	xlog_recover_free_trans(trans);

I think this still leaks the trans structure when an error occurs.
Indeed, I think this is a pre-existing leak, as the current code
will skip freeing the trans structure on item recovery failure and
nothing else frees it.  So it appears to me to be busted before this
patch is added.

Hence on a xlog_recover_items_pass2() error we need to splice the
ra-list to the done_list and free trans. i.e. the "if (error) goto
out;" lines in the above hunk do not need to be there, and the
"out:" label moved to above the call to xlog_recover_free_trans() so
the main loop does the right thing when an error occurs.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v3] xfs: introduce object readahead to log recovery

[Dave Chinner]

On Wed, Jul 31, 2013 at 04:42:45PM +0800, zwu.kernel@gmail.com wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
>   It can take a long time to run log recovery operation because it is
> single threaded and is bound by read latency. We can find that it took
> most of the time to wait for the read IO to occur, so if one object
> readahead is introduced to log recovery, it will obviously reduce the
> log recovery time.
> 
> Log recovery time stat:
> 
>           w/o this patch        w/ this patch
> 
> real:        0m15.023s             0m7.802s
> user:        0m0.001s              0m0.001s
> sys:         0m0.246s              0m0.107s

This version works as advertised as well.

> @@ -3216,6 +3351,18 @@ xlog_recover_commit_trans(
>  			goto out;
>  	}
>  
> +	if (!list_empty(&ra_list)) {
> +		error = xlog_recover_items_pass2(log, trans,
> +				&buffer_list, &ra_list);
> +		if (error)
> +			goto out;
> +
> +		list_splice_tail_init(&ra_list, &done_list);
> +	}
> +
> +	if (!list_empty(&done_list))
> +		list_splice_init(&done_list, &trans->r_itemq);
> +
>  	xlog_recover_free_trans(trans);

I think this still leaks the trans structure when an error occurs.
Indeed, I think this is a pre-existing leak, as the current code
will skip freeing the trans structure on item recovery failure and
nothing else frees it.  So it appears to me to be busted before this
patch is added.

Hence on a xlog_recover_items_pass2() error we need to splice the
ra-list to the done_list and free trans. i.e. the "if (error) goto
out;" lines in the above hunk do not need to be there, and the
"out:" label moved to above the call to xlog_recover_free_trans() so
the main loop does the right thing when an error occurs.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH v3] xfs: introduce object readahead to log recovery

[Zhi Yong Wu]

On Wed, Aug 14, 2013 at 1:35 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Wed, Jul 31, 2013 at 04:42:45PM +0800, zwu.kernel@gmail.com wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>>   It can take a long time to run log recovery operation because it is
>> single threaded and is bound by read latency. We can find that it took
>> most of the time to wait for the read IO to occur, so if one object
>> readahead is introduced to log recovery, it will obviously reduce the
>> log recovery time.
>>
>> Log recovery time stat:
>>
>>           w/o this patch        w/ this patch
>>
>> real:        0m15.023s             0m7.802s
>> user:        0m0.001s              0m0.001s
>> sys:         0m0.246s              0m0.107s
>
> This version works as advertised as well.
>
>> @@ -3216,6 +3351,18 @@ xlog_recover_commit_trans(
>>                       goto out;
>>       }
>>
>> +     if (!list_empty(&ra_list)) {
>> +             error = xlog_recover_items_pass2(log, trans,
>> +                             &buffer_list, &ra_list);
>> +             if (error)
>> +                     goto out;
>> +
>> +             list_splice_tail_init(&ra_list, &done_list);
>> +     }
>> +
>> +     if (!list_empty(&done_list))
>> +             list_splice_init(&done_list, &trans->r_itemq);
>> +
>>       xlog_recover_free_trans(trans);
>
> I think this still leaks the trans structure when an error occurs.
> Indeed, I think this is a pre-existing leak, as the current code
> will skip freeing the trans structure on item recovery failure and
> nothing else frees it.  So it appears to me to be busted before this
> patch is added.
Yes, i also found this and think so.
>
> Hence on a xlog_recover_items_pass2() error we need to splice the
> ra-list to the done_list and free trans. i.e. the "if (error) goto
> out;" lines in the above hunk do not need to be there, and the
> "out:" label moved to above the call to xlog_recover_free_trans() so
> the main loop does the right thing when an error occurs.
Do you need to draft one patch to fix trans leaking? or can it be
fixed in this patch?
or will you draft one patch?

>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com



-- 
Regards,

Zhi Yong Wu

Re: [PATCH v3] xfs: introduce object readahead to log recovery

[Zhi Yong Wu]

On Wed, Aug 14, 2013 at 1:35 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Wed, Jul 31, 2013 at 04:42:45PM +0800, zwu.kernel@gmail.com wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>>   It can take a long time to run log recovery operation because it is
>> single threaded and is bound by read latency. We can find that it took
>> most of the time to wait for the read IO to occur, so if one object
>> readahead is introduced to log recovery, it will obviously reduce the
>> log recovery time.
>>
>> Log recovery time stat:
>>
>>           w/o this patch        w/ this patch
>>
>> real:        0m15.023s             0m7.802s
>> user:        0m0.001s              0m0.001s
>> sys:         0m0.246s              0m0.107s
>
> This version works as advertised as well.
>
>> @@ -3216,6 +3351,18 @@ xlog_recover_commit_trans(
>>                       goto out;
>>       }
>>
>> +     if (!list_empty(&ra_list)) {
>> +             error = xlog_recover_items_pass2(log, trans,
>> +                             &buffer_list, &ra_list);
>> +             if (error)
>> +                     goto out;
>> +
>> +             list_splice_tail_init(&ra_list, &done_list);
>> +     }
>> +
>> +     if (!list_empty(&done_list))
>> +             list_splice_init(&done_list, &trans->r_itemq);
>> +
>>       xlog_recover_free_trans(trans);
>
> I think this still leaks the trans structure when an error occurs.
> Indeed, I think this is a pre-existing leak, as the current code
> will skip freeing the trans structure on item recovery failure and
> nothing else frees it.  So it appears to me to be busted before this
> patch is added.
Yes, i also found this and think so.
>
> Hence on a xlog_recover_items_pass2() error we need to splice the
> ra-list to the done_list and free trans. i.e. the "if (error) goto
> out;" lines in the above hunk do not need to be there, and the
> "out:" label moved to above the call to xlog_recover_free_trans() so
> the main loop does the right thing when an error occurs.
Do you need to draft one patch to fix trans leaking? or can it be
fixed in this patch?
or will you draft one patch?

>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com



-- 
Regards,

Zhi Yong Wu

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

Re: [PATCH v3] xfs: introduce object readahead to log recovery

[Dave Chinner]

On Wed, Aug 14, 2013 at 01:59:02PM +0800, Zhi Yong Wu wrote:
> On Wed, Aug 14, 2013 at 1:35 PM, Dave Chinner <david@fromorbit.com> wrote:
> > On Wed, Jul 31, 2013 at 04:42:45PM +0800, zwu.kernel@gmail.com wrote:
> >> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> >>
> >>   It can take a long time to run log recovery operation because it is
> >> single threaded and is bound by read latency. We can find that it took
> >> most of the time to wait for the read IO to occur, so if one object
> >> readahead is introduced to log recovery, it will obviously reduce the
> >> log recovery time.
> >>
> >> Log recovery time stat:
> >>
> >>           w/o this patch        w/ this patch
> >>
> >> real:        0m15.023s             0m7.802s
> >> user:        0m0.001s              0m0.001s
> >> sys:         0m0.246s              0m0.107s
> >
> > This version works as advertised as well.
> >
> >> @@ -3216,6 +3351,18 @@ xlog_recover_commit_trans(
> >>                       goto out;
> >>       }
> >>
> >> +     if (!list_empty(&ra_list)) {
> >> +             error = xlog_recover_items_pass2(log, trans,
> >> +                             &buffer_list, &ra_list);
> >> +             if (error)
> >> +                     goto out;
> >> +
> >> +             list_splice_tail_init(&ra_list, &done_list);
> >> +     }
> >> +
> >> +     if (!list_empty(&done_list))
> >> +             list_splice_init(&done_list, &trans->r_itemq);
> >> +
> >>       xlog_recover_free_trans(trans);
> >
> > I think this still leaks the trans structure when an error occurs.
> > Indeed, I think this is a pre-existing leak, as the current code
> > will skip freeing the trans structure on item recovery failure and
> > nothing else frees it.  So it appears to me to be busted before this
> > patch is added.
> Yes, i also found this and think so.
> >
> > Hence on a xlog_recover_items_pass2() error we need to splice the
> > ra-list to the done_list and free trans. i.e. the "if (error) goto
> > out;" lines in the above hunk do not need to be there, and the
> > "out:" label moved to above the call to xlog_recover_free_trans() so
> > the main loop does the right thing when an error occurs.
> Do you need to draft one patch to fix trans leaking? or can it be
> fixed in this patch?

Just fix it in this patch - the above paragraph explains how to fix
it....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v3] xfs: introduce object readahead to log recovery

[Dave Chinner]

On Wed, Aug 14, 2013 at 01:59:02PM +0800, Zhi Yong Wu wrote:
> On Wed, Aug 14, 2013 at 1:35 PM, Dave Chinner <david@fromorbit.com> wrote:
> > On Wed, Jul 31, 2013 at 04:42:45PM +0800, zwu.kernel@gmail.com wrote:
> >> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> >>
> >>   It can take a long time to run log recovery operation because it is
> >> single threaded and is bound by read latency. We can find that it took
> >> most of the time to wait for the read IO to occur, so if one object
> >> readahead is introduced to log recovery, it will obviously reduce the
> >> log recovery time.
> >>
> >> Log recovery time stat:
> >>
> >>           w/o this patch        w/ this patch
> >>
> >> real:        0m15.023s             0m7.802s
> >> user:        0m0.001s              0m0.001s
> >> sys:         0m0.246s              0m0.107s
> >
> > This version works as advertised as well.
> >
> >> @@ -3216,6 +3351,18 @@ xlog_recover_commit_trans(
> >>                       goto out;
> >>       }
> >>
> >> +     if (!list_empty(&ra_list)) {
> >> +             error = xlog_recover_items_pass2(log, trans,
> >> +                             &buffer_list, &ra_list);
> >> +             if (error)
> >> +                     goto out;
> >> +
> >> +             list_splice_tail_init(&ra_list, &done_list);
> >> +     }
> >> +
> >> +     if (!list_empty(&done_list))
> >> +             list_splice_init(&done_list, &trans->r_itemq);
> >> +
> >>       xlog_recover_free_trans(trans);
> >
> > I think this still leaks the trans structure when an error occurs.
> > Indeed, I think this is a pre-existing leak, as the current code
> > will skip freeing the trans structure on item recovery failure and
> > nothing else frees it.  So it appears to me to be busted before this
> > patch is added.
> Yes, i also found this and think so.
> >
> > Hence on a xlog_recover_items_pass2() error we need to splice the
> > ra-list to the done_list and free trans. i.e. the "if (error) goto
> > out;" lines in the above hunk do not need to be there, and the
> > "out:" label moved to above the call to xlog_recover_free_trans() so
> > the main loop does the right thing when an error occurs.
> Do you need to draft one patch to fix trans leaking? or can it be
> fixed in this patch?

Just fix it in this patch - the above paragraph explains how to fix
it....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

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