2009/8/31 Rafael J. Wysocki <[email protected]>:
> On Monday 31 August 2009, Zdenek Kabelac wrote:
>> Hi
>>
>> I've noticed that my machine freezes while doing s2r and having
>> mounted filesystem from SD card.
>>
>> My system: Lenovo T61, 4GB, C2D, kernel 2.6.31-rc8
>>
>> Here is the trace of ?suspend when SD card is inserted, follows resume
>> and again suspend and this time with mounted filesystem from SD card.
>>
>> System locks with: PM: Removing info for mmc:mmc0:b368
>> Also btusb_bulk_complete: errors are interesting - I've noticed few
>> bugzillas in the google.
>> I could try bisect to find the patch - but it will take time....
>> So hopefully this trace will help.
>
> Not really. :-(
>
> Is this a new regression in 2.6.31-rc, or does 2.6.30 also fail?
>
Well - all I know for now is this: v2.6.30 goes to suspend (i.e.
machine turns to sleep), but resume fails - I do not get a single line
of output on serial console either - so it's hard to tell whether it
works or not - before suspend there are some INFO traces about lockdep
problems in cpu frequency. v2.6.31-rc1 doesn't boot on my machine.
v2.6.31-rc2 seems to lock on suspend so this version is already
definitely broken. I may try to do some bisecting between these
kernels - but it might probably easily lead to some dead-ends probably
:(...
Also I'm using mmc debugs for compilation of these kernels - but
nothing seems to printed from mmc subsystem at the end of suspend log
and the end is still the same - last printed lines:
mmc0: card b368 removed
PM: Removing info for mmc:mmc0:b368
Zdenek
2009/9/1 Zdenek Kabelac <[email protected]>:
> 2009/8/31 Rafael J. Wysocki <[email protected]>:
>> On Monday 31 August 2009, Zdenek Kabelac wrote:
>>> Hi
>>>
>>> I've noticed that my machine freezes while doing s2r and having
>>> mounted filesystem from SD card.
>>>
>>> My system: Lenovo T61, 4GB, C2D, kernel 2.6.31-rc8
>>>
>>> Here is the trace of ?suspend when SD card is inserted, follows resume
>>> and again suspend and this time with mounted filesystem from SD card.
>>>
>>> System locks with: PM: Removing info for mmc:mmc0:b368
>>> Also btusb_bulk_complete: errors are interesting - I've noticed few
>>> bugzillas in the google.
>>> I could try bisect to find the patch - but it will take time....
>>> So hopefully this trace will help.
>>
>> Not really. :-(
>>
>> Is this a new regression in 2.6.31-rc, or does 2.6.30 also fail?
>>
>
> Well - all I know for now is this: ?v2.6.30 goes to suspend (i.e.
> machine turns to sleep), but resume fails - I do not get a single line
> of output on serial console either - so it's hard to tell whether it
> works or not - before suspend there are some INFO traces about lockdep
> problems in cpu frequency. ? v2.6.31-rc1 doesn't boot on my machine.
> v2.6.31-rc2 seems to lock on suspend so this version is already
> definitely broken. I may try to do some bisecting between these
> kernels - but it might probably easily lead to some dead-ends probably
> :(...
> Also I'm using ?mmc debugs for compilation of these kernels - but
> nothing seems to printed from mmc subsystem at the end of suspend log
> and the end is still the same - last printed lines:
>
> ?mmc0: card b368 removed
> ?PM: Removing info for mmc:mmc0:b368
>
Ok - another bisect game played - and unexpected winner is:
(fat: add ->sync_fs)
f83d6d46e7adf241a064a4a425e5cd8a8fd8925f
Reverting this commit with current -rc8 kernel makes the system happy
during the suspend/resume cycle. Obviously it has it price :) so just
plain revert is probably not a good solution so the problem looks
'more serious' (fat is not the only fs with this patch) thus adding
original author to this thread.
Zdenek
On Fri, Sep 04, 2009 at 12:29:04AM +0200, Zdenek Kabelac wrote:
> Ok - another bisect game played - and unexpected winner is:
>
> (fat: add ->sync_fs)
>
> f83d6d46e7adf241a064a4a425e5cd8a8fd8925f
>
> Reverting this commit with current -rc8 kernel makes the system happy
> during the suspend/resume cycle. Obviously it has it price :) so just
> plain revert is probably not a good solution so the problem looks
> 'more serious' (fat is not the only fs with this patch) thus adding
> original author to this thread.
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.
Christoph Hellwig <[email protected]> writes:
> On Fri, Sep 04, 2009 at 12:29:04AM +0200, Zdenek Kabelac wrote:
>> Ok - another bisect game played - and unexpected winner is:
>>
>> (fat: add ->sync_fs)
>>
>> f83d6d46e7adf241a064a4a425e5cd8a8fd8925f
>>
>> Reverting this commit with current -rc8 kernel makes the system happy
>> during the suspend/resume cycle. Obviously it has it price :) so just
>> plain revert is probably not a good solution so the problem looks
>> 'more serious' (fat is not the only fs with this patch) thus adding
>> original author to this thread.
>From it, I suspect the possible reason seems to read mmc after remove
event. I.e. the following sequence or something
sync fs process
[...]
removed mmc event
[...]
fat_sync_fs() <- sync again?
fat_clusters_flush()
sb_bread() <- read block on removed mmc
Can you add dump_stack() to the top of fat_sync_fs()? I hope it tells
why fat_sync_fs() is called (it is called from device unplug event?).
Well, that commit seems a bit strange. It calls fat_clusters_flush()
unconditionally without checking sb->s_dirt. However, if my guess is
right, "sync after removed event" itself sounds like the issue in
suspend process.
Thanks.
> 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.
--
OGAWA Hirofumi <[email protected]>
2009/9/4 OGAWA Hirofumi <[email protected]>:
> Christoph Hellwig <[email protected]> writes:
>
>> On Fri, Sep 04, 2009 at 12:29:04AM +0200, Zdenek Kabelac wrote:
>>> Ok - another bisect game played - and unexpected winner is:
>>>
>>> (fat: add ->sync_fs)
>>>
>>> f83d6d46e7adf241a064a4a425e5cd8a8fd8925f
>>>
>>> Reverting this commit with current -rc8 kernel makes the system happy
>>> during the suspend/resume cycle. Obviously it has it price :) so just
>>> plain revert is probably not a good solution so the problem looks
>>> 'more serious' ?(fat is not the only fs with this patch) thus adding
>>> original author to this thread.
>
> From it, I suspect the possible reason seems to read mmc after remove
> event. I.e. the following sequence or something
>
> ? ?sync fs process
> ? ?[...]
> ? ?removed mmc event
> ? ?[...]
> ? ?fat_sync_fs() ? ? ? ? ? ? ? ? ? <- sync again?
> ? ? ? ?fat_clusters_flush()
> ? ? ? ? ? ?sb_bread() ? ? ? ? ? ? ?<- read block on removed mmc
>
> Can you add dump_stack() to the top of fat_sync_fs()? I hope it tells
> why fat_sync_fs() is called (it is called from device unplug event?).
>
> Well, that commit seems a bit strange. It calls fat_clusters_flush()
> unconditionally without checking sb->s_dirt. However, if my guess is
> right, "sync after removed event" itself sounds like the issue in
> suspend process.
>
> Thanks.
>
>> 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.
Zdenek
usb 3-1: USB disconnect, address 2
btusb_intr_complete: hci0 urb ffff880137fdd630 failed to resubmit (19)
btusb_bulk_complete: hci0 urb ffff880137fdd738 failed to resubmit (19)
btusb_bulk_complete: hci0 urb ffff880137fdd840 failed to resubmit (19)
PM: Removing info for No Bus:ep_81
PM: Removing info for No Bus:ep_82
PM: Removing info for No Bus:ep_02
PM: Removing info for usb:3-1:1.0
btusb_send_frame: hci0 urb ffff88013356b528 submission failed
PM: Removing info for No Bus:rfkill0
PM: Removing info for No Bus:hci0
PM: Removing info for No Bus:ep_83
PM: Removing info for No Bus:ep_03
PM: Removing info for usb:3-1:1.1
PM: Removing info for No Bus:ep_84
PM: Removing info for No Bus:ep_04
PM: Removing info for usb:3-1:1.2
PM: Removing info for usb:3-1:1.3
PM: Removing info for No Bus:ep_00
PM: Removing info for usb:3-1
PM: Removing info for No Bus:usbdev3.2
PM: Adding info for No Bus:vcs63
PM: Adding info for No Bus:vcsa63
FAT: dump before lock
Pid: 2271, comm: sync Not tainted 2.6.31-rc8-00043-g54a3792 #32
Call Trace:
[<ffffffffa0537d94>] fat_sync_fs+0x24/0x80 [fat]
[<ffffffff81132206>] __sync_filesystem+0x36/0x50
[<ffffffff81132318>] sync_filesystems+0xf8/0x130
[<ffffffff811323a7>] sys_sync+0x17/0x40
[<ffffffff8100c15b>] system_call_fastpath+0x16/0x1b
FAT: fat_cluster_flush
FAT: before unlock
FAT: leaving fat_sync_fs
FAT: dump before lock
Pid: 2271, comm: sync Not tainted 2.6.31-rc8-00043-g54a3792 #32
Call Trace:
[<ffffffffa0537d94>] fat_sync_fs+0x24/0x80 [fat]
[<ffffffff81132206>] __sync_filesystem+0x36/0x50
[<ffffffff81132318>] sync_filesystems+0xf8/0x130
[<ffffffff811323b1>] sys_sync+0x21/0x40
[<ffffffff8100c15b>] system_call_fastpath+0x16/0x1b
FAT: fat_cluster_flush
FAT: before unlock
FAT: leaving fat_sync_fs
PM: Removing info for No Bus:iwl-phy0::assoc
PM: Syncing filesystems ...
FAT: dump before lock
Pid: 2143, comm: pm-suspend Not tainted 2.6.31-rc8-00043-g54a3792 #32
Call Trace:
[<ffffffffa0537d94>] fat_sync_fs+0x24/0x80 [fat]
[<ffffffff81132206>] __sync_filesystem+0x36/0x50
[<ffffffff81132318>] sync_filesystems+0xf8/0x130
[<ffffffff811323a7>] sys_sync+0x17/0x40
[<ffffffff8108d22b>] enter_state+0x6b/0x150
[<ffffffff8108c7f9>] state_store+0x99/0x100
[<ffffffff81226807>] kobj_attr_store+0x17/0x20
[<ffffffff8116e8f9>] sysfs_write_file+0xd9/0x160
[<ffffffff8110cc78>] vfs_write+0xb8/0x1a0
[<ffffffff8109eb8b>] ? audit_syscall_entry+0x28b/0x2b0
[<ffffffff8110d781>] sys_write+0x51/0x90
[<ffffffff8100c15b>] system_call_fastpath+0x16/0x1b
FAT: fat_cluster_flush
FAT: before unlock
FAT: leaving fat_sync_fs
PM: Removing info for No Bus:iwl-phy0::RX
PM: Removing info for No Bus:iwl-phy0::TX
PM: Removing info for No Bus:iwl-phy0::radio
FAT: dump before lock
Pid: 2143, comm: pm-suspend Not tainted 2.6.31-rc8-00043-g54a3792 #32
Call Trace:
[<ffffffffa0537d94>] fat_sync_fs+0x24/0x80 [fat]
[<ffffffff81132206>] __sync_filesystem+0x36/0x50
[<ffffffff81132318>] sync_filesystems+0xf8/0x130
[<ffffffff811323b1>] sys_sync+0x21/0x40
[<ffffffff8108d22b>] enter_state+0x6b/0x150
[<ffffffff8108c7f9>] state_store+0x99/0x100
[<ffffffff81226807>] kobj_attr_store+0x17/0x20
[<ffffffff8116e8f9>] sysfs_write_file+0xd9/0x160
[<ffffffff8110cc78>] vfs_write+0xb8/0x1a0
[<ffffffff8109eb8b>] ? audit_syscall_entry+0x28b/0x2b0
[<ffffffff8110d781>] sys_write+0x51/0x90
[<ffffffff8100c15b>] system_call_fastpath+0x16/0x1b
FAT: fat_cluster_flush
FAT: before unlock
FAT: leaving fat_sync_fs
done.
PM: Preparing system for mem sleep
Freezing user space processes ... (elapsed 0.00 seconds) done.
Freezing remaining freezable tasks ... (elapsed 0.00 seconds) done.
PM: Entering mem sleep
platform dock.0: preparing suspend
platform dock.1: preparing suspend
platform dock.2: preparing suspend
agpgart-intel 0000:00:00.0: preparing suspend
pci 0000:00:02.0: preparing suspend
pci 0000:00:02.1: preparing suspend
e1000e 0000:00:19.0: preparing suspend, may wakeup
uhci_hcd 0000:00:1a.0: preparing suspend
uhci_hcd 0000:00:1a.1: preparing suspend
ehci_hcd 0000:00:1a.7: preparing suspend
HDA Intel 0000:00:1b.0: preparing suspend
pcieport-driver 0000:00:1c.0: preparing suspend
pcieport-driver 0000:00:1c.1: preparing suspend
pcieport-driver 0000:00:1c.2: preparing suspend
pcieport-driver 0000:00:1c.3: preparing suspend
pcieport-driver 0000:00:1c.4: preparing suspend
uhci_hcd 0000:00:1d.0: preparing suspend
uhci_hcd 0000:00:1d.1: preparing suspend
uhci_hcd 0000:00:1d.2: preparing suspend
ehci_hcd 0000:00:1d.7: preparing suspend
pci 0000:00:1e.0: preparing suspend
pci 0000:00:1f.0: preparing suspend
ata_piix 0000:00:1f.1: preparing suspend
ahci 0000:00:1f.2: preparing suspend
i801_smbus 0000:00:1f.3: preparing suspend
iwl3945 0000:03:00.0: preparing suspend
pci 0000:15:00.0: preparing suspend
sdhci-pci 0000:15:00.2: preparing suspend
pci 0000:15:00.3: preparing suspend
pci 0000:15:00.4: preparing suspend
pci 0000:15:00.5: preparing suspend
platform vesafb.0: preparing suspend
serial8250 serial8250: preparing suspend
i8042 i8042: preparing suspend
platform hdaps: preparing suspend
usb usb1: preparing type suspend, may wakeup
usb usb2: preparing type suspend, may wakeup
usb usb3: preparing type suspend, may wakeup
usb usb4: preparing type suspend, may wakeup
usb usb5: preparing type suspend, may wakeup
usb usb6: preparing type suspend, may wakeup
usb usb7: preparing type suspend, may wakeup
usb 1-4: preparing type suspend, may wakeup
usb 3-2: preparing type suspend, may wakeup
usb 1-4.4: preparing type suspend, may wakeup
iTCO_wdt iTCO_wdt: preparing suspend
platform regulatory.0: preparing suspend
thinkpad_acpi thinkpad_acpi: preparing suspend
thinkpad_hwmon thinkpad_hwmon: preparing suspend
backlight acpi_video0: legacy class suspend
drm card0: legacy class suspend
pci 0000:00:02.0: power state changed by ACPI to D3
mmcblk mmc0:b368: legacy suspend
leds mmc0::: legacy class suspend
rfkill rfkill2: legacy class suspend
ieee80211 phy0: legacy class suspend
leds tpacpi::thinkvantage: legacy class suspend
leds tpacpi::standby: legacy class suspend
leds tpacpi::power: legacy class suspend
leds tpacpi::thinklight: legacy class suspend
rfkill rfkill1: legacy class suspend
thinkpad_hwmon thinkpad_hwmon: suspend
thinkpad_acpi thinkpad_acpi: suspend
psmouse serio2: legacy suspend
platform regulatory.0: suspend
iTCO_wdt iTCO_wdt: suspend
usb 1-4.4: type suspend, may wakeup
usb 3-2: type suspend, may wakeup
usb 1-4: type suspend, may wakeup
usb usb7: type suspend, may wakeup
usb usb6: type suspend, may wakeup
usb usb5: type suspend, may wakeup
usb usb4: type suspend, may wakeup
usb usb3: type suspend, may wakeup
usb usb2: type suspend, may wakeup
usb usb1: type suspend, may wakeup
sr 3:0:0:0: legacy suspend
scsi target3:0:0: legacy suspend
sd 0:0:0:0: legacy suspend
sd 0:0:0:0: [sda] Synchronizing SCSI cache
sd 0:0:0:0: [sda] Stopping disk
scsi target0:0:0: legacy suspend
platform hdaps: suspend
psmouse serio1: legacy suspend
atkbd serio0: legacy suspend
i8042 i8042: suspend
scsi host4: legacy suspend
scsi host3: legacy suspend
scsi host2: legacy suspend
scsi host1: legacy suspend
scsi host0: legacy suspend
serial8250 serial8250: suspend
platform vesafb.0: suspend
pnp 00:0a: legacy suspend
i8042 aux 00:09: legacy suspend
i8042 kbd 00:08: legacy suspend
rtc_cmos 00:07: legacy suspend, may wakeup
pnp 00:06: legacy suspend
pnp 00:05: legacy suspend
pnp 00:04: legacy suspend
pnp 00:03: legacy suspend
system 00:02: legacy suspend
pnp 00:01: legacy suspend
system 00:00: legacy suspend
pci 0000:15:00.5: suspend
pci 0000:15:00.4: suspend
pci 0000:15:00.3: suspend
sdhci-pci 0000:15:00.2: suspend
mmc0: card b368 removed
PM: Removing info for mmc:mmc0:b368
FAT: dump before lock
Pid: 2143, comm: pm-suspend Not tainted 2.6.31-rc8-00043-g54a3792 #32
Call Trace:
[<ffffffffa0537d94>] fat_sync_fs+0x24/0x80 [fat]
[<ffffffff81132206>] __sync_filesystem+0x36/0x50
[<ffffffff8113240a>] sync_filesystem+0x3a/0x70
[<ffffffff8113b8ce>] fsync_bdev+0x2e/0x70
[<ffffffff8121b3ce>] invalidate_partition+0x2e/0x50
[<ffffffff81169d2f>] del_gendisk+0x3f/0x140
[<ffffffffa02791ee>] mmc_blk_remove+0x2e/0x60 [mmc_block]
[<ffffffffa022b2f7>] mmc_bus_remove+0x17/0x20 [mmc_core]
[<ffffffff812d2036>] __device_release_driver+0x66/0xc0
[<ffffffff812d219d>] device_release_driver+0x2d/0x40
[<ffffffff812d112c>] bus_remove_device+0xac/0xe0
[<ffffffff812cf2af>] device_del+0x12f/0x1a0
[<ffffffffa022b3db>] mmc_remove_card+0x5b/0x90 [mmc_core]
[<ffffffffa022d097>] mmc_sd_remove+0x27/0x50 [mmc_core]
[<ffffffffa022ae9d>] mmc_suspend_host+0xed/0x120 [mmc_core]
[<ffffffffa023bda8>] sdhci_suspend_host+0x38/0x60 [sdhci]
[<ffffffffa025d270>] sdhci_pci_suspend+0x50/0x130 [sdhci_pci]
[<ffffffff81242c3d>] pci_legacy_suspend+0x4d/0xf0
[<ffffffff8124369d>] pci_pm_suspend+0xdd/0x130
[<ffffffff812d660b>] pm_op+0x15b/0x1b0
[<ffffffff812d7763>] dpm_suspend_start+0x423/0x580
[<ffffffff8108d04f>] suspend_devices_and_enter+0x5f/0x1d0
[<ffffffff8108d2e8>] enter_state+0x128/0x150
[<ffffffff8108c7f9>] state_store+0x99/0x100
[<ffffffff81226807>] kobj_attr_store+0x17/0x20
[<ffffffff8116e8f9>] sysfs_write_file+0xd9/0x160
[<ffffffff8110cc78>] vfs_write+0xb8/0x1a0
[<ffffffff8109eb8b>] ? audit_syscall_entry+0x28b/0x2b0
[<ffffffff8110d781>] sys_write+0x51/0x90
[<ffffffff8100c15b>] system_call_fastpath+0x16/0x1b
FAT: fat_cluster_flush
FAT: before unlock
FAT: leaving fat_sync_fs
FAT: dump before lock
Pid: 2143, comm: pm-suspend Not tainted 2.6.31-rc8-00043-g54a3792 #32
Call Trace:
[<ffffffffa0537d94>] fat_sync_fs+0x24/0x80 [fat]
[<ffffffff81132206>] __sync_filesystem+0x36/0x50
[<ffffffff8113241b>] sync_filesystem+0x4b/0x70
[<ffffffff8113b8ce>] fsync_bdev+0x2e/0x70
[<ffffffff8121b3ce>] invalidate_partition+0x2e/0x50
[<ffffffff81169d2f>] del_gendisk+0x3f/0x140
[<ffffffffa02791ee>] mmc_blk_remove+0x2e/0x60 [mmc_block]
[<ffffffffa022b2f7>] mmc_bus_remove+0x17/0x20 [mmc_core]
[<ffffffff812d2036>] __device_release_driver+0x66/0xc0
[<ffffffff812d219d>] device_release_driver+0x2d/0x40
[<ffffffff812d112c>] bus_remove_device+0xac/0xe0
[<ffffffff812cf2af>] device_del+0x12f/0x1a0
[<ffffffffa022b3db>] mmc_remove_card+0x5b/0x90 [mmc_core]
[<ffffffffa022d097>] mmc_sd_remove+0x27/0x50 [mmc_core]
[<ffffffffa022ae9d>] mmc_suspend_host+0xed/0x120 [mmc_core]
[<ffffffffa023bda8>] sdhci_suspend_host+0x38/0x60 [sdhci]
[<ffffffffa025d270>] sdhci_pci_suspend+0x50/0x130 [sdhci_pci]
[<ffffffff81242c3d>] pci_legacy_suspend+0x4d/0xf0
[<ffffffff8124369d>] pci_pm_suspend+0xdd/0x130
[<ffffffff812d660b>] pm_op+0x15b/0x1b0
[<ffffffff812d7763>] dpm_suspend_start+0x423/0x580
[<ffffffff8108d04f>] suspend_devices_and_enter+0x5f/0x1d0
[<ffffffff8108d2e8>] enter_state+0x128/0x150
[<ffffffff8108c7f9>] state_store+0x99/0x100
[<ffffffff81226807>] kobj_attr_store+0x17/0x20
[<ffffffff8116e8f9>] sysfs_write_file+0xd9/0x160
[<ffffffff8110cc78>] vfs_write+0xb8/0x1a0
[<ffffffff8109eb8b>] ? audit_syscall_entry+0x28b/0x2b0
[<ffffffff8110d781>] sys_write+0x51/0x90
[<ffffffff8100c15b>] system_call_fastpath+0x16/0x1b
FAT: fat_cluster_flush
FAT: before unlock
FAT: leaving fat_sync_fs
Zdenek Kabelac <[email protected]> writes:
> 2009/9/4 OGAWA Hirofumi <[email protected]>:
>> Christoph Hellwig <[email protected]> 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.
--
OGAWA Hirofumi <[email protected]>
Signed-off-by: OGAWA Hirofumi <[email protected]>
---
fs/fat/fat.h | 2 +-
fs/fat/inode.c | 14 +++++++++-----
fs/fat/misc.c | 8 +++++---
3 files changed, 15 insertions(+), 9 deletions(-)
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);
_
2009/9/5 OGAWA Hirofumi <[email protected]>:
> Zdenek Kabelac <[email protected]> writes:
>
>> 2009/9/4 OGAWA Hirofumi <[email protected]>:
>>> Christoph Hellwig <[email protected]> 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
Zdenek Kabelac <[email protected]> writes:
> 2009/9/5 OGAWA Hirofumi <[email protected]>:
>> Zdenek Kabelac <[email protected]> writes:
>>
>>> 2009/9/4 OGAWA Hirofumi <[email protected]>:
>>>> Christoph Hellwig <[email protected]> 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 <[email protected]>
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?
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
2009/9/6 OGAWA Hirofumi <[email protected]>:
> Zdenek Kabelac <[email protected]> writes:
>
>> 2009/9/5 OGAWA Hirofumi <[email protected]>:
>>> Zdenek Kabelac <[email protected]> writes:
>>>
>>>> 2009/9/4 OGAWA Hirofumi <[email protected]>:
>>>>> Christoph Hellwig <[email protected]> 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 <[email protected]>
Just a side note - Could be there any connection with my previous report?
http://lkml.org/lkml/2009/8/28/90
Zdenek
On Fri, Sep 04, 2009 at 09:47:46AM +0900, OGAWA Hirofumi wrote:
> Well, that commit seems a bit strange. It calls fat_clusters_flush()
> unconditionally without checking sb->s_dirt. However, if my guess is
> right, "sync after removed event" itself sounds like the issue in
> suspend process.
The idea of ->sync_fs is that we always perform the sync activity,
and not just the usual background superblock writeback trigerred by
s_dirt. If FAT doesn't need that and never has races around s_dirt
you can add the check back, but I would recommend against it.
Also when you hack around this in FAt MMC will still fail with every
other filesystem.
On Tuesday 08 September 2009, Christoph Hellwig wrote:
> On Fri, Sep 04, 2009 at 09:47:46AM +0900, OGAWA Hirofumi wrote:
> > Well, that commit seems a bit strange. It calls fat_clusters_flush()
> > unconditionally without checking sb->s_dirt. However, if my guess is
> > right, "sync after removed event" itself sounds like the issue in
> > suspend process.
>
> The idea of ->sync_fs is that we always perform the sync activity,
> and not just the usual background superblock writeback trigerred by
> s_dirt. If FAT doesn't need that and never has races around s_dirt
> you can add the check back, but I would recommend against it.
>
> Also when you hack around this in FAt MMC will still fail with every
> other filesystem.
So, what should be done in your opinion?
Rafael
Zdenek Kabelac <[email protected]> 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 <[email protected]>
Pavel Machek <[email protected]> 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.
--
OGAWA Hirofumi <[email protected]>
Christoph Hellwig <[email protected]> writes:
> On Fri, Sep 04, 2009 at 09:47:46AM +0900, OGAWA Hirofumi wrote:
>> Well, that commit seems a bit strange. It calls fat_clusters_flush()
>> unconditionally without checking sb->s_dirt. However, if my guess is
>> right, "sync after removed event" itself sounds like the issue in
>> suspend process.
>
> The idea of ->sync_fs is that we always perform the sync activity,
> and not just the usual background superblock writeback trigerred by
> s_dirt. If FAT doesn't need that and never has races around s_dirt
> you can add the check back, but I would recommend against it.
I'm not sure the detail of your idea of ->sync_fs. "always perform" is
not the goal of it, right? Anyway, we should consider about unnecessary
write reduces the lifetime of flash base device.
And what races of s_dirt? ("always perform" fixed those? and why we
gave up to fix the real problems or root-casue?) Maybe, I already
noticed one of those, but I may not be noticing all of those. If you
can explain the detail of those known problems, I appreciate and would
be useful.
And write_super() of FAT doesn't affect to fs consistency, it's one of
reasons why I moved it to write_super().
Thanks.
--
OGAWA Hirofumi <[email protected]>
On Wed 2009-09-09 22:21:56, OGAWA Hirofumi wrote:
> Pavel Machek <[email protected]> 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?
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel Machek <[email protected]> writes:
> On Wed 2009-09-09 22:21:56, OGAWA Hirofumi wrote:
>> Pavel Machek <[email protected]> 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.
--
OGAWA Hirofumi <[email protected]>
> 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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
2009/9/11 Pavel Machek <[email protected]>:
>
>> 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
> 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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
2009/9/11 Pavel Machek <[email protected]>:
>
>> 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
On Fri 2009-09-11 23:45:01, Zdenek Kabelac wrote:
> 2009/9/11 Pavel Machek <[email protected]>:
> >
> >> 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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Friday 11 September 2009, OGAWA Hirofumi wrote:
> Pavel Machek <[email protected]> writes:
>
> > On Wed 2009-09-09 22:21:56, OGAWA Hirofumi wrote:
> >> Pavel Machek <[email protected]> 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
On Sat 2009-09-12 00:04:02, Rafael J. Wysocki wrote:
> On Friday 11 September 2009, OGAWA Hirofumi wrote:
> > Pavel Machek <[email protected]> writes:
> >
> > > On Wed 2009-09-09 22:21:56, OGAWA Hirofumi wrote:
> > >> Pavel Machek <[email protected]> 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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Friday 11 September 2009, Pavel Machek wrote:
> On Fri 2009-09-11 23:45:01, Zdenek Kabelac wrote:
> > 2009/9/11 Pavel Machek <[email protected]>:
> > >
> > >> 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
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 <[email protected]>
One Laptop Per Child
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.
--
Chris Ball <[email protected]>
One Laptop Per Child
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 <[email protected]> writes:
> > >
> > > > On Wed 2009-09-09 22:21:56, OGAWA Hirofumi wrote:
> > > >> Pavel Machek <[email protected]> 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
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
2009/9/12 Rafael J. Wysocki <[email protected]>:
> 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
On Monday 14 September 2009, Zdenek Kabelac wrote:
> 2009/9/12 Rafael J. Wysocki <[email protected]>:
> > 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
On Fri, 11 Sep 2009 23:51:17 +0200
Pavel Machek <[email protected]> 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
--
-- Pierre Ossman
WARNING: This correspondence is being monitored by the
Swedish government. Make sure your server uses encryption
for SMTP traffic and consider using PGP for end-to-end
encryption.
On Mon 2009-09-14 22:05:10, Pierre Ossman wrote:
> On Fri, 11 Sep 2009 23:51:17 +0200
> Pavel Machek <[email protected]> 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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon 2009-09-14 10:39:44, Zdenek Kabelac wrote:
> 2009/9/12 Rafael J. Wysocki <[email protected]>:
> > 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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
"Rafael J. Wysocki" <[email protected]> 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.
--
OGAWA Hirofumi <[email protected]>
On Friday 18 September 2009, OGAWA Hirofumi wrote:
> "Rafael J. Wysocki" <[email protected]> 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