| Message ID | 20181116085847.10066-1-junxiao.bi@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ocfs2: clear journal dirty flag after shutdown journal | expand |
Hi Junxiao, I'm very interested in this bug, and it seems causing read-only if involving metadata. Could you help show how to reproduce this problem? Thanks, Jun On 2018/11/16 16:58, Junxiao Bi wrote: > Dirty flag of the journal should be cleared at the last stage of umount, > if do it before jbd2_journal_destroy(), then some metadata in uncommitted > transaction could be lost due to io error, but as dirty flag of journal > was already cleared, we can't find that until run a full fsck. This may > cause system panic or other corruption. > > Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com> > --- > fs/ocfs2/journal.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c > index b63c97f4318e..25d678c92fbb 100644 > --- a/fs/ocfs2/journal.c > +++ b/fs/ocfs2/journal.c > @@ -1017,6 +1017,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb) > mlog_errno(status); > } > > + /* Shutdown the kernel journal system */ > + jbd2_journal_destroy(journal->j_journal); > + journal->j_journal = NULL; > + > if (status == 0) { > /* > * Do not toggle if flush was unsuccessful otherwise > @@ -1027,10 +1031,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb) > mlog_errno(status); > } > > - /* Shutdown the kernel journal system */ > - jbd2_journal_destroy(journal->j_journal); > - journal->j_journal = NULL; > - > OCFS2_I(inode)->ip_open_count--; > > /* unlock our journal */ >
Hi Jun,
ocfs2_journal_shutdown() was only invoked during umount, jbd2 will force all uncommitted metadata to disk, there is no set fs readonly for io error in that case.
To reproduce, you need cause storage return io error between ocfs2_journal_toggle_dirty() and jbd2_journal_destroy().
1020 if (status == 0) {
1021 /*
1022 * Do not toggle if flush was unsuccessful otherwise
1023 * will leave dirty metadata in a "clean" journal
1024 */
1025 status = ocfs2_journal_toggle_dirty(osb, 0, 0);
1026 if (status < 0)
1027 mlog_errno(status);
1028 }
1029
1030 /* Shutdown the kernel journal system */
1031 jbd2_journal_destroy(journal->j_journal);
Thanks,
Junxiao.
On 11/19/18 9:32 AM, piaojun wrote:
> Hi Junxiao,
>
> I'm very interested in this bug, and it seems causing read-only if
> involving metadata. Could you help show how to reproduce this problem?
>
> Thanks,
> Jun
>
> On 2018/11/16 16:58, Junxiao Bi wrote:
>> Dirty flag of the journal should be cleared at the last stage of umount,
>> if do it before jbd2_journal_destroy(), then some metadata in uncommitted
>> transaction could be lost due to io error, but as dirty flag of journal
>> was already cleared, we can't find that until run a full fsck. This may
>> cause system panic or other corruption.
>>
>> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
>> ---
>> fs/ocfs2/journal.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
>> index b63c97f4318e..25d678c92fbb 100644
>> --- a/fs/ocfs2/journal.c
>> +++ b/fs/ocfs2/journal.c
>> @@ -1017,6 +1017,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
>> mlog_errno(status);
>> }
>>
>> + /* Shutdown the kernel journal system */
>> + jbd2_journal_destroy(journal->j_journal);
>> + journal->j_journal = NULL;
>> +
>> if (status == 0) {
>> /*
>> * Do not toggle if flush was unsuccessful otherwise
>> @@ -1027,10 +1031,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
>> mlog_errno(status);
>> }
>>
>> - /* Shutdown the kernel journal system */
>> - jbd2_journal_destroy(journal->j_journal);
>> - journal->j_journal = NULL;
>> -
>> OCFS2_I(inode)->ip_open_count--;
>>
>> /* unlock our journal */
>>
On 2018/11/16 16:58, Junxiao Bi wrote: > Dirty flag of the journal should be cleared at the last stage of umount, > if do it before jbd2_journal_destroy(), then some metadata in uncommitted > transaction could be lost due to io error, but as dirty flag of journal > was already cleared, we can't find that until run a full fsck. This may > cause system panic or other corruption. > > Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com> > --- > fs/ocfs2/journal.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c > index b63c97f4318e..25d678c92fbb 100644 > --- a/fs/ocfs2/journal.c > +++ b/fs/ocfs2/journal.c > @@ -1017,6 +1017,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb) > mlog_errno(status); > } > > + /* Shutdown the kernel journal system */ > + jbd2_journal_destroy(journal->j_journal); > + journal->j_journal = NULL; > + Hi Junxiao, I feel this adjustment doesn't make any sense. When jbd2_journal_destroy() is done, it still call ocfs2_journal_toggle_dirty() to clean dirty flag. Am I wrong or understand error ? Thanks. Yiwen. > if (status == 0) { > /* > * Do not toggle if flush was unsuccessful otherwise > @@ -1027,10 +1031,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb) > mlog_errno(status); > } > > - /* Shutdown the kernel journal system */ > - jbd2_journal_destroy(journal->j_journal); > - journal->j_journal = NULL; > - > OCFS2_I(inode)->ip_open_count--; > > /* unlock our journal */ >
On 11/19/18 10:28 AM, jiangyiwen wrote: > On 2018/11/16 16:58, Junxiao Bi wrote: >> Dirty flag of the journal should be cleared at the last stage of umount, >> if do it before jbd2_journal_destroy(), then some metadata in uncommitted >> transaction could be lost due to io error, but as dirty flag of journal >> was already cleared, we can't find that until run a full fsck. This may >> cause system panic or other corruption. >> >> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com> >> --- >> fs/ocfs2/journal.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c >> index b63c97f4318e..25d678c92fbb 100644 >> --- a/fs/ocfs2/journal.c >> +++ b/fs/ocfs2/journal.c >> @@ -1017,6 +1017,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb) >> mlog_errno(status); >> } >> >> + /* Shutdown the kernel journal system */ >> + jbd2_journal_destroy(journal->j_journal); >> + journal->j_journal = NULL; >> + > Hi Junxiao, > > I feel this adjustment doesn't make any sense. When jbd2_journal_destroy() is > done, it still call ocfs2_journal_toggle_dirty() to clean dirty flag. Am I > wrong or understand error ? Oh, missed checking the return value of jbd2_journal_destroy(), if failed, should not call ocfs2_journal_toggle_dirty(). Thanks for pointing this. Thanks, Junxiao. > > Thanks. > Yiwen. > >> if (status == 0) { >> /* >> * Do not toggle if flush was unsuccessful otherwise >> @@ -1027,10 +1031,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb) >> mlog_errno(status); >> } >> >> - /* Shutdown the kernel journal system */ >> - jbd2_journal_destroy(journal->j_journal); >> - journal->j_journal = NULL; >> - >> OCFS2_I(inode)->ip_open_count--; >> >> /* unlock our journal */ >> >
diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c index b63c97f4318e..25d678c92fbb 100644 --- a/fs/ocfs2/journal.c +++ b/fs/ocfs2/journal.c @@ -1017,6 +1017,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb) mlog_errno(status); } + /* Shutdown the kernel journal system */ + jbd2_journal_destroy(journal->j_journal); + journal->j_journal = NULL; + if (status == 0) { /* * Do not toggle if flush was unsuccessful otherwise @@ -1027,10 +1031,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb) mlog_errno(status); } - /* Shutdown the kernel journal system */ - jbd2_journal_destroy(journal->j_journal); - journal->j_journal = NULL; - OCFS2_I(inode)->ip_open_count--; /* unlock our journal */
Dirty flag of the journal should be cleared at the last stage of umount, if do it before jbd2_journal_destroy(), then some metadata in uncommitted transaction could be lost due to io error, but as dirty flag of journal was already cleared, we can't find that until run a full fsck. This may cause system panic or other corruption. Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com> --- fs/ocfs2/journal.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)