Patchwork Regression in suspend to ram in 2.6.31-rc kernels

login
register
mail settings
Submitter OGAWA Hirofumi
Date Sept. 5, 2009, 5:22 p.m.
Message ID <87iqfx5mss.fsf@devron.myhome.or.jp>
Download mbox | patch
Permalink /patch/45945/
State New, archived
Headers show

Comments

OGAWA Hirofumi - Sept. 5, 2009, 5:22 p.m.
Zdenek Kabelac <zdenek.kabelac@gmail.com> writes:

> 2009/9/4 OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>> Christoph Hellwig <hch@lst.de> writes:
>>
>>> Note that when you rever this patch on a current kernel you do actually
>>> get different behvaviour than when going back to before this commit.
>>>
>>> In 2.6.30 we called ->write_super in the various sync functions and
>>> then ->sync_fs, in 2.6.31-rc8 you would not call any syncing at all
>>> anymore.  I think this patch might just be a symptom for a situation
>>> where the suspend code causes a sync and the mmc driver can't handle
>>> it anymore.
>
> So - here is the console trace from suspend when I've added
> dump_stack() to the fat_sync_fs()   (and also added debug prints
> around each call in this function -so its obvious the function is
> actually left - but then it freezes later somewhere.)
>
> It's interesting that 3 calls to sync happens.

It seems

    1) sync() (probabry "sync" command)
    2) sync as part of suspend sequence
    3) sync_filesystem() by mmc remove event

I guess the root-cause of the problem would be 3). However, it would not
be easy to fix, at least, we would need to think about what we want to
do for it. So, to workaround it for now, I've made this patch.

Can you try whether this patch fixes this problem?

Thanks.
Zdenek Kabelac - Sept. 5, 2009, 7:53 p.m.
2009/9/5 OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Zdenek Kabelac <zdenek.kabelac@gmail.com> writes:
>
>> 2009/9/4 OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>>> Christoph Hellwig <hch@lst.de> writes:
>>>
>>>> Note that when you rever this patch on a current kernel you do actually
>>>> get different behvaviour than when going back to before this commit.
>>>>
>>>> In 2.6.30 we called ->write_super in the various sync functions and
>>>> then ->sync_fs, in 2.6.31-rc8 you would not call any syncing at all
>>>> anymore.  I think this patch might just be a symptom for a situation
>>>> where the suspend code causes a sync and the mmc driver can't handle
>>>> it anymore.
>>
>> So - here is the console trace from suspend when I've added
>> dump_stack() to the fat_sync_fs()   (and also added debug prints
>> around each call in this function -so its obvious the function is
>> actually left - but then it freezes later somewhere.)
>>
>> It's interesting that 3 calls to sync happens.
>
> It seems
>
>    1) sync() (probabry "sync" command)
>    2) sync as part of suspend sequence
>    3) sync_filesystem() by mmc remove event
>
> I guess the root-cause of the problem would be 3). However, it would not
> be easy to fix, at least, we would need to think about what we want to
> do for it. So, to workaround it for now, I've made this patch.
>
> Can you try whether this patch fixes this problem?
>

So I've tested your patch - it seems to fix the problem in suspend -
the machine sleeps - however after resume the mmc SD card becomes
unusable to the system and appended call trace filled my message log
very quickly:

After reboot the filesystem appeared to be usable again without errors.

Call Trace:
 [<ffffffff81134f6b>] __getblk+0x2cb/0x300
 [<ffffffff813dcb58>] ? _spin_unlock_irqrestore+0x38/0x80
 [<ffffffff81135122>] __breadahead+0x12/0x40
 [<ffffffffa0520cb7>] fat_count_free_clusters+0x307/0x320 [fat]
 [<ffffffff81103a58>] ? check_object+0xd8/0x260
 [<ffffffff8107d83d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffffa0522d55>] fat_statfs+0xf5/0x110 [fat]
 [<ffffffff8110be5c>] vfs_statfs+0x7c/0xa0
 [<ffffffff8110c0b0>] vfs_statfs_native+0x20/0xb0
 [<ffffffff8110c243>] sys_statfs+0x73/0xb0
 [<ffffffff8122ab2b>] ? __up_write+0xcb/0x120
 [<ffffffff8100c18c>] ? sysret_check+0x27/0x62
 [<ffffffff8109eb8b>] ? audit_syscall_entry+0x28b/0x2b0
 [<ffffffff8107d7ed>] ? trace_hardirqs_on_caller+0x15d/0x1a0
 [<ffffffff813dbf1e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
 [<ffffffff8100c15b>] system_call_fastpath+0x16/0x1b
getblk(): invalid block size 512 requested
logical block size: 27499

 [<ffffffff81135122>] __breadahead+0x12/0x40
 [<ffffffffa0520cb7>] fat_count_free_clusters+0x307/0x320 [fat]
 [<ffffffff81103a58>] ? check_object+0xd8/0x260
 [<ffffffff8107d83d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffffa0522d55>] fat_statfs+0xf5/0x110 [fat]
 [<ffffffff8110be5c>] vfs_statfs+0x7c/0xa0
 [<ffffffff8110c0b0>] vfs_statfs_native+0x20/0xb0
 [<ffffffff8110c243>] sys_statfs+0x73/0xb0
 [<ffffffff8122ab2b>] ? __up_write+0xcb/0x120
 [<ffffffff8100c18c>] ? sysret_check+0x27/0x62
 [<ffffffff8109eb8b>] ? audit_syscall_entry+0x28b/0x2b0
 [<ffffffff8107d7ed>] ? trace_hardirqs_on_caller+0x15d/0x1a0
 [<ffffffff813dbf1e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
 [<ffffffff8100c15b>] system_call_fastpath+0x16/0x1b

Zdenek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
OGAWA Hirofumi - Sept. 5, 2009, 10:42 p.m.
Zdenek Kabelac <zdenek.kabelac@gmail.com> writes:

> 2009/9/5 OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>> Zdenek Kabelac <zdenek.kabelac@gmail.com> writes:
>>
>>> 2009/9/4 OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>>>> Christoph Hellwig <hch@lst.de> writes:
>>>>
>>>>> Note that when you rever this patch on a current kernel you do actually
>>>>> get different behvaviour than when going back to before this commit.
>>>>>
>>>>> In 2.6.30 we called ->write_super in the various sync functions and
>>>>> then ->sync_fs, in 2.6.31-rc8 you would not call any syncing at all
>>>>> anymore.  I think this patch might just be a symptom for a situation
>>>>> where the suspend code causes a sync and the mmc driver can't handle
>>>>> it anymore.
>>>
>>> So - here is the console trace from suspend when I've added
>>> dump_stack() to the fat_sync_fs()   (and also added debug prints
>>> around each call in this function -so its obvious the function is
>>> actually left - but then it freezes later somewhere.)
>>>
>>> It's interesting that 3 calls to sync happens.
>>
>> It seems
>>
>>    1) sync() (probabry "sync" command)
>>    2) sync as part of suspend sequence
>>    3) sync_filesystem() by mmc remove event
>>
>> I guess the root-cause of the problem would be 3). However, it would not
>> be easy to fix, at least, we would need to think about what we want to
>> do for it. So, to workaround it for now, I've made this patch.
>>
>> Can you try whether this patch fixes this problem?
>>
>
> So I've tested your patch - it seems to fix the problem in suspend -
> the machine sleeps - however after resume the mmc SD card becomes
> unusable to the system and appended call trace filled my message log
> very quickly:
>
> After reboot the filesystem appeared to be usable again without errors.

Thanks for testing.  "logical block size: 27499" is wrong value
completely. Of course, fatfs is assuming the blocksize is not changed.
(mmc wasn't resumed correctly?)

Well, this problem seems to be completely different problem, and it
seems the problem of suspend or mmc (or block?) stuff, or something.

It would need to be analyzed by those people.

Meanwhile, I'll apply this patch to workaround suspend problem and to
remove unneeded write for next window.

Thanks.

> Call Trace:
>  [<ffffffff81134f6b>] __getblk+0x2cb/0x300
>  [<ffffffff813dcb58>] ? _spin_unlock_irqrestore+0x38/0x80
>  [<ffffffff81135122>] __breadahead+0x12/0x40
>  [<ffffffffa0520cb7>] fat_count_free_clusters+0x307/0x320 [fat]
>  [<ffffffff81103a58>] ? check_object+0xd8/0x260
>  [<ffffffff8107d83d>] ? trace_hardirqs_on+0xd/0x10
>  [<ffffffffa0522d55>] fat_statfs+0xf5/0x110 [fat]
>  [<ffffffff8110be5c>] vfs_statfs+0x7c/0xa0
>  [<ffffffff8110c0b0>] vfs_statfs_native+0x20/0xb0
>  [<ffffffff8110c243>] sys_statfs+0x73/0xb0
>  [<ffffffff8122ab2b>] ? __up_write+0xcb/0x120
>  [<ffffffff8100c18c>] ? sysret_check+0x27/0x62
>  [<ffffffff8109eb8b>] ? audit_syscall_entry+0x28b/0x2b0
>  [<ffffffff8107d7ed>] ? trace_hardirqs_on_caller+0x15d/0x1a0
>  [<ffffffff813dbf1e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
>  [<ffffffff8100c15b>] system_call_fastpath+0x16/0x1b
> getblk(): invalid block size 512 requested
> logical block size: 27499
>
>  [<ffffffff81135122>] __breadahead+0x12/0x40
>  [<ffffffffa0520cb7>] fat_count_free_clusters+0x307/0x320 [fat]
>  [<ffffffff81103a58>] ? check_object+0xd8/0x260
>  [<ffffffff8107d83d>] ? trace_hardirqs_on+0xd/0x10
>  [<ffffffffa0522d55>] fat_statfs+0xf5/0x110 [fat]
>  [<ffffffff8110be5c>] vfs_statfs+0x7c/0xa0
>  [<ffffffff8110c0b0>] vfs_statfs_native+0x20/0xb0
>  [<ffffffff8110c243>] sys_statfs+0x73/0xb0
>  [<ffffffff8122ab2b>] ? __up_write+0xcb/0x120
>  [<ffffffff8100c18c>] ? sysret_check+0x27/0x62
>  [<ffffffff8109eb8b>] ? audit_syscall_entry+0x28b/0x2b0
>  [<ffffffff8107d7ed>] ? trace_hardirqs_on_caller+0x15d/0x1a0
>  [<ffffffff813dbf1e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
>  [<ffffffff8100c15b>] system_call_fastpath+0x16/0x1b
Pavel Machek - Sept. 7, 2009, 12:51 p.m.
Hi!

> >>> Note that when you rever this patch on a current kernel you do actually
> >>> get different behvaviour than when going back to before this commit.
> >>>
> >>> In 2.6.30 we called ->write_super in the various sync functions and
> >>> then ->sync_fs, in 2.6.31-rc8 you would not call any syncing at all
> >>> anymore. ?I think this patch might just be a symptom for a situation
> >>> where the suspend code causes a sync and the mmc driver can't handle
> >>> it anymore.
> >
> > So - here is the console trace from suspend when I've added
> > dump_stack() to the fat_sync_fs()   (and also added debug prints
> > around each call in this function -so its obvious the function is
> > actually left - but then it freezes later somewhere.)
> >
> > It's interesting that 3 calls to sync happens.
> 
> It seems
> 
>     1) sync() (probabry "sync" command)
>     2) sync as part of suspend sequence
>     3) sync_filesystem() by mmc remove event
> 
> I guess the root-cause of the problem would be 3). However, it would not
> be easy to fix, at least, we would need to think about what we want to
> do for it. So, to workaround it for now, I've made this patch.

MMC driver trying to synchronize filesystems looks like ugly layering
violation to me. Why are we doing that?
Zdenek Kabelac - Sept. 8, 2009, 8:10 a.m.
2009/9/6 OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Zdenek Kabelac <zdenek.kabelac@gmail.com> writes:
>
>> 2009/9/5 OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>>> Zdenek Kabelac <zdenek.kabelac@gmail.com> writes:
>>>
>>>> 2009/9/4 OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>>>>> Christoph Hellwig <hch@lst.de> writes:
>>>>>
>>>>>> Note that when you rever this patch on a current kernel you do actually
>>>>>> get different behvaviour than when going back to before this commit.
>>>>>>
>>>>>> In 2.6.30 we called ->write_super in the various sync functions and
>>>>>> then ->sync_fs, in 2.6.31-rc8 you would not call any syncing at all
>>>>>> anymore.  I think this patch might just be a symptom for a situation
>>>>>> where the suspend code causes a sync and the mmc driver can't handle
>>>>>> it anymore.
>>>>
>>>> So - here is the console trace from suspend when I've added
>>>> dump_stack() to the fat_sync_fs()   (and also added debug prints
>>>> around each call in this function -so its obvious the function is
>>>> actually left - but then it freezes later somewhere.)
>>>>
>>>> It's interesting that 3 calls to sync happens.
>>>
>>> It seems
>>>
>>>    1) sync() (probabry "sync" command)
>>>    2) sync as part of suspend sequence
>>>    3) sync_filesystem() by mmc remove event
>>>
>>> I guess the root-cause of the problem would be 3). However, it would not
>>> be easy to fix, at least, we would need to think about what we want to
>>> do for it. So, to workaround it for now, I've made this patch.
>>>
>>> Can you try whether this patch fixes this problem?
>>>
>>
>> So I've tested your patch - it seems to fix the problem in suspend -
>> the machine sleeps - however after resume the mmc SD card becomes
>> unusable to the system and appended call trace filled my message log
>> very quickly:
>>
>> After reboot the filesystem appeared to be usable again without errors.
>
> Thanks for testing.  "logical block size: 27499" is wrong value
> completely. Of course, fatfs is assuming the blocksize is not changed.
> (mmc wasn't resumed correctly?)
>
> Well, this problem seems to be completely different problem, and it
> seems the problem of suspend or mmc (or block?) stuff, or something.
>
> It would need to be analyzed by those people.
>
> Meanwhile, I'll apply this patch to workaround suspend problem and to
> remove unneeded write for next window.
>
> Thanks.
>
>> Call Trace:
>>  [<ffffffff81134f6b>] __getblk+0x2cb/0x300
>>  [<ffffffff813dcb58>] ? _spin_unlock_irqrestore+0x38/0x80
>>  [<ffffffff81135122>] __breadahead+0x12/0x40
>>  [<ffffffffa0520cb7>] fat_count_free_clusters+0x307/0x320 [fat]
>>  [<ffffffff81103a58>] ? check_object+0xd8/0x260
>>  [<ffffffff8107d83d>] ? trace_hardirqs_on+0xd/0x10
>>  [<ffffffffa0522d55>] fat_statfs+0xf5/0x110 [fat]
>>  [<ffffffff8110be5c>] vfs_statfs+0x7c/0xa0
>>  [<ffffffff8110c0b0>] vfs_statfs_native+0x20/0xb0
>>  [<ffffffff8110c243>] sys_statfs+0x73/0xb0
>>  [<ffffffff8122ab2b>] ? __up_write+0xcb/0x120
>>  [<ffffffff8100c18c>] ? sysret_check+0x27/0x62
>>  [<ffffffff8109eb8b>] ? audit_syscall_entry+0x28b/0x2b0
>>  [<ffffffff8107d7ed>] ? trace_hardirqs_on_caller+0x15d/0x1a0
>>  [<ffffffff813dbf1e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
>>  [<ffffffff8100c15b>] system_call_fastpath+0x16/0x1b
>> getblk(): invalid block size 512 requested
>> logical block size: 27499
>>
>>  [<ffffffff81135122>] __breadahead+0x12/0x40
>>  [<ffffffffa0520cb7>] fat_count_free_clusters+0x307/0x320 [fat]
>>  [<ffffffff81103a58>] ? check_object+0xd8/0x260
>>  [<ffffffff8107d83d>] ? trace_hardirqs_on+0xd/0x10
>>  [<ffffffffa0522d55>] fat_statfs+0xf5/0x110 [fat]
>>  [<ffffffff8110be5c>] vfs_statfs+0x7c/0xa0
>>  [<ffffffff8110c0b0>] vfs_statfs_native+0x20/0xb0
>>  [<ffffffff8110c243>] sys_statfs+0x73/0xb0
>>  [<ffffffff8122ab2b>] ? __up_write+0xcb/0x120
>>  [<ffffffff8100c18c>] ? sysret_check+0x27/0x62
>>  [<ffffffff8109eb8b>] ? audit_syscall_entry+0x28b/0x2b0
>>  [<ffffffff8107d7ed>] ? trace_hardirqs_on_caller+0x15d/0x1a0
>>  [<ffffffff813dbf1e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
>>  [<ffffffff8100c15b>] system_call_fastpath+0x16/0x1b
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>


Just a side note - Could be there any connection with my previous report?

http://lkml.org/lkml/2009/8/28/90


Zdenek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
OGAWA Hirofumi - Sept. 9, 2009, 1:15 p.m.
Zdenek Kabelac <zdenek.kabelac@gmail.com> writes:

> Just a side note - Could be there any connection with my previous report?
>
> http://lkml.org/lkml/2009/8/28/90

As far as I can see, it doesn't seem related problem to this. It seems
mmc's driver problem (or problem of unusual device).

Thanks.
OGAWA Hirofumi - Sept. 9, 2009, 1:21 p.m.
Pavel Machek <pavel@ucw.cz> writes:

>> It seems
>> 
>>     1) sync() (probabry "sync" command)
>>     2) sync as part of suspend sequence
>>     3) sync_filesystem() by mmc remove event
>> 
>> I guess the root-cause of the problem would be 3). However, it would not
>> be easy to fix, at least, we would need to think about what we want to
>> do for it. So, to workaround it for now, I've made this patch.
>
> MMC driver trying to synchronize filesystems looks like ugly layering
> violation to me. Why are we doing that?

There is no _layering violation_ here. IIRC, mmc just tells card removed
event to another layer (on some points of view, to tell event can be
wrong though). The partition (block) layer does it by event.

Thanks.
Pavel Machek - Sept. 10, 2009, 7:23 p.m.
On Wed 2009-09-09 22:21:56, OGAWA Hirofumi wrote:
> Pavel Machek <pavel@ucw.cz> writes:
> 
> >> It seems
> >> 
> >>     1) sync() (probabry "sync" command)
> >>     2) sync as part of suspend sequence
> >>     3) sync_filesystem() by mmc remove event
> >> 
> >> I guess the root-cause of the problem would be 3). However, it would not
> >> be easy to fix, at least, we would need to think about what we want to
> >> do for it. So, to workaround it for now, I've made this patch.
> >
> > MMC driver trying to synchronize filesystems looks like ugly layering
> > violation to me. Why are we doing that?
> 
> There is no _layering violation_ here. IIRC, mmc just tells card removed
> event to another layer (on some points of view, to tell event can be
> wrong though). The partition (block) layer does it by event.

So what is the problem? Emulating sync when card is already removed
seems little ... interesting?
OGAWA Hirofumi - Sept. 11, 2009, 6:39 a.m.
Pavel Machek <pavel@ucw.cz> writes:

> On Wed 2009-09-09 22:21:56, OGAWA Hirofumi wrote:
>> Pavel Machek <pavel@ucw.cz> writes:
>> 
>> >> It seems
>> >> 
>> >>     1) sync() (probabry "sync" command)
>> >>     2) sync as part of suspend sequence
>> >>     3) sync_filesystem() by mmc remove event
>> >> 
>> >> I guess the root-cause of the problem would be 3). However, it would not
>> >> be easy to fix, at least, we would need to think about what we want to
>> >> do for it. So, to workaround it for now, I've made this patch.
>> >
>> > MMC driver trying to synchronize filesystems looks like ugly layering
>> > violation to me. Why are we doing that?
>> 
>> There is no _layering violation_ here. IIRC, mmc just tells card removed
>> event to another layer (on some points of view, to tell event can be
>> wrong though). The partition (block) layer does it by event.
>
> So what is the problem?  Emulating sync when card is already removed
> seems little ... interesting?

Um..., sorry, I'm not sure what are you talking about. Of course, the
problem of this is that system freeze on suspend.

Or are you asking my guess of the cause, or something?  If so, although
I'm not reading all emails on this thread, from Zdenek's backtrace, the
sequence would be

    1) suspend mmc
    2) mmc generates card removed event
    3) prepare to invalidate blockdev
    4) sync fs on invalidating blockdev
    5) flush buffers on invalidating blockdev (partitions)
    6) delete blockdev (partitions)

or like the above. And I can guess some possible issues/root-cause we
have to handle from it.

    a) card removed event from mmc for suspend is right design?
    b) the card can be changed/removed before system was resumed, mmc
       can be detect/handle it properly?
    c) flushing buffers on _deleted_ device is right design?

and I suspect there are more issues in detail and resume process though.

Thanks.
Pavel Machek - Sept. 11, 2009, 8:09 p.m.
> Um..., sorry, I'm not sure what are you talking about. Of course, the
> problem of this is that system freeze on suspend.
> 
> Or are you asking my guess of the cause, or something?  If so, although
> I'm not reading all emails on this thread, from Zdenek's backtrace, the
> sequence would be
> 
>     1) suspend mmc
>     2) mmc generates card removed event
>     3) prepare to invalidate blockdev
>     4) sync fs on invalidating blockdev
>     5) flush buffers on invalidating blockdev (partitions)
>     6) delete blockdev (partitions)
> 
> or like the above. And I can guess some possible issues/root-cause we
> have to handle from it.
> 
>     a) card removed event from mmc for suspend is right design?
>     b) the card can be changed/removed before system was resumed, mmc
>        can be detect/handle it properly?
>     c) flushing buffers on _deleted_ device is right design?
> 
> and I suspect there are more issues in detail and resume process though.

I guess c) is main problem. If device is not there, how do you want to
flush buffers on it?

								Pavel
Zdenek Kabelac - Sept. 11, 2009, 9:14 p.m.
2009/9/11 Pavel Machek <pavel@ucw.cz>:
>
>> Um..., sorry, I'm not sure what are you talking about. Of course, the
>> problem of this is that system freeze on suspend.
>>
>> Or are you asking my guess of the cause, or something?  If so, although
>> I'm not reading all emails on this thread, from Zdenek's backtrace, the
>> sequence would be
>>
>>     1) suspend mmc
>>     2) mmc generates card removed event
>>     3) prepare to invalidate blockdev
>>     4) sync fs on invalidating blockdev
>>     5) flush buffers on invalidating blockdev (partitions)
>>     6) delete blockdev (partitions)
>>
>> or like the above. And I can guess some possible issues/root-cause we
>> have to handle from it.
>>
>>     a) card removed event from mmc for suspend is right design?
>>     b) the card can be changed/removed before system was resumed, mmc
>>        can be detect/handle it properly?
>>     c) flushing buffers on _deleted_ device is right design?
>>
>> and I suspect there are more issues in detail and resume process though.
>
> I guess c) is main problem. If device is not there, how do you want to
> flush buffers on it?
>

Well I do not even understand why someone wants to sync something when
card is removed ??
And why suspend of mmc should generate card removal ??

IMHO steps 2..6 are only valid for the case I would 'remove'
unexpectedly card - but
if I suspend and resume my laptop and I keep the card inside - all
those step looks plain wrong.

Zdenek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pavel Machek - Sept. 11, 2009, 9:32 p.m.
> And why suspend of mmc should generate card removal ??

Card is powered down during suspend -> mmc can't guarantee the card is
same and unchanged -> it makes some sense to simulate
removal/reinsert.

> if I suspend and resume my laptop and I keep the card inside - all
> those step looks plain wrong.

Unfortunately system can't tell.
								Pavel
Zdenek Kabelac - Sept. 11, 2009, 9:45 p.m.
2009/9/11 Pavel Machek <pavel@ucw.cz>:
>
>> And why suspend of mmc should generate card removal ??
>
> Card is powered down during suspend -> mmc can't guarantee the card is
> same and unchanged -> it makes some sense to simulate
> removal/reinsert.

But how is this going to work when I keep the device mounted and
blockdev is basically destroyed -  what if I'm reading file from card
during suspend ?

>> if I suspend and resume my laptop and I keep the card inside - all
>> those step looks plain wrong.
>
> Unfortunately system can't tell.

Well system could check basic card ids if they match after resume - if
some users wants to crash his card by randomly swapping it during
suspend/resume - I'd have no problem with that....

Zdenek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pavel Machek - Sept. 11, 2009, 9:51 p.m.
On Fri 2009-09-11 23:45:01, Zdenek Kabelac wrote:
> 2009/9/11 Pavel Machek <pavel@ucw.cz>:
> >
> >> And why suspend of mmc should generate card removal ??
> >
> > Card is powered down during suspend -> mmc can't guarantee the card is
> > same and unchanged -> it makes some sense to simulate
> > removal/reinsert.
> 
> But how is this going to work when I keep the device mounted and
> blockdev is basically destroyed -  what if I'm reading file from card
> during suspend ?

"Don't do it".

> >> if I suspend and resume my laptop and I keep the card inside - all
> >> those step looks plain wrong.
> >
> > Unfortunately system can't tell.
> 
> Well system could check basic card ids if they match after resume - if
> some users wants to crash his card by randomly swapping it during
> suspend/resume - I'd have no problem with that....

Well, I do have small problem with that :-).

Anyway, patch for rechecking IDs would probably be accepted, but
that's not how it works now.
								Pavel
Rafael Wysocki - Sept. 11, 2009, 10:04 p.m.
On Friday 11 September 2009, OGAWA Hirofumi wrote:
> Pavel Machek <pavel@ucw.cz> writes:
> 
> > On Wed 2009-09-09 22:21:56, OGAWA Hirofumi wrote:
> >> Pavel Machek <pavel@ucw.cz> writes:
> >> 
> >> >> It seems
> >> >> 
> >> >>     1) sync() (probabry "sync" command)
> >> >>     2) sync as part of suspend sequence
> >> >>     3) sync_filesystem() by mmc remove event
> >> >> 
> >> >> I guess the root-cause of the problem would be 3). However, it would not
> >> >> be easy to fix, at least, we would need to think about what we want to
> >> >> do for it. So, to workaround it for now, I've made this patch.
> >> >
> >> > MMC driver trying to synchronize filesystems looks like ugly layering
> >> > violation to me. Why are we doing that?
> >> 
> >> There is no _layering violation_ here. IIRC, mmc just tells card removed
> >> event to another layer (on some points of view, to tell event can be
> >> wrong though). The partition (block) layer does it by event.
> >
> > So what is the problem?  Emulating sync when card is already removed
> > seems little ... interesting?
> 
> Um..., sorry, I'm not sure what are you talking about. Of course, the
> problem of this is that system freeze on suspend.
> 
> Or are you asking my guess of the cause, or something?  If so, although
> I'm not reading all emails on this thread, from Zdenek's backtrace, the
> sequence would be
> 
>     1) suspend mmc
>     2) mmc generates card removed event

Which shouldn't happen.

>     3) prepare to invalidate blockdev
>     4) sync fs on invalidating blockdev
>     5) flush buffers on invalidating blockdev (partitions)
>     6) delete blockdev (partitions)
> 
> or like the above. And I can guess some possible issues/root-cause we
> have to handle from it.
> 
>     a) card removed event from mmc for suspend is right design?

Not with the current suspend/resume design.

>     b) the card can be changed/removed before system was resumed, mmc
>        can be detect/handle it properly?
>     c) flushing buffers on _deleted_ device is right design?
> 
> and I suspect there are more issues in detail and resume process though.

Well, first, there's a limit to which file systems can ignore the
suspend/resume process and we're hitting it right now.

Second, we need a general solution for handling file systems over
suspend/resume _and_ possibly removable devices that can be gone while
suspended.  We don't have any solution like this right now and I have a little
experience with file systems, so I'm not going to take care of this in the
foreseeable future.  If someone else can, that's going to be appreciated very
much.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pavel Machek - Sept. 11, 2009, 10:21 p.m.
On Sat 2009-09-12 00:04:02, Rafael J. Wysocki wrote:
> On Friday 11 September 2009, OGAWA Hirofumi wrote:
> > Pavel Machek <pavel@ucw.cz> writes:
> > 
> > > On Wed 2009-09-09 22:21:56, OGAWA Hirofumi wrote:
> > >> Pavel Machek <pavel@ucw.cz> writes:
> > >> 
> > >> >> It seems
> > >> >> 
> > >> >>     1) sync() (probabry "sync" command)
> > >> >>     2) sync as part of suspend sequence
> > >> >>     3) sync_filesystem() by mmc remove event
> > >> >> 
> > >> >> I guess the root-cause of the problem would be 3). However, it would not
> > >> >> be easy to fix, at least, we would need to think about what we want to
> > >> >> do for it. So, to workaround it for now, I've made this patch.
> > >> >
> > >> > MMC driver trying to synchronize filesystems looks like ugly layering
> > >> > violation to me. Why are we doing that?
> > >> 
> > >> There is no _layering violation_ here. IIRC, mmc just tells card removed
> > >> event to another layer (on some points of view, to tell event can be
> > >> wrong though). The partition (block) layer does it by event.
> > >
> > > So what is the problem?  Emulating sync when card is already removed
> > > seems little ... interesting?
> > 
> > Um..., sorry, I'm not sure what are you talking about. Of course, the
> > problem of this is that system freeze on suspend.
> > 
> > Or are you asking my guess of the cause, or something?  If so, although
> > I'm not reading all emails on this thread, from Zdenek's backtrace, the
> > sequence would be
> > 
> >     1) suspend mmc
> >     2) mmc generates card removed event
> 
> Which shouldn't happen.

Are you sure? IIRC it depends on  CONFIG_MMC_UNSAFE_RESUME.
									Pavel
Rafael Wysocki - Sept. 11, 2009, 10:22 p.m.
On Friday 11 September 2009, Pavel Machek wrote:
> On Fri 2009-09-11 23:45:01, Zdenek Kabelac wrote:
> > 2009/9/11 Pavel Machek <pavel@ucw.cz>:
> > >
> > >> And why suspend of mmc should generate card removal ??
> > >
> > > Card is powered down during suspend -> mmc can't guarantee the card is
> > > same and unchanged -> it makes some sense to simulate
> > > removal/reinsert.
> > 
> > But how is this going to work when I keep the device mounted and
> > blockdev is basically destroyed -  what if I'm reading file from card
> > during suspend ?
> 
> "Don't do it".
> 
> > >> if I suspend and resume my laptop and I keep the card inside - all
> > >> those step looks plain wrong.
> > >
> > > Unfortunately system can't tell.
> > 
> > Well system could check basic card ids if they match after resume - if
> > some users wants to crash his card by randomly swapping it during
> > suspend/resume - I'd have no problem with that....
> 
> Well, I do have small problem with that :-).
> 
> Anyway, patch for rechecking IDs would probably be accepted,

By whom exactly?

> but that's not how it works now.

Indeed.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Chris Ball - Sept. 11, 2009, 10:22 p.m.
Hi,

   > IMHO steps 2..6 are only valid for the case I would 'remove'
   > unexpectedly card - but if I suspend and resume my laptop and I
   > keep the card inside - all those step looks plain wrong.

How can the MMC stack tell whether you kept the card inside or
modified it during suspend?

There's no way to know, and an incorrect guess gives you filesystem
corruption, so we remove cards on suspend and reprobe them on resume.
(If you did know that cards would never be removed during suspend,
you could set CONFIG_MMC_UNSAFE_RESUME=y.)

So, I'd say:

   >> a) card removed event from mmc for suspend is right design?

Yes, for a card containing a filesystem with CONFIG_MMC_UNSAFE_RESUME
not set.

   >> b) the card can be changed/removed before system was resumed, mmc
   >> can be detect/handle it properly?

Yes, precisely because we removed it before suspend.

   >> c) flushing buffers on _deleted_ device is right design?

No, something's obviously gone wrong here.
Chris Ball - Sept. 11, 2009, 10:29 p.m.
Hi,

   > Well system could check basic card ids if they match after resume

No.  That (arguably) guarantees that it's the same card, but not that
it wasn't modified in another machine during the suspend.

   > if some users wants to crash his card by randomly swapping it
   > during suspend/resume - I'd have no problem with that....

You should have a problem with it.  Taking a card from a suspended
machine and working on it with a different machine is not a bizarre
thing to want to do.
Rafael Wysocki - Sept. 11, 2009, 10:32 p.m.
On Saturday 12 September 2009, Pavel Machek wrote:
> On Sat 2009-09-12 00:04:02, Rafael J. Wysocki wrote:
> > On Friday 11 September 2009, OGAWA Hirofumi wrote:
> > > Pavel Machek <pavel@ucw.cz> writes:
> > > 
> > > > On Wed 2009-09-09 22:21:56, OGAWA Hirofumi wrote:
> > > >> Pavel Machek <pavel@ucw.cz> writes:
> > > >> 
> > > >> >> It seems
> > > >> >> 
> > > >> >>     1) sync() (probabry "sync" command)
> > > >> >>     2) sync as part of suspend sequence
> > > >> >>     3) sync_filesystem() by mmc remove event
> > > >> >> 
> > > >> >> I guess the root-cause of the problem would be 3). However, it would not
> > > >> >> be easy to fix, at least, we would need to think about what we want to
> > > >> >> do for it. So, to workaround it for now, I've made this patch.
> > > >> >
> > > >> > MMC driver trying to synchronize filesystems looks like ugly layering
> > > >> > violation to me. Why are we doing that?
> > > >> 
> > > >> There is no _layering violation_ here. IIRC, mmc just tells card removed
> > > >> event to another layer (on some points of view, to tell event can be
> > > >> wrong though). The partition (block) layer does it by event.
> > > >
> > > > So what is the problem?  Emulating sync when card is already removed
> > > > seems little ... interesting?
> > > 
> > > Um..., sorry, I'm not sure what are you talking about. Of course, the
> > > problem of this is that system freeze on suspend.
> > > 
> > > Or are you asking my guess of the cause, or something?  If so, although
> > > I'm not reading all emails on this thread, from Zdenek's backtrace, the
> > > sequence would be
> > > 
> > >     1) suspend mmc
> > >     2) mmc generates card removed event
> > 
> > Which shouldn't happen.
> 
> Are you sure? IIRC it depends on  CONFIG_MMC_UNSAFE_RESUME.

Generating the event at this point is too late, because there's no way to
handle it cleanly with the current suspend/resume design.

It probably will work if the event is generated before we freeze the user
space, for example with the help of a suspend notifier, but generating it
from a driver's suspend routine is not valid.

Again, that's a consequence of the lack of a general solution for handling
"removable" file systems over suspend/resume.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rafael Wysocki - Sept. 11, 2009, 10:36 p.m.
On Saturday 12 September 2009, Chris Ball wrote:
> Hi,
> 
>    > Well system could check basic card ids if they match after resume
> 
> No.  That (arguably) guarantees that it's the same card, but not that
> it wasn't modified in another machine during the suspend.

Generally speaking, we'd also need to check superblocks for this to work.
 
>    > if some users wants to crash his card by randomly swapping it
>    > during suspend/resume - I'd have no problem with that....
> 
> You should have a problem with it.  Taking a card from a suspended
> machine and working on it with a different machine is not a bizarre
> thing to want to do.

Agreed.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Zdenek Kabelac - Sept. 14, 2009, 8:39 a.m.
2009/9/12 Rafael J. Wysocki <rjw@sisk.pl>:
> On Saturday 12 September 2009, Chris Ball wrote:
>> Hi,
>>
>>    > Well system could check basic card ids if they match after resume
>>
>> No.  That (arguably) guarantees that it's the same card, but not that
>> it wasn't modified in another machine during the suspend.
>
> Generally speaking, we'd also need to check superblocks for this to work.
>
>>    > if some users wants to crash his card by randomly swapping it
>>    > during suspend/resume - I'd have no problem with that....
>>
>> You should have a problem with it.  Taking a card from a suspended
>> machine and working on it with a different machine is not a bizarre
>> thing to want to do.
>
> Agreed.


Well - ok - so let me ask this question - if I'll replace local hard
drive during suspend - what will happen - is this prohibited by hw
(e.i. to switch SATA cables) ?

IMHO filesystem should be able to detect corruption of its data
structures - (assuming fs is notified about suspend/resume operation)

Also there could be one simple quick solution/hack - to require to
have at least all remote drives unmounted - so suspend would be
refused if it runs mounted card/usb drive -  this would be 100% better
than current solution which effectively kills my laptop if I forget to
unmount card in mmc reader - especially if dmesg contains message with
the reason why my suspend fails.


Zdenek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Rafael Wysocki - Sept. 14, 2009, 7:17 p.m.
On Monday 14 September 2009, Zdenek Kabelac wrote:
> 2009/9/12 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Saturday 12 September 2009, Chris Ball wrote:
> >> Hi,
> >>
> >>    > Well system could check basic card ids if they match after resume
> >>
> >> No.  That (arguably) guarantees that it's the same card, but not that
> >> it wasn't modified in another machine during the suspend.
> >
> > Generally speaking, we'd also need to check superblocks for this to work.
> >
> >>    > if some users wants to crash his card by randomly swapping it
> >>    > during suspend/resume - I'd have no problem with that....
> >>
> >> You should have a problem with it.  Taking a card from a suspended
> >> machine and working on it with a different machine is not a bizarre
> >> thing to want to do.
> >
> > Agreed.
> 
> 
> Well - ok - so let me ask this question - if I'll replace local hard
> drive during suspend - what will happen - is this prohibited by hw
> (e.i. to switch SATA cables) ?

That I'm unsure of, but if you replace some other major components, such
as the CPU or memory, the hardware will detect that and the resume will fail.

> IMHO filesystem should be able to detect corruption of its data
> structures - (assuming fs is notified about suspend/resume operation)

Well, the problem is that at the moment such a notification mechanism doesn't
exist.

> Also there could be one simple quick solution/hack

No hacks, please.

> - to require to have at least all remote drives unmounted - so suspend would

Define "remote".  It isn't that simple, even your root fs can be on USB, iSCSI,
whatever.

> be refused if it runs mounted card/usb drive -  this would be 100% better
> than current solution which effectively kills my laptop if I forget to
> unmount card in mmc reader - especially if dmesg contains message with
> the reason why my suspend fails.

You can make the suspend scripts check for that, there's no reason for the
kernel to do it IMO.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Pierre Ossman - Sept. 14, 2009, 8:05 p.m.
On Fri, 11 Sep 2009 23:51:17 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> On Fri 2009-09-11 23:45:01, Zdenek Kabelac wrote:
> > 
> > Well system could check basic card ids if they match after resume - if
> > some users wants to crash his card by randomly swapping it during
> > suspend/resume - I'd have no problem with that....
> 
> Well, I do have small problem with that :-).
> 
> Anyway, patch for rechecking IDs would probably be accepted, but
> that's not how it works now.
> 								Pavel

It _does_ check the card id when you have UNSAFE_RESUME selected. It
doesn't just hook up whatever card you happen to have in the slot with
your old block device. I may have a lot of opponents when it comes
to my suspend design choices, but I'm not completely crazy. ;)

Rgds
Pavel Machek - Sept. 14, 2009, 8:25 p.m.
On Mon 2009-09-14 22:05:10, Pierre Ossman wrote:
> On Fri, 11 Sep 2009 23:51:17 +0200
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > On Fri 2009-09-11 23:45:01, Zdenek Kabelac wrote:
> > > 
> > > Well system could check basic card ids if they match after resume - if
> > > some users wants to crash his card by randomly swapping it during
> > > suspend/resume - I'd have no problem with that....
> > 
> > Well, I do have small problem with that :-).
> > 
> > Anyway, patch for rechecking IDs would probably be accepted, but
> > that's not how it works now.
> 
> It _does_ check the card id when you have UNSAFE_RESUME selected. It
> doesn't just hook up whatever card you happen to have in the slot with
> your old block device. I may have a lot of opponents when it comes
> to my suspend design choices, but I'm not completely crazy. ;)

:-) Ok, ok. Patch checking filesystem timestamps would be welcome in
that case ;-).
									Pavel
Pavel Machek - Sept. 14, 2009, 8:27 p.m.
On Mon 2009-09-14 10:39:44, Zdenek Kabelac wrote:
> 2009/9/12 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Saturday 12 September 2009, Chris Ball wrote:
> >> Hi,
> >>
> >>    > Well system could check basic card ids if they match after resume
> >>
> >> No.  That (arguably) guarantees that it's the same card, but not that
> >> it wasn't modified in another machine during the suspend.
> >
> > Generally speaking, we'd also need to check superblocks for this to work.
> >
> >>    > if some users wants to crash his card by randomly swapping it
> >>    > during suspend/resume - I'd have no problem with that....
> >>
> >> You should have a problem with it.  Taking a card from a suspended
> >> machine and working on it with a different machine is not a bizarre
> >> thing to want to do.
> >
> > Agreed.
> 
> 
> Well - ok - so let me ask this question - if I'll replace local hard
> drive during suspend - what will happen - is this prohibited by hw
> (e.i. to switch SATA cables) ?

During _suspend_: yes. You are not expected to open your machine while
powered up.

> IMHO filesystem should be able to detect corruption of its data
> structures - (assuming fs is notified about suspend/resume
> operation)

Patch welcome.

> Also there could be one simple quick solution/hack - to require to
> have at least all remote drives unmounted - so suspend would be
> refused if it runs mounted card/usb drive -  this would be 100% better
> than current solution which effectively kills my laptop if I forget to
> unmount card in mmc reader - especially if dmesg contains message with
> the reason why my suspend fails.

It should not _kill_ your laptop -- that's a bug we want to
fix. Instead it is designed to behave as if you hot-unplugged your
card while mounted.

									Pavel
OGAWA Hirofumi - Sept. 18, 2009, 11:15 a.m.
"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> On Saturday 12 September 2009, Chris Ball wrote:
>> Hi,
>> 
>>    > Well system could check basic card ids if they match after resume
>> 
>> No.  That (arguably) guarantees that it's the same card, but not that
>> it wasn't modified in another machine during the suspend.
>
> Generally speaking, we'd also need to check superblocks for this to work.
>  
>>    > if some users wants to crash his card by randomly swapping it
>>    > during suspend/resume - I'd have no problem with that....
>> 
>> You should have a problem with it.  Taking a card from a suspended
>> machine and working on it with a different machine is not a bizarre
>> thing to want to do.
>
> Agreed.

Um...

What happen if we moved remove event to resume sequence?  I.e. The
resume generates remove and insert event (or such revalidate).  With
this, I hope the suspend is not bothered by complex one, and the resume
just ignores (if needed) previous state and notify it to userland by
remove/insert event.

And, userland process should unmount for removal devices before suspend
process (as part of userland preparation)?

If we assumed the removable device can be changed before resume, fs
would need to recover process, to make sure in-core and on-disk state
has consistent.

Um..., for now, I feel the umount before suspend is only safe way.

Thanks.
Rafael Wysocki - Sept. 18, 2009, 9:39 p.m.
On Friday 18 September 2009, OGAWA Hirofumi wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > On Saturday 12 September 2009, Chris Ball wrote:
> >> Hi,
> >> 
> >>    > Well system could check basic card ids if they match after resume
> >> 
> >> No.  That (arguably) guarantees that it's the same card, but not that
> >> it wasn't modified in another machine during the suspend.
> >
> > Generally speaking, we'd also need to check superblocks for this to work.
> >  
> >>    > if some users wants to crash his card by randomly swapping it
> >>    > during suspend/resume - I'd have no problem with that....
> >> 
> >> You should have a problem with it.  Taking a card from a suspended
> >> machine and working on it with a different machine is not a bizarre
> >> thing to want to do.
> >
> > Agreed.
> 
> Um...
> 
> What happen if we moved remove event to resume sequence?  I.e. The
> resume generates remove and insert event (or such revalidate).  With
> this, I hope the suspend is not bothered by complex one, and the resume
> just ignores (if needed) previous state and notify it to userland by
> remove/insert event.
> 
> And, userland process should unmount for removal devices before suspend
> process (as part of userland preparation)?
> 
> If we assumed the removable device can be changed before resume, fs
> would need to recover process, to make sure in-core and on-disk state
> has consistent.
> 
> Um..., for now, I feel the umount before suspend is only safe way.

Yes, with the current design it's the only really safe way.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch

diff -puN fs/fat/inode.c~fat_sync_fs fs/fat/inode.c
--- linux-2.6/fs/fat/inode.c~fat_sync_fs	2009-09-03 21:24:03.000000000 +0900
+++ linux-2.6-hirofumi/fs/fat/inode.c	2009-09-06 02:07:07.000000000 +0900
@@ -451,12 +451,16 @@  static void fat_write_super(struct super
 
 static int fat_sync_fs(struct super_block *sb, int wait)
 {
-	lock_super(sb);
-	fat_clusters_flush(sb);
-	sb->s_dirt = 0;
-	unlock_super(sb);
+	int err = 0;
 
-	return 0;
+	if (sb->s_dirt) {
+		lock_super(sb);
+		sb->s_dirt = 0;
+		err = fat_clusters_flush(sb);
+		unlock_super(sb);
+	}
+
+	return err;
 }
 
 static void fat_put_super(struct super_block *sb)
diff -puN fs/fat/misc.c~fat_sync_fs fs/fat/misc.c
--- linux-2.6/fs/fat/misc.c~fat_sync_fs	2009-09-03 21:24:03.000000000 +0900
+++ linux-2.6-hirofumi/fs/fat/misc.c	2009-09-06 02:02:56.000000000 +0900
@@ -43,19 +43,19 @@  EXPORT_SYMBOL_GPL(fat_fs_error);
 
 /* Flushes the number of free clusters on FAT32 */
 /* XXX: Need to write one per FSINFO block.  Currently only writes 1 */
-void fat_clusters_flush(struct super_block *sb)
+int fat_clusters_flush(struct super_block *sb)
 {
 	struct msdos_sb_info *sbi = MSDOS_SB(sb);
 	struct buffer_head *bh;
 	struct fat_boot_fsinfo *fsinfo;
 
 	if (sbi->fat_bits != 32)
-		return;
+		return 0;
 
 	bh = sb_bread(sb, sbi->fsinfo_sector);
 	if (bh == NULL) {
 		printk(KERN_ERR "FAT: bread failed in fat_clusters_flush\n");
-		return;
+		return -EIO;
 	}
 
 	fsinfo = (struct fat_boot_fsinfo *)bh->b_data;
@@ -74,6 +74,8 @@  void fat_clusters_flush(struct super_blo
 		mark_buffer_dirty(bh);
 	}
 	brelse(bh);
+
+	return 0;
 }
 
 /*
diff -puN fs/fat/fat.h~fat_sync_fs fs/fat/fat.h
--- linux-2.6/fs/fat/fat.h~fat_sync_fs	2009-09-03 21:24:03.000000000 +0900
+++ linux-2.6-hirofumi/fs/fat/fat.h	2009-09-06 02:02:56.000000000 +0900
@@ -323,7 +323,7 @@  extern int fat_flush_inodes(struct super
 /* fat/misc.c */
 extern void fat_fs_error(struct super_block *s, const char *fmt, ...)
 	__attribute__ ((format (printf, 2, 3))) __cold;
-extern void fat_clusters_flush(struct super_block *sb);
+extern int fat_clusters_flush(struct super_block *sb);
 extern int fat_chain_add(struct inode *inode, int new_dclus, int nr_cluster);
 extern void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec *ts,
 			      __le16 __time, __le16 __date, u8 time_cs);