* [Ocfs2-devel] [PATCH] ocfs2: need rollback when journal_access failed in ocfs2_orphan_add()
@ 2013-06-19 8:57 Younger Liu
2013-06-19 10:53 ` Jeff Liu
0 siblings, 1 reply; 6+ messages in thread
From: Younger Liu @ 2013-06-19 8:57 UTC (permalink / raw)
To: ocfs2-devel
While adding a file into orphan dir in ocfs2_orphan_add(),
it calls __ocfs2_add_entry() before ocfs2_journal_access_di().
If ocfs2_journal_access_di() failed, the file is added into
orphan dir, and orphan dir dinode updated, but file dinode
has not been updated.
Accordingly, the data is not consistent between file dinode
and orphan dir.
So, need to call ocfs2_journal_access_di() before __ocfs2_add_entry(),
and if ocfs2_journal_access_di() failed, orphan_fe and
orphan_dir_inode->i_nlink need rollback.
Signed-off-by: Younger Liu <younger.liu@huawei.com>
---
fs/ocfs2/namei.c | 38 ++++++++++++++++++++++----------------
1 file changed, 22 insertions(+), 16 deletions(-)
diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index f53471d..f2da854 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -2012,6 +2012,21 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
goto leave;
}
+ /*
+ * We're going to journal the change of i_flags and i_orphaned_slot.
+ * It's safe anyway, though some callers may duplicate the journaling.
+ * Journaling within the func just make the logic look more
+ * straightforward.
+ */
+ status = ocfs2_journal_access_di(handle,
+ INODE_CACHE(inode),
+ fe_bh,
+ OCFS2_JOURNAL_ACCESS_WRITE);
+ if (status < 0) {
+ mlog_errno(status);
+ goto leave;
+ }
+
/* we're a cluster, and nlink can change on disk from
* underneath us... */
orphan_fe = (struct ocfs2_dinode *) orphan_dir_bh->b_data;
@@ -2026,22 +2041,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
orphan_dir_bh, lookup);
if (status < 0) {
mlog_errno(status);
- goto leave;
- }
-
- /*
- * We're going to journal the change of i_flags and i_orphaned_slot.
- * It's safe anyway, though some callers may duplicate the journaling.
- * Journaling within the func just make the logic look more
- * straightforward.
- */
- status = ocfs2_journal_access_di(handle,
- INODE_CACHE(inode),
- fe_bh,
- OCFS2_JOURNAL_ACCESS_WRITE);
- if (status < 0) {
- mlog_errno(status);
- goto leave;
+ goto rollback;
}
fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);
@@ -2057,6 +2057,12 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
trace_ocfs2_orphan_add_end((unsigned long long)OCFS2_I(inode)->ip_blkno,
osb->slot_num);
+rollback:
+ if (status < 0 && S_ISDIR(inode->i_mode)) {
+ ocfs2_add_links_count(orphan_fe, -1);
+ set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
+ }
+
leave:
brelse(orphan_dir_bh);
-- 1.7.9.7
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: need rollback when journal_access failed in ocfs2_orphan_add()
2013-06-19 8:57 [Ocfs2-devel] [PATCH] ocfs2: need rollback when journal_access failed in ocfs2_orphan_add() Younger Liu
@ 2013-06-19 10:53 ` Jeff Liu
2013-06-20 1:25 ` Younger Liu
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Liu @ 2013-06-19 10:53 UTC (permalink / raw)
To: ocfs2-devel
Hey Younger,
On 06/19/2013 04:57 PM, Younger Liu wrote:
> While adding a file into orphan dir in ocfs2_orphan_add(),
> it calls __ocfs2_add_entry() before ocfs2_journal_access_di().
> If ocfs2_journal_access_di() failed, the file is added into
> orphan dir, and orphan dir dinode updated, but file dinode
> has not been updated.
> Accordingly, the data is not consistent between file dinode
> and orphan dir.
>
> So, need to call ocfs2_journal_access_di() before __ocfs2_add_entry(),
> and if ocfs2_journal_access_di() failed, orphan_fe and
> orphan_dir_inode->i_nlink need rollback.
Yes, and this has been introduced by commits 3939fda4.
Please see my comments below.
>
> Signed-off-by: Younger Liu <younger.liu@huawei.com>
> ---
> fs/ocfs2/namei.c | 38 ++++++++++++++++++++++----------------
> 1 file changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> index f53471d..f2da854 100644
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -2012,6 +2012,21 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
> goto leave;
> }
>
> + /*
> + * We're going to journal the change of i_flags and i_orphaned_slot.
> + * It's safe anyway, though some callers may duplicate the journaling.
> + * Journaling within the func just make the logic look more
> + * straightforward.
> + */
> + status = ocfs2_journal_access_di(handle,
> + INODE_CACHE(inode),
> + fe_bh,
> + OCFS2_JOURNAL_ACCESS_WRITE);
> + if (status < 0) {
> + mlog_errno(status);
> + goto leave;
> + }
> +
> /* we're a cluster, and nlink can change on disk from
> * underneath us... */
> orphan_fe = (struct ocfs2_dinode *) orphan_dir_bh->b_data;
> @@ -2026,22 +2041,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
> orphan_dir_bh, lookup);
> if (status < 0) {
> mlog_errno(status);
> - goto leave;
> - }
> -
> - /*
> - * We're going to journal the change of i_flags and i_orphaned_slot.
> - * It's safe anyway, though some callers may duplicate the journaling.
> - * Journaling within the func just make the logic look more
> - * straightforward.
> - */
> - status = ocfs2_journal_access_di(handle,
> - INODE_CACHE(inode),
> - fe_bh,
> - OCFS2_JOURNAL_ACCESS_WRITE);
> - if (status < 0) {
> - mlog_errno(status);
> - goto leave;
> + goto rollback;
> }
>
> fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);
> @@ -2057,6 +2057,12 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
> trace_ocfs2_orphan_add_end((unsigned long long)OCFS2_I(inode)->ip_blkno,
> osb->slot_num);
>
> +rollback:
> + if (status < 0 && S_ISDIR(inode->i_mode)) {
^^^^ <-- But that might not be a directory, right? :-P
How about the following?
if (status < 0) {
if (S_ISDIR(inode->i_mode))
ocfs2_add_links_count(orphan_fe, -1);
set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
}
> + ocfs2_add_links_count(orphan_fe, -1);
> + set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
> + }
> +
> leave:
> brelse(orphan_dir_bh);
>
> -- 1.7.9.7
Thanks,
-Jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: need rollback when journal_access failed in ocfs2_orphan_add()
2013-06-19 10:53 ` Jeff Liu
@ 2013-06-20 1:25 ` Younger Liu
2013-06-20 1:42 ` Li Zefan
2013-06-20 5:52 ` Jeff Liu
0 siblings, 2 replies; 6+ messages in thread
From: Younger Liu @ 2013-06-20 1:25 UTC (permalink / raw)
To: ocfs2-devel
Hi Jeff
On 2013/6/19 18:53, Jeff Liu wrote:
> Hey Younger,
>
> On 06/19/2013 04:57 PM, Younger Liu wrote:
>
>> While adding a file into orphan dir in ocfs2_orphan_add(),
>> it calls __ocfs2_add_entry() before ocfs2_journal_access_di().
>> If ocfs2_journal_access_di() failed, the file is added into
>> orphan dir, and orphan dir dinode updated, but file dinode
>> has not been updated.
>> Accordingly, the data is not consistent between file dinode
>> and orphan dir.
>>
>> So, need to call ocfs2_journal_access_di() before __ocfs2_add_entry(),
>> and if ocfs2_journal_access_di() failed, orphan_fe and
>> orphan_dir_inode->i_nlink need rollback.
>
> Yes, and this has been introduced by commits 3939fda4.
I do not find the patch with 3939fda4. Can you tell me the link?
Thanks.
> Please see my comments below.
>
>>
>> Signed-off-by: Younger Liu <younger.liu@huawei.com>
>> ---
>> fs/ocfs2/namei.c | 38 ++++++++++++++++++++++----------------
>> 1 file changed, 22 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
>> index f53471d..f2da854 100644
>> --- a/fs/ocfs2/namei.c
>> +++ b/fs/ocfs2/namei.c
>> @@ -2012,6 +2012,21 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
>> goto leave;
>> }
>>
>> + /*
>> + * We're going to journal the change of i_flags and i_orphaned_slot.
>> + * It's safe anyway, though some callers may duplicate the journaling.
>> + * Journaling within the func just make the logic look more
>> + * straightforward.
>> + */
>> + status = ocfs2_journal_access_di(handle,
>> + INODE_CACHE(inode),
>> + fe_bh,
>> + OCFS2_JOURNAL_ACCESS_WRITE);
>> + if (status < 0) {
>> + mlog_errno(status);
>> + goto leave;
>> + }
>> +
>> /* we're a cluster, and nlink can change on disk from
>> * underneath us... */
>> orphan_fe = (struct ocfs2_dinode *) orphan_dir_bh->b_data;
>> @@ -2026,22 +2041,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
>> orphan_dir_bh, lookup);
>> if (status < 0) {
>> mlog_errno(status);
>> - goto leave;
>> - }
>> -
>> - /*
>> - * We're going to journal the change of i_flags and i_orphaned_slot.
>> - * It's safe anyway, though some callers may duplicate the journaling.
>> - * Journaling within the func just make the logic look more
>> - * straightforward.
>> - */
>> - status = ocfs2_journal_access_di(handle,
>> - INODE_CACHE(inode),
>> - fe_bh,
>> - OCFS2_JOURNAL_ACCESS_WRITE);
>> - if (status < 0) {
>> - mlog_errno(status);
>> - goto leave;
>> + goto rollback;
>> }
>>
>> fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);
>> @@ -2057,6 +2057,12 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
>> trace_ocfs2_orphan_add_end((unsigned long long)OCFS2_I(inode)->ip_blkno,
>> osb->slot_num);
>>
>> +rollback:
>> + if (status < 0 && S_ISDIR(inode->i_mode)) {
>
> ^^^^ <-- But that might not be a directory, right? :-P
> How about the following?
>
> if (status < 0) {
> if (S_ISDIR(inode->i_mode))
> ocfs2_add_links_count(orphan_fe, -1);
> set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
> }
>
Of course, that may be not a directory.
But if it is not a directory, there is no change for orphan_dir_inode->i_nlink.
So, in my opinion, it is not necessary to change orphan_dir_inode->i_nlink.
>> + ocfs2_add_links_count(orphan_fe, -1);
>> + set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
>> + }
>> +
>> leave:
>> brelse(orphan_dir_bh);
>>
>> -- 1.7.9.7
>
>
> Thanks,
> -Jeff
>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: need rollback when journal_access failed in ocfs2_orphan_add()
2013-06-20 1:25 ` Younger Liu
@ 2013-06-20 1:42 ` Li Zefan
2013-06-20 5:52 ` Jeff Liu
1 sibling, 0 replies; 6+ messages in thread
From: Li Zefan @ 2013-06-20 1:42 UTC (permalink / raw)
To: ocfs2-devel
? 2013/6/20 9:25, Younger Liu ??:
> Hi Jeff
>
> On 2013/6/19 18:53, Jeff Liu wrote:
>> Hey Younger,
>>
>> On 06/19/2013 04:57 PM, Younger Liu wrote:
>>
>>> While adding a file into orphan dir in ocfs2_orphan_add(),
>>> it calls __ocfs2_add_entry() before ocfs2_journal_access_di().
>>> If ocfs2_journal_access_di() failed, the file is added into
>>> orphan dir, and orphan dir dinode updated, but file dinode
>>> has not been updated.
>>> Accordingly, the data is not consistent between file dinode
>>> and orphan dir.
>>>
>>> So, need to call ocfs2_journal_access_di() before __ocfs2_add_entry(),
>>> and if ocfs2_journal_access_di() failed, orphan_fe and
>>> orphan_dir_inode->i_nlink need rollback.
>>
>> Yes, and this has been introduced by commits 3939fda4.
>
> I do not find the patch with 3939fda4. Can you tell me the link?
> Thanks.
>
# git show 3939fda4
commit 3939fda4b389993caf8741df5739b3e49f33a263
Author: Tristan Ye <tristan.ye@oracle.com>
Date: Fri Mar 19 09:21:09 2010 +0800
Ocfs2: Journaling i_flags and i_orphaned_slot when adding inode to orphan dir.
Currently, some callers were missing to journal the dirty inode after
adding it to orphan dir.
Now we're going to journal such modifications within the ocfs2_orphan_add()
itself, It's safe to do so, though some existing caller may duplicate this,
and it makes the logic look more straightforward anyway.
Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
Signed-off-by: Joel Becker <joel.becker@oracle.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: need rollback when journal_access failed in ocfs2_orphan_add()
2013-06-20 1:25 ` Younger Liu
2013-06-20 1:42 ` Li Zefan
@ 2013-06-20 5:52 ` Jeff Liu
2013-06-20 6:33 ` Younger Liu
1 sibling, 1 reply; 6+ messages in thread
From: Jeff Liu @ 2013-06-20 5:52 UTC (permalink / raw)
To: ocfs2-devel
On 06/20/2013 09:25 AM, Younger Liu wrote:
> Hi Jeff
>
> On 2013/6/19 18:53, Jeff Liu wrote:
>> Hey Younger,
>>
>> On 06/19/2013 04:57 PM, Younger Liu wrote:
>>
>>> While adding a file into orphan dir in ocfs2_orphan_add(),
>>> it calls __ocfs2_add_entry() before ocfs2_journal_access_di().
>>> If ocfs2_journal_access_di() failed, the file is added into
>>> orphan dir, and orphan dir dinode updated, but file dinode
>>> has not been updated.
>>> Accordingly, the data is not consistent between file dinode
>>> and orphan dir.
>>>
>>> So, need to call ocfs2_journal_access_di() before __ocfs2_add_entry(),
>>> and if ocfs2_journal_access_di() failed, orphan_fe and
>>> orphan_dir_inode->i_nlink need rollback.
>>
>> Yes, and this has been introduced by commits 3939fda4.
>
> I do not find the patch with 3939fda4. Can you tell me the link?
> Thanks.
>
>> Please see my comments below.
>>
>>>
>>> Signed-off-by: Younger Liu <younger.liu@huawei.com>
>>> ---
>>> fs/ocfs2/namei.c | 38 ++++++++++++++++++++++----------------
>>> 1 file changed, 22 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
>>> index f53471d..f2da854 100644
>>> --- a/fs/ocfs2/namei.c
>>> +++ b/fs/ocfs2/namei.c
>>> @@ -2012,6 +2012,21 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
>>> goto leave;
>>> }
>>>
>>> + /*
>>> + * We're going to journal the change of i_flags and i_orphaned_slot.
>>> + * It's safe anyway, though some callers may duplicate the journaling.
>>> + * Journaling within the func just make the logic look more
>>> + * straightforward.
>>> + */
>>> + status = ocfs2_journal_access_di(handle,
>>> + INODE_CACHE(inode),
>>> + fe_bh,
>>> + OCFS2_JOURNAL_ACCESS_WRITE);
>>> + if (status < 0) {
>>> + mlog_errno(status);
>>> + goto leave;
>>> + }
>>> +
>>> /* we're a cluster, and nlink can change on disk from
>>> * underneath us... */
>>> orphan_fe = (struct ocfs2_dinode *) orphan_dir_bh->b_data;
>>> @@ -2026,22 +2041,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
>>> orphan_dir_bh, lookup);
>>> if (status < 0) {
>>> mlog_errno(status);
>>> - goto leave;
>>> - }
>>> -
>>> - /*
>>> - * We're going to journal the change of i_flags and i_orphaned_slot.
>>> - * It's safe anyway, though some callers may duplicate the journaling.
>>> - * Journaling within the func just make the logic look more
>>> - * straightforward.
>>> - */
>>> - status = ocfs2_journal_access_di(handle,
>>> - INODE_CACHE(inode),
>>> - fe_bh,
>>> - OCFS2_JOURNAL_ACCESS_WRITE);
>>> - if (status < 0) {
>>> - mlog_errno(status);
>>> - goto leave;
>>> + goto rollback;
>>> }
>>>
>>> fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);
>>> @@ -2057,6 +2057,12 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
>>> trace_ocfs2_orphan_add_end((unsigned long long)OCFS2_I(inode)->ip_blkno,
>>> osb->slot_num);
>>>
>>> +rollback:
>>> + if (status < 0 && S_ISDIR(inode->i_mode)) {
>>
>> ^^^^ <-- But that might not be a directory, right? :-P
>> How about the following?
>>
>> if (status < 0) {
>> if (S_ISDIR(inode->i_mode))
>> ocfs2_add_links_count(orphan_fe, -1);
>> set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
>> }
>>
>
> Of course, that may be not a directory.
> But if it is not a directory, there is no change for orphan_dir_inode->i_nlink.
> So, in my opinion, it is not necessary to change orphan_dir_inode->i_nlink.
if (S_ISDIR(inode->i_mode))
ocfs2_add_links_count(orphan_fe, 1);
set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
^^^^^^^^^
So we set nlink against orphan_dir_inode no matter IS_DIR(inode->i_mode) or not.
ocfs2_journal_dirty(handle, orphan_dir_bh);
Any then orphan_dir buffer got dirty.
if (status < 0 && S_ISDIR(inode->i_mode)) {
In this way, AFAICS the trans rollback can be triggered only if
the target inode is a dir.
}
As a result, should we rollback in case of:
if (status < 0)
set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
Thanks,
-Jeff
>
>>> + ocfs2_add_links_count(orphan_fe, -1);
>>> + set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
>>> + }
>>> +
>>> leave:
>>> brelse(orphan_dir_bh);
>>>
>>> -- 1.7.9.7
>>
>>
>> Thanks,
>> -Jeff
>>
>> .
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: need rollback when journal_access failed in ocfs2_orphan_add()
2013-06-20 5:52 ` Jeff Liu
@ 2013-06-20 6:33 ` Younger Liu
0 siblings, 0 replies; 6+ messages in thread
From: Younger Liu @ 2013-06-20 6:33 UTC (permalink / raw)
To: ocfs2-devel
Ok, thanks.
I will resent the patch.
On 2013/6/20 13:52, Jeff Liu wrote:
> On 06/20/2013 09:25 AM, Younger Liu wrote:
>
>> Hi Jeff
>>
>> On 2013/6/19 18:53, Jeff Liu wrote:
>>> Hey Younger,
>>>
>>> On 06/19/2013 04:57 PM, Younger Liu wrote:
>>>
>>>> While adding a file into orphan dir in ocfs2_orphan_add(),
>>>> it calls __ocfs2_add_entry() before ocfs2_journal_access_di().
>>>> If ocfs2_journal_access_di() failed, the file is added into
>>>> orphan dir, and orphan dir dinode updated, but file dinode
>>>> has not been updated.
>>>> Accordingly, the data is not consistent between file dinode
>>>> and orphan dir.
>>>>
>>>> So, need to call ocfs2_journal_access_di() before __ocfs2_add_entry(),
>>>> and if ocfs2_journal_access_di() failed, orphan_fe and
>>>> orphan_dir_inode->i_nlink need rollback.
>>>
>>> Yes, and this has been introduced by commits 3939fda4.
>>
>> I do not find the patch with 3939fda4. Can you tell me the link?
>> Thanks.
>>
>>> Please see my comments below.
>>>
>>>>
>>>> Signed-off-by: Younger Liu <younger.liu@huawei.com>
>>>> ---
>>>> fs/ocfs2/namei.c | 38 ++++++++++++++++++++++----------------
>>>> 1 file changed, 22 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
>>>> index f53471d..f2da854 100644
>>>> --- a/fs/ocfs2/namei.c
>>>> +++ b/fs/ocfs2/namei.c
>>>> @@ -2012,6 +2012,21 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
>>>> goto leave;
>>>> }
>>>>
>>>> + /*
>>>> + * We're going to journal the change of i_flags and i_orphaned_slot.
>>>> + * It's safe anyway, though some callers may duplicate the journaling.
>>>> + * Journaling within the func just make the logic look more
>>>> + * straightforward.
>>>> + */
>>>> + status = ocfs2_journal_access_di(handle,
>>>> + INODE_CACHE(inode),
>>>> + fe_bh,
>>>> + OCFS2_JOURNAL_ACCESS_WRITE);
>>>> + if (status < 0) {
>>>> + mlog_errno(status);
>>>> + goto leave;
>>>> + }
>>>> +
>>>> /* we're a cluster, and nlink can change on disk from
>>>> * underneath us... */
>>>> orphan_fe = (struct ocfs2_dinode *) orphan_dir_bh->b_data;
>>>> @@ -2026,22 +2041,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
>>>> orphan_dir_bh, lookup);
>>>> if (status < 0) {
>>>> mlog_errno(status);
>>>> - goto leave;
>>>> - }
>>>> -
>>>> - /*
>>>> - * We're going to journal the change of i_flags and i_orphaned_slot.
>>>> - * It's safe anyway, though some callers may duplicate the journaling.
>>>> - * Journaling within the func just make the logic look more
>>>> - * straightforward.
>>>> - */
>>>> - status = ocfs2_journal_access_di(handle,
>>>> - INODE_CACHE(inode),
>>>> - fe_bh,
>>>> - OCFS2_JOURNAL_ACCESS_WRITE);
>>>> - if (status < 0) {
>>>> - mlog_errno(status);
>>>> - goto leave;
>>>> + goto rollback;
>>>> }
>>>>
>>>> fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL);
>>>> @@ -2057,6 +2057,12 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb,
>>>> trace_ocfs2_orphan_add_end((unsigned long long)OCFS2_I(inode)->ip_blkno,
>>>> osb->slot_num);
>>>>
>>>> +rollback:
>>>> + if (status < 0 && S_ISDIR(inode->i_mode)) {
>>>
>>> ^^^^ <-- But that might not be a directory, right? :-P
>>> How about the following?
>>>
>>> if (status < 0) {
>>> if (S_ISDIR(inode->i_mode))
>>> ocfs2_add_links_count(orphan_fe, -1);
>>> set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
>>> }
>>>
>>
>> Of course, that may be not a directory.
>> But if it is not a directory, there is no change for orphan_dir_inode->i_nlink.
>> So, in my opinion, it is not necessary to change orphan_dir_inode->i_nlink.
>
>
> if (S_ISDIR(inode->i_mode))
> ocfs2_add_links_count(orphan_fe, 1);
> set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
> ^^^^^^^^^
> So we set nlink against orphan_dir_inode no matter IS_DIR(inode->i_mode) or not.
> ocfs2_journal_dirty(handle, orphan_dir_bh);
> Any then orphan_dir buffer got dirty.
>
> if (status < 0 && S_ISDIR(inode->i_mode)) {
> In this way, AFAICS the trans rollback can be triggered only if
> the target inode is a dir.
> }
>
> As a result, should we rollback in case of:
> if (status < 0)
> set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
>
> Thanks,
> -Jeff
>
>>
>>>> + ocfs2_add_links_count(orphan_fe, -1);
>>>> + set_nlink(orphan_dir_inode, ocfs2_read_links_count(orphan_fe));
>>>> + }
>>>> +
>>>> leave:
>>>> brelse(orphan_dir_bh);
>>>>
>>>> -- 1.7.9.7
>>>
>>>
>>> Thanks,
>>> -Jeff
>>>
>>> .
>>>
>>
>
>
>
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-06-20 6:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-19 8:57 [Ocfs2-devel] [PATCH] ocfs2: need rollback when journal_access failed in ocfs2_orphan_add() Younger Liu
2013-06-19 10:53 ` Jeff Liu
2013-06-20 1:25 ` Younger Liu
2013-06-20 1:42 ` Li Zefan
2013-06-20 5:52 ` Jeff Liu
2013-06-20 6:33 ` Younger 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.