* [Ocfs2-devel] [PATCH v2] ocfs2: Rework transaction rollback in ocfs2_relink_block_group()
@ 2013-06-20 5:29 Jeff Liu
2013-06-20 13:10 ` Younger Liu
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jeff Liu @ 2013-06-20 5:29 UTC (permalink / raw)
To: ocfs2-devel
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;
}
--
1.7.9.5
^ permalink raw reply related [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
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
end of thread, other threads:[~2013-06-28 4:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2013-06-28 4:11 ` Jeff Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.