xfs: introduce object readahead to log recovery
Subsystems (2)
PATCH v2 1/1 xfs: introduce object readahead to log recovery
From: Zhi Yong Wu <[email protected]>
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
--- fs/xfs/xfs_log_recover.c | 162 +++++++++++++++++++++++++++++++++++++++++++++-- fs/xfs/xfs_log_recover.h | 2 + 2 files changed, 159 insertions(+), 5 deletions(-)
fs/xfs/xfs_log_recover.c +157 -5
@@ -3116,6 +3116,111 @@ xlog_recover_free_trans(kmem_free (trans); } +STATIC void +xlog_recover_buffer_ra_pass2 ( +struct xlog *log, +struct xlog_recover_item *item) +{ +xfs_buf_log_format_t *buf_f = item->ri_buf[0 ].i_addr; +xfs_mount_t *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) +{ +xfs_inode_log_format_t in_buf, *in_f; +xfs_mount_t *mp = log->l_mp; +int error; + +if (item->ri_buf[0 ].i_len ==sizeof (xfs_inode_log_format_t)) { + in_f = item->ri_buf[0 ].i_addr; + }else { + in_f = &in_buf; +memset (in_f,0 ,sizeof (*in_f)); + error =xfs_inode_item_format_convert (&item->ri_buf[0 ], in_f); +if (error) +return ; + } + +if (xlog_check_buffer_cancelled (log, in_f->ilf_blkno, + in_f->ilf_len,0 )) +return ; + +xfs_buf_readahead (mp->m_ddev_targp, in_f->ilf_blkno, + in_f->ilf_len, &xfs_inode_buf_ops); +} + +STATIC void +xlog_recover_dquot_ra_pass2 ( +struct xlog *log, +struct xlog_recover_item *item) +{ +xfs_mount_t *mp = log->l_mp; +struct xfs_disk_dquot *recddq; +int error; +xfs_dq_logformat_t *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 (xfs_disk_dquot_t)) +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); + error =xfs_qm_dqcheck (mp, recddq, dq_f->qlf_id,0 ,XFS_QMOPT_DOWARN , +"xlog_recover_dquot_ra_pass2 (log copy)" ); +if (error) +return ; +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 +3282,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 *ra_list) +{ +int error =0 ; +xlog_recover_item_t *item; + +list_for_each_entry (item, ra_list, ri_list) { + error =xlog_recover_commit_pass2 (log, trans, + buffer_list, item); +if (error) +return error; + } + +return error; +} +/* * Perform the transaction. *
@@ -3189,9 +3314,11 @@ xlog_recover_commit_trans(struct xlog_recover *trans, int pass) { - int error = 0, error2; -xlog_recover_item_t *item; + int error = 0, error2, ra_qdepth = 0; +xlog_recover_item_t *item, *next;LIST_HEAD (buffer_list); +LIST_HEAD (ra_list); +LIST_HEAD (all_ra_list);hlist_del (&trans->r_list);
@@ -3199,14 +3326,21 @@ 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); +if (ra_qdepth++ >=XLOG_RECOVER_MAX_QDEPTH ) { + error =xlog_recover_items_pass2 (log, trans, + &buffer_list, &ra_list); +list_splice_tail_init (&ra_list, &all_ra_list); + ra_qdepth =0 ; + }else { +xlog_recover_ra_pass2 (log, item); +list_move_tail (&item->ri_list, &ra_list); + }break ;default :ASSERT (0 );
@@ -3216,9 +3350,27 @@ 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, &all_ra_list); + } + +if (!list_empty (&all_ra_list)) +list_splice_init (&all_ra_list, &trans->r_itemq); +xlog_recover_free_trans (trans); out: +if (!list_empty (&ra_list)) +list_splice_tail_init (&ra_list, &all_ra_list); + +if (!list_empty (&all_ra_list)) +list_splice_init (&all_ra_list, &trans->r_itemq); + error2 =xfs_buf_delwri_submit (&buffer_list);return error ? error : error2; }
fs/xfs/xfs_log_recover.h +2 -0
@@ -63,4 +63,6 @@ typedef struct xlog_recover {#define XLOG_RECOVER_PASS1 1#define XLOG_RECOVER_PASS2 2 +#define XLOG_RECOVER_MAX_QDEPTH 100 +#endif /* __XFS_LOG_RECOVER_H__ */
Signature · 1 line
1.7.11.7
Quoted 10 lines · On 07/30/2013 05:59 AM, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
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:
Quoted 8 lines
w/o this patch w/ this patch
real: 0m15.023s 0m7.802s user: 0m0.001s 0m0.001s sys: 0m0.246s 0m0.107sSigned-off-by Zhi Yong Wu <[email protected]>---
Cool patch. I'm not terribly familiar with the log recovery code so take my comments with a grain of salt, but a couple things I noticed on a quick skim...
Quoted 4 lines
fs/xfs/xfs_log_recover.c | 162 +++++++++++++++++++++++++++++++++++++++++++++-- fs/xfs/xfs_log_recover.h | 2 + 2 files changed, 159 insertions(+), 5 deletions(-)
...
fs/xfs/xfs_log_recover.c xlog_recover_commit_pass2( · line 3290
Quoted 7 lines
fs/xfs/xfs_log_recover.c +6
-0+STATIC int +xlog_recover_items_pass2 ( +struct xlog *log, +struct xlog_recover *trans, +struct list_head *buffer_list, +struct list_head *ra_list)
A nit, but technically this function doesn't have to be involved with readahead. Perhaps rename ra_list to item_list..?
Quoted 35 lines
fs/xfs/xfs_log_recover.c +18
-2+{ +int error =0 ; +xlog_recover_item_t *item; + +list_for_each_entry (item, ra_list, ri_list) { + error =xlog_recover_commit_pass2 (log, trans, + buffer_list, item); +if (error) +return error; + } + +return error; +} +/* * Perform the transaction. * @@ -3189,9 +3314,11 @@ xlog_recover_commit_trans(struct xlog_recover *trans, int pass) { - int error = 0, error2; -xlog_recover_item_t *item; + int error = 0, error2, ra_qdepth = 0; +xlog_recover_item_t *item, *next;LIST_HEAD (buffer_list); +LIST_HEAD (ra_list); +LIST_HEAD (all_ra_list);hlist_del (&trans->r_list);@@ -3199,14 +3326,21 @@ xlog_recover_commit_trans(if (error)return error;
fs/xfs/xfs_log_recover.c xlog_recover_commit_trans( · line 3335
Quoted 10 lines
fs/xfs/xfs_log_recover.c +2
-3-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); +if (ra_qdepth++ >=XLOG_RECOVER_MAX_QDEPTH ) {
The counting mechanism looks strange and easy to break with future changes. Why not increment ra_qdepth in the else bracket where it is explicitly tied to the operation it tracks?
fs/xfs/xfs_log_recover.c xlog_recover_commit_trans( · line 3339
Quoted 4 lines
fs/xfs/xfs_log_recover.c +4
-0+ error =xlog_recover_items_pass2 (log, trans, + &buffer_list, &ra_list); +list_splice_tail_init (&ra_list, &all_ra_list); + ra_qdepth =0 ;
So now we've queued up a bunch of items we've issued readahead on in ra_list and we've executed the recovery on the list. What happens to the current item?
Brian
Quoted 41 lines
fs/xfs/xfs_log_recover.c +22
-0+ }else { +xlog_recover_ra_pass2 (log, item); +list_move_tail (&item->ri_list, &ra_list); + }break ;default :ASSERT (0 );@@ -3216,9 +3350,27 @@ 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, &all_ra_list); + } + +if (!list_empty (&all_ra_list)) +list_splice_init (&all_ra_list, &trans->r_itemq); +xlog_recover_free_trans (trans); out: +if (!list_empty (&ra_list)) +list_splice_tail_init (&ra_list, &all_ra_list); + +if (!list_empty (&all_ra_list)) +list_splice_init (&all_ra_list, &trans->r_itemq); + error2 =xfs_buf_delwri_submit (&buffer_list);return error ? error : error2; }fs/xfs/xfs_log_recover.h +0
-0
fs/xfs/xfs_log_recover.h typedef struct xlog_recover { · line 68
Quoted 6 lines
fs/xfs/xfs_log_recover.h +2
-0@@ -63,4 +63,6 @@ typedef struct xlog_recover {#define XLOG_RECOVER_PASS1 1#define XLOG_RECOVER_PASS2 2 +#define XLOG_RECOVER_MAX_QDEPTH 100 +#endif /* __XFS_LOG_RECOVER_H__ */
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 30 lines · Brian Foster
Quoted 10 lines · On 07/30/2013 05:59 AM, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
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:
Quoted 8 lines
w/o this patch w/ this patch
real: 0m15.023s 0m7.802s user: 0m0.001s 0m0.001s sys: 0m0.246s 0m0.107sSigned-off-by Zhi Yong Wu <[email protected]>---
Cool patch. I'm not terribly familiar with the log recovery code so take my comments with a grain of salt, but a couple things I noticed on a quick skim...
Quoted 4 lines
fs/xfs/xfs_log_recover.c | 162 +++++++++++++++++++++++++++++++++++++++++++++-- fs/xfs/xfs_log_recover.h | 2 + 2 files changed, 159 insertions(+), 5 deletions(-)...
Quoted 1 line
Quoted 9 lines
fs/xfs/xfs_log_recover.c xlog_recover_commit_pass2( · line 3290
Quoted 6 lines
fs/xfs/xfs_log_recover.c +6
-0+STATIC int +xlog_recover_items_pass2 ( +struct xlog *log, +struct xlog_recover *trans, +struct list_head *buffer_list, +struct list_head *ra_list)A nit, but technically this function doesn't have to be involved with readahead. Perhaps rename
ra_listtoitem_list..?
ok, applied.
Quoted 36 lines
Quoted 36 lines
fs/xfs/xfs_log_recover.c +18
-2+{ +int error =0 ; +xlog_recover_item_t *item; + +list_for_each_entry (item, ra_list, ri_list) { + error =xlog_recover_commit_pass2 (log, trans, + buffer_list, item); +if (error) +return error; + } + +return error; +} +/* * Perform the transaction. * @@ -3189,9 +3314,11 @@ xlog_recover_commit_trans(struct xlog_recover *trans, int pass) { - int error = 0, error2; -xlog_recover_item_t *item; + int error = 0, error2, ra_qdepth = 0; +xlog_recover_item_t *item, *next;LIST_HEAD (buffer_list); +LIST_HEAD (ra_list); +LIST_HEAD (all_ra_list);hlist_del (&trans->r_list);@@ -3199,14 +3326,21 @@ xlog_recover_commit_trans(if (error)return error;
Quoted 14 lines
fs/xfs/xfs_log_recover.c xlog_recover_commit_trans( · line 3335
Quoted 10 lines
fs/xfs/xfs_log_recover.c +2
-3-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); +if (ra_qdepth++ >=XLOG_RECOVER_MAX_QDEPTH ) {The counting mechanism looks strange and easy to break with future changes. Why not increment
ra_qdepthin the else bracket where it is explicitly tied to the operation it tracks?
ok.
Quoted 9 lines
fs/xfs/xfs_log_recover.c xlog_recover_commit_trans( · line 3339
Quoted 5 lines
fs/xfs/xfs_log_recover.c +4
-0+ error =xlog_recover_items_pass2 (log, trans, + &buffer_list, &ra_list); +list_splice_tail_init (&ra_list, &all_ra_list); + ra_qdepth =0 ;So now we've queued up a bunch of items we've issued readahead on in
ra_listand we've executed the recovery on the list. What happens to the current item?
Good catch, the current item was missed. Done.
Quoted 51 lines
Brian
Quoted 47 lines
fs/xfs/xfs_log_recover.c +22
-0+ }else { +xlog_recover_ra_pass2 (log, item); +list_move_tail (&item->ri_list, &ra_list); + }break ;default :ASSERT (0 );@@ -3216,9 +3350,27 @@ 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, &all_ra_list); + } + +if (!list_empty (&all_ra_list)) +list_splice_init (&all_ra_list, &trans->r_itemq); +xlog_recover_free_trans (trans); out: +if (!list_empty (&ra_list)) +list_splice_tail_init (&ra_list, &all_ra_list); + +if (!list_empty (&all_ra_list)) +list_splice_init (&all_ra_list, &trans->r_itemq); + error2 =xfs_buf_delwri_submit (&buffer_list);return error ? error : error2; }fs/xfs/xfs_log_recover.h +2
-0@@ -63,4 +63,6 @@ typedef struct xlog_recover {#define XLOG_RECOVER_PASS1 1#define XLOG_RECOVER_PASS2 2 +#define XLOG_RECOVER_MAX_QDEPTH 100 +#endif /* __XFS_LOG_RECOVER_H__ */
Signature · 3 lines
Regards, Zhi Yong Wu
Quoted 27 lines · On Tue, Jul 30, 2013 at 05:59:07PM +0800, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
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.107sSigned-off-by Zhi Yong Wu <[email protected]>--- fs/xfs/xfs_log_recover.c | 162 +++++++++++++++++++++++++++++++++++++++++++++-- fs/xfs/xfs_log_recover.h | 2 + 2 files changed, 159 insertions(+), 5 deletions(-)fs/xfs/xfs_log_recover.c +0
-0
fs/xfs/xfs_log_recover.c xlog_recover_free_trans( · line 3125
Quoted 10 lines
fs/xfs/xfs_log_recover.c +7
-0@@ -3116,6 +3116,111 @@ xlog_recover_free_trans(kmem_free (trans); } +STATIC void +xlog_recover_buffer_ra_pass2 ( +struct xlog *log, +struct xlog_recover_item *item) +{ +xfs_buf_log_format_t *buf_f = item->ri_buf[0 ].i_addr; +xfs_mount_t *mp = log->l_mp;
struct xfs_buf_log_format
struct xfs_mount
Quoted 7 lines
fs/xfs/xfs_log_recover.c +7
-0+ +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,
fs/xfs/xfs_log_recover.c xlog_recover_free_trans( · line 3142
Quoted 10 lines
fs/xfs/xfs_log_recover.c +10
-0+ buf_f->blf_len, NULL); +} + +STATIC void +xlog_recover_inode_ra_pass2 ( +struct xlog *log, +struct xlog_recover_item *item) +{ +xfs_inode_log_format_t in_buf, *in_f; +xfs_mount_t *mp = log->l_mp;
struct xfs_inode_log_format
struct xfs_mount
and a separate declaration for each variable.
Also, in_buf and in_f are not very good names as tehy don't follow any commonly used XFs naming convention. The shorthand for a struct xfs_inode_log_format variable is "ilf" and a pointer to such an object is "ilfp". i.e:
struct xfs_inode_log_format ilf_buf; struct xfs_inode_log_format *ilfp;
fs/xfs/xfs_log_recover.c xlog_recover_free_trans( · line 3172
Quoted 9 lines
fs/xfs/xfs_log_recover.c +9
-0+xlog_recover_dquot_ra_pass2 ( + struct xlog *log, + struct xlog_recover_item *item) +{ + xfs_mount_t *mp = log->l_mp; +struct xfs_disk_dquot *recddq; +int error; +xfs_dq_logformat_t *dq_f; +uint type;
More typedefs.
Quoted 10 lines
fs/xfs/xfs_log_recover.c +10
-0+ + +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 (xfs_disk_dquot_t)) +return ;
fs/xfs/xfs_log_recover.c xlog_recover_free_trans( · line 3192
Quoted 10 lines
fs/xfs/xfs_log_recover.c +10
-0+ + 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); + error =xfs_qm_dqcheck (mp, recddq, dq_f->qlf_id,0 ,XFS_QMOPT_DOWARN , +"xlog_recover_dquot_ra_pass2 (log copy)" );
You don't need to do validation of the dquot in the transaction here - it's unlikely to be corrupt. Just do the readahead like for a normal buffer, and the validation can occur when recovering the item in the next pass.
Quoted 37 lines
fs/xfs/xfs_log_recover.c +31
-0+if (error) +return ; +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 +3282,26 @@ xlog_recover_commit_pass2( } }
fs/xfs/xfs_log_recover.c xlog_recover_commit_pass2( · line 3293
Quoted 10 lines
fs/xfs/xfs_log_recover.c +9
-0+STATIC int +xlog_recover_items_pass2 ( +struct xlog *log, +struct xlog_recover *trans, +struct list_head *buffer_list, +struct list_head *ra_list) +{ +int error =0 ; +xlog_recover_item_t *item;
typedef.
Quoted 15 lines
fs/xfs/xfs_log_recover.c +11
-0+ +list_for_each_entry (item, ra_list, ri_list) { + error =xlog_recover_commit_pass2 (log, trans, + buffer_list, item); +if (error) +return error; + } + +return error; +} +/* * Perform the transaction. *
fs/xfs/xfs_log_recover.c xlog_recover_commit_trans( · line 3318
Quoted 7 lines
fs/xfs/xfs_log_recover.c +2
-2@@ -3189,9 +3314,11 @@ xlog_recover_commit_trans(struct xlog_recover *trans, int pass) { - int error = 0, error2; -xlog_recover_item_t *item; + int error = 0, error2, ra_qdepth = 0; +xlog_recover_item_t *item, *next;
typedef, one variable per line.
fs/xfs/xfs_log_recover.c xlog_recover_commit_trans( · line 3321
Quoted 3 lines
fs/xfs/xfs_log_recover.c +2
-0LIST_HEAD (buffer_list); +LIST_HEAD (ra_list); +LIST_HEAD (all_ra_list);
Isn't the second the "recovered" list?
i.e. objects are moved to the ra_list when readhead is issued, then when they are committed they are moved to the "recovered" list?
Quoted 6 lines
fs/xfs/xfs_log_recover.c +0
-0hlist_del (&trans->r_list);@@ -3199,14 +3326,21 @@ xlog_recover_commit_trans(if (error)return error;
fs/xfs/xfs_log_recover.c xlog_recover_commit_trans( · line 3335
Quoted 10 lines
fs/xfs/xfs_log_recover.c +2
-3-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); +if (ra_qdepth++ >=XLOG_RECOVER_MAX_QDEPTH ) {
I'd define XLOG_RECOVER_MAX_QDEPTH inside this function with all the local variables. It has not scope outside this function.
Also, "items_queued" is a better name than ra_qdepth - we are tracking how many items we've queued for recovery, not the number of readahead IOs we have in progress. Similar for XLOG_RECOVER_MAX_QDEPTH - XLOG_RECOVER_COMMIT_QUEUE_MAX might be better.
Quoted 25 lines
fs/xfs/xfs_log_recover.c +18
-0+ error =xlog_recover_items_pass2 (log, trans, + &buffer_list, &ra_list); +list_splice_tail_init (&ra_list, &all_ra_list); + ra_qdepth =0 ; + }else { +xlog_recover_ra_pass2 (log, item); +list_move_tail (&item->ri_list, &ra_list); + }break ;default :ASSERT (0 );@@ -3216,9 +3350,27 @@ 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, &all_ra_list); + } + +if (!list_empty (&all_ra_list))
fs/xfs/xfs_log_recover.c xlog_recover_commit_trans( · line 3372
Quoted 10 lines
fs/xfs/xfs_log_recover.c +7
-0+list_splice_init (&all_ra_list, &trans->r_itemq); +xlog_recover_free_trans (trans); out: +if (!list_empty (&ra_list)) +list_splice_tail_init (&ra_list, &all_ra_list); + +if (!list_empty (&all_ra_list)) +list_splice_init (&all_ra_list, &trans->r_itemq);
The error handling here is all wrong. xlog_recover_free_trans() frees everything on the trans->r_itemq, and then frees trans, so this is both leaky and a use after free. This is all you need to do here:
@@ -3216,6 +3359,13 @@ xlog_recover_commit_trans(
if (error)
goto out;
}
+ if (!list_empty(&ra_list)) {
+ error = recover_commit(log, trans, &buffer_list, &ra_list);
+ list_splice_init(&ra_list, &done_list);
+ }
+
+ if (!list_empty(&done_list))
+ list_splice(&done_list, &trans->r_itemq);
xlog_recover_free_trans(trans);
i.e. run the recovery of the remainder of the ra_list, splice it to the done list, the splice the done list back to the trans and then free the trans. The error path falls through naturally and without needing any special handling....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 31 lines · Dave Chinner
Quoted 30 lines · On Tue, Jul 30, 2013 at 05:59:07PM +0800, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
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.107sSigned-off-by Zhi Yong Wu <[email protected]>--- fs/xfs/xfs_log_recover.c | 162 +++++++++++++++++++++++++++++++++++++++++++++-- fs/xfs/xfs_log_recover.h | 2 + 2 files changed, 159 insertions(+), 5 deletions(-)fs/xfs/xfs_log_recover.c +0
-0@@ -3116,6 +3116,111 @@ xlog_recover_free_trans(kmem_free (trans); }
Quoted 10 lines
fs/xfs/xfs_log_recover.c xlog_recover_free_trans( · line 3125
Quoted 7 lines
fs/xfs/xfs_log_recover.c +7
-0+STATIC void +xlog_recover_buffer_ra_pass2 ( +struct xlog *log, +struct xlog_recover_item *item) +{ +xfs_buf_log_format_t *buf_f = item->ri_buf[0 ].i_addr; +xfs_mount_t *mp = log->l_mp;struct
xfs_buf_log_format
structxfs_mount
Why? *_t is also used in a lot of other places.
Quoted 61 lines
Quoted 8 lines
fs/xfs/xfs_log_recover.c +7
-0+ +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,fs/xfs/xfs_log_recover.c xlog_recover_free_trans( · line 3142
Quoted 10 lines
fs/xfs/xfs_log_recover.c +10
-0+ buf_f->blf_len, NULL); +} + +STATIC void +xlog_recover_inode_ra_pass2 ( +struct xlog *log, +struct xlog_recover_item *item) +{ +xfs_inode_log_format_t in_buf, *in_f; +xfs_mount_t *mp = log->l_mp;struct
xfs_inode_log_format
structxfs_mountand a separate declaration for each variable.
Also,
in_bufandin_fare not very good names as tehy don't follow any commonly used XFs naming convention. The shorthand for a structxfs_inode_log_formatvariable is "ilf" and a pointer to such an object is "ilfp". i.e:struct xfs_inode_log_format ilf_buf; struct xfs_inode_log_format *ilfp;fs/xfs/xfs_log_recover.c xlog_recover_free_trans( · line 3172
Quoted 9 lines
fs/xfs/xfs_log_recover.c +9
-0+xlog_recover_dquot_ra_pass2 ( + struct xlog *log, + struct xlog_recover_item *item) +{ + xfs_mount_t *mp = log->l_mp; +struct xfs_disk_dquot *recddq; +int error; +xfs_dq_logformat_t *dq_f; +uint type;More typedefs.
Quoted 17 lines
fs/xfs/xfs_log_recover.c +17
-0+ + +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 (xfs_disk_dquot_t)) +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;
Quoted 8 lines
fs/xfs/xfs_log_recover.c xlog_recover_free_trans( · line 3192
Quoted 3 lines
fs/xfs/xfs_log_recover.c +3
-0+ASSERT (dq_f); + error =xfs_qm_dqcheck (mp, recddq, dq_f->qlf_id,0 ,XFS_QMOPT_DOWARN , +"xlog_recover_dquot_ra_pass2 (log copy)" );You don't need to do validation of the dquot in the transaction here - it's unlikely to be corrupt. Just do the readahead like for a normal buffer, and the validation can occur when recovering the item in the next pass.
Agreed, done.
Quoted 106 lines
Quoted 38 lines
fs/xfs/xfs_log_recover.c +31
-0+if (error) +return ; +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 +3282,26 @@ xlog_recover_commit_pass2( } }fs/xfs/xfs_log_recover.c xlog_recover_commit_pass2( · line 3293
Quoted 10 lines
fs/xfs/xfs_log_recover.c +9
-0+STATIC int +xlog_recover_items_pass2 ( +struct xlog *log, +struct xlog_recover *trans, +struct list_head *buffer_list, +struct list_head *ra_list) +{ +int error =0 ; +xlog_recover_item_t *item;typedef.
Quoted 15 lines
fs/xfs/xfs_log_recover.c +11
-0+ +list_for_each_entry (item, ra_list, ri_list) { + error =xlog_recover_commit_pass2 (log, trans, + buffer_list, item); +if (error) +return error; + } + +return error; +} +/* * Perform the transaction. * fs/xfs/xfs_log_recover.c xlog_recover_commit_trans( · line 3318
Quoted 7 lines
fs/xfs/xfs_log_recover.c +2
-2@@ -3189,9 +3314,11 @@ xlog_recover_commit_trans(struct xlog_recover *trans, int pass) { - int error = 0, error2; -xlog_recover_item_t *item; + int error = 0, error2, ra_qdepth = 0; +xlog_recover_item_t *item, *next;typedef, one variable per line.
fs/xfs/xfs_log_recover.c xlog_recover_commit_trans( · line 3321
Quoted 3 lines
fs/xfs/xfs_log_recover.c +2
-0LIST_HEAD (buffer_list); +LIST_HEAD (ra_list); +LIST_HEAD (all_ra_list);Isn't the second the "recovered" list?
i.e. objects are moved to the
ra_listwhen readhead is issued, then when they are committed they are moved to the "recovered" list?Quoted 6 lines
fs/xfs/xfs_log_recover.c +0
-0hlist_del (&trans->r_list);@@ -3199,14 +3326,21 @@ xlog_recover_commit_trans(if (error)return error;fs/xfs/xfs_log_recover.c xlog_recover_commit_trans( · line 3335
Quoted 10 lines
fs/xfs/xfs_log_recover.c +2
-3-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); +if (ra_qdepth++ >=XLOG_RECOVER_MAX_QDEPTH ) {I'd define
XLOG_RECOVER_MAX_QDEPTHinside this function with all the local variables. It has not scope outside this function.
Quoted 5 lines
Also, "
items_queued" is a better name thanra_qdepth- we are tracking how many items we've queued for recovery, not the number of readahead IOs we have in progress. Similar forXLOG_RECOVER_MAX_QDEPTH-XLOG_RECOVER_COMMIT_QUEUE_MAXmight be better.
Applied all above.
Quoted 31 lines
Quoted 31 lines
fs/xfs/xfs_log_recover.c +20
-0+ error =xlog_recover_items_pass2 (log, trans, + &buffer_list, &ra_list); +list_splice_tail_init (&ra_list, &all_ra_list); + ra_qdepth =0 ; + }else { +xlog_recover_ra_pass2 (log, item); +list_move_tail (&item->ri_list, &ra_list); + }break ;default :ASSERT (0 );@@ -3216,9 +3350,27 @@ 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, &all_ra_list); + } + +if (!list_empty (&all_ra_list)) +list_splice_init (&all_ra_list, &trans->r_itemq); +xlog_recover_free_trans (trans);
Quoted 10 lines
fs/xfs/xfs_log_recover.c xlog_recover_commit_trans( · line 3372
Quoted 6 lines
fs/xfs/xfs_log_recover.c +5
-0out: +if (!list_empty (&ra_list)) +list_splice_tail_init (&ra_list, &all_ra_list); + +if (!list_empty (&all_ra_list)) +list_splice_init (&all_ra_list, &trans->r_itemq);The error handling here is all wrong.
xlog_recover_free_trans()frees everything on thetrans->r_itemq, and then frees trans, so this is both leaky and a use after free. This is all you need to do
For normal path, the above two list_splice_* are not executed at all.
Quoted 17 lines
here:
(quoted diff) +7
-0@@ -3216,6 +3359,13 @@ xlog_recover_commit_trans( if (error) goto out; } + if (!list_empty(&ra_list)) { + error = recover_commit(log, trans, &buffer_list, &ra_list); + list_splice_init(&ra_list, &done_list); + } + + if (!list_empty(&done_list)) + list_splice(&done_list, &trans->r_itemq); xlog_recover_free_trans(trans);
Quoted 4 lines
i.e. run the recovery of the remainder of the
ra_list, splice it to the done list, the splice the done list back to the trans and then free the trans. The error path falls through naturally and without needing any special handling....
For error path, do you not need to splice ra_list and done_list to the trans? I thought that this transaction may be continued to recovery log later.
Quoted 7 lines
Cheers,
Dave.
--
Dave Chinner
[email protected]
Signature · 8 lines
Regards, Zhi Yong Wu _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Hey Zhi,
Quoted 42 lines · Zhi Yong Wu
Quoted 31 lines · Dave Chinner
Quoted 30 lines · On Tue, Jul 30, 2013 at 05:59:07PM +0800, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
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.107sSigned-off-by Zhi Yong Wu <[email protected]>--- fs/xfs/xfs_log_recover.c | 162 +++++++++++++++++++++++++++++++++++++++++++++-- fs/xfs/xfs_log_recover.h | 2 + 2 files changed, 159 insertions(+), 5 deletions(-)fs/xfs/xfs_log_recover.c +0
-0@@ -3116,6 +3116,111 @@ xlog_recover_free_trans(kmem_free (trans); }Quoted 10 lines
fs/xfs/xfs_log_recover.c xlog_recover_free_trans( · line 3125
Quoted 7 lines
fs/xfs/xfs_log_recover.c +7
-0+STATIC void +xlog_recover_buffer_ra_pass2 ( +struct xlog *log, +struct xlog_recover_item *item) +{ +xfs_buf_log_format_t *buf_f = item->ri_buf[0 ].i_addr; +xfs_mount_t *mp = log->l_mp;struct
xfs_buf_log_format
structxfs_mount
Quoted 1 line
Why? *_t is also used in a lot of other places.
It is just a general style preference for using the struct instead of the _t in the xfs codebase. Over the course of the past few years they've slowly been converted in this direction, and we prefer not to add any more _t if it can be avoided.
-Ben
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 45 lines · Ben Myers
Hey Zhi,
Quoted 42 lines · Zhi Yong Wu
Quoted 41 lines · Dave Chinner
Quoted 27 lines · On Tue, Jul 30, 2013 at 05:59:07PM +0800, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
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.107sSigned-off-by Zhi Yong Wu <[email protected]>--- fs/xfs/xfs_log_recover.c | 162 +++++++++++++++++++++++++++++++++++++++++++++-- fs/xfs/xfs_log_recover.h | 2 + 2 files changed, 159 insertions(+), 5 deletions(-)fs/xfs/xfs_log_recover.c +0
-0fs/xfs/xfs_log_recover.c xlog_recover_free_trans( · line 3125
Quoted 10 lines
fs/xfs/xfs_log_recover.c +7
-0@@ -3116,6 +3116,111 @@ xlog_recover_free_trans(kmem_free (trans); } +STATIC void +xlog_recover_buffer_ra_pass2 ( +struct xlog *log, +struct xlog_recover_item *item) +{ +xfs_buf_log_format_t *buf_f = item->ri_buf[0 ].i_addr; +xfs_mount_t *mp = log->l_mp;struct
xfs_buf_log_format
structxfs_mount
Quoted 6 lines
Quoted 1 line
Why? *_t is also used in a lot of other places.
It is just a general style preference for using the struct instead of the _t in the xfs codebase. Over the course of the past few years they've slowly been converted in this direction, and we prefer not to add any more _t if it can be avoided.
Got it, thanks. I have sent out v3 with this style change.
Quoted 2 lines
-Ben
Signature · 3 lines
Regards, Zhi Yong Wu
Quoted 45 lines · Ben Myers
Hey Zhi,
Quoted 42 lines · Zhi Yong Wu
Quoted 41 lines · Dave Chinner
Quoted 27 lines · On Tue, Jul 30, 2013 at 05:59:07PM +0800, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
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.107sSigned-off-by Zhi Yong Wu <[email protected]>--- fs/xfs/xfs_log_recover.c | 162 +++++++++++++++++++++++++++++++++++++++++++++-- fs/xfs/xfs_log_recover.h | 2 + 2 files changed, 159 insertions(+), 5 deletions(-)fs/xfs/xfs_log_recover.c +0
-0fs/xfs/xfs_log_recover.c xlog_recover_free_trans( · line 3125
Quoted 10 lines
fs/xfs/xfs_log_recover.c +7
-0@@ -3116,6 +3116,111 @@ xlog_recover_free_trans(kmem_free (trans); } +STATIC void +xlog_recover_buffer_ra_pass2 ( +struct xlog *log, +struct xlog_recover_item *item) +{ +xfs_buf_log_format_t *buf_f = item->ri_buf[0 ].i_addr; +xfs_mount_t *mp = log->l_mp;struct
xfs_buf_log_format
structxfs_mount
Quoted 6 lines
Quoted 1 line
Why? *_t is also used in a lot of other places.
It is just a general style preference for using the struct instead of the _t in the xfs codebase. Over the course of the past few years they've slowly been converted in this direction, and we prefer not to add any more _t if it can be avoided.
Actually, it's not so much a preference but a long term code maintenance direction. Documentation/CodingStyle says:
Chapter 5: Typedefs
Please don't use things like "vps_t".
It's a _mistake_ to use typedef for structures and pointers.
,,,,
The original XFS code inherited from Irix used typedefs, and we are slowly removing them as we modify the code that uses the them.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs