diff mbox

ocfs2: fix BUG due to uncleaned localalloc during mount

Message ID 5654684A.1070305@huawei.com
State New, archived
Headers show

Commit Message

Joseph Qi Nov. 24, 2015, 1:38 p.m. UTC
Tariq has reported a BUG before and posted a fix at:
https://oss.oracle.com/pipermail/ocfs2-devel/2015-April/010696.html

This is because during umount, localalloc shutdown relies on journal
shutdown. But during journal shutdown, it just stops commit thread
without checking its result. So it may happen that localalloc shutdown
uncleaned during I/O error and after that, journal then has been marked
clean if I/O restores.
Then during mount, localalloc won't be recovered because of clean
journal and then trigger BUG when claiming clusters from localalloc.

In Tariq's fix, we have to run fsck offline and a separate fix to fsck
is needed because it currently does not support clearing out localalloc
inode. And my way to fix this issue is checking localalloc before
actually loading it during mount. And this is somewhat online.

Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
---
 fs/ocfs2/localalloc.c | 19 ++++++++++++-------
 fs/ocfs2/localalloc.h |  2 +-
 fs/ocfs2/super.c      | 17 ++++++++++++++---
 3 files changed, 27 insertions(+), 11 deletions(-)

Comments

Junxiao Bi Dec. 1, 2015, 8:02 a.m. UTC | #1
Hi Joseph,

On 11/24/2015 09:38 PM, Joseph Qi wrote:
> Tariq has reported a BUG before and posted a fix at:
> https://oss.oracle.com/pipermail/ocfs2-devel/2015-April/010696.html
> 
> This is because during umount, localalloc shutdown relies on journal
> shutdown. But during journal shutdown, it just stops commit thread
> without checking its result. So it may happen that localalloc shutdown
> uncleaned during I/O error and after that, journal then has been marked
> clean if I/O restores.
The above is a storage issue. In this condition, io error can even
happen to journal commit, some transactions may have wrong data. Let fs
go without a fsck may cause corruption.
I am thinking whether we can fail the mount and mark the journal dirty
again. Then we can do fsck to it withoug a fsck patch.

Thanks,
Junxiao.

> Then during mount, localalloc won't be recovered because of clean
> journal and then trigger BUG when claiming clusters from localalloc.
> 
> In Tariq's fix, we have to run fsck offline and a separate fix to fsck
> is needed because it currently does not support clearing out localalloc
> inode. And my way to fix this issue is checking localalloc before
> actually loading it during mount. And this is somewhat online.
> 
> Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
> ---
>  fs/ocfs2/localalloc.c | 19 ++++++++++++-------
>  fs/ocfs2/localalloc.h |  2 +-
>  fs/ocfs2/super.c      | 17 ++++++++++++++---
>  3 files changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ocfs2/localalloc.c b/fs/ocfs2/localalloc.c
> index 0a4457f..ceebaef 100644
> --- a/fs/ocfs2/localalloc.c
> +++ b/fs/ocfs2/localalloc.c
> @@ -281,7 +281,7 @@ bail:
>  	return ret;
>  }
> 
> -int ocfs2_load_local_alloc(struct ocfs2_super *osb)
> +int ocfs2_load_local_alloc(struct ocfs2_super *osb, int check, int *recovery)
>  {
>  	int status = 0;
>  	struct ocfs2_dinode *alloc = NULL;
> @@ -345,21 +345,26 @@ int ocfs2_load_local_alloc(struct ocfs2_super *osb)
>  	if (num_used
>  	    || alloc->id1.bitmap1.i_used
>  	    || alloc->id1.bitmap1.i_total
> -	    || la->la_bm_off)
> +	    || la->la_bm_off) {
>  		mlog(ML_ERROR, "Local alloc hasn't been recovered!\n"
>  		     "found = %u, set = %u, taken = %u, off = %u\n",
>  		     num_used, le32_to_cpu(alloc->id1.bitmap1.i_used),
>  		     le32_to_cpu(alloc->id1.bitmap1.i_total),
>  		     OCFS2_LOCAL_ALLOC(alloc)->la_bm_off);
> +		status = -EINVAL;
> +		*recovery = 1;
> +		goto bail;
> +	}
> 
> -	osb->local_alloc_bh = alloc_bh;
> -	osb->local_alloc_state = OCFS2_LA_ENABLED;
> +	if (!check) {
> +		osb->local_alloc_bh = alloc_bh;
> +		osb->local_alloc_state = OCFS2_LA_ENABLED;
> +	}
> 
>  bail:
> -	if (status < 0)
> +	if (status < 0 || check)
>  		brelse(alloc_bh);
> -	if (inode)
> -		iput(inode);
> +	iput(inode);
> 
>  	trace_ocfs2_load_local_alloc(osb->local_alloc_bits);
> 
> diff --git a/fs/ocfs2/localalloc.h b/fs/ocfs2/localalloc.h
> index 44a7d1f..a913841 100644
> --- a/fs/ocfs2/localalloc.h
> +++ b/fs/ocfs2/localalloc.h
> @@ -26,7 +26,7 @@
>  #ifndef OCFS2_LOCALALLOC_H
>  #define OCFS2_LOCALALLOC_H
> 
> -int ocfs2_load_local_alloc(struct ocfs2_super *osb);
> +int ocfs2_load_local_alloc(struct ocfs2_super *osb, int check, int *recovery);
> 
>  void ocfs2_shutdown_local_alloc(struct ocfs2_super *osb);
> 
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index 2de4c8a..4004b29 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -2428,6 +2428,7 @@ static int ocfs2_check_volume(struct ocfs2_super *osb)
>  	int status;
>  	int dirty;
>  	int local;
> +	int la_dirty = 0, recovery = 0;
>  	struct ocfs2_dinode *local_alloc = NULL; /* only used if we
>  						  * recover
>  						  * ourselves. */
> @@ -2449,6 +2450,16 @@ static int ocfs2_check_volume(struct ocfs2_super *osb)
>  	 * recover anything. Otherwise, journal_load will do that
>  	 * dirty work for us :) */
>  	if (!dirty) {
> +		/* It may happen that local alloc is unclean shutdown, but
> +		 * journal has been marked clean, so check it here and do
> +		 * recovery if needed */
> +		status = ocfs2_load_local_alloc(osb, 1, &recovery);
> +		if (recovery) {
> +			printk(KERN_NOTICE "ocfs2: local alloc needs recovery "
> +					"on device (%s).\n", osb->dev_str);
> +			la_dirty = 1;
> +		}
> +
>  		status = ocfs2_journal_wipe(osb->journal, 0);
>  		if (status < 0) {
>  			mlog_errno(status);
> @@ -2477,7 +2488,7 @@ static int ocfs2_check_volume(struct ocfs2_super *osb)
>  				JBD2_FEATURE_COMPAT_CHECKSUM, 0,
>  				JBD2_FEATURE_INCOMPAT_ASYNC_COMMIT);
> 
> -	if (dirty) {
> +	if (dirty || la_dirty) {
>  		/* recover my local alloc if we didn't unmount cleanly. */
>  		status = ocfs2_begin_local_alloc_recovery(osb,
>  							  osb->slot_num,
> @@ -2490,13 +2501,13 @@ static int ocfs2_check_volume(struct ocfs2_super *osb)
>  		 * ourselves as mounted. */
>  	}
> 
> -	status = ocfs2_load_local_alloc(osb);
> +	status = ocfs2_load_local_alloc(osb, 0, &recovery);
>  	if (status < 0) {
>  		mlog_errno(status);
>  		goto finally;
>  	}
> 
> -	if (dirty) {
> +	if (dirty || la_dirty) {
>  		/* Recovery will be completed after we've mounted the
>  		 * rest of the volume. */
>  		osb->dirty = 1;
>
Andrew Morton Dec. 2, 2015, 12:44 a.m. UTC | #2
On Tue, 1 Dec 2015 16:02:55 +0800 Junxiao Bi <junxiao.bi@oracle.com> wrote:

> On 11/24/2015 09:38 PM, Joseph Qi wrote:
> > Tariq has reported a BUG before and posted a fix at:
> > https://oss.oracle.com/pipermail/ocfs2-devel/2015-April/010696.html
> > 
> > This is because during umount, localalloc shutdown relies on journal
> > shutdown. But during journal shutdown, it just stops commit thread
> > without checking its result. So it may happen that localalloc shutdown
> > uncleaned during I/O error and after that, journal then has been marked
> > clean if I/O restores.
> The above is a storage issue. In this condition, io error can even
> happen to journal commit, some transactions may have wrong data. Let fs
> go without a fsck may cause corruption.
> I am thinking whether we can fail the mount and mark the journal dirty
> again. Then we can do fsck to it withoug a fsck patch.

hm, was that an ack, a nack or a quack?
Junxiao Bi Dec. 2, 2015, 1:44 a.m. UTC | #3
On 12/02/2015 08:44 AM, Andrew Morton wrote:
> On Tue, 1 Dec 2015 16:02:55 +0800 Junxiao Bi <junxiao.bi@oracle.com> wrote:
> 
>> On 11/24/2015 09:38 PM, Joseph Qi wrote:
>>> Tariq has reported a BUG before and posted a fix at:
>>> https://oss.oracle.com/pipermail/ocfs2-devel/2015-April/010696.html
>>>
>>> This is because during umount, localalloc shutdown relies on journal
>>> shutdown. But during journal shutdown, it just stops commit thread
>>> without checking its result. So it may happen that localalloc shutdown
>>> uncleaned during I/O error and after that, journal then has been marked
>>> clean if I/O restores.
>> The above is a storage issue. In this condition, io error can even
>> happen to journal commit, some transactions may have wrong data. Let fs
>> go without a fsck may cause corruption.
>> I am thinking whether we can fail the mount and mark the journal dirty
>> again. Then we can do fsck to it withoug a fsck patch.
> 
> hm, was that an ack, a nack or a quack?
nack, i think we need another way to fix this issue.

Thanks,
Junxiao.
>
Xue jiufei Dec. 2, 2015, 6:52 a.m. UTC | #4
Hi Junxiao,
On 2015/12/1 16:02, Junxiao Bi wrote:
> Hi Joseph,
> 
> On 11/24/2015 09:38 PM, Joseph Qi wrote:
>> Tariq has reported a BUG before and posted a fix at:
>> https://oss.oracle.com/pipermail/ocfs2-devel/2015-April/010696.html
>>
>> This is because during umount, localalloc shutdown relies on journal
>> shutdown. But during journal shutdown, it just stops commit thread
>> without checking its result. So it may happen that localalloc shutdown
>> uncleaned during I/O error and after that, journal then has been marked
>> clean if I/O restores.
> The above is a storage issue. In this condition, io error can even
> happen to journal commit, some transactions may have wrong data. Let fs
> go without a fsck may cause corruption.
> I am thinking whether we can fail the mount and mark the journal dirty
> again. Then we can do fsck to it withoug a fsck patch.
> 
Can you explain which situation would cause file system corruption. I think
if IO error happens to journal commit and commit block have not reach the disk,
the whole transactions is skipped while recovering the journal. So file system
is still consistent.
Thanks,
Xuejiufei

> Thanks,
> Junxiao.
> 
>> Then during mount, localalloc won't be recovered because of clean
>> journal and then trigger BUG when claiming clusters from localalloc.
>>
>> In Tariq's fix, we have to run fsck offline and a separate fix to fsck
>> is needed because it currently does not support clearing out localalloc
>> inode. And my way to fix this issue is checking localalloc before
>> actually loading it during mount. And this is somewhat online.
>>
>> Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
>> ---
>>  fs/ocfs2/localalloc.c | 19 ++++++++++++-------
>>  fs/ocfs2/localalloc.h |  2 +-
>>  fs/ocfs2/super.c      | 17 ++++++++++++++---
>>  3 files changed, 27 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/ocfs2/localalloc.c b/fs/ocfs2/localalloc.c
>> index 0a4457f..ceebaef 100644
>> --- a/fs/ocfs2/localalloc.c
>> +++ b/fs/ocfs2/localalloc.c
>> @@ -281,7 +281,7 @@ bail:
>>  	return ret;
>>  }
>>
>> -int ocfs2_load_local_alloc(struct ocfs2_super *osb)
>> +int ocfs2_load_local_alloc(struct ocfs2_super *osb, int check, int *recovery)
>>  {
>>  	int status = 0;
>>  	struct ocfs2_dinode *alloc = NULL;
>> @@ -345,21 +345,26 @@ int ocfs2_load_local_alloc(struct ocfs2_super *osb)
>>  	if (num_used
>>  	    || alloc->id1.bitmap1.i_used
>>  	    || alloc->id1.bitmap1.i_total
>> -	    || la->la_bm_off)
>> +	    || la->la_bm_off) {
>>  		mlog(ML_ERROR, "Local alloc hasn't been recovered!\n"
>>  		     "found = %u, set = %u, taken = %u, off = %u\n",
>>  		     num_used, le32_to_cpu(alloc->id1.bitmap1.i_used),
>>  		     le32_to_cpu(alloc->id1.bitmap1.i_total),
>>  		     OCFS2_LOCAL_ALLOC(alloc)->la_bm_off);
>> +		status = -EINVAL;
>> +		*recovery = 1;
>> +		goto bail;
>> +	}
>>
>> -	osb->local_alloc_bh = alloc_bh;
>> -	osb->local_alloc_state = OCFS2_LA_ENABLED;
>> +	if (!check) {
>> +		osb->local_alloc_bh = alloc_bh;
>> +		osb->local_alloc_state = OCFS2_LA_ENABLED;
>> +	}
>>
>>  bail:
>> -	if (status < 0)
>> +	if (status < 0 || check)
>>  		brelse(alloc_bh);
>> -	if (inode)
>> -		iput(inode);
>> +	iput(inode);
>>
>>  	trace_ocfs2_load_local_alloc(osb->local_alloc_bits);
>>
>> diff --git a/fs/ocfs2/localalloc.h b/fs/ocfs2/localalloc.h
>> index 44a7d1f..a913841 100644
>> --- a/fs/ocfs2/localalloc.h
>> +++ b/fs/ocfs2/localalloc.h
>> @@ -26,7 +26,7 @@
>>  #ifndef OCFS2_LOCALALLOC_H
>>  #define OCFS2_LOCALALLOC_H
>>
>> -int ocfs2_load_local_alloc(struct ocfs2_super *osb);
>> +int ocfs2_load_local_alloc(struct ocfs2_super *osb, int check, int *recovery);
>>
>>  void ocfs2_shutdown_local_alloc(struct ocfs2_super *osb);
>>
>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
>> index 2de4c8a..4004b29 100644
>> --- a/fs/ocfs2/super.c
>> +++ b/fs/ocfs2/super.c
>> @@ -2428,6 +2428,7 @@ static int ocfs2_check_volume(struct ocfs2_super *osb)
>>  	int status;
>>  	int dirty;
>>  	int local;
>> +	int la_dirty = 0, recovery = 0;
>>  	struct ocfs2_dinode *local_alloc = NULL; /* only used if we
>>  						  * recover
>>  						  * ourselves. */
>> @@ -2449,6 +2450,16 @@ static int ocfs2_check_volume(struct ocfs2_super *osb)
>>  	 * recover anything. Otherwise, journal_load will do that
>>  	 * dirty work for us :) */
>>  	if (!dirty) {
>> +		/* It may happen that local alloc is unclean shutdown, but
>> +		 * journal has been marked clean, so check it here and do
>> +		 * recovery if needed */
>> +		status = ocfs2_load_local_alloc(osb, 1, &recovery);
>> +		if (recovery) {
>> +			printk(KERN_NOTICE "ocfs2: local alloc needs recovery "
>> +					"on device (%s).\n", osb->dev_str);
>> +			la_dirty = 1;
>> +		}
>> +
>>  		status = ocfs2_journal_wipe(osb->journal, 0);
>>  		if (status < 0) {
>>  			mlog_errno(status);
>> @@ -2477,7 +2488,7 @@ static int ocfs2_check_volume(struct ocfs2_super *osb)
>>  				JBD2_FEATURE_COMPAT_CHECKSUM, 0,
>>  				JBD2_FEATURE_INCOMPAT_ASYNC_COMMIT);
>>
>> -	if (dirty) {
>> +	if (dirty || la_dirty) {
>>  		/* recover my local alloc if we didn't unmount cleanly. */
>>  		status = ocfs2_begin_local_alloc_recovery(osb,
>>  							  osb->slot_num,
>> @@ -2490,13 +2501,13 @@ static int ocfs2_check_volume(struct ocfs2_super *osb)
>>  		 * ourselves as mounted. */
>>  	}
>>
>> -	status = ocfs2_load_local_alloc(osb);
>> +	status = ocfs2_load_local_alloc(osb, 0, &recovery);
>>  	if (status < 0) {
>>  		mlog_errno(status);
>>  		goto finally;
>>  	}
>>
>> -	if (dirty) {
>> +	if (dirty || la_dirty) {
>>  		/* Recovery will be completed after we've mounted the
>>  		 * rest of the volume. */
>>  		osb->dirty = 1;
>>
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
> 
> .
>
Junxiao Bi Dec. 2, 2015, 7:07 a.m. UTC | #5
On 12/02/2015 02:52 PM, Xue jiufei wrote:
> Hi Junxiao,
> On 2015/12/1 16:02, Junxiao Bi wrote:
>> Hi Joseph,
>>
>> On 11/24/2015 09:38 PM, Joseph Qi wrote:
>>> Tariq has reported a BUG before and posted a fix at:
>>> https://oss.oracle.com/pipermail/ocfs2-devel/2015-April/010696.html
>>>
>>> This is because during umount, localalloc shutdown relies on journal
>>> shutdown. But during journal shutdown, it just stops commit thread
>>> without checking its result. So it may happen that localalloc shutdown
>>> uncleaned during I/O error and after that, journal then has been marked
>>> clean if I/O restores.
>> The above is a storage issue. In this condition, io error can even
>> happen to journal commit, some transactions may have wrong data. Let fs
>> go without a fsck may cause corruption.
>> I am thinking whether we can fail the mount and mark the journal dirty
>> again. Then we can do fsck to it withoug a fsck patch.
>>
> Can you explain which situation would cause file system corruption. I think
> if IO error happens to journal commit and commit block have not reach the disk,
> the whole transactions is skipped while recovering the journal. So file system
> is still consistent.
At least local alloc inconsistent as this storage error, right? I think
it can't be sure whether this caused some other metadata inconsistent,
so a full fsck deserved.

Thanks,
Junxiao.

> Thanks,
> Xuejiufei
> 
>> Thanks,
>> Junxiao.
>>
>>> Then during mount, localalloc won't be recovered because of clean
>>> journal and then trigger BUG when claiming clusters from localalloc.
>>>
>>> In Tariq's fix, we have to run fsck offline and a separate fix to fsck
>>> is needed because it currently does not support clearing out localalloc
>>> inode. And my way to fix this issue is checking localalloc before
>>> actually loading it during mount. And this is somewhat online.
>>>
>>> Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
>>> ---
>>>  fs/ocfs2/localalloc.c | 19 ++++++++++++-------
>>>  fs/ocfs2/localalloc.h |  2 +-
>>>  fs/ocfs2/super.c      | 17 ++++++++++++++---
>>>  3 files changed, 27 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/localalloc.c b/fs/ocfs2/localalloc.c
>>> index 0a4457f..ceebaef 100644
>>> --- a/fs/ocfs2/localalloc.c
>>> +++ b/fs/ocfs2/localalloc.c
>>> @@ -281,7 +281,7 @@ bail:
>>>  	return ret;
>>>  }
>>>
>>> -int ocfs2_load_local_alloc(struct ocfs2_super *osb)
>>> +int ocfs2_load_local_alloc(struct ocfs2_super *osb, int check, int *recovery)
>>>  {
>>>  	int status = 0;
>>>  	struct ocfs2_dinode *alloc = NULL;
>>> @@ -345,21 +345,26 @@ int ocfs2_load_local_alloc(struct ocfs2_super *osb)
>>>  	if (num_used
>>>  	    || alloc->id1.bitmap1.i_used
>>>  	    || alloc->id1.bitmap1.i_total
>>> -	    || la->la_bm_off)
>>> +	    || la->la_bm_off) {
>>>  		mlog(ML_ERROR, "Local alloc hasn't been recovered!\n"
>>>  		     "found = %u, set = %u, taken = %u, off = %u\n",
>>>  		     num_used, le32_to_cpu(alloc->id1.bitmap1.i_used),
>>>  		     le32_to_cpu(alloc->id1.bitmap1.i_total),
>>>  		     OCFS2_LOCAL_ALLOC(alloc)->la_bm_off);
>>> +		status = -EINVAL;
>>> +		*recovery = 1;
>>> +		goto bail;
>>> +	}
>>>
>>> -	osb->local_alloc_bh = alloc_bh;
>>> -	osb->local_alloc_state = OCFS2_LA_ENABLED;
>>> +	if (!check) {
>>> +		osb->local_alloc_bh = alloc_bh;
>>> +		osb->local_alloc_state = OCFS2_LA_ENABLED;
>>> +	}
>>>
>>>  bail:
>>> -	if (status < 0)
>>> +	if (status < 0 || check)
>>>  		brelse(alloc_bh);
>>> -	if (inode)
>>> -		iput(inode);
>>> +	iput(inode);
>>>
>>>  	trace_ocfs2_load_local_alloc(osb->local_alloc_bits);
>>>
>>> diff --git a/fs/ocfs2/localalloc.h b/fs/ocfs2/localalloc.h
>>> index 44a7d1f..a913841 100644
>>> --- a/fs/ocfs2/localalloc.h
>>> +++ b/fs/ocfs2/localalloc.h
>>> @@ -26,7 +26,7 @@
>>>  #ifndef OCFS2_LOCALALLOC_H
>>>  #define OCFS2_LOCALALLOC_H
>>>
>>> -int ocfs2_load_local_alloc(struct ocfs2_super *osb);
>>> +int ocfs2_load_local_alloc(struct ocfs2_super *osb, int check, int *recovery);
>>>
>>>  void ocfs2_shutdown_local_alloc(struct ocfs2_super *osb);
>>>
>>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
>>> index 2de4c8a..4004b29 100644
>>> --- a/fs/ocfs2/super.c
>>> +++ b/fs/ocfs2/super.c
>>> @@ -2428,6 +2428,7 @@ static int ocfs2_check_volume(struct ocfs2_super *osb)
>>>  	int status;
>>>  	int dirty;
>>>  	int local;
>>> +	int la_dirty = 0, recovery = 0;
>>>  	struct ocfs2_dinode *local_alloc = NULL; /* only used if we
>>>  						  * recover
>>>  						  * ourselves. */
>>> @@ -2449,6 +2450,16 @@ static int ocfs2_check_volume(struct ocfs2_super *osb)
>>>  	 * recover anything. Otherwise, journal_load will do that
>>>  	 * dirty work for us :) */
>>>  	if (!dirty) {
>>> +		/* It may happen that local alloc is unclean shutdown, but
>>> +		 * journal has been marked clean, so check it here and do
>>> +		 * recovery if needed */
>>> +		status = ocfs2_load_local_alloc(osb, 1, &recovery);
>>> +		if (recovery) {
>>> +			printk(KERN_NOTICE "ocfs2: local alloc needs recovery "
>>> +					"on device (%s).\n", osb->dev_str);
>>> +			la_dirty = 1;
>>> +		}
>>> +
>>>  		status = ocfs2_journal_wipe(osb->journal, 0);
>>>  		if (status < 0) {
>>>  			mlog_errno(status);
>>> @@ -2477,7 +2488,7 @@ static int ocfs2_check_volume(struct ocfs2_super *osb)
>>>  				JBD2_FEATURE_COMPAT_CHECKSUM, 0,
>>>  				JBD2_FEATURE_INCOMPAT_ASYNC_COMMIT);
>>>
>>> -	if (dirty) {
>>> +	if (dirty || la_dirty) {
>>>  		/* recover my local alloc if we didn't unmount cleanly. */
>>>  		status = ocfs2_begin_local_alloc_recovery(osb,
>>>  							  osb->slot_num,
>>> @@ -2490,13 +2501,13 @@ static int ocfs2_check_volume(struct ocfs2_super *osb)
>>>  		 * ourselves as mounted. */
>>>  	}
>>>
>>> -	status = ocfs2_load_local_alloc(osb);
>>> +	status = ocfs2_load_local_alloc(osb, 0, &recovery);
>>>  	if (status < 0) {
>>>  		mlog_errno(status);
>>>  		goto finally;
>>>  	}
>>>
>>> -	if (dirty) {
>>> +	if (dirty || la_dirty) {
>>>  		/* Recovery will be completed after we've mounted the
>>>  		 * rest of the volume. */
>>>  		osb->dirty = 1;
>>>
>>
>>
>> _______________________________________________
>> Ocfs2-devel mailing list
>> Ocfs2-devel@oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>>
>> .
>>
> 
>
diff mbox

Patch

diff --git a/fs/ocfs2/localalloc.c b/fs/ocfs2/localalloc.c
index 0a4457f..ceebaef 100644
--- a/fs/ocfs2/localalloc.c
+++ b/fs/ocfs2/localalloc.c
@@ -281,7 +281,7 @@  bail:
 	return ret;
 }

-int ocfs2_load_local_alloc(struct ocfs2_super *osb)
+int ocfs2_load_local_alloc(struct ocfs2_super *osb, int check, int *recovery)
 {
 	int status = 0;
 	struct ocfs2_dinode *alloc = NULL;
@@ -345,21 +345,26 @@  int ocfs2_load_local_alloc(struct ocfs2_super *osb)
 	if (num_used
 	    || alloc->id1.bitmap1.i_used
 	    || alloc->id1.bitmap1.i_total
-	    || la->la_bm_off)
+	    || la->la_bm_off) {
 		mlog(ML_ERROR, "Local alloc hasn't been recovered!\n"
 		     "found = %u, set = %u, taken = %u, off = %u\n",
 		     num_used, le32_to_cpu(alloc->id1.bitmap1.i_used),
 		     le32_to_cpu(alloc->id1.bitmap1.i_total),
 		     OCFS2_LOCAL_ALLOC(alloc)->la_bm_off);
+		status = -EINVAL;
+		*recovery = 1;
+		goto bail;
+	}

-	osb->local_alloc_bh = alloc_bh;
-	osb->local_alloc_state = OCFS2_LA_ENABLED;
+	if (!check) {
+		osb->local_alloc_bh = alloc_bh;
+		osb->local_alloc_state = OCFS2_LA_ENABLED;
+	}

 bail:
-	if (status < 0)
+	if (status < 0 || check)
 		brelse(alloc_bh);
-	if (inode)
-		iput(inode);
+	iput(inode);

 	trace_ocfs2_load_local_alloc(osb->local_alloc_bits);

diff --git a/fs/ocfs2/localalloc.h b/fs/ocfs2/localalloc.h
index 44a7d1f..a913841 100644
--- a/fs/ocfs2/localalloc.h
+++ b/fs/ocfs2/localalloc.h
@@ -26,7 +26,7 @@ 
 #ifndef OCFS2_LOCALALLOC_H
 #define OCFS2_LOCALALLOC_H

-int ocfs2_load_local_alloc(struct ocfs2_super *osb);
+int ocfs2_load_local_alloc(struct ocfs2_super *osb, int check, int *recovery);

 void ocfs2_shutdown_local_alloc(struct ocfs2_super *osb);

diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index 2de4c8a..4004b29 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -2428,6 +2428,7 @@  static int ocfs2_check_volume(struct ocfs2_super *osb)
 	int status;
 	int dirty;
 	int local;
+	int la_dirty = 0, recovery = 0;
 	struct ocfs2_dinode *local_alloc = NULL; /* only used if we
 						  * recover
 						  * ourselves. */
@@ -2449,6 +2450,16 @@  static int ocfs2_check_volume(struct ocfs2_super *osb)
 	 * recover anything. Otherwise, journal_load will do that
 	 * dirty work for us :) */
 	if (!dirty) {
+		/* It may happen that local alloc is unclean shutdown, but
+		 * journal has been marked clean, so check it here and do
+		 * recovery if needed */
+		status = ocfs2_load_local_alloc(osb, 1, &recovery);
+		if (recovery) {
+			printk(KERN_NOTICE "ocfs2: local alloc needs recovery "
+					"on device (%s).\n", osb->dev_str);
+			la_dirty = 1;
+		}
+
 		status = ocfs2_journal_wipe(osb->journal, 0);
 		if (status < 0) {
 			mlog_errno(status);
@@ -2477,7 +2488,7 @@  static int ocfs2_check_volume(struct ocfs2_super *osb)
 				JBD2_FEATURE_COMPAT_CHECKSUM, 0,
 				JBD2_FEATURE_INCOMPAT_ASYNC_COMMIT);

-	if (dirty) {
+	if (dirty || la_dirty) {
 		/* recover my local alloc if we didn't unmount cleanly. */
 		status = ocfs2_begin_local_alloc_recovery(osb,
 							  osb->slot_num,
@@ -2490,13 +2501,13 @@  static int ocfs2_check_volume(struct ocfs2_super *osb)
 		 * ourselves as mounted. */
 	}

-	status = ocfs2_load_local_alloc(osb);
+	status = ocfs2_load_local_alloc(osb, 0, &recovery);
 	if (status < 0) {
 		mlog_errno(status);
 		goto finally;
 	}

-	if (dirty) {
+	if (dirty || la_dirty) {
 		/* Recovery will be completed after we've mounted the
 		 * rest of the volume. */
 		osb->dirty = 1;