* [Ocfs2-devel] [PATCH v2] ocfs2: Rework transaction rollback in ocfs2_relink_block_group()
2013-06-20 5:29 [Ocfs2-devel] [PATCH v2] ocfs2: Rework transaction rollback in ocfs2_relink_block_group() Jeff Liu
@ 2013-06-20 13:10 ` Younger Liu
2013-06-26 22:13 ` Andrew Morton
2013-06-28 2:11 ` Jensen
2 siblings, 0 replies; 6+ messages in thread
From: Younger Liu @ 2013-06-20 13:10 UTC (permalink / raw)
To: ocfs2-devel
On 2013/6/20 13:29, Jeff Liu wrote:
> From: Jie Liu <jeff.liu@oracle.com>
>
> In ocfs2_relink_block_group(), we roll back all those changes if
> notify intent to modify buffers for metadata update failed even
> if the relevant buffer has not yet been modified/got dirty at that
> point, that are not quite right because of:
>
> - None buffer has been modified/dirty if failed to call
> ocfs2_journal_access_gd() against the previous block group buffer
> - Only the previous block group buffer has got dirty if failed to
> call ocfs2_journal_access_gd() against the block group buffer
> - There is no need to roll back the change for file entry buffer at all
>
> Those problems will not cause anything wrong but unnecessary.
> This patch fix them and kill the useless bg_ptr variable as well.
>
> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> Cc: Younger Liu <younger.liu@huawei.com>
Fine.
Reviewed-by: Younger Liu <younger.liu@huawei.com>
> ---
> fs/ocfs2/suballoc.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> index b7e74b5..101d16d 100644
> --- a/fs/ocfs2/suballoc.c
> +++ b/fs/ocfs2/suballoc.c
> @@ -1422,7 +1422,7 @@ static int ocfs2_relink_block_group(handle_t *handle,
> int status;
> /* there is a really tiny chance the journal calls could fail,
> * but we wouldn't want inconsistent blocks in *any* case. */
> - u64 fe_ptr, bg_ptr, prev_bg_ptr;
> + u64 bg_ptr, prev_bg_ptr;
> struct ocfs2_dinode *fe = (struct ocfs2_dinode *) fe_bh->b_data;
> struct ocfs2_group_desc *bg = (struct ocfs2_group_desc *) bg_bh->b_data;
> struct ocfs2_group_desc *prev_bg = (struct ocfs2_group_desc *) prev_bg_bh->b_data;
> @@ -1437,7 +1437,6 @@ static int ocfs2_relink_block_group(handle_t *handle,
> (unsigned long long)le64_to_cpu(bg->bg_blkno),
> (unsigned long long)le64_to_cpu(prev_bg->bg_blkno));
>
> - fe_ptr = le64_to_cpu(fe->id2.i_chain.cl_recs[chain].c_blkno);
> bg_ptr = le64_to_cpu(bg->bg_next_group);
> prev_bg_ptr = le64_to_cpu(prev_bg->bg_next_group);
>
> @@ -1446,7 +1445,7 @@ static int ocfs2_relink_block_group(handle_t *handle,
> OCFS2_JOURNAL_ACCESS_WRITE);
> if (status < 0) {
> mlog_errno(status);
> - goto out_rollback;
> + goto out;
> }
>
> prev_bg->bg_next_group = bg->bg_next_group;
> @@ -1456,7 +1455,7 @@ static int ocfs2_relink_block_group(handle_t *handle,
> bg_bh, OCFS2_JOURNAL_ACCESS_WRITE);
> if (status < 0) {
> mlog_errno(status);
> - goto out_rollback;
> + goto out_rollback_prev_bg;
> }
>
> bg->bg_next_group = fe->id2.i_chain.cl_recs[chain].c_blkno;
> @@ -1466,21 +1465,21 @@ static int ocfs2_relink_block_group(handle_t *handle,
> fe_bh, OCFS2_JOURNAL_ACCESS_WRITE);
> if (status < 0) {
> mlog_errno(status);
> - goto out_rollback;
> + goto out_rollback_bg;
> }
>
> fe->id2.i_chain.cl_recs[chain].c_blkno = bg->bg_blkno;
> ocfs2_journal_dirty(handle, fe_bh);
>
> -out_rollback:
> - if (status < 0) {
> - fe->id2.i_chain.cl_recs[chain].c_blkno = cpu_to_le64(fe_ptr);
> - bg->bg_next_group = cpu_to_le64(bg_ptr);
> - prev_bg->bg_next_group = cpu_to_le64(prev_bg_ptr);
> - }
> +out:
> + return status;
>
> - if (status)
> - mlog_errno(status);
> +out_rollback_bg:
> + bg->bg_next_group = cpu_to_le64(bg_ptr);
> +out_rollback_prev_bg:
> + prev_bg->bg_next_group = cpu_to_le64(prev_bg_ptr);
> +
> + mlog_errno(status);
> return status;
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH v2] ocfs2: Rework transaction rollback in ocfs2_relink_block_group()
2013-06-20 5:29 [Ocfs2-devel] [PATCH v2] ocfs2: Rework transaction rollback in ocfs2_relink_block_group() Jeff Liu
2013-06-20 13:10 ` Younger Liu
@ 2013-06-26 22:13 ` Andrew Morton
2013-06-27 3:42 ` Jeff Liu
2013-06-28 2:11 ` Jensen
2 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2013-06-26 22:13 UTC (permalink / raw)
To: ocfs2-devel
On Thu, 20 Jun 2013 13:29:33 +0800 Jeff Liu <jeff.liu@oracle.com> wrote:
> From: Jie Liu <jeff.liu@oracle.com>
>
> In ocfs2_relink_block_group(), we roll back all those changes if
> notify intent to modify buffers for metadata update failed even
> if the relevant buffer has not yet been modified/got dirty at that
> point, that are not quite right because of:
>
> - None buffer has been modified/dirty if failed to call
> ocfs2_journal_access_gd() against the previous block group buffer
> - Only the previous block group buffer has got dirty if failed to
> call ocfs2_journal_access_gd() against the block group buffer
> - There is no need to roll back the change for file entry buffer at all
>
> Those problems will not cause anything wrong but unnecessary.
> This patch fix them and kill the useless bg_ptr variable as well.
>
> ...
>
> --- a/fs/ocfs2/suballoc.c
> +++ b/fs/ocfs2/suballoc.c
> @@ -1422,7 +1422,7 @@ static int ocfs2_relink_block_group(handle_t *handle,
> int status;
> /* there is a really tiny chance the journal calls could fail,
> * but we wouldn't want inconsistent blocks in *any* case. */
> - u64 fe_ptr, bg_ptr, prev_bg_ptr;
> + u64 bg_ptr, prev_bg_ptr;
> struct ocfs2_dinode *fe = (struct ocfs2_dinode *) fe_bh->b_data;
> struct ocfs2_group_desc *bg = (struct ocfs2_group_desc *) bg_bh->b_data;
> struct ocfs2_group_desc *prev_bg = (struct ocfs2_group_desc *) prev_bg_bh->b_data;
> @@ -1437,7 +1437,6 @@ static int ocfs2_relink_block_group(handle_t *handle,
> (unsigned long long)le64_to_cpu(bg->bg_blkno),
> (unsigned long long)le64_to_cpu(prev_bg->bg_blkno));
>
> - fe_ptr = le64_to_cpu(fe->id2.i_chain.cl_recs[chain].c_blkno);
> bg_ptr = le64_to_cpu(bg->bg_next_group);
> prev_bg_ptr = le64_to_cpu(prev_bg->bg_next_group);
>
> @@ -1446,7 +1445,7 @@ static int ocfs2_relink_block_group(handle_t *handle,
> OCFS2_JOURNAL_ACCESS_WRITE);
> if (status < 0) {
> mlog_errno(status);
> - goto out_rollback;
> + goto out;
> }
>
> prev_bg->bg_next_group = bg->bg_next_group;
> @@ -1456,7 +1455,7 @@ static int ocfs2_relink_block_group(handle_t *handle,
> bg_bh, OCFS2_JOURNAL_ACCESS_WRITE);
> if (status < 0) {
> mlog_errno(status);
> - goto out_rollback;
> + goto out_rollback_prev_bg;
> }
>
> bg->bg_next_group = fe->id2.i_chain.cl_recs[chain].c_blkno;
> @@ -1466,21 +1465,21 @@ static int ocfs2_relink_block_group(handle_t *handle,
> fe_bh, OCFS2_JOURNAL_ACCESS_WRITE);
> if (status < 0) {
> mlog_errno(status);
> - goto out_rollback;
> + goto out_rollback_bg;
> }
>
> fe->id2.i_chain.cl_recs[chain].c_blkno = bg->bg_blkno;
> ocfs2_journal_dirty(handle, fe_bh);
>
> -out_rollback:
> - if (status < 0) {
> - fe->id2.i_chain.cl_recs[chain].c_blkno = cpu_to_le64(fe_ptr);
> - bg->bg_next_group = cpu_to_le64(bg_ptr);
> - prev_bg->bg_next_group = cpu_to_le64(prev_bg_ptr);
> - }
> +out:
> + return status;
>
> - if (status)
> - mlog_errno(status);
> +out_rollback_bg:
> + bg->bg_next_group = cpu_to_le64(bg_ptr);
> +out_rollback_prev_bg:
> + prev_bg->bg_next_group = cpu_to_le64(prev_bg_ptr);
> +
> + mlog_errno(status);
We already called mlog_errno() for this status in all cases.
How does this look?
--- a/fs/ocfs2/suballoc.c~ocfs2-rework-transaction-rollback-in-ocfs2_relink_block_group-fix
+++ a/fs/ocfs2/suballoc.c
@@ -1443,44 +1443,38 @@ static int ocfs2_relink_block_group(hand
status = ocfs2_journal_access_gd(handle, INODE_CACHE(alloc_inode),
prev_bg_bh,
OCFS2_JOURNAL_ACCESS_WRITE);
- if (status < 0) {
- mlog_errno(status);
+ if (status < 0)
goto out;
- }
prev_bg->bg_next_group = bg->bg_next_group;
ocfs2_journal_dirty(handle, prev_bg_bh);
status = ocfs2_journal_access_gd(handle, INODE_CACHE(alloc_inode),
bg_bh, OCFS2_JOURNAL_ACCESS_WRITE);
- if (status < 0) {
- mlog_errno(status);
+ if (status < 0)
goto out_rollback_prev_bg;
- }
bg->bg_next_group = fe->id2.i_chain.cl_recs[chain].c_blkno;
ocfs2_journal_dirty(handle, bg_bh);
status = ocfs2_journal_access_di(handle, INODE_CACHE(alloc_inode),
fe_bh, OCFS2_JOURNAL_ACCESS_WRITE);
- if (status < 0) {
- mlog_errno(status);
+ if (status < 0)
goto out_rollback_bg;
- }
fe->id2.i_chain.cl_recs[chain].c_blkno = bg->bg_blkno;
ocfs2_journal_dirty(handle, fe_bh);
out:
+ if (status < 0)
+ mlog_errno(status);
return status;
out_rollback_bg:
bg->bg_next_group = cpu_to_le64(bg_ptr);
out_rollback_prev_bg:
prev_bg->bg_next_group = cpu_to_le64(prev_bg_ptr);
-
- mlog_errno(status);
- return status;
+ goto out;
}
btw, the ocfs2 source and executable cold be made about half the size if
mlog_errno() were to immediately return if status>=0.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH v2] ocfs2: Rework transaction rollback in ocfs2_relink_block_group()
2013-06-26 22:13 ` Andrew Morton
@ 2013-06-27 3:42 ` Jeff Liu
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Liu @ 2013-06-27 3:42 UTC (permalink / raw)
To: ocfs2-devel
On 06/27/2013 06:13 AM, Andrew Morton wrote:
> On Thu, 20 Jun 2013 13:29:33 +0800 Jeff Liu <jeff.liu@oracle.com> wrote:
>
>> From: Jie Liu <jeff.liu@oracle.com>
>>
>> In ocfs2_relink_block_group(), we roll back all those changes if
>> notify intent to modify buffers for metadata update failed even
>> if the relevant buffer has not yet been modified/got dirty at that
>> point, that are not quite right because of:
>>
>> - None buffer has been modified/dirty if failed to call
>> ocfs2_journal_access_gd() against the previous block group buffer
>> - Only the previous block group buffer has got dirty if failed to
>> call ocfs2_journal_access_gd() against the block group buffer
>> - There is no need to roll back the change for file entry buffer at all
>>
>> Those problems will not cause anything wrong but unnecessary.
>> This patch fix them and kill the useless bg_ptr variable as well.
>>
>> ...
>>
>> --- a/fs/ocfs2/suballoc.c
>> +++ b/fs/ocfs2/suballoc.c
>> @@ -1422,7 +1422,7 @@ static int ocfs2_relink_block_group(handle_t *handle,
>> int status;
>> /* there is a really tiny chance the journal calls could fail,
>> * but we wouldn't want inconsistent blocks in *any* case. */
>> - u64 fe_ptr, bg_ptr, prev_bg_ptr;
>> + u64 bg_ptr, prev_bg_ptr;
>> struct ocfs2_dinode *fe = (struct ocfs2_dinode *) fe_bh->b_data;
>> struct ocfs2_group_desc *bg = (struct ocfs2_group_desc *) bg_bh->b_data;
>> struct ocfs2_group_desc *prev_bg = (struct ocfs2_group_desc *) prev_bg_bh->b_data;
>> @@ -1437,7 +1437,6 @@ static int ocfs2_relink_block_group(handle_t *handle,
>> (unsigned long long)le64_to_cpu(bg->bg_blkno),
>> (unsigned long long)le64_to_cpu(prev_bg->bg_blkno));
>>
>> - fe_ptr = le64_to_cpu(fe->id2.i_chain.cl_recs[chain].c_blkno);
>> bg_ptr = le64_to_cpu(bg->bg_next_group);
>> prev_bg_ptr = le64_to_cpu(prev_bg->bg_next_group);
>>
>> @@ -1446,7 +1445,7 @@ static int ocfs2_relink_block_group(handle_t *handle,
>> OCFS2_JOURNAL_ACCESS_WRITE);
>> if (status < 0) {
>> mlog_errno(status);
>> - goto out_rollback;
>> + goto out;
>> }
>>
>> prev_bg->bg_next_group = bg->bg_next_group;
>> @@ -1456,7 +1455,7 @@ static int ocfs2_relink_block_group(handle_t *handle,
>> bg_bh, OCFS2_JOURNAL_ACCESS_WRITE);
>> if (status < 0) {
>> mlog_errno(status);
>> - goto out_rollback;
>> + goto out_rollback_prev_bg;
>> }
>>
>> bg->bg_next_group = fe->id2.i_chain.cl_recs[chain].c_blkno;
>> @@ -1466,21 +1465,21 @@ static int ocfs2_relink_block_group(handle_t *handle,
>> fe_bh, OCFS2_JOURNAL_ACCESS_WRITE);
>> if (status < 0) {
>> mlog_errno(status);
>> - goto out_rollback;
>> + goto out_rollback_bg;
>> }
>>
>> fe->id2.i_chain.cl_recs[chain].c_blkno = bg->bg_blkno;
>> ocfs2_journal_dirty(handle, fe_bh);
>>
>> -out_rollback:
>> - if (status < 0) {
>> - fe->id2.i_chain.cl_recs[chain].c_blkno = cpu_to_le64(fe_ptr);
>> - bg->bg_next_group = cpu_to_le64(bg_ptr);
>> - prev_bg->bg_next_group = cpu_to_le64(prev_bg_ptr);
>> - }
>> +out:
>> + return status;
>>
>> - if (status)
>> - mlog_errno(status);
>> +out_rollback_bg:
>> + bg->bg_next_group = cpu_to_le64(bg_ptr);
>> +out_rollback_prev_bg:
>> + prev_bg->bg_next_group = cpu_to_le64(prev_bg_ptr);
>> +
>> + mlog_errno(status);
>
> We already called mlog_errno() for this status in all cases.
Yep.
>
> How does this look?
Looks fine :-P
>
> --- a/fs/ocfs2/suballoc.c~ocfs2-rework-transaction-rollback-in-ocfs2_relink_block_group-fix
> +++ a/fs/ocfs2/suballoc.c
> @@ -1443,44 +1443,38 @@ static int ocfs2_relink_block_group(hand
> status = ocfs2_journal_access_gd(handle, INODE_CACHE(alloc_inode),
> prev_bg_bh,
> OCFS2_JOURNAL_ACCESS_WRITE);
> - if (status < 0) {
> - mlog_errno(status);
> + if (status < 0)
> goto out;
> - }
>
> prev_bg->bg_next_group = bg->bg_next_group;
> ocfs2_journal_dirty(handle, prev_bg_bh);
>
> status = ocfs2_journal_access_gd(handle, INODE_CACHE(alloc_inode),
> bg_bh, OCFS2_JOURNAL_ACCESS_WRITE);
> - if (status < 0) {
> - mlog_errno(status);
> + if (status < 0)
> goto out_rollback_prev_bg;
> - }
>
> bg->bg_next_group = fe->id2.i_chain.cl_recs[chain].c_blkno;
> ocfs2_journal_dirty(handle, bg_bh);
>
> status = ocfs2_journal_access_di(handle, INODE_CACHE(alloc_inode),
> fe_bh, OCFS2_JOURNAL_ACCESS_WRITE);
> - if (status < 0) {
> - mlog_errno(status);
> + if (status < 0)
> goto out_rollback_bg;
> - }
>
> fe->id2.i_chain.cl_recs[chain].c_blkno = bg->bg_blkno;
> ocfs2_journal_dirty(handle, fe_bh);
>
> out:
> + if (status < 0)
> + mlog_errno(status);
> return status;
>
> out_rollback_bg:
> bg->bg_next_group = cpu_to_le64(bg_ptr);
> out_rollback_prev_bg:
> prev_bg->bg_next_group = cpu_to_le64(prev_bg_ptr);
> -
> - mlog_errno(status);
> - return status;
> + goto out;
> }
>
>
> btw, the ocfs2 source and executable cold be made about half the size if
> mlog_errno() were to immediately return if status>=0.
I'm going to deal with it, thanks for pointing this out!
-Jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH v2] ocfs2: Rework transaction rollback in ocfs2_relink_block_group()
2013-06-20 5:29 [Ocfs2-devel] [PATCH v2] ocfs2: Rework transaction rollback in ocfs2_relink_block_group() Jeff Liu
2013-06-20 13:10 ` Younger Liu
2013-06-26 22:13 ` Andrew Morton
@ 2013-06-28 2:11 ` Jensen
2013-06-28 4:11 ` Jeff Liu
2 siblings, 1 reply; 6+ messages in thread
From: Jensen @ 2013-06-28 2:11 UTC (permalink / raw)
To: ocfs2-devel
it looks good , but it is better that if moving the following two sentence to before 'ocfs2_journal_dirty(handle, fe_bh);' sentence.
1.ocfs2_journal_dirty(handle, prev_bg_bh);
2.ocfs2_journal_dirty(handle, bg_bh);
because if it fail, it maybe commit by jbd2 journal.
Thanks,
Jensen
On 2013/6/20 13:29, Jeff Liu wrote:
> From: Jie Liu <jeff.liu@oracle.com>
>
> In ocfs2_relink_block_group(), we roll back all those changes if
> notify intent to modify buffers for metadata update failed even
> if the relevant buffer has not yet been modified/got dirty at that
> point, that are not quite right because of:
>
> - None buffer has been modified/dirty if failed to call
> ocfs2_journal_access_gd() against the previous block group buffer
> - Only the previous block group buffer has got dirty if failed to
> call ocfs2_journal_access_gd() against the block group buffer
> - There is no need to roll back the change for file entry buffer at all
>
> Those problems will not cause anything wrong but unnecessary.
> This patch fix them and kill the useless bg_ptr variable as well.
>
> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> Cc: Younger Liu <younger.liu@huawei.com>
> ---
> fs/ocfs2/suballoc.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> index b7e74b5..101d16d 100644
> --- a/fs/ocfs2/suballoc.c
> +++ b/fs/ocfs2/suballoc.c
> @@ -1422,7 +1422,7 @@ static int ocfs2_relink_block_group(handle_t *handle,
> int status;
> /* there is a really tiny chance the journal calls could fail,
> * but we wouldn't want inconsistent blocks in *any* case. */
> - u64 fe_ptr, bg_ptr, prev_bg_ptr;
> + u64 bg_ptr, prev_bg_ptr;
> struct ocfs2_dinode *fe = (struct ocfs2_dinode *) fe_bh->b_data;
> struct ocfs2_group_desc *bg = (struct ocfs2_group_desc *) bg_bh->b_data;
> struct ocfs2_group_desc *prev_bg = (struct ocfs2_group_desc *) prev_bg_bh->b_data;
> @@ -1437,7 +1437,6 @@ static int ocfs2_relink_block_group(handle_t *handle,
> (unsigned long long)le64_to_cpu(bg->bg_blkno),
> (unsigned long long)le64_to_cpu(prev_bg->bg_blkno));
>
> - fe_ptr = le64_to_cpu(fe->id2.i_chain.cl_recs[chain].c_blkno);
> bg_ptr = le64_to_cpu(bg->bg_next_group);
> prev_bg_ptr = le64_to_cpu(prev_bg->bg_next_group);
>
> @@ -1446,7 +1445,7 @@ static int ocfs2_relink_block_group(handle_t *handle,
> OCFS2_JOURNAL_ACCESS_WRITE);
> if (status < 0) {
> mlog_errno(status);
> - goto out_rollback;
> + goto out;
> }
>
> prev_bg->bg_next_group = bg->bg_next_group;
> @@ -1456,7 +1455,7 @@ static int ocfs2_relink_block_group(handle_t *handle,
> bg_bh, OCFS2_JOURNAL_ACCESS_WRITE);
> if (status < 0) {
> mlog_errno(status);
> - goto out_rollback;
> + goto out_rollback_prev_bg;
> }
>
> bg->bg_next_group = fe->id2.i_chain.cl_recs[chain].c_blkno;
> @@ -1466,21 +1465,21 @@ static int ocfs2_relink_block_group(handle_t *handle,
> fe_bh, OCFS2_JOURNAL_ACCESS_WRITE);
> if (status < 0) {
> mlog_errno(status);
> - goto out_rollback;
> + goto out_rollback_bg;
> }
>
> fe->id2.i_chain.cl_recs[chain].c_blkno = bg->bg_blkno;
> ocfs2_journal_dirty(handle, fe_bh);
>
> -out_rollback:
> - if (status < 0) {
> - fe->id2.i_chain.cl_recs[chain].c_blkno = cpu_to_le64(fe_ptr);
> - bg->bg_next_group = cpu_to_le64(bg_ptr);
> - prev_bg->bg_next_group = cpu_to_le64(prev_bg_ptr);
> - }
> +out:
> + return status;
>
> - if (status)
> - mlog_errno(status);
> +out_rollback_bg:
> + bg->bg_next_group = cpu_to_le64(bg_ptr);
> +out_rollback_prev_bg:
> + prev_bg->bg_next_group = cpu_to_le64(prev_bg_ptr);
> +
> + mlog_errno(status);
> return status;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH v2] ocfs2: Rework transaction rollback in ocfs2_relink_block_group()
2013-06-28 2:11 ` Jensen
@ 2013-06-28 4:11 ` Jeff Liu
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Liu @ 2013-06-28 4:11 UTC (permalink / raw)
To: ocfs2-devel
On 06/28/2013 10:11 AM, Jensen wrote:
> it looks good , but it is better that if moving the following two sentence to before 'ocfs2_journal_dirty(handle, fe_bh);' sentence.
> 1.ocfs2_journal_dirty(handle, prev_bg_bh);
> 2.ocfs2_journal_dirty(handle, bg_bh);
Thanks for your review, but are you sure you gone over the code?
-Jeff
>
> because if it fail, it maybe commit by jbd2 journal.
>
> Thanks,
> Jensen
>
> On 2013/6/20 13:29, Jeff Liu wrote:
>
>> From: Jie Liu <jeff.liu@oracle.com>
>>
>> In ocfs2_relink_block_group(), we roll back all those changes if
>> notify intent to modify buffers for metadata update failed even
>> if the relevant buffer has not yet been modified/got dirty at that
>> point, that are not quite right because of:
>>
>> - None buffer has been modified/dirty if failed to call
>> ocfs2_journal_access_gd() against the previous block group buffer
>> - Only the previous block group buffer has got dirty if failed to
>> call ocfs2_journal_access_gd() against the block group buffer
>> - There is no need to roll back the change for file entry buffer at all
>>
>> Those problems will not cause anything wrong but unnecessary.
>> This patch fix them and kill the useless bg_ptr variable as well.
>>
>> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
>> Cc: Younger Liu <younger.liu@huawei.com>
>> ---
>> fs/ocfs2/suballoc.c | 25 ++++++++++++-------------
>> 1 file changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
>> index b7e74b5..101d16d 100644
>> --- a/fs/ocfs2/suballoc.c
>> +++ b/fs/ocfs2/suballoc.c
>> @@ -1422,7 +1422,7 @@ static int ocfs2_relink_block_group(handle_t *handle,
>> int status;
>> /* there is a really tiny chance the journal calls could fail,
>> * but we wouldn't want inconsistent blocks in *any* case. */
>> - u64 fe_ptr, bg_ptr, prev_bg_ptr;
>> + u64 bg_ptr, prev_bg_ptr;
>> struct ocfs2_dinode *fe = (struct ocfs2_dinode *) fe_bh->b_data;
>> struct ocfs2_group_desc *bg = (struct ocfs2_group_desc *) bg_bh->b_data;
>> struct ocfs2_group_desc *prev_bg = (struct ocfs2_group_desc *) prev_bg_bh->b_data;
>> @@ -1437,7 +1437,6 @@ static int ocfs2_relink_block_group(handle_t *handle,
>> (unsigned long long)le64_to_cpu(bg->bg_blkno),
>> (unsigned long long)le64_to_cpu(prev_bg->bg_blkno));
>>
>> - fe_ptr = le64_to_cpu(fe->id2.i_chain.cl_recs[chain].c_blkno);
>> bg_ptr = le64_to_cpu(bg->bg_next_group);
>> prev_bg_ptr = le64_to_cpu(prev_bg->bg_next_group);
>>
>> @@ -1446,7 +1445,7 @@ static int ocfs2_relink_block_group(handle_t *handle,
>> OCFS2_JOURNAL_ACCESS_WRITE);
>> if (status < 0) {
>> mlog_errno(status);
>> - goto out_rollback;
>> + goto out;
>> }
>>
>> prev_bg->bg_next_group = bg->bg_next_group;
>> @@ -1456,7 +1455,7 @@ static int ocfs2_relink_block_group(handle_t *handle,
>> bg_bh, OCFS2_JOURNAL_ACCESS_WRITE);
>> if (status < 0) {
>> mlog_errno(status);
>> - goto out_rollback;
>> + goto out_rollback_prev_bg;
>> }
>>
>> bg->bg_next_group = fe->id2.i_chain.cl_recs[chain].c_blkno;
>> @@ -1466,21 +1465,21 @@ static int ocfs2_relink_block_group(handle_t *handle,
>> fe_bh, OCFS2_JOURNAL_ACCESS_WRITE);
>> if (status < 0) {
>> mlog_errno(status);
>> - goto out_rollback;
>> + goto out_rollback_bg;
>> }
>>
>> fe->id2.i_chain.cl_recs[chain].c_blkno = bg->bg_blkno;
>> ocfs2_journal_dirty(handle, fe_bh);
>>
>> -out_rollback:
>> - if (status < 0) {
>> - fe->id2.i_chain.cl_recs[chain].c_blkno = cpu_to_le64(fe_ptr);
>> - bg->bg_next_group = cpu_to_le64(bg_ptr);
>> - prev_bg->bg_next_group = cpu_to_le64(prev_bg_ptr);
>> - }
>> +out:
>> + return status;
>>
>> - if (status)
>> - mlog_errno(status);
>> +out_rollback_bg:
>> + bg->bg_next_group = cpu_to_le64(bg_ptr);
>> +out_rollback_prev_bg:
>> + prev_bg->bg_next_group = cpu_to_le64(prev_bg_ptr);
>> +
>> + mlog_errno(status);
>> return status;
>> }
>>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread