* Re: interesting MD-xfs bug
From: Joe Landman @ 2015-04-09 22:20 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs, linux-raid
In-Reply-To: <20150409221846.GG13731@dastard>
On 04/09/2015 06:18 PM, Dave Chinner wrote:
> On Thu, Apr 09, 2015 at 05:02:33PM -0400, Joe Landman wrote:
>> If I build an MD raid0 with a non power of 2 chunk size, it appears
>> that I can mkfs.xfs a file system, but it doesn't show up in blkid
>> and is not mountable. Yet, using a power of 2 chunk size, this does
>> work correctly. This is kernel 3.18.9.
>>
[...]
> That looks more like a blkid or udev problem. try using blkid -p so
> that it doesn't look up the cache but directly probes devices for
> the signatures. strace might tell you a bit more, too. And if the
> filesystem mounts, then it definitely isn't an XFS problem ;)
Thats the thing, it didn't mount, even when I used the device name
directly.
Good point on stracing though. I'll do that tomorrow and report back.
Thanks!
Joe
>
> Cheers,
>
> Dave.
--
Joe Landman
e: joe.landman@gmail.com
t: @sijoe
^ permalink raw reply
* Re: interesting MD-xfs bug
From: Dave Chinner @ 2015-04-09 22:18 UTC (permalink / raw)
To: Joe Landman; +Cc: xfs, linux-raid
In-Reply-To: <5526E8E9.3030805@gmail.com>
On Thu, Apr 09, 2015 at 05:02:33PM -0400, Joe Landman wrote:
> If I build an MD raid0 with a non power of 2 chunk size, it appears
> that I can mkfs.xfs a file system, but it doesn't show up in blkid
> and is not mountable. Yet, using a power of 2 chunk size, this does
> work correctly. This is kernel 3.18.9.
>
>
> For example, non-power of 2 chunk:
>
> root@unison:~# wipefs -a /dev/sdb
> 4 bytes were erased at offset 0x1000 (linux_raid_member)
> they were: fc 4e 2b a9
> root@unison:~# wipefs -a /dev/sda
> 4 bytes were erased at offset 0x1000 (linux_raid_member)
> they were: fc 4e 2b a9
> root@unison:~# mdadm --create /dev/md20 --level=0 --metadata=1.2
> --chunk=1152 --auto=yes --raid-disks=2 /dev/sd[ab]
> mdadm: array /dev/md20 started.
>
> root@unison:~# mkfs.xfs /dev/md20
> log stripe unit (1179648 bytes) is too large (maximum is 256KiB)
> log stripe unit adjusted to 32KiB
> meta-data=/dev/md20 isize=256 agcount=50,
> agsize=268435296 blks
> = sectsz=512 attr=2, projid32bit=0
> data = bsize=4096 blocks=13164865984, imaxpct=5
> = sunit=288 swidth=576 blks
> naming =version 2 bsize=4096 ascii-ci=0
> log =internal log bsize=4096 blocks=521728, version=2
> = sectsz=512 sunit=8 blks, lazy-count=1
> realtime =none extsz=4096 blocks=0, rtextents=0
>
> root@unison:~# blkid | grep xfs
That looks more like a blkid or udev problem. try using blkid -p so
that it doesn't look up the cache but directly probes devices for
the signatures. strace might tell you a bit more, too. And if the
filesystem mounts, then it definitely isn't an XFS problem ;)
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* interesting MD-xfs bug
From: Joe Landman @ 2015-04-09 21:02 UTC (permalink / raw)
To: xfs, linux-raid
If I build an MD raid0 with a non power of 2 chunk size, it appears that
I can mkfs.xfs a file system, but it doesn't show up in blkid and is not
mountable. Yet, using a power of 2 chunk size, this does work
correctly. This is kernel 3.18.9.
For example, non-power of 2 chunk:
root@unison:~# wipefs -a /dev/sdb
4 bytes were erased at offset 0x1000 (linux_raid_member)
they were: fc 4e 2b a9
root@unison:~# wipefs -a /dev/sda
4 bytes were erased at offset 0x1000 (linux_raid_member)
they were: fc 4e 2b a9
root@unison:~# mdadm --create /dev/md20 --level=0 --metadata=1.2
--chunk=1152 --auto=yes --raid-disks=2 /dev/sd[ab]
mdadm: array /dev/md20 started.
root@unison:~# mkfs.xfs /dev/md20
log stripe unit (1179648 bytes) is too large (maximum is 256KiB)
log stripe unit adjusted to 32KiB
meta-data=/dev/md20 isize=256 agcount=50,
agsize=268435296 blks
= sectsz=512 attr=2, projid32bit=0
data = bsize=4096 blocks=13164865984, imaxpct=5
= sunit=288 swidth=576 blks
naming =version 2 bsize=4096 ascii-ci=0
log =internal log bsize=4096 blocks=521728, version=2
= sectsz=512 sunit=8 blks, lazy-count=1
realtime =none extsz=4096 blocks=0, rtextents=0
root@unison:~# blkid | grep xfs
root@unison:~#
Same system, with power of 2 chunk size:
root@unison:~# mdadm -S /dev/md20
mdadm: stopped /dev/md20
root@unison:~# wipefs -a /dev/sda
4 bytes were erased at offset 0x1000 (linux_raid_member)
they were: fc 4e 2b a9
root@unison:~# wipefs -a /dev/sdb
4 bytes were erased at offset 0x1000 (linux_raid_member)
they were: fc 4e 2b a9
root@unison:~# mdadm --create /dev/md20 --level=0 --metadata=1.2
--chunk=1024 --auto=yes --raid-disks=2 /dev/sd[ab]
mdadm: array /dev/md20 started.
root@unison:~# mkfs.xfs /dev/md20
log stripe unit (1048576 bytes) is too large (maximum is 256KiB)
log stripe unit adjusted to 32KiB
meta-data=/dev/md20 isize=256 agcount=50,
agsize=268435200 blks
= sectsz=512 attr=2, projid32bit=0
data = bsize=4096 blocks=13164866048, imaxpct=5
= sunit=256 swidth=512 blks
naming =version 2 bsize=4096 ascii-ci=0
log =internal log bsize=4096 blocks=521728, version=2
= sectsz=512 sunit=8 blks, lazy-count=1
realtime =none extsz=4096 blocks=0, rtextents=0
root@unison:~# blkid | grep xfs
/dev/md20: UUID="5e965ae7-198e-4e58-8920-a65c4b6bbe60" TYPE="xfs"
I am not sure which code base might be at "fault" or even if there is a
"fault" (beyond simply saying "don't do non-power-of-two chunks"). If
its the latter, happy to work on a warning message patch for mdadm if
needed. If it should work, then happy to poke around if someone can
give me a pointer where something might be relevant.
^ permalink raw reply
* md: NULL ptr deref on xfstests generic/040
From: Sasha Levin @ 2015-04-09 19:37 UTC (permalink / raw)
To: linux-raid; +Cc: neilb, LKML
Hi all,
I'm seeing the following warnings and NULL ptr deref when running xfstest generic/040
on the latest -next kernel.
[ 7023.673973] run fstests generic/040 at 2015-04-09 10:31:57
[ 7025.777329] kobject: 'sdd' (ffff8837b7c5e0a8): kobject_uevent_env
[ 7025.777344] kobject: 'sdd' (ffff8837b7c5e0a8): fill_kobj_path: path = '/devices/pci0000:00/0000:00:03.2/0000:50:00.0/host0/target0:2:3/0:2:3:0/block/sdd'
[ 7025.969112] kobject: '251:0' (ffff881ff2554810): kobject_add_internal: parent: 'bdi', set: 'devices'
[ 7025.969161] ------------[ cut here ]------------
[ 7025.969181] WARNING: CPU: 7 PID: 30467 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x86/0xa0()
[ 7025.969187] sysfs: cannot create duplicate filename '/devices/virtual/bdi/251:0'
[ 7025.969192] Modules linked in: dm_flakey intel_rapl ast iosf_mbi x86_pkg_temp_thermal ttm intel_powerclamp coretemp drm_kms_helper kvm_intel kvm drm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel aes_x86_64 lrw glue_helper ablk_helper cryptd joydev i2c_algo_bit syscopyarea sysfillrect sysimgblt ipmi_si sb_edac ipmi_msghandler edac_core ioatdma shpchp lpc_ich mac_hid btrfs xor mlx4_en vxlan raid6_pq hid_generic usbhid hid ixgbe mlx4_core ahci dca ptp libahci megaraid_sas pps_core mdio
[ 7025.969328] CPU: 7 PID: 30467 Comm: dmsetup Not tainted 4.0.0-rc7-next-20150408+ #6
[ 7025.969335] Hardware name: Oracle Corporation OVCA X3-2 /ASSY,MOTHERBOARD,1U , BIOS 17021300 06/19/2012
[ 7025.969342] ffffffff82b37a40 ffff881fda8073f8 ffffffff82947148 0000000000000000
[ 7025.969354] ffff881fda807478 ffff881fda807448 ffffffff8115a04a 0000000000000001
[ 7025.969365] ffffffff81770d56 ffff881fda807498 ffffed03fb500e8b ffff881ff2533a30
[ 7025.969376] Call Trace:
[ 7025.969389] dump_stack (lib/dump_stack.c:52)
[ 7025.969400] warn_slowpath_common (kernel/panic.c:447)
[ 7025.969410] ? sysfs_warn_dup (fs/sysfs/dir.c:33)
[ 7025.969418] warn_slowpath_fmt (kernel/panic.c:453)
[ 7025.969427] ? warn_slowpath_common (kernel/panic.c:453)
[ 7025.969439] ? trace_hardirqs_on (kernel/locking/lockdep.c:2630)
[ 7025.969448] sysfs_warn_dup (fs/sysfs/dir.c:33)
[ 7025.969458] sysfs_create_dir_ns (fs/sysfs/dir.c:59)
[ 7025.969471] kobject_add_internal (lib/kobject.c:72 lib/kobject.c:229)
[ 7025.969481] ? debug_lockdep_rcu_enabled (kernel/rcu/update.c:195)
[ 7025.969493] ? __mutex_unlock_slowpath (./arch/x86/include/asm/paravirt.h:809 kernel/locking/mutex.c:755 kernel/locking/mutex.c:766)
[ 7025.969502] kobject_add (lib/kobject.c:384)
[ 7025.969509] ? kobject_add_internal (lib/kobject.c:384)
[ 7025.969518] ? mutex_unlock (kernel/locking/mutex.c:444)
[ 7025.969532] device_add (drivers/base/core.c:1025)
[ 7025.969541] ? device_private_init (drivers/base/core.c:977)
[ 7025.969554] ? kfree (include/trace/events/kmem.h:136 mm/slub.c:3422)
[ 7025.969564] device_create_groups_vargs (drivers/base/core.c:1618)
[ 7025.969572] ? debug_check_no_locks_freed (kernel/locking/lockdep.c:3091)
[ 7025.969581] device_create_vargs (drivers/base/core.c:1660)
[ 7025.969592] bdi_register (mm/backing-dev.c:347)
[ 7025.969600] ? wait_iff_congested (mm/backing-dev.c:337)
[ 7025.969609] ? vsnprintf (lib/vsprintf.c:2008)
[ 7025.969617] bdi_register_dev (mm/backing-dev.c:367)
[ 7025.969629] add_disk (block/genhd.c:616)
[ 7025.969636] ? pointer.isra.23 (lib/vsprintf.c:1878)
[ 7025.969644] ? lockdep_init_map_type (kernel/locking/lockdep.c:3009)
[ 7025.969651] ? trace_hardirqs_on (kernel/locking/lockdep.c:2630)
[ 7025.969660] ? blk_alloc_devt (block/genhd.c:583)
[ 7025.969667] ? sprintf (lib/vsprintf.c:2138)
[ 7025.969673] ? scnprintf (lib/vsprintf.c:2138)
[ 7025.969682] ? lockdep_init_map (kernel/locking/lockdep.c:3041)
[ 7025.969692] dm_create (drivers/md/dm.c:2318 drivers/md/dm.c:2598)
[ 7025.969701] dev_create (drivers/md/dm-ioctl.c:747)
[ 7025.969709] ? list_version_get_info (drivers/md/dm-ioctl.c:735)
[ 7025.969716] ctl_ioctl (drivers/md/dm-ioctl.c:1848)
[ 7025.969726] ? semctl_main (ipc/sem.c:1330)
[ 7025.969734] ? debug_lockdep_rcu_enabled (kernel/rcu/update.c:195)
[ 7025.969741] ? list_version_get_info (drivers/md/dm-ioctl.c:735)
[ 7025.969751] ? free_params (drivers/md/dm-ioctl.c:1793)
[ 7025.969760] ? SYSC_semtimedop (ipc/sem.c:2010)
[ 7025.969771] dm_ctl_ioctl (drivers/md/dm-ioctl.c:1866)
[ 7025.969783] do_vfs_ioctl (fs/ioctl.c:44 fs/ioctl.c:607)
[ 7025.969792] ? ioctl_preallocate (fs/ioctl.c:557)
[ 7025.969803] ? mntput (fs/namespace.c:1106)
[ 7025.969812] ? SyS_semctl (ipc/sem.c:1601 ipc/sem.c:1577)
[ 7025.969820] ? debug_lockdep_rcu_enabled (kernel/rcu/update.c:195)
[ 7025.969828] ? __fget_light (fs/file.c:684)
[ 7025.969836] SyS_ioctl (fs/ioctl.c:622 fs/ioctl.c:613)
[ 7025.969845] system_call_fastpath (arch/x86/kernel/entry_64.S:261)
[ 7025.969853] ---[ end trace 734c93b316c19e43 ]---
[ 7025.969862] ------------[ cut here ]------------
[ 7025.969873] WARNING: CPU: 7 PID: 30467 at lib/kobject.c:240 kobject_add_internal+0x6ff/0x920()
[ 7025.969879] kobject_add_internal failed for 251:0 with -EEXIST, don't try to register things with the same name in the same directory.
[ 7025.969883] Modules linked in: dm_flakey intel_rapl ast iosf_mbi x86_pkg_temp_thermal ttm intel_powerclamp coretemp drm_kms_helper kvm_intel kvm drm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel aes_x86_64 lrw glue_helper ablk_helper cryptd joydev i2c_algo_bit syscopyarea sysfillrect sysimgblt ipmi_si sb_edac ipmi_msghandler edac_core ioatdma shpchp lpc_ich mac_hid btrfs xor mlx4_en vxlan raid6_pq hid_generic usbhid hid ixgbe mlx4_core ahci dca ptp libahci megaraid_sas pps_core mdio
[ 7025.969997] CPU: 7 PID: 30467 Comm: dmsetup Tainted: G W 4.0.0-rc7-next-20150408+ #6
[ 7025.970003] Hardware name: Oracle Corporation OVCA X3-2 /ASSY,MOTHERBOARD,1U , BIOS 17021300 06/19/2012
[ 7025.970008] ffffffff82bc7b60 ffff881fda807458 ffffffff82947148 0000000000000000
[ 7025.970019] ffff881fda8074d8 ffff881fda8074a8 ffffffff8115a04a 0000000000000001
[ 7025.970029] ffffffff81bdf32f ffff881fda8074f8 ffffed03fb500e97 00000000ffffffef
[ 7025.970040] Call Trace:
[ 7025.970048] dump_stack (lib/dump_stack.c:52)
[ 7025.970056] warn_slowpath_common (kernel/panic.c:447)
[ 7025.970064] ? kobject_add_internal (lib/kobject.c:237 (discriminator 1))
[ 7025.970072] warn_slowpath_fmt (kernel/panic.c:453)
[ 7025.970080] ? warn_slowpath_common (kernel/panic.c:453)
[ 7025.970089] ? _raw_spin_unlock (./arch/x86/include/asm/preempt.h:77 include/linux/spinlock_api_smp.h:154 kernel/locking/spinlock.c:183)
[ 7025.970097] kobject_add_internal (lib/kobject.c:237 (discriminator 1))
[ 7025.970105] ? debug_lockdep_rcu_enabled (kernel/rcu/update.c:195)
[ 7025.970113] ? __mutex_unlock_slowpath (./arch/x86/include/asm/paravirt.h:809 kernel/locking/mutex.c:755 kernel/locking/mutex.c:766)
[ 7025.970121] kobject_add (lib/kobject.c:384)
[ 7025.970129] ? kobject_add_internal (lib/kobject.c:384)
[ 7025.970137] ? mutex_unlock (kernel/locking/mutex.c:444)
[ 7025.970146] device_add (drivers/base/core.c:1025)
[ 7025.970155] ? device_private_init (drivers/base/core.c:977)
[ 7025.970164] ? kfree (include/trace/events/kmem.h:136 mm/slub.c:3422)
[ 7025.970173] device_create_groups_vargs (drivers/base/core.c:1618)
[ 7025.970181] ? debug_check_no_locks_freed (kernel/locking/lockdep.c:3091)
[ 7025.970190] device_create_vargs (drivers/base/core.c:1660)
[ 7025.970197] bdi_register (mm/backing-dev.c:347)
[ 7025.970205] ? wait_iff_congested (mm/backing-dev.c:337)
[ 7025.970213] ? vsnprintf (lib/vsprintf.c:2008)
[ 7025.970221] bdi_register_dev (mm/backing-dev.c:367)
[ 7025.970229] add_disk (block/genhd.c:616)
[ 7025.970235] ? pointer.isra.23 (lib/vsprintf.c:1878)
[ 7025.970243] ? lockdep_init_map_type (kernel/locking/lockdep.c:3009)
[ 7025.970250] ? trace_hardirqs_on (kernel/locking/lockdep.c:2630)
[ 7025.970259] ? blk_alloc_devt (block/genhd.c:583)
[ 7025.970265] ? sprintf (lib/vsprintf.c:2138)
[ 7025.970271] ? scnprintf (lib/vsprintf.c:2138)
[ 7025.970280] ? lockdep_init_map (kernel/locking/lockdep.c:3041)
[ 7025.970287] dm_create (drivers/md/dm.c:2318 drivers/md/dm.c:2598)
[ 7025.970295] dev_create (drivers/md/dm-ioctl.c:747)
[ 7025.970303] ? list_version_get_info (drivers/md/dm-ioctl.c:735)
[ 7025.970310] ctl_ioctl (drivers/md/dm-ioctl.c:1848)
[ 7025.970318] ? semctl_main (ipc/sem.c:1330)
[ 7025.970326] ? debug_lockdep_rcu_enabled (kernel/rcu/update.c:195)
[ 7025.970333] ? list_version_get_info (drivers/md/dm-ioctl.c:735)
[ 7025.970341] ? free_params (drivers/md/dm-ioctl.c:1793)
[ 7025.970350] ? SYSC_semtimedop (ipc/sem.c:2010)
[ 7025.970361] dm_ctl_ioctl (drivers/md/dm-ioctl.c:1866)
[ 7025.970369] do_vfs_ioctl (fs/ioctl.c:44 fs/ioctl.c:607)
[ 7025.970377] ? ioctl_preallocate (fs/ioctl.c:557)
[ 7025.970385] ? mntput (fs/namespace.c:1106)
[ 7025.970393] ? SyS_semctl (ipc/sem.c:1601 ipc/sem.c:1577)
[ 7025.970402] ? debug_lockdep_rcu_enabled (kernel/rcu/update.c:195)
[ 7025.970409] ? __fget_light (fs/file.c:684)
[ 7025.970417] SyS_ioctl (fs/ioctl.c:622 fs/ioctl.c:613)
[ 7025.970426] system_call_fastpath (arch/x86/kernel/entry_64.S:261)
[ 7025.970433] ---[ end trace 734c93b316c19e44 ]---
[ 7025.970445] kobject: '251:0' (ffff881ff2554810): kobject_release, parent (null) (delayed 100)
[ 7025.970484] kobject: 'dm-0' (ffff881ff25578a8): kobject_add_internal: parent: 'block', set: 'devices'
[ 7025.970968] kobject: 'dm-0' (ffff881ff25578a8): kobject_uevent_env
[ 7025.970976] kobject: 'dm-0' (ffff881ff25578a8): kobject_uevent_env: uevent_suppress caused the event to drop!
[ 7025.971006] kobject: 'holders' (ffff881fea16ae00): kobject_add_internal: parent: 'dm-0', set: '<NULL>'
[ 7025.971022] kobject: 'slaves' (ffff881fea16ac00): kobject_add_internal: parent: 'dm-0', set: '<NULL>'
[ 7025.971034] kobject: 'dm-0' (ffff881ff25578a8): kobject_uevent_env
[ 7025.971044] kobject: 'dm-0' (ffff881ff25578a8): fill_kobj_path: path = '/devices/virtual/block/dm-0'
[ 7025.971141] kobject: 'queue' (ffff881fe5f007a8): kobject_add_internal: parent: 'dm-0', set: '<NULL>'
[ 7025.971286] kobject: 'queue' (ffff881fe5f007a8): kobject_uevent_env
[ 7025.971291] kobject: 'queue' (ffff881fe5f007a8): kobject_uevent_env: filter function caused the event to drop!
[ 7025.971307] CONFIG_KASAN_INLINE enabled
[ 7025.971382] GPF could be caused by NULL-ptr deref or user memory accessgeneral protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN
[ 7025.971571] Modules linked in: dm_flakey intel_rapl ast iosf_mbi x86_pkg_temp_thermal ttm intel_powerclamp coretemp drm_kms_helper kvm_intel kvm drm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel aes_x86_64 lrw glue_helper ablk_helper cryptd joydev i2c_algo_bit syscopyarea sysfillrect sysimgblt ipmi_si sb_edac ipmi_msghandler edac_core ioatdma shpchp lpc_ich mac_hid btrfs xor mlx4_en vxlan raid6_pq hid_generic usbhid hid ixgbe mlx4_core ahci dca ptp libahci megaraid_sas pps_core mdio
[ 7025.972551] CPU: 23 PID: 30467 Comm: dmsetup Tainted: G W 4.0.0-rc7-next-20150408+ #6
[ 7025.972770] Hardware name: Oracle Corporation OVCA X3-2 /ASSY,MOTHERBOARD,1U , BIOS 17021300 06/19/2012
[ 7025.973033] task: ffff881ff0878000 ti: ffff881fda800000 task.ti: ffff881fda800000
[ 7025.973213] RIP: sysfs_do_create_link_sd.isra.2 (fs/sysfs/symlink.c:35)
[ 7025.973475] RSP: 0018:ffff881fda807878 EFLAGS: 00010202
[ 7025.973615] RAX: dffffc0000000000 RBX: 0000000000000040 RCX: 000000001a561a54
[ 7025.973802] RDX: 0000000000000008 RSI: 00000000000000db RDI: ffffffff833dd704
[ 7025.973982] RBP: ffff881fda8078a8 R08: 0000000000000000 R09: 0000000000000000
[ 7025.974163] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
[ 7025.974348] R13: ffffffff82bbc720 R14: ffff881fe5ffd400 R15: ffff881fe5f00000
[ 7025.974532] FS: 00007fc654bcc840(0000) GS:ffff881fffdc0000(0000) knlGS:0000000000000000
[ 7025.974745] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 7025.974874] CR2: 00007fed40c85798 CR3: 0000001fe0ab5000 CR4: 00000000000407e0
[ 7025.974972] Stack:
[ 7025.975003] ffff881fda807888 ffff881ff25578a8 0000000000000001 ffff881ff2557800
[ 7025.975119] ffff881fda8079e8 ffff881fe5f00000 ffff881fda8078d8 ffffffff81771658
[ 7025.975234] ffff881fda8078d8 ffffffff81b25cb6 ffff881fda8079e8 ffff881fda8079a8
[ 7025.975349] Call Trace:
[ 7025.975393] sysfs_create_link (fs/sysfs/symlink.c:93)
[ 7025.975474] ? blk_get_queue (block/blk-core.c:794)
[ 7025.975552] add_disk (block/genhd.c:629 (discriminator 8))
[ 7025.975625] ? lockdep_init_map_type (kernel/locking/lockdep.c:3009)
[ 7025.975715] ? trace_hardirqs_on (kernel/locking/lockdep.c:2630)
[ 7025.975798] ? blk_alloc_devt (block/genhd.c:583)
[ 7025.975879] ? sprintf (lib/vsprintf.c:2138)
[ 7025.975949] ? scnprintf (lib/vsprintf.c:2138)
[ 7025.976023] ? lockdep_init_map (kernel/locking/lockdep.c:3041)
[ 7025.976103] dm_create (drivers/md/dm.c:2318 drivers/md/dm.c:2598)
[ 7025.976177] dev_create (drivers/md/dm-ioctl.c:747)
[ 7025.976252] ? list_version_get_info (drivers/md/dm-ioctl.c:735)
[ 7025.976341] ctl_ioctl (drivers/md/dm-ioctl.c:1848)
[ 7025.976414] ? semctl_main (ipc/sem.c:1330)
[ 7025.976492] ? debug_lockdep_rcu_enabled (kernel/rcu/update.c:195)
[ 7025.976583] ? list_version_get_info (drivers/md/dm-ioctl.c:735)
[ 7025.976672] ? free_params (drivers/md/dm-ioctl.c:1793)
[ 7025.976749] ? SYSC_semtimedop (ipc/sem.c:2010)
[ 7025.980019] dm_ctl_ioctl (drivers/md/dm-ioctl.c:1866)
[ 7025.983301] do_vfs_ioctl (fs/ioctl.c:44 fs/ioctl.c:607)
[ 7025.986587] ? ioctl_preallocate (fs/ioctl.c:557)
[ 7025.989788] ? mntput (fs/namespace.c:1106)
[ 7025.992915] ? SyS_semctl (ipc/sem.c:1601 ipc/sem.c:1577)
[ 7025.995989] ? debug_lockdep_rcu_enabled (kernel/rcu/update.c:195)
[ 7025.998981] ? __fget_light (fs/file.c:684)
[ 7026.001895] SyS_ioctl (fs/ioctl.c:622 fs/ioctl.c:613)
[ 7026.004724] system_call_fastpath (arch/x86/kernel/entry_64.S:261)
[ 7026.007455] Code: d7 3d 83 41 55 49 89 d5 41 54 41 89 cc 53 48 89 f3 48 83 ec 08 e8 10 b6 1e 01 48 89 da 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 <80> 3c 02 00 0f 85 9a 00 00 00 48 8b 1b 48 85 db 74 4d 48 89 df
All code
========
0: d7 xlat %ds:(%rbx)
1: 3d 83 41 55 49 cmp $0x49554183,%eax
6: 89 d5 mov %edx,%ebp
8: 41 54 push %r12
a: 41 89 cc mov %ecx,%r12d
d: 53 push %rbx
e: 48 89 f3 mov %rsi,%rbx
11: 48 83 ec 08 sub $0x8,%rsp
15: e8 10 b6 1e 01 callq 0x11eb62a
1a: 48 89 da mov %rbx,%rdx
1d: 48 b8 00 00 00 00 00 movabs $0xdffffc0000000000,%rax
24: fc ff df
27: 48 c1 ea 03 shr $0x3,%rdx
2b:* 80 3c 02 00 cmpb $0x0,(%rdx,%rax,1) <-- trapping instruction
2f: 0f 85 9a 00 00 00 jne 0xcf
35: 48 8b 1b mov (%rbx),%rbx
38: 48 85 db test %rbx,%rbx
3b: 74 4d je 0x8a
3d: 48 89 df mov %rbx,%rdi
Code starting with the faulting instruction
===========================================
0: 80 3c 02 00 cmpb $0x0,(%rdx,%rax,1)
4: 0f 85 9a 00 00 00 jne 0xa4
a: 48 8b 1b mov (%rbx),%rbx
d: 48 85 db test %rbx,%rbx
10: 74 4d je 0x5f
12: 48 89 df mov %rbx,%rdi
[ 7026.013551] RIP sysfs_do_create_link_sd.isra.2 (fs/sysfs/symlink.c:35)
[ 7026.016243] RSP <ffff881fda807878>
[ 7026.142471] kobject: 'holders' (ffff881ff0e9fe00): kobject_cleanup, parent ffff8837b82ee0a8
[ 7026.144127] kobject: 'holders' (ffff881ff0e9fe00): auto cleanup kobject_del
[ 7026.498540] ---[ end trace 734c93b316c19e45 ]---
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH 1/3] PM suspend/hibernate: Call notifier after freezing processes
From: Rafael J. Wysocki @ 2015-04-09 17:13 UTC (permalink / raw)
To: Pali Rohár
Cc: Alasdair Kergon, Mike Snitzer, Neil Brown, Len Brown,
Pavel Machek, dm-devel, linux-raid, linux-kernel, linux-pm
In-Reply-To: <201504090836.57365@pali>
On Thursday, April 09, 2015 08:36:57 AM Pali Rohár wrote:
>
> --nextPart2566388.gOmNIJrIqI
> Content-Type: Text/Plain;
> charset="utf-8"
> Content-Transfer-Encoding: quoted-printable
>
> On Thursday 09 April 2015 02:28:41 Rafael J. Wysocki wrote:
> > On Sunday, April 05, 2015 07:20:17 PM Pali Roh=C3=A1r wrote:
> > > To prevent race conditions on userspace processes with I/O
> > > some taks must be called after processes are freezed. This
> > > patch adds new events which are delivered by
> > > pm_notifier_call_chain() after freezing processes when
> > > doing suspend or hibernate action.
> > >=20
> > > Signed-off-by: Pali Roh=C3=A1r <pali.rohar@gmail.com>
> >=20
> > Please don't add more notifiers. Just call whatever you need
> > directly from where you need to call that.
> >=20
> > If that is device-related, try to use device PM
> > suspend/hibernate callbacks instead.
> >=20
>
> Hi! It is not possible to use any exiting pm notifiers! This is=20
> reason why I added new ones. As I wrote wiping dm crypt keys must=20
> be done *after* userspace processes are freezed to prevent race=20
> conditions...
I'm not talking about using the existing notifiers. I'm talking about
calling the function you need to call directly from a suitable place
in the system suspend code.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH 1/3] PM suspend/hibernate: Call notifier after freezing processes
From: Pali Rohár @ 2015-04-09 16:55 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Alasdair Kergon, Mike Snitzer, Neil Brown, Len Brown,
Pavel Machek, dm-devel, linux-raid, linux-kernel, linux-pm
In-Reply-To: <1856560.gNHsvxOS0D@vostro.rjw.lan>
[-- Attachment #1: Type: Text/Plain, Size: 1776 bytes --]
On Thursday 09 April 2015 19:13:55 Rafael J. Wysocki wrote:
> On Thursday, April 09, 2015 08:36:57 AM Pali Rohár wrote:
> > --nextPart2566388.gOmNIJrIqI
> > Content-Type: Text/Plain;
> >
> > charset="utf-8"
> >
> > Content-Transfer-Encoding: quoted-printable
> >
> > On Thursday 09 April 2015 02:28:41 Rafael J. Wysocki wrote:
> > > On Sunday, April 05, 2015 07:20:17 PM Pali Roh=C3=A1r
> > > wrote:
> > > > To prevent race conditions on userspace processes with
> > > > I/O some taks must be called after processes are
> > > > freezed. This patch adds new events which are delivered
> > > > by
> > > > pm_notifier_call_chain() after freezing processes when
> > > > doing suspend or hibernate action.
> > > >
> > > >=20
> > > >
> > > > Signed-off-by: Pali Roh=C3=A1r <pali.rohar@gmail.com>
> > >
> > >=20
> > >
> > > Please don't add more notifiers. Just call whatever you
> > > need directly from where you need to call that.
> > >
> > >=20
> > >
> > > If that is device-related, try to use device PM
> > > suspend/hibernate callbacks instead.
> > >
> > >=20
> >
> > Hi! It is not possible to use any exiting pm notifiers! This
> > is=20 reason why I added new ones. As I wrote wiping dm
> > crypt keys must=20 be done *after* userspace processes are
> > freezed to prevent race=20 conditions...
>
> I'm not talking about using the existing notifiers. I'm
> talking about calling the function you need to call directly
> from a suitable place in the system suspend code.
I need to wipe crypto keys from dm-crypt module. That module can
be compiled as external .ko file and so kernel cannot call
directly needed function. This is reason why I'm adding new
notifier event.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [RFC] raid5: add a log device to fix raid5/6 write hole issue
From: Shaohua Li @ 2015-04-09 16:03 UTC (permalink / raw)
To: Dan Williams; +Cc: NeilBrown, linux-raid, Song Liu, Kernel-team
In-Reply-To: <CAPcyv4j2RjqY=Ns8rXMUypGovU7_beNfpuL-iW0Fx9522ogPkA@mail.gmail.com>
On Thu, Apr 09, 2015 at 08:37:03AM -0700, Dan Williams wrote:
> On Wed, Apr 8, 2015 at 11:15 PM, Shaohua Li <shli@fb.com> wrote:
> > On Thu, Apr 09, 2015 at 03:04:59PM +1000, NeilBrown wrote:
> >> On Wed, 8 Apr 2015 17:43:11 -0700 Shaohua Li <shli@fb.com> wrote:
> >>
> >> > Hi,
> >> > This is what I'm working on now, and hopefully had the basic code
> >> > running next week. The new design will do cache and fix the write hole
> >> > issue too. Before I post the code out, I'd like to check if the design
> >> > has obvious issues.
> >>
> >> I can't say I'm excited about it....
> >>
> >> You still haven't explained why you would ever want to read data from the
> >> "cache"? Why not just keep everything in the stripe-cache until it is safe
> >> in the RAID. I asked before and you said:
> >>
> >> >> I'm not enthusiastic to use stripe cache though, we can't keep all data
> >> >> in stripe cache. What we really need is an index.
> >>
> >> which is hardly an answer. Why cannot you keep all the data in the stripe
> >> cache? How much data is there? How much memory can you afford to dedicate?
> >>
> >> You must have some very long sustained bursts of writes which are much faster
> >> than the RAID can accept in order to not be able to keep everything in memory.
> >>
> >>
> >> Your cache layout seems very rigid. I would much rather a layout that was
> >> very general and flexible. If you want to always allocate a chunk at a time
> >> then fine, but don't force that on the cache layout.
> >>
> >> The log really should be very simple. A block describing what comes next,
> >> then lots of data/parity. Then another block and more data etc etc.
> >> Each metadata block points to the next one.
> >> If you need an index of the cache, you keep that in memory. On restart, you
> >> read all of the metadata blocks and built up the index.
> >>
> >> I think that space in the log should be reclaimed in exactly the order that
> >> it is written, so the active part of the log is contiguous. Obviously
> >> individual blocks become inactive in arbitrary order as they are written to
> >> the RAID, but each extent of the log becomes free in order.
> >> If you want that to happen out of order, you would need to present a very
> >> good reason.
> >
> > I came to the same idea when I'm thinking about a caching layer, but the
> > memory size is the main blocking issue. If the solution requires a large
> > amount of extra memory, it's not cost effective, so a hard sell to
> > replace hardware raid with software raid. The design completely depends
> > on if we can store all data in memory. I don't have an anwser yet how
> > much memory we should use to make the aggregation efficient. Guess only
> > number can talk. I'll try to collect some data and get back to you.
> >
>
> Another consideration to keep in mind is persistent memory. I'm
> working on an in-kernel mechanism to claim and map pmem and a
> raid-write-cache is an obvious first application. I'll include you on
> the initial submission of that capability.
Exactly, we are planing to use pmem in the future when it's mature and
popular. SSD is still the best option before pmem is popular and widely
used.
Thanks,
Shaohua
^ permalink raw reply
* Re: [RFC] raid5: add a log device to fix raid5/6 write hole issue
From: Dan Williams @ 2015-04-09 15:37 UTC (permalink / raw)
To: Shaohua Li; +Cc: NeilBrown, linux-raid, Song Liu, Kernel-team
In-Reply-To: <20150409061545.GA864165@devbig257.prn2.facebook.com>
On Wed, Apr 8, 2015 at 11:15 PM, Shaohua Li <shli@fb.com> wrote:
> On Thu, Apr 09, 2015 at 03:04:59PM +1000, NeilBrown wrote:
>> On Wed, 8 Apr 2015 17:43:11 -0700 Shaohua Li <shli@fb.com> wrote:
>>
>> > Hi,
>> > This is what I'm working on now, and hopefully had the basic code
>> > running next week. The new design will do cache and fix the write hole
>> > issue too. Before I post the code out, I'd like to check if the design
>> > has obvious issues.
>>
>> I can't say I'm excited about it....
>>
>> You still haven't explained why you would ever want to read data from the
>> "cache"? Why not just keep everything in the stripe-cache until it is safe
>> in the RAID. I asked before and you said:
>>
>> >> I'm not enthusiastic to use stripe cache though, we can't keep all data
>> >> in stripe cache. What we really need is an index.
>>
>> which is hardly an answer. Why cannot you keep all the data in the stripe
>> cache? How much data is there? How much memory can you afford to dedicate?
>>
>> You must have some very long sustained bursts of writes which are much faster
>> than the RAID can accept in order to not be able to keep everything in memory.
>>
>>
>> Your cache layout seems very rigid. I would much rather a layout that was
>> very general and flexible. If you want to always allocate a chunk at a time
>> then fine, but don't force that on the cache layout.
>>
>> The log really should be very simple. A block describing what comes next,
>> then lots of data/parity. Then another block and more data etc etc.
>> Each metadata block points to the next one.
>> If you need an index of the cache, you keep that in memory. On restart, you
>> read all of the metadata blocks and built up the index.
>>
>> I think that space in the log should be reclaimed in exactly the order that
>> it is written, so the active part of the log is contiguous. Obviously
>> individual blocks become inactive in arbitrary order as they are written to
>> the RAID, but each extent of the log becomes free in order.
>> If you want that to happen out of order, you would need to present a very
>> good reason.
>
> I came to the same idea when I'm thinking about a caching layer, but the
> memory size is the main blocking issue. If the solution requires a large
> amount of extra memory, it's not cost effective, so a hard sell to
> replace hardware raid with software raid. The design completely depends
> on if we can store all data in memory. I don't have an anwser yet how
> much memory we should use to make the aggregation efficient. Guess only
> number can talk. I'll try to collect some data and get back to you.
>
Another consideration to keep in mind is persistent memory. I'm
working on an in-kernel mechanism to claim and map pmem and a
raid-write-cache is an obvious first application. I'll include you on
the initial submission of that capability.
^ permalink raw reply
* Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation
From: Pali Rohár @ 2015-04-09 14:38 UTC (permalink / raw)
To: Mike Snitzer
Cc: Alasdair Kergon, Neil Brown, Rafael J. Wysocki, Len Brown,
Pavel Machek, dm-devel, linux-raid, linux-kernel, linux-pm
In-Reply-To: <20150409142658.GA9839@redhat.com>
On Thursday 09 April 2015 10:26:58 Mike Snitzer wrote:
> On Thu, Apr 09 2015 at 10:16am -0400,
> Pali Rohár <pali.rohar@gmail.com> wrote:
>
> > On Thursday 09 April 2015 10:08:43 Mike Snitzer wrote:
> > > On Thu, Apr 09 2015 at 9:28am -0400,
> > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > >
> > > > On Thursday 09 April 2015 09:12:08 Mike Snitzer wrote:
> > > > > On Mon, Apr 06 2015 at 9:29am -0400,
> > > > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > >
> > > > > > On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> > > > > > > On Sun, Apr 05 2015 at 1:20pm -0400,
> > > > > > >
> > > > > > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > > > This patch series increase security of suspend and hibernate
> > > > > > > > actions. It allows user to safely wipe crypto keys before
> > > > > > > > suspend and hibernate actions starts without race
> > > > > > > > conditions on userspace process with heavy I/O.
> > > > > > > >
> > > > > > > > To automatically wipe cryto key for <device> before
> > > > > > > > hibernate action call: $ dmsetup message <device> 0 key
> > > > > > > > wipe_on_hibernation 1
> > > > > > > >
> > > > > > > > To automatically wipe cryto key for <device> before suspend
> > > > > > > > action call: $ dmsetup message <device> 0 key
> > > > > > > > wipe_on_suspend 1
> > > > > > > >
> > > > > > > > (Value 0 after wipe_* string reverts original behaviour - to
> > > > > > > > not wipe key)
> > > > > > >
> > > > > > > Can you elaborate on the attack vector your changes are meant
> > > > > > > to protect against? The user already authorized access, why
> > > > > > > is it inherently dangerous to _not_ wipe the associated key
> > > > > > > across these events?
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > yes, I will try to explain current problems with cryptsetup
> > > > > > luksSuspend command and hibernation.
> > > > > >
> > > > > > First, sometimes it is needed to put machine into other hands.
> > > > > > You can still watch other person what is doing with machine, but
> > > > > > once if you let machine unlocked (e.g opened luks disk), she/he
> > > > > > can access encrypted data.
> > > > > >
> > > > > > If you turn off machine, it could be safe, because luks disk
> > > > > > devices are locked. But if you enter machine into suspend or
> > > > > > hibernate state luks devices are still open. And my patches try
> > > > > > to achieve similar security as when machine is off (= no crypto
> > > > > > keys in RAM or on swap).
> > > > > >
> > > > > > When doing hibernate on unencrypted swap it is to prevent leaking
> > > > > > crypto keys to hibernate image (which is stored in swap).
> > > > > >
> > > > > > When doing suspend action it is again to prevent leaking crypto
> > > > > > keys. E.g when you suspend laptop and put it off (somebody can
> > > > > > remove RAMs and do some cold boot attack).
> > > > > >
> > > > > > The most common situation is:
> > > > > > You have mounted partition from dm-crypt device (e.g. /home/),
> > > > > > some userspace processes access it (e.g opened firefox which
> > > > > > still reads/writes to cache ~/.firefox/) and you want to drop
> > > > > > crypto keys from kernel for some time.
> > > > > >
> > > > > > For that operation there is command cryptsetup luksSuspend, which
> > > > > > suspend dm device and then tell kernel to wipe crypto keys. All
> > > > > > I/O operations are then stopped and userspace processes which
> > > > > > want to do some those I/O operations are stopped too (until you
> > > > > > call cryptsetup luksResume and enter correct key).
> > > > > >
> > > > > > Now if you want to suspend/hiberate your machine (when some of dm
> > > > > > devices are suspeneded and some processes are stopped due to
> > > > > > pending I/O) it is not possible. Kernel freeze_processes function
> > > > > > will fail because userspace processes are still stopped inside
> > > > > > some I/O syscall (read/write, etc,...).
> > > > > >
> > > > > > My patches fixes this problem and do those operations (suspend dm
> > > > > > device, wipe crypto keys, enter suspend/hiberate) in correct
> > > > > > order and without race condition.
> > > > > >
> > > > > > dm device is suspended *after* userspace processes are freezed
> > > > > > and after that are crypto keys wiped. And then computer/laptop
> > > > > > enters into suspend/hibernate state.
> > > > >
> > > > > Wouldn't it be better to fix freeze_processes() to be tolerant of
> > > > > processes that are hung as a side-effect of their backing storage being
> > > > > suspended? A hibernate shouldn't fail simply because a user chose to
> > > > > suspend a DM device.
> > > > >
> > > > > Then this entire problem goes away and the key can be wiped from
> > > > > userspace (like you said above).
> > > >
> > > > Still there will be race condition. Before hibernation (and device
> > > > poweroff) we should have synced disks and filesystems to prevent data
> > > > lose (or other damage) as more as we can. And if there will be some
> > > > application which using lot of I/O (e.g normal firefox) then there
> > > > always will be race condtion.
> > >
> > > The DM suspend will take care of flushing any pending I/O. So I don't
> > > see where the supposed race is...
> > >
> >
> > Any I/O operation after DM suspend is race condition and could cause
> > data lost.
> >
> > > Anything else that is trapped in userspace memory will be there when the
> > > machine resumes.
> > >
> >
> > You are expecting that machine resumes always at 100% and correctly. But
> > this is not truth in real world. There are planty of users who reported
> > lot of random problems with suspend or hibernate...
>
> But the system was left in a crash consistent state. Properly written
> apps will wait for I/O to ensure data loss (in the event of a failed
> resume) isn't a problem.
>
I think you are too optimistic about ideal world...
"Properly written apps" "ensure data loss"
> > > > So proper way is to wipe luks crypto keys *after* userspace processes
> > > > are freezed.
> > >
> > > I know you believe that I'm just not accepting that at face value.
> >
> > If disks are synced before any DM suspend operation then we have higher
> > chance of preventing data corruption.
>
> disks are already synced as part of the DM suspend operation!
>
Yes, but part of hibernate operation is also sync call.
> But you're saying that all user processes are frozen (and associated
> I/O flushed) before the DM suspend, that is different:
>
Yes, I want to ensure that. So processes wont be able to do any other
I/O.
> > I still think that correct order is only:
> >
> > * freeze processes (which doing continous I/O)
> > * fs & disk sync
> > * DM suspend
> > * wipe crypto keys
> > * enter hibernate
>
> I just don't think that extreme is _required_ to have a hibernate/resume
> that incorporates dm-crypt key wiping.
Ok, and what other developers think?
I'm saying that if I want to wipe luks keys before suspend/hibernate and
have system in consistant state as much as possible, keys must be wiped
*after* userspace processes are freezed. Or do you have relevant or
functional argument why not? Or is there any problem in my thinking?
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation
From: Mike Snitzer @ 2015-04-09 14:26 UTC (permalink / raw)
To: Pali Rohár
Cc: Alasdair Kergon, Neil Brown, Rafael J. Wysocki, Len Brown,
Pavel Machek, dm-devel, linux-raid, linux-kernel, linux-pm
In-Reply-To: <20150409141633.GJ12339@atrey.karlin.mff.cuni.cz>
On Thu, Apr 09 2015 at 10:16am -0400,
Pali Rohár <pali.rohar@gmail.com> wrote:
> On Thursday 09 April 2015 10:08:43 Mike Snitzer wrote:
> > On Thu, Apr 09 2015 at 9:28am -0400,
> > Pali Rohár <pali.rohar@gmail.com> wrote:
> >
> > > On Thursday 09 April 2015 09:12:08 Mike Snitzer wrote:
> > > > On Mon, Apr 06 2015 at 9:29am -0400,
> > > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > >
> > > > > On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> > > > > > On Sun, Apr 05 2015 at 1:20pm -0400,
> > > > > >
> > > > > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > > This patch series increase security of suspend and hibernate
> > > > > > > actions. It allows user to safely wipe crypto keys before
> > > > > > > suspend and hibernate actions starts without race
> > > > > > > conditions on userspace process with heavy I/O.
> > > > > > >
> > > > > > > To automatically wipe cryto key for <device> before
> > > > > > > hibernate action call: $ dmsetup message <device> 0 key
> > > > > > > wipe_on_hibernation 1
> > > > > > >
> > > > > > > To automatically wipe cryto key for <device> before suspend
> > > > > > > action call: $ dmsetup message <device> 0 key
> > > > > > > wipe_on_suspend 1
> > > > > > >
> > > > > > > (Value 0 after wipe_* string reverts original behaviour - to
> > > > > > > not wipe key)
> > > > > >
> > > > > > Can you elaborate on the attack vector your changes are meant
> > > > > > to protect against? The user already authorized access, why
> > > > > > is it inherently dangerous to _not_ wipe the associated key
> > > > > > across these events?
> > > > >
> > > > > Hi,
> > > > >
> > > > > yes, I will try to explain current problems with cryptsetup
> > > > > luksSuspend command and hibernation.
> > > > >
> > > > > First, sometimes it is needed to put machine into other hands.
> > > > > You can still watch other person what is doing with machine, but
> > > > > once if you let machine unlocked (e.g opened luks disk), she/he
> > > > > can access encrypted data.
> > > > >
> > > > > If you turn off machine, it could be safe, because luks disk
> > > > > devices are locked. But if you enter machine into suspend or
> > > > > hibernate state luks devices are still open. And my patches try
> > > > > to achieve similar security as when machine is off (= no crypto
> > > > > keys in RAM or on swap).
> > > > >
> > > > > When doing hibernate on unencrypted swap it is to prevent leaking
> > > > > crypto keys to hibernate image (which is stored in swap).
> > > > >
> > > > > When doing suspend action it is again to prevent leaking crypto
> > > > > keys. E.g when you suspend laptop and put it off (somebody can
> > > > > remove RAMs and do some cold boot attack).
> > > > >
> > > > > The most common situation is:
> > > > > You have mounted partition from dm-crypt device (e.g. /home/),
> > > > > some userspace processes access it (e.g opened firefox which
> > > > > still reads/writes to cache ~/.firefox/) and you want to drop
> > > > > crypto keys from kernel for some time.
> > > > >
> > > > > For that operation there is command cryptsetup luksSuspend, which
> > > > > suspend dm device and then tell kernel to wipe crypto keys. All
> > > > > I/O operations are then stopped and userspace processes which
> > > > > want to do some those I/O operations are stopped too (until you
> > > > > call cryptsetup luksResume and enter correct key).
> > > > >
> > > > > Now if you want to suspend/hiberate your machine (when some of dm
> > > > > devices are suspeneded and some processes are stopped due to
> > > > > pending I/O) it is not possible. Kernel freeze_processes function
> > > > > will fail because userspace processes are still stopped inside
> > > > > some I/O syscall (read/write, etc,...).
> > > > >
> > > > > My patches fixes this problem and do those operations (suspend dm
> > > > > device, wipe crypto keys, enter suspend/hiberate) in correct
> > > > > order and without race condition.
> > > > >
> > > > > dm device is suspended *after* userspace processes are freezed
> > > > > and after that are crypto keys wiped. And then computer/laptop
> > > > > enters into suspend/hibernate state.
> > > >
> > > > Wouldn't it be better to fix freeze_processes() to be tolerant of
> > > > processes that are hung as a side-effect of their backing storage being
> > > > suspended? A hibernate shouldn't fail simply because a user chose to
> > > > suspend a DM device.
> > > >
> > > > Then this entire problem goes away and the key can be wiped from
> > > > userspace (like you said above).
> > >
> > > Still there will be race condition. Before hibernation (and device
> > > poweroff) we should have synced disks and filesystems to prevent data
> > > lose (or other damage) as more as we can. And if there will be some
> > > application which using lot of I/O (e.g normal firefox) then there
> > > always will be race condtion.
> >
> > The DM suspend will take care of flushing any pending I/O. So I don't
> > see where the supposed race is...
> >
>
> Any I/O operation after DM suspend is race condition and could cause
> data lost.
>
> > Anything else that is trapped in userspace memory will be there when the
> > machine resumes.
> >
>
> You are expecting that machine resumes always at 100% and correctly. But
> this is not truth in real world. There are planty of users who reported
> lot of random problems with suspend or hibernate...
But the system was left in a crash consistent state. Properly written
apps will wait for I/O to ensure data loss (in the event of a failed
resume) isn't a problem.
> > > So proper way is to wipe luks crypto keys *after* userspace processes
> > > are freezed.
> >
> > I know you believe that I'm just not accepting that at face value.
>
> If disks are synced before any DM suspend operation then we have higher
> chance of preventing data corruption.
disks are already synced as part of the DM suspend operation!
But you're saying that all user processes are frozen (and associated
I/O flushed) before the DM suspend, that is different:
> I still think that correct order is only:
>
> * freeze processes (which doing continous I/O)
> * fs & disk sync
> * DM suspend
> * wipe crypto keys
> * enter hibernate
I just don't think that extreme is _required_ to have a hibernate/resume
that incorporates dm-crypt key wiping.
^ permalink raw reply
* Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation
From: Pali Rohár @ 2015-04-09 14:16 UTC (permalink / raw)
To: Mike Snitzer
Cc: Alasdair Kergon, Neil Brown, Rafael J. Wysocki, Len Brown,
Pavel Machek, dm-devel, linux-raid, linux-kernel, linux-pm
In-Reply-To: <20150409140842.GA9779@redhat.com>
On Thursday 09 April 2015 10:08:43 Mike Snitzer wrote:
> On Thu, Apr 09 2015 at 9:28am -0400,
> Pali Rohár <pali.rohar@gmail.com> wrote:
>
> > On Thursday 09 April 2015 09:12:08 Mike Snitzer wrote:
> > > On Mon, Apr 06 2015 at 9:29am -0400,
> > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > >
> > > > On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> > > > > On Sun, Apr 05 2015 at 1:20pm -0400,
> > > > >
> > > > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > This patch series increase security of suspend and hibernate
> > > > > > actions. It allows user to safely wipe crypto keys before
> > > > > > suspend and hibernate actions starts without race
> > > > > > conditions on userspace process with heavy I/O.
> > > > > >
> > > > > > To automatically wipe cryto key for <device> before
> > > > > > hibernate action call: $ dmsetup message <device> 0 key
> > > > > > wipe_on_hibernation 1
> > > > > >
> > > > > > To automatically wipe cryto key for <device> before suspend
> > > > > > action call: $ dmsetup message <device> 0 key
> > > > > > wipe_on_suspend 1
> > > > > >
> > > > > > (Value 0 after wipe_* string reverts original behaviour - to
> > > > > > not wipe key)
> > > > >
> > > > > Can you elaborate on the attack vector your changes are meant
> > > > > to protect against? The user already authorized access, why
> > > > > is it inherently dangerous to _not_ wipe the associated key
> > > > > across these events?
> > > >
> > > > Hi,
> > > >
> > > > yes, I will try to explain current problems with cryptsetup
> > > > luksSuspend command and hibernation.
> > > >
> > > > First, sometimes it is needed to put machine into other hands.
> > > > You can still watch other person what is doing with machine, but
> > > > once if you let machine unlocked (e.g opened luks disk), she/he
> > > > can access encrypted data.
> > > >
> > > > If you turn off machine, it could be safe, because luks disk
> > > > devices are locked. But if you enter machine into suspend or
> > > > hibernate state luks devices are still open. And my patches try
> > > > to achieve similar security as when machine is off (= no crypto
> > > > keys in RAM or on swap).
> > > >
> > > > When doing hibernate on unencrypted swap it is to prevent leaking
> > > > crypto keys to hibernate image (which is stored in swap).
> > > >
> > > > When doing suspend action it is again to prevent leaking crypto
> > > > keys. E.g when you suspend laptop and put it off (somebody can
> > > > remove RAMs and do some cold boot attack).
> > > >
> > > > The most common situation is:
> > > > You have mounted partition from dm-crypt device (e.g. /home/),
> > > > some userspace processes access it (e.g opened firefox which
> > > > still reads/writes to cache ~/.firefox/) and you want to drop
> > > > crypto keys from kernel for some time.
> > > >
> > > > For that operation there is command cryptsetup luksSuspend, which
> > > > suspend dm device and then tell kernel to wipe crypto keys. All
> > > > I/O operations are then stopped and userspace processes which
> > > > want to do some those I/O operations are stopped too (until you
> > > > call cryptsetup luksResume and enter correct key).
> > > >
> > > > Now if you want to suspend/hiberate your machine (when some of dm
> > > > devices are suspeneded and some processes are stopped due to
> > > > pending I/O) it is not possible. Kernel freeze_processes function
> > > > will fail because userspace processes are still stopped inside
> > > > some I/O syscall (read/write, etc,...).
> > > >
> > > > My patches fixes this problem and do those operations (suspend dm
> > > > device, wipe crypto keys, enter suspend/hiberate) in correct
> > > > order and without race condition.
> > > >
> > > > dm device is suspended *after* userspace processes are freezed
> > > > and after that are crypto keys wiped. And then computer/laptop
> > > > enters into suspend/hibernate state.
> > >
> > > Wouldn't it be better to fix freeze_processes() to be tolerant of
> > > processes that are hung as a side-effect of their backing storage being
> > > suspended? A hibernate shouldn't fail simply because a user chose to
> > > suspend a DM device.
> > >
> > > Then this entire problem goes away and the key can be wiped from
> > > userspace (like you said above).
> >
> > Still there will be race condition. Before hibernation (and device
> > poweroff) we should have synced disks and filesystems to prevent data
> > lose (or other damage) as more as we can. And if there will be some
> > application which using lot of I/O (e.g normal firefox) then there
> > always will be race condtion.
>
> The DM suspend will take care of flushing any pending I/O. So I don't
> see where the supposed race is...
>
Any I/O operation after DM suspend is race condition and could cause
data lost.
> Anything else that is trapped in userspace memory will be there when the
> machine resumes.
>
You are expecting that machine resumes always at 100% and correctly. But
this is not truth in real world. There are planty of users who reported
lot of random problems with suspend or hibernate...
> > So proper way is to wipe luks crypto keys *after* userspace processes
> > are freezed.
>
> I know you believe that I'm just not accepting that at face value.
If disks are synced before any DM suspend operation then we have higher
chance of preventing data corruption.
I still think that correct order is only:
* freeze processes (which doing continous I/O)
* fs & disk sync
* DM suspend
* wipe crypto keys
* enter hibernate
--
Pali Rohár
pali.rohar@gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation
From: Mike Snitzer @ 2015-04-09 14:08 UTC (permalink / raw)
To: Pali Rohár
Cc: Alasdair Kergon, Neil Brown, Rafael J. Wysocki, Len Brown,
Pavel Machek, dm-devel, linux-raid, linux-kernel, linux-pm
In-Reply-To: <20150409132836.GD12339@atrey.karlin.mff.cuni.cz>
On Thu, Apr 09 2015 at 9:28am -0400,
Pali Rohár <pali.rohar@gmail.com> wrote:
> On Thursday 09 April 2015 09:12:08 Mike Snitzer wrote:
> > On Mon, Apr 06 2015 at 9:29am -0400,
> > Pali Rohár <pali.rohar@gmail.com> wrote:
> >
> > > On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> > > > On Sun, Apr 05 2015 at 1:20pm -0400,
> > > >
> > > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > This patch series increase security of suspend and hibernate
> > > > > actions. It allows user to safely wipe crypto keys before
> > > > > suspend and hibernate actions starts without race
> > > > > conditions on userspace process with heavy I/O.
> > > > >
> > > > > To automatically wipe cryto key for <device> before
> > > > > hibernate action call: $ dmsetup message <device> 0 key
> > > > > wipe_on_hibernation 1
> > > > >
> > > > > To automatically wipe cryto key for <device> before suspend
> > > > > action call: $ dmsetup message <device> 0 key
> > > > > wipe_on_suspend 1
> > > > >
> > > > > (Value 0 after wipe_* string reverts original behaviour - to
> > > > > not wipe key)
> > > >
> > > > Can you elaborate on the attack vector your changes are meant
> > > > to protect against? The user already authorized access, why
> > > > is it inherently dangerous to _not_ wipe the associated key
> > > > across these events?
> > >
> > > Hi,
> > >
> > > yes, I will try to explain current problems with cryptsetup
> > > luksSuspend command and hibernation.
> > >
> > > First, sometimes it is needed to put machine into other hands.
> > > You can still watch other person what is doing with machine, but
> > > once if you let machine unlocked (e.g opened luks disk), she/he
> > > can access encrypted data.
> > >
> > > If you turn off machine, it could be safe, because luks disk
> > > devices are locked. But if you enter machine into suspend or
> > > hibernate state luks devices are still open. And my patches try
> > > to achieve similar security as when machine is off (= no crypto
> > > keys in RAM or on swap).
> > >
> > > When doing hibernate on unencrypted swap it is to prevent leaking
> > > crypto keys to hibernate image (which is stored in swap).
> > >
> > > When doing suspend action it is again to prevent leaking crypto
> > > keys. E.g when you suspend laptop and put it off (somebody can
> > > remove RAMs and do some cold boot attack).
> > >
> > > The most common situation is:
> > > You have mounted partition from dm-crypt device (e.g. /home/),
> > > some userspace processes access it (e.g opened firefox which
> > > still reads/writes to cache ~/.firefox/) and you want to drop
> > > crypto keys from kernel for some time.
> > >
> > > For that operation there is command cryptsetup luksSuspend, which
> > > suspend dm device and then tell kernel to wipe crypto keys. All
> > > I/O operations are then stopped and userspace processes which
> > > want to do some those I/O operations are stopped too (until you
> > > call cryptsetup luksResume and enter correct key).
> > >
> > > Now if you want to suspend/hiberate your machine (when some of dm
> > > devices are suspeneded and some processes are stopped due to
> > > pending I/O) it is not possible. Kernel freeze_processes function
> > > will fail because userspace processes are still stopped inside
> > > some I/O syscall (read/write, etc,...).
> > >
> > > My patches fixes this problem and do those operations (suspend dm
> > > device, wipe crypto keys, enter suspend/hiberate) in correct
> > > order and without race condition.
> > >
> > > dm device is suspended *after* userspace processes are freezed
> > > and after that are crypto keys wiped. And then computer/laptop
> > > enters into suspend/hibernate state.
> >
> > Wouldn't it be better to fix freeze_processes() to be tolerant of
> > processes that are hung as a side-effect of their backing storage being
> > suspended? A hibernate shouldn't fail simply because a user chose to
> > suspend a DM device.
> >
> > Then this entire problem goes away and the key can be wiped from
> > userspace (like you said above).
>
> Still there will be race condition. Before hibernation (and device
> poweroff) we should have synced disks and filesystems to prevent data
> lose (or other damage) as more as we can. And if there will be some
> application which using lot of I/O (e.g normal firefox) then there
> always will be race condtion.
The DM suspend will take care of flushing any pending I/O. So I don't
see where the supposed race is...
Anything else that is trapped in userspace memory will be there when the
machine resumes.
> So proper way is to wipe luks crypto keys *after* userspace processes
> are freezed.
I know you believe that I'm just not accepting that at face value.
^ permalink raw reply
* Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation
From: Pali Rohár @ 2015-04-09 13:28 UTC (permalink / raw)
To: Mike Snitzer
Cc: Alasdair Kergon, Neil Brown, Rafael J. Wysocki, Len Brown,
Pavel Machek, dm-devel, linux-raid, linux-kernel, linux-pm
In-Reply-To: <20150409131208.GA9504@redhat.com>
On Thursday 09 April 2015 09:12:08 Mike Snitzer wrote:
> On Mon, Apr 06 2015 at 9:29am -0400,
> Pali Rohár <pali.rohar@gmail.com> wrote:
>
> > On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> > > On Sun, Apr 05 2015 at 1:20pm -0400,
> > >
> > > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > This patch series increase security of suspend and hibernate
> > > > actions. It allows user to safely wipe crypto keys before
> > > > suspend and hibernate actions starts without race
> > > > conditions on userspace process with heavy I/O.
> > > >
> > > > To automatically wipe cryto key for <device> before
> > > > hibernate action call: $ dmsetup message <device> 0 key
> > > > wipe_on_hibernation 1
> > > >
> > > > To automatically wipe cryto key for <device> before suspend
> > > > action call: $ dmsetup message <device> 0 key
> > > > wipe_on_suspend 1
> > > >
> > > > (Value 0 after wipe_* string reverts original behaviour - to
> > > > not wipe key)
> > >
> > > Can you elaborate on the attack vector your changes are meant
> > > to protect against? The user already authorized access, why
> > > is it inherently dangerous to _not_ wipe the associated key
> > > across these events?
> >
> > Hi,
> >
> > yes, I will try to explain current problems with cryptsetup
> > luksSuspend command and hibernation.
> >
> > First, sometimes it is needed to put machine into other hands.
> > You can still watch other person what is doing with machine, but
> > once if you let machine unlocked (e.g opened luks disk), she/he
> > can access encrypted data.
> >
> > If you turn off machine, it could be safe, because luks disk
> > devices are locked. But if you enter machine into suspend or
> > hibernate state luks devices are still open. And my patches try
> > to achieve similar security as when machine is off (= no crypto
> > keys in RAM or on swap).
> >
> > When doing hibernate on unencrypted swap it is to prevent leaking
> > crypto keys to hibernate image (which is stored in swap).
> >
> > When doing suspend action it is again to prevent leaking crypto
> > keys. E.g when you suspend laptop and put it off (somebody can
> > remove RAMs and do some cold boot attack).
> >
> > The most common situation is:
> > You have mounted partition from dm-crypt device (e.g. /home/),
> > some userspace processes access it (e.g opened firefox which
> > still reads/writes to cache ~/.firefox/) and you want to drop
> > crypto keys from kernel for some time.
> >
> > For that operation there is command cryptsetup luksSuspend, which
> > suspend dm device and then tell kernel to wipe crypto keys. All
> > I/O operations are then stopped and userspace processes which
> > want to do some those I/O operations are stopped too (until you
> > call cryptsetup luksResume and enter correct key).
> >
> > Now if you want to suspend/hiberate your machine (when some of dm
> > devices are suspeneded and some processes are stopped due to
> > pending I/O) it is not possible. Kernel freeze_processes function
> > will fail because userspace processes are still stopped inside
> > some I/O syscall (read/write, etc,...).
> >
> > My patches fixes this problem and do those operations (suspend dm
> > device, wipe crypto keys, enter suspend/hiberate) in correct
> > order and without race condition.
> >
> > dm device is suspended *after* userspace processes are freezed
> > and after that are crypto keys wiped. And then computer/laptop
> > enters into suspend/hibernate state.
>
> Wouldn't it be better to fix freeze_processes() to be tolerant of
> processes that are hung as a side-effect of their backing storage being
> suspended? A hibernate shouldn't fail simply because a user chose to
> suspend a DM device.
>
> Then this entire problem goes away and the key can be wiped from
> userspace (like you said above).
Still there will be race condition. Before hibernation (and device
poweroff) we should have synced disks and filesystems to prevent data
lose (or other damage) as more as we can. And if there will be some
application which using lot of I/O (e.g normal firefox) then there
always will be race condtion.
So proper way is to wipe luks crypto keys *after* userspace processes
are freezed.
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH 0/3] dm-crypt: Adds support for wiping key when doing suspend/hibernation
From: Mike Snitzer @ 2015-04-09 13:12 UTC (permalink / raw)
To: Pali Rohár
Cc: Alasdair Kergon, Neil Brown, Rafael J. Wysocki, Len Brown,
Pavel Machek, dm-devel, linux-raid, linux-kernel, linux-pm
In-Reply-To: <201504061529.57299@pali>
On Mon, Apr 06 2015 at 9:29am -0400,
Pali Rohár <pali.rohar@gmail.com> wrote:
> On Monday 06 April 2015 15:00:46 Mike Snitzer wrote:
> > On Sun, Apr 05 2015 at 1:20pm -0400,
> >
> > Pali Rohár <pali.rohar@gmail.com> wrote:
> > > This patch series increase security of suspend and hibernate
> > > actions. It allows user to safely wipe crypto keys before
> > > suspend and hibernate actions starts without race
> > > conditions on userspace process with heavy I/O.
> > >
> > > To automatically wipe cryto key for <device> before
> > > hibernate action call: $ dmsetup message <device> 0 key
> > > wipe_on_hibernation 1
> > >
> > > To automatically wipe cryto key for <device> before suspend
> > > action call: $ dmsetup message <device> 0 key
> > > wipe_on_suspend 1
> > >
> > > (Value 0 after wipe_* string reverts original behaviour - to
> > > not wipe key)
> >
> > Can you elaborate on the attack vector your changes are meant
> > to protect against? The user already authorized access, why
> > is it inherently dangerous to _not_ wipe the associated key
> > across these events?
>
> Hi,
>
> yes, I will try to explain current problems with cryptsetup
> luksSuspend command and hibernation.
>
> First, sometimes it is needed to put machine into other hands.
> You can still watch other person what is doing with machine, but
> once if you let machine unlocked (e.g opened luks disk), she/he
> can access encrypted data.
>
> If you turn off machine, it could be safe, because luks disk
> devices are locked. But if you enter machine into suspend or
> hibernate state luks devices are still open. And my patches try
> to achieve similar security as when machine is off (= no crypto
> keys in RAM or on swap).
>
> When doing hibernate on unencrypted swap it is to prevent leaking
> crypto keys to hibernate image (which is stored in swap).
>
> When doing suspend action it is again to prevent leaking crypto
> keys. E.g when you suspend laptop and put it off (somebody can
> remove RAMs and do some cold boot attack).
>
> The most common situation is:
> You have mounted partition from dm-crypt device (e.g. /home/),
> some userspace processes access it (e.g opened firefox which
> still reads/writes to cache ~/.firefox/) and you want to drop
> crypto keys from kernel for some time.
>
> For that operation there is command cryptsetup luksSuspend, which
> suspend dm device and then tell kernel to wipe crypto keys. All
> I/O operations are then stopped and userspace processes which
> want to do some those I/O operations are stopped too (until you
> call cryptsetup luksResume and enter correct key).
>
> Now if you want to suspend/hiberate your machine (when some of dm
> devices are suspeneded and some processes are stopped due to
> pending I/O) it is not possible. Kernel freeze_processes function
> will fail because userspace processes are still stopped inside
> some I/O syscall (read/write, etc,...).
>
> My patches fixes this problem and do those operations (suspend dm
> device, wipe crypto keys, enter suspend/hiberate) in correct
> order and without race condition.
>
> dm device is suspended *after* userspace processes are freezed
> and after that are crypto keys wiped. And then computer/laptop
> enters into suspend/hibernate state.
Wouldn't it be better to fix freeze_processes() to be tolerant of
processes that are hung as a side-effect of their backing storage being
suspended? A hibernate shouldn't fail simply because a user chose to
suspend a DM device.
Then this entire problem goes away and the key can be wiped from
userspace (like you said above).
^ permalink raw reply
* Re: [PATCH 1/3] PM suspend/hibernate: Call notifier after freezing processes
From: Pali Rohár @ 2015-04-09 6:36 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Alasdair Kergon, Mike Snitzer, Neil Brown, Len Brown,
Pavel Machek, dm-devel, linux-raid, linux-kernel, linux-pm
In-Reply-To: <1530775.n2Kjx1bffW@vostro.rjw.lan>
[-- Attachment #1: Type: Text/Plain, Size: 2972 bytes --]
On Thursday 09 April 2015 02:28:41 Rafael J. Wysocki wrote:
> On Sunday, April 05, 2015 07:20:17 PM Pali Rohár wrote:
> > To prevent race conditions on userspace processes with I/O
> > some taks must be called after processes are freezed. This
> > patch adds new events which are delivered by
> > pm_notifier_call_chain() after freezing processes when
> > doing suspend or hibernate action.
> >
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>
> Please don't add more notifiers. Just call whatever you need
> directly from where you need to call that.
>
> If that is device-related, try to use device PM
> suspend/hibernate callbacks instead.
>
Hi! It is not possible to use any exiting pm notifiers! This is
reason why I added new ones. As I wrote wiping dm crypt keys must
be done *after* userspace processes are freezed to prevent race
conditions...
> > ---
> >
> > include/linux/suspend.h | 2 ++
> > kernel/power/hibernate.c | 2 ++
> > kernel/power/suspend.c | 4 +++-
> > 3 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/suspend.h
> > b/include/linux/suspend.h index 5efe743..bc743c8 100644
> > --- a/include/linux/suspend.h
> > +++ b/include/linux/suspend.h
> > @@ -368,6 +368,8 @@ static inline bool
> > hibernation_available(void) { return false; }
> >
> > #define PM_POST_SUSPEND 0x0004 /* Suspend finished */
> > #define PM_RESTORE_PREPARE 0x0005 /* Going to restore a
> > saved image */ #define PM_POST_RESTORE 0x0006 /*
> > Restore
> > failed */
> >
> > +#define PM_HIBERNATION_AFTER_FREEZE 0x0007 /* After
> > hibernation freeze */ +#define
> > PM_SUSPEND_AFTER_FREEZE 0x0008 /* After suspend freeze */
> >
> > extern struct mutex pm_mutex;
> >
> > diff --git a/kernel/power/hibernate.c
> > b/kernel/power/hibernate.c index 2329daa..184f7ee 100644
> > --- a/kernel/power/hibernate.c
> > +++ b/kernel/power/hibernate.c
> > @@ -671,6 +671,8 @@ int hibernate(void)
> >
> > if (error)
> >
> > goto Exit;
> >
> > + pm_notifier_call_chain(PM_HIBERNATION_AFTER_FREEZE);
> > +
> >
> > lock_device_hotplug();
> > /* Allocate memory management structures */
> > error = create_basic_memory_bitmaps();
> >
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index b7d6b3a..1776938 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -268,8 +268,10 @@ static int
> > suspend_prepare(suspend_state_t state)
> >
> > trace_suspend_resume(TPS("freeze_processes"), 0, true);
> > error = suspend_freeze_processes();
> > trace_suspend_resume(TPS("freeze_processes"), 0, false);
> >
> > - if (!error)
> > + if (!error) {
> > + pm_notifier_call_chain(PM_SUSPEND_AFTER_FREEZE);
> >
> > return 0;
> >
> > + }
> >
> > suspend_stats.failed_freeze++;
> > dpm_save_failed_step(SUSPEND_FREEZE);
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [RFC] raid5: add a log device to fix raid5/6 write hole issue
From: Shaohua Li @ 2015-04-09 6:15 UTC (permalink / raw)
To: NeilBrown; +Cc: dan.j.williams, linux-raid, songliubraving, Kernel-team
In-Reply-To: <20150409150459.320c668a@notabene.brown>
On Thu, Apr 09, 2015 at 03:04:59PM +1000, NeilBrown wrote:
> On Wed, 8 Apr 2015 17:43:11 -0700 Shaohua Li <shli@fb.com> wrote:
>
> > Hi,
> > This is what I'm working on now, and hopefully had the basic code
> > running next week. The new design will do cache and fix the write hole
> > issue too. Before I post the code out, I'd like to check if the design
> > has obvious issues.
>
> I can't say I'm excited about it....
>
> You still haven't explained why you would ever want to read data from the
> "cache"? Why not just keep everything in the stripe-cache until it is safe
> in the RAID. I asked before and you said:
>
> >> I'm not enthusiastic to use stripe cache though, we can't keep all data
> >> in stripe cache. What we really need is an index.
>
> which is hardly an answer. Why cannot you keep all the data in the stripe
> cache? How much data is there? How much memory can you afford to dedicate?
>
> You must have some very long sustained bursts of writes which are much faster
> than the RAID can accept in order to not be able to keep everything in memory.
>
>
> Your cache layout seems very rigid. I would much rather a layout that was
> very general and flexible. If you want to always allocate a chunk at a time
> then fine, but don't force that on the cache layout.
>
> The log really should be very simple. A block describing what comes next,
> then lots of data/parity. Then another block and more data etc etc.
> Each metadata block points to the next one.
> If you need an index of the cache, you keep that in memory. On restart, you
> read all of the metadata blocks and built up the index.
>
> I think that space in the log should be reclaimed in exactly the order that
> it is written, so the active part of the log is contiguous. Obviously
> individual blocks become inactive in arbitrary order as they are written to
> the RAID, but each extent of the log becomes free in order.
> If you want that to happen out of order, you would need to present a very
> good reason.
I came to the same idea when I'm thinking about a caching layer, but the
memory size is the main blocking issue. If the solution requires a large
amount of extra memory, it's not cost effective, so a hard sell to
replace hardware raid with software raid. The design completely depends
on if we can store all data in memory. I don't have an anwser yet how
much memory we should use to make the aggregation efficient. Guess only
number can talk. I'll try to collect some data and get back to you.
Thanks,
Shaohua
^ permalink raw reply
* Re: [RFC] raid5: add a log device to fix raid5/6 write hole issue
From: NeilBrown @ 2015-04-09 5:04 UTC (permalink / raw)
To: Shaohua Li; +Cc: dan.j.williams, linux-raid, songliubraving, Kernel-team
In-Reply-To: <20150409004238.GA186860@devbig257.prn2.facebook.com>
[-- Attachment #1: Type: text/plain, Size: 6718 bytes --]
On Wed, 8 Apr 2015 17:43:11 -0700 Shaohua Li <shli@fb.com> wrote:
> Hi,
> This is what I'm working on now, and hopefully had the basic code
> running next week. The new design will do cache and fix the write hole
> issue too. Before I post the code out, I'd like to check if the design
> has obvious issues.
I can't say I'm excited about it....
You still haven't explained why you would ever want to read data from the
"cache"? Why not just keep everything in the stripe-cache until it is safe
in the RAID. I asked before and you said:
>> I'm not enthusiastic to use stripe cache though, we can't keep all data
>> in stripe cache. What we really need is an index.
which is hardly an answer. Why cannot you keep all the data in the stripe
cache? How much data is there? How much memory can you afford to dedicate?
You must have some very long sustained bursts of writes which are much faster
than the RAID can accept in order to not be able to keep everything in memory.
Your cache layout seems very rigid. I would much rather a layout that was
very general and flexible. If you want to always allocate a chunk at a time
then fine, but don't force that on the cache layout.
The log really should be very simple. A block describing what comes next,
then lots of data/parity. Then another block and more data etc etc.
Each metadata block points to the next one.
If you need an index of the cache, you keep that in memory. On restart, you
read all of the metadata blocks and built up the index.
I think that space in the log should be reclaimed in exactly the order that
it is written, so the active part of the log is contiguous. Obviously
individual blocks become inactive in arbitrary order as they are written to
the RAID, but each extent of the log becomes free in order.
If you want that to happen out of order, you would need to present a very
good reason.
Best to start as simple as possible....
NeilBrown
>
> Thanks,
> Shaohua
>
> Main goal is to aggregate write IO to hopefully make full stripe IO and fix the
> write hole issue. This might speed up read too, but it's not optimized for
> read, eg, we don't proactivly cache data for read. The aggregation makes a lot
> of sense for workloads which sequentially write to several files. Such
> workloads are popular in today's datacenter.
>
> Here cache = cache disk, generally SSD. raid = raid array or raid disks
> (excluding cache disk)
> -------------------------
> cache layout will like this:
>
> |super|chunk descriptor|chunk data|
>
> We divide cache to equal sized chunks. each chunk will have a descriptor.
> Its size will be raid_chunk_size * raid_disks. That is the cache chunk can
> store a whole raid chunk data and parity.
>
> Write IO will store to cache chunks first and then flush to raid chunks. We use
> fixed size chunk:
> -manage cache space easily. We don't need a complex tree-like index
>
> -flush data from cache to raid easily. data and parity are in the same chunk
>
> -reclaim space is easy. when there is no free chunk in cache, we must try to
> free some chunks, eg, reclaim. We do reclaim in chunk unit. reclaim a chunk
> just means flush the chunk from cache to raid. If we use complex data
> structure, we will need garbage collection and so on.
>
> -The downside is we waste space. Eg, a single 4k data will use a whole chunk in
> cache. But we can reclaim chunks with low utilization quickly to mitgate this
> issue partially.
>
> --------------------
> chunk descriptor looks like this:
> chunk_desc {
> u64 seq;
> u64 raid_chunk_index;
> u32 state;
> u8 bitmaps[];
> }
>
> seq: seq can be used to implement LRU-like algorithm for chunk reclaim. Every
> time data is written to the chunk, we update the chunk's seq. When we flush a
> chunk from cache to raid, we freeze the chunk (eg, the chunk can't accept new
> IO). If there is new IO, we write the new IO to another chunk. The new chunk
> will have a bigger seq than original chunk. crash and reboot can use the seq to
> detinguish which chunk is newer.
>
> raid_chunk_index: where the chunk should be flushed to raid
>
> state: chunk state. Currently I defined 3 states
> -FREE, the chunk is free
> -RUNNING, the chunk maps to raid chunk and accepts new IO
> -PARITY_INCORE, the chunk has both data and parity stored in cache
>
> bitmaps: each page of data and parity has one bit. 1 means present. Store data
> bits first.
>
> -----IO READ PATH------
> IO READ will check each chunk desc. If data is present in cache, dispatch to
> cache. otherwise to raid.
>
> -----IO WRITE PATH------
> 1. find or create a chunk in cache
> 2. write to cache
> 3. write descriptor
>
> We write descriptor immediately in asynchronous way to reduce data loss, the
> chunk will be RUNNING state.
>
> -For normal write, IO return after 2. This will cut latency too. If there is a
> crash, the chunk state might be FREE or bitmap isn't set. In either case, this
> is the first write to the chunk, IO READ will read raid and get old data. We
> meet the symantics. If data isn't in cache, we will read old data in cache, we
> meet the symantics too.
>
> -For FUA write, 2 will be a FUA write. When 2 finishes, run 3 with FUA. IO
> return after 3. Crash after IO return deosn't impact symantics. We will read
> old or new data if crash happens before IO return, which is the similar like
> the normal write case.
>
> -For FLUSH, wait all previous descriptor write finish and then flush cache disk
> cache. In this way, we guarantee all previous write hit cache.
>
> -----chunk reclaim--------
> 1. select a chunk
> 2. freeze the chunk
> 3. copy chunk data from cache to raid, so stripe state machine runs, eg,
> calculate parity and so on
> 4. Hook to raid5 run_io. We write parity to cache
> 5. flush cache disk cache
> 6. mark descriptor PARITY_INCORE, and WRITE_FUA to cache
> 7. raid5 run_io continue run. data and parity write to raid disks
> 8. flush all raid disk cache
> 9. mark descriptor FREE, WRITE_FUA to cache
>
> We will batch several chunks for reclaim for better performance. FUA write can
> be replaced with FLUSH too.
>
> If there is a crash before 6, descriptor state will be RUNNING. Recovery just
> need discard the parity bitmap. If there is a crash before 9, descriptor state
> will be PARITY_INCORE, recovery must copy both data and parity to raid.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2][v2] blk-plug: don't flush nested plug lists
From: Dave Chinner @ 2015-04-09 0:54 UTC (permalink / raw)
To: Jeff Moyer
Cc: linux-kernel, dm-devel, xen-devel, linux-raid, linux-scsi,
target-devel, linux-fsdevel, linux-aio, linux-btrfs, linux-ext4,
linux-f2fs-devel, cluster-devel, linux-nfs, linux-mm
In-Reply-To: <20150408230203.GG15810@dastard>
[ Sending again with a trimmed CC list to just the lists. Jeff - cc
lists that large get blocked by mailing lists... ]
On Tue, Apr 07, 2015 at 02:55:13PM -0400, Jeff Moyer wrote:
> The way the on-stack plugging currently works, each nesting level
> flushes its own list of I/Os. This can be less than optimal (read
> awful) for certain workloads. For example, consider an application
> that issues asynchronous O_DIRECT I/Os. It can send down a bunch of
> I/Os together in a single io_submit call, only to have each of them
> dispatched individually down in the bowels of the dirct I/O code.
> The reason is that there are blk_plug-s instantiated both at the upper
> call site in do_io_submit and down in do_direct_IO. The latter will
> submit as little as 1 I/O at a time (if you have a small enough I/O
> size) instead of performing the batching that the plugging
> infrastructure is supposed to provide.
I'm wondering what impact this will have on filesystem metadata IO
that needs to be issued immediately. e.g. we are doing writeback, so
there is a high level plug in place and we need to page in btree
blocks to do extent allocation. We do readahead at this point,
but it looks like this change will prevent the readahead from being
issued by the unplug in xfs_buf_iosubmit().
So while I can see how this can make your single microbenchmark
better (because it's only doing concurrent direct IO to the block
device and hence there are no dependencies between individual IOs),
I have significant reservations that it's actually a win for
filesystem-based workloads where we need direct control of flushing
to minimise IO latency due to IO dependencies...
Patches like this one:
https://lkml.org/lkml/2015/3/20/442
show similar real-world workload improvements to your patchset by
being smarter about using high level plugging to enable cross-file
merging of IO, but it still relies on the lower layers of plugging
to resolve latency bubbles caused by IO dependencies in the
filesystems.
> NOTE TO SUBSYSTEM MAINTAINERS: Before this patch, blk_finish_plug
> would always flush the plug list. After this patch, this is only the
> case for the outer-most plug. If you require the plug list to be
> flushed, you should be calling blk_flush_plug(current). Btrfs and dm
> maintainers should take a close look at this patch and ensure they get
> the right behavior in the end.
IOWs, you are saying we need to change all our current unplugs to
blk_flush_plug(current) to *try* to maintain the same behaviour as
we currently have? I say *try*, because no instead of just flushing
the readahead IO on the plug, we'll also flush all the queued data
writeback IO onthe high level plug. We don't actually want to do
that; we only want to submit the readahead and not the bulk IO that
will delay the latency sensitive dependent IOs....
If that is the case, shouldn't you actually be trying to fix the
specific plugging problem you've identified (i.e. do_direct_IO() is
flushing far too frequently) rather than making a sweeping
generalisation that the IO stack plugging infrastructure
needs fundamental change?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: [RFC] raid5: add a log device to fix raid5/6 write hole issue
From: Shaohua Li @ 2015-04-09 0:43 UTC (permalink / raw)
To: NeilBrown; +Cc: dan.j.williams, linux-raid, songliubraving, Kernel-team
In-Reply-To: <20150402040749.GA4025688@devbig257.prn2.facebook.com>
Hi,
This is what I'm working on now, and hopefully had the basic code
running next week. The new design will do cache and fix the write hole
issue too. Before I post the code out, I'd like to check if the design
has obvious issues.
Thanks,
Shaohua
Main goal is to aggregate write IO to hopefully make full stripe IO and fix the
write hole issue. This might speed up read too, but it's not optimized for
read, eg, we don't proactivly cache data for read. The aggregation makes a lot
of sense for workloads which sequentially write to several files. Such
workloads are popular in today's datacenter.
Here cache = cache disk, generally SSD. raid = raid array or raid disks
(excluding cache disk)
-------------------------
cache layout will like this:
|super|chunk descriptor|chunk data|
We divide cache to equal sized chunks. each chunk will have a descriptor.
Its size will be raid_chunk_size * raid_disks. That is the cache chunk can
store a whole raid chunk data and parity.
Write IO will store to cache chunks first and then flush to raid chunks. We use
fixed size chunk:
-manage cache space easily. We don't need a complex tree-like index
-flush data from cache to raid easily. data and parity are in the same chunk
-reclaim space is easy. when there is no free chunk in cache, we must try to
free some chunks, eg, reclaim. We do reclaim in chunk unit. reclaim a chunk
just means flush the chunk from cache to raid. If we use complex data
structure, we will need garbage collection and so on.
-The downside is we waste space. Eg, a single 4k data will use a whole chunk in
cache. But we can reclaim chunks with low utilization quickly to mitgate this
issue partially.
--------------------
chunk descriptor looks like this:
chunk_desc {
u64 seq;
u64 raid_chunk_index;
u32 state;
u8 bitmaps[];
}
seq: seq can be used to implement LRU-like algorithm for chunk reclaim. Every
time data is written to the chunk, we update the chunk's seq. When we flush a
chunk from cache to raid, we freeze the chunk (eg, the chunk can't accept new
IO). If there is new IO, we write the new IO to another chunk. The new chunk
will have a bigger seq than original chunk. crash and reboot can use the seq to
detinguish which chunk is newer.
raid_chunk_index: where the chunk should be flushed to raid
state: chunk state. Currently I defined 3 states
-FREE, the chunk is free
-RUNNING, the chunk maps to raid chunk and accepts new IO
-PARITY_INCORE, the chunk has both data and parity stored in cache
bitmaps: each page of data and parity has one bit. 1 means present. Store data
bits first.
-----IO READ PATH------
IO READ will check each chunk desc. If data is present in cache, dispatch to
cache. otherwise to raid.
-----IO WRITE PATH------
1. find or create a chunk in cache
2. write to cache
3. write descriptor
We write descriptor immediately in asynchronous way to reduce data loss, the
chunk will be RUNNING state.
-For normal write, IO return after 2. This will cut latency too. If there is a
crash, the chunk state might be FREE or bitmap isn't set. In either case, this
is the first write to the chunk, IO READ will read raid and get old data. We
meet the symantics. If data isn't in cache, we will read old data in cache, we
meet the symantics too.
-For FUA write, 2 will be a FUA write. When 2 finishes, run 3 with FUA. IO
return after 3. Crash after IO return deosn't impact symantics. We will read
old or new data if crash happens before IO return, which is the similar like
the normal write case.
-For FLUSH, wait all previous descriptor write finish and then flush cache disk
cache. In this way, we guarantee all previous write hit cache.
-----chunk reclaim--------
1. select a chunk
2. freeze the chunk
3. copy chunk data from cache to raid, so stripe state machine runs, eg,
calculate parity and so on
4. Hook to raid5 run_io. We write parity to cache
5. flush cache disk cache
6. mark descriptor PARITY_INCORE, and WRITE_FUA to cache
7. raid5 run_io continue run. data and parity write to raid disks
8. flush all raid disk cache
9. mark descriptor FREE, WRITE_FUA to cache
We will batch several chunks for reclaim for better performance. FUA write can
be replaced with FLUSH too.
If there is a crash before 6, descriptor state will be RUNNING. Recovery just
need discard the parity bitmap. If there is a crash before 9, descriptor state
will be PARITY_INCORE, recovery must copy both data and parity to raid.
^ permalink raw reply
* Re: [PATCH 1/3] PM suspend/hibernate: Call notifier after freezing processes
From: Rafael J. Wysocki @ 2015-04-09 0:28 UTC (permalink / raw)
To: Pali Rohár
Cc: Alasdair Kergon, Mike Snitzer, Neil Brown, Len Brown,
Pavel Machek, dm-devel, linux-raid, linux-kernel, linux-pm
In-Reply-To: <1428254419-7334-2-git-send-email-pali.rohar@gmail.com>
On Sunday, April 05, 2015 07:20:17 PM Pali Rohár wrote:
> To prevent race conditions on userspace processes with I/O some taks must be
> called after processes are freezed. This patch adds new events which are
> delivered by pm_notifier_call_chain() after freezing processes when doing
> suspend or hibernate action.
>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Please don't add more notifiers. Just call whatever you need directly from
where you need to call that.
If that is device-related, try to use device PM suspend/hibernate callbacks
instead.
> ---
> include/linux/suspend.h | 2 ++
> kernel/power/hibernate.c | 2 ++
> kernel/power/suspend.c | 4 +++-
> 3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 5efe743..bc743c8 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -368,6 +368,8 @@ static inline bool hibernation_available(void) { return false; }
> #define PM_POST_SUSPEND 0x0004 /* Suspend finished */
> #define PM_RESTORE_PREPARE 0x0005 /* Going to restore a saved image */
> #define PM_POST_RESTORE 0x0006 /* Restore failed */
> +#define PM_HIBERNATION_AFTER_FREEZE 0x0007 /* After hibernation freeze */
> +#define PM_SUSPEND_AFTER_FREEZE 0x0008 /* After suspend freeze */
>
> extern struct mutex pm_mutex;
>
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 2329daa..184f7ee 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -671,6 +671,8 @@ int hibernate(void)
> if (error)
> goto Exit;
>
> + pm_notifier_call_chain(PM_HIBERNATION_AFTER_FREEZE);
> +
> lock_device_hotplug();
> /* Allocate memory management structures */
> error = create_basic_memory_bitmaps();
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index b7d6b3a..1776938 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -268,8 +268,10 @@ static int suspend_prepare(suspend_state_t state)
> trace_suspend_resume(TPS("freeze_processes"), 0, true);
> error = suspend_freeze_processes();
> trace_suspend_resume(TPS("freeze_processes"), 0, false);
> - if (!error)
> + if (!error) {
> + pm_notifier_call_chain(PM_SUSPEND_AFTER_FREEZE);
> return 0;
> + }
>
> suspend_stats.failed_freeze++;
> dpm_save_failed_step(SUSPEND_FREEZE);
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH 4/4] md-cluster: re-add
From: NeilBrown @ 2015-04-08 23:55 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: linux-raid, GQJiang
In-Reply-To: <20150408192414.GA9693@shrek.lan>
[-- Attachment #1: Type: text/plain, Size: 7857 bytes --]
On Wed, 8 Apr 2015 14:24:14 -0500 Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> This extends the capabilites of re-adding a failed device
> to the clustering environment.
>
> A new function gather_bitmaps gathers set bits from bitmaps of
> all nodes, sends a message to all nodes to readd the disk
> and then initiates the recovery process.
>
> Question: Do you see a race in sending a READD and then performing
> the bitmap resync/recovery? Should the initiating node perform the
> recovery before sending the READD message? The recovery will send a
> METADATA_UPDATE anyways.
The RE-ADD has to happen *before* the bitmaps are gathered.
After the RE-ADD, all writes will go to the new device.
Any write before that RE-ADD will be recorded in the bitmap.
To ensure that the recovery handles all regions affected by writes, it needs
to know about all writes that didn't go to the new device. So it needs to
collect bitmaps only once new writes have started going to the new device.
Is that clear? If not, I'll try again.
NeilBrown
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> drivers/md/bitmap.c | 20 +++++++++++---------
> drivers/md/bitmap.h | 2 +-
> drivers/md/md-cluster.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-
> drivers/md/md-cluster.h | 1 +
> drivers/md/md.c | 2 ++
> 5 files changed, 64 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> index eccfa27..3e69583 100644
> --- a/drivers/md/bitmap.c
> +++ b/drivers/md/bitmap.c
> @@ -1869,7 +1869,7 @@ EXPORT_SYMBOL_GPL(bitmap_load);
> * to our bitmap
> */
> int bitmap_copy_from_slot(struct mddev *mddev, int slot,
> - sector_t *low, sector_t *high)
> + sector_t *low, sector_t *high, bool clear_bits)
> {
> int rv = 0, i, j;
> sector_t block, lo = 0, hi = 0;
> @@ -1896,14 +1896,16 @@ int bitmap_copy_from_slot(struct mddev *mddev, int slot,
> }
> }
>
> - bitmap_update_sb(bitmap);
> - /* Setting this for the ev_page should be enough.
> - * And we do not require both write_all and PAGE_DIRT either
> - */
> - for (i = 0; i < bitmap->storage.file_pages; i++)
> - set_page_attr(bitmap, i, BITMAP_PAGE_DIRTY);
> - bitmap_write_all(bitmap);
> - bitmap_unplug(bitmap);
> + if (clear_bits) {
> + bitmap_update_sb(bitmap);
> + /* Setting this for the ev_page should be enough.
> + * And we do not require both write_all and PAGE_DIRT either
> + */
> + for (i = 0; i < bitmap->storage.file_pages; i++)
> + set_page_attr(bitmap, i, BITMAP_PAGE_DIRTY);
> + bitmap_write_all(bitmap);
> + bitmap_unplug(bitmap);
> + }
> *low = lo;
> *high = hi;
> err:
> diff --git a/drivers/md/bitmap.h b/drivers/md/bitmap.h
> index e838ea7..74bffc7 100644
> --- a/drivers/md/bitmap.h
> +++ b/drivers/md/bitmap.h
> @@ -263,7 +263,7 @@ void bitmap_daemon_work(struct mddev *mddev);
> int bitmap_resize(struct bitmap *bitmap, sector_t blocks,
> int chunksize, int init);
> int bitmap_copy_from_slot(struct mddev *mddev, int slot,
> - sector_t *lo, sector_t *hi);
> + sector_t *lo, sector_t *hi, bool clear_bits);
> #endif
>
> #endif
> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index d036c83..afffbee 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -50,6 +50,7 @@ struct md_cluster_info {
> /* dlm lock space and resources for clustered raid. */
> dlm_lockspace_t *lockspace;
> int slot_number;
> + int total_slots;
> struct completion completion;
> struct dlm_lock_resource *sb_lock;
> struct mutex sb_mutex;
> @@ -73,6 +74,7 @@ enum msg_type {
> RESYNCING,
> NEWDISK,
> REMOVE,
> + READD,
> };
>
> struct cluster_msg {
> @@ -267,7 +269,7 @@ void recover_bitmaps(struct md_thread *thread)
> str, ret);
> goto clear_bit;
> }
> - ret = bitmap_copy_from_slot(mddev, slot, &lo, &hi);
> + ret = bitmap_copy_from_slot(mddev, slot, &lo, &hi, true);
> if (ret) {
> pr_err("md-cluster: Could not copy data from bitmap %d\n", slot);
> goto dlm_unlock;
> @@ -427,6 +429,17 @@ static void process_remove_disk(struct mddev *mddev, struct cluster_msg *msg)
> pr_warn("%s: %d Could not find disk with uuid: %s", __func__, __LINE__, pretty_uuid(uuid, msg->uuid));
> }
>
> +static void process_readd_disk(struct mddev *mddev, struct cluster_msg *msg)
> +{
> + struct md_rdev *rdev = find_rdev_uuid(mddev, msg->uuid);
> + char uuid[32];
> +
> + if (rdev)
> + clear_bit(Faulty, &rdev->flags);
> + else
> + pr_warn("%s: %d Could not find disk with uuid: %s", __func__, __LINE__, pretty_uuid(uuid, msg->uuid));
> +}
> +
> static void process_recvd_msg(struct mddev *mddev, struct cluster_msg *msg)
> {
> switch (msg->type) {
> @@ -451,6 +464,11 @@ static void process_recvd_msg(struct mddev *mddev, struct cluster_msg *msg)
> __func__, __LINE__, msg->slot);
> process_remove_disk(mddev, msg);
> break;
> + case READD:
> + pr_info("%s: %d Received READD from %d\n",
> + __func__, __LINE__, msg->slot);
> + process_readd_disk(mddev, msg);
> + break;
> default:
> pr_warn("%s:%d Received unknown message from %d\n",
> __func__, __LINE__, msg->slot);
> @@ -653,6 +671,7 @@ static int join(struct mddev *mddev, int nodes)
> ret = -ERANGE;
> goto err;
> }
> + cinfo->total_slots = nodes;
> cinfo->sb_lock = lockres_init(mddev, "cmd-super",
> NULL, 0);
> if (!cinfo->sb_lock) {
> @@ -900,6 +919,34 @@ static int remove_disk(struct mddev *mddev, struct md_rdev *rdev)
> return __sendmsg(cinfo, &cmsg);
> }
>
> +static int gather_bitmaps(struct md_rdev *rdev)
> +{
> + int sn, err;
> + sector_t lo, hi;
> + struct cluster_msg cmsg;
> + struct mddev *mddev = rdev->mddev;
> + struct md_cluster_info *cinfo = mddev->cluster_info;
> + struct mdp_superblock_1 *sb = page_address(rdev->sb_page);
> + char *uuid = sb->device_uuid;
> +
> + for (sn = 0; sn < cinfo->total_slots; sn++) {
> + if (sn == (cinfo->slot_number - 1))
> + continue;
> + err = bitmap_copy_from_slot(mddev, sn, &lo, &hi, false);
> + if (err) {
> + pr_warn("md-cluster: Could not gather bitmaps from slot %d", sn);
> + goto out;
> + }
> + if ((hi > 0) && (lo < mddev->recovery_cp))
> + mddev->recovery_cp = lo;
> + }
> + cmsg.type = READD;
> + memcpy(cmsg.uuid, uuid, 16);
> + err = sendmsg(cinfo, &cmsg);
> +out:
> + return err;
> +}
> +
> static struct md_cluster_operations cluster_ops = {
> .join = join,
> .leave = leave,
> @@ -915,6 +962,7 @@ static struct md_cluster_operations cluster_ops = {
> .add_new_disk_finish = add_new_disk_finish,
> .new_disk_ack = new_disk_ack,
> .remove_disk = remove_disk,
> + .gather_bitmaps = gather_bitmaps,
> };
>
> static int __init cluster_init(void)
> diff --git a/drivers/md/md-cluster.h b/drivers/md/md-cluster.h
> index 71e5143..6817ee0 100644
> --- a/drivers/md/md-cluster.h
> +++ b/drivers/md/md-cluster.h
> @@ -23,6 +23,7 @@ struct md_cluster_operations {
> int (*add_new_disk_finish)(struct mddev *mddev);
> int (*new_disk_ack)(struct mddev *mddev, bool ack);
> int (*remove_disk)(struct mddev *mddev, struct md_rdev *rdev);
> + int (*gather_bitmaps)(struct md_rdev *rdev);
> };
>
> #endif /* _MD_CLUSTER_H */
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 83a8e91..a233c09 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -2847,6 +2847,8 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len)
> err = 0;
> }
> } else if (cmd_match(buf, "re-add")) {
> + if (mddev_is_clustered(rdev->mddev))
> + md_cluster_ops->gather_bitmaps(rdev);
> clear_bit(Faulty, &rdev->flags);
> err = add_bound_rdev(rdev);
> }
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [PATCH 3/4] re-add a failed disk
From: NeilBrown @ 2015-04-08 23:32 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: linux-raid, GQJiang
In-Reply-To: <20150408192337.GA9686@shrek.lan>
[-- Attachment #1: Type: text/plain, Size: 4147 bytes --]
On Wed, 8 Apr 2015 14:23:37 -0500 Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> This adds the capability of re-adding a failed disk by
> writing "re-add" to /sys/block/mdXX/md/dev-YYY/state.
>
> This works for generic md, and is not (completely)
> related to clustering.
Again, more words here would help.
Explain why "remove, then add" isn't sufficient.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> drivers/md/md.c | 56 ++++++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 0c65e51..83a8e91 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -79,6 +79,7 @@ static struct workqueue_struct *md_misc_wq;
>
> static int remove_and_add_spares(struct mddev *mddev,
> struct md_rdev *this);
> +static int add_bound_rdev(struct md_rdev *);
>
> #define MD_BUG(x...) { printk("md: bug in file %s, line %d\n", __FILE__, __LINE__); md_print_devices(); }
>
> @@ -2845,6 +2846,9 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len)
> clear_bit(Replacement, &rdev->flags);
> err = 0;
> }
> + } else if (cmd_match(buf, "re-add")) {
> + clear_bit(Faulty, &rdev->flags);
> + err = add_bound_rdev(rdev);
Shouldn't this fail if the device isn't Faulty, of if raid_disk isn't '-1' ??
> }
> if (!err)
> sysfs_notify_dirent_safe(rdev->sysfs_state);
> @@ -5861,6 +5865,37 @@ static int get_disk_info(struct mddev * mddev, void __user * arg)
> return 0;
> }
>
> +static int add_bound_rdev(struct md_rdev *rdev)
> +{
> + struct mddev *mddev = rdev->mddev;
> + int err = 0;
> +
> + if (!mddev->pers->hot_remove_disk) {
> + /* If there is hot_add_disk but no hot_remove_disk
> + * then added disks for geometry changes,
> + * and should be added immediately.
> + */
> + super_types[mddev->major_version].
> + validate_super(mddev, rdev);
> + err = mddev->pers->hot_add_disk(mddev, rdev);
> + if (err)
> + unbind_rdev_from_array(rdev);
> + }
> + if (err)
> + export_rdev(rdev);
> + else
> + sysfs_notify_dirent_safe(rdev->sysfs_state);
> +
> + set_bit(MD_CHANGE_DEVS, &mddev->flags);
> + if (mddev->degraded)
> + set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> + set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> + if (!err)
> + md_new_event(mddev);
> + md_wakeup_thread(mddev->thread);
> + return err;
> +}
> +
> static int add_new_disk(struct mddev * mddev, mdu_disk_info_t *info)
> {
> char b[BDEVNAME_SIZE], b2[BDEVNAME_SIZE];
> @@ -5989,29 +6024,10 @@ static int add_new_disk(struct mddev * mddev, mdu_disk_info_t *info)
>
> rdev->raid_disk = -1;
> err = bind_rdev_to_array(rdev, mddev);
> - if (!err && !mddev->pers->hot_remove_disk) {
> - /* If there is hot_add_disk but no hot_remove_disk
> - * then added disks for geometry changes,
> - * and should be added immediately.
> - */
> - super_types[mddev->major_version].
> - validate_super(mddev, rdev);
> - err = mddev->pers->hot_add_disk(mddev, rdev);
> - if (err)
> - unbind_rdev_from_array(rdev);
> - }
> if (err)
> export_rdev(rdev);
> else
> - sysfs_notify_dirent_safe(rdev->sysfs_state);
> -
> - set_bit(MD_CHANGE_DEVS, &mddev->flags);
> - if (mddev->degraded)
> - set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> - set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> - if (!err)
> - md_new_event(mddev);
> - md_wakeup_thread(mddev->thread);
> + err = add_bound_rdev(rdev);
> if (mddev_is_clustered(mddev) &&
> (info->state & (1 << MD_DISK_CLUSTER_ADD)))
> md_cluster_ops->add_new_disk_finish(mddev);
This is a rather nice clean-up. Possibly it could go in a separate patch -
or maybe not.
Also I think there is more room for improvement.
There is only one place in add_bound_rdev() that can set 'err', and I think
that when that happens we can just clean up and return. There is no real
need for the 'set_bit's or the wakeup.
Would you mind cleaning up a bit more?
Thanks,
NeilBrown
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [PATCH 2/4] md-cluster: remove capabilities
From: NeilBrown @ 2015-04-08 23:24 UTC (permalink / raw)
To: Goldwyn Rodrigues; +Cc: linux-raid, GQJiang
In-Reply-To: <20150408192247.GA9682@shrek.lan>
[-- Attachment #1: Type: text/plain, Size: 5181 bytes --]
On Wed, 8 Apr 2015 14:22:47 -0500 Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Hi Goldwyn,
where really need to be more words here. What is the purpose of this
"remove capabilities"? How are they used?
> ---
> drivers/md/md-cluster.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> drivers/md/md-cluster.h | 1 +
> drivers/md/md.c | 24 +++++++++++++++---------
> drivers/md/md.h | 1 +
> 4 files changed, 64 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index 96679b2..d036c83 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -72,6 +72,7 @@ enum msg_type {
> METADATA_UPDATED = 0,
> RESYNCING,
> NEWDISK,
> + REMOVE,
> };
>
> struct cluster_msg {
> @@ -186,6 +187,20 @@ static char *pretty_uuid(char *dest, char *src)
> return dest;
> }
>
> +static struct md_rdev *find_rdev_uuid(struct mddev *mddev, char *uuid)
> +{
> + struct md_rdev *rdev;
> + struct mdp_superblock_1 *sb;
> +
> + rdev_for_each_rcu(rdev, mddev) {
> + sb = page_address(rdev->sb_page);
> + if (!strncmp(uuid, sb->device_uuid, 16)) {
> + return rdev;
> + }
> + }
> + return NULL;
> +}
I'm not at all comfortable about this.
Any code that "knows" about a particular metadata format should be accessed
through the super_types[] array.
I think I would rather you used 'desc_nr' to identify the device to be
removed. This number is determined from the metadata so every node will see
the same number for the same device. And it is independent of metadata type.
> +
> static void add_resync_info(struct mddev *mddev, struct dlm_lock_resource *lockres,
> sector_t lo, sector_t hi)
> {
> @@ -401,6 +416,17 @@ static void process_metadata_update(struct mddev *mddev, struct cluster_msg *msg
> dlm_lock_sync(cinfo->no_new_dev_lockres, DLM_LOCK_CR);
> }
>
> +static void process_remove_disk(struct mddev *mddev, struct cluster_msg *msg)
> +{
> + struct md_rdev *rdev = find_rdev_uuid(mddev, msg->uuid);
> + char uuid[32];
> +
> + if (rdev)
> + md_kick_rdev_from_array(rdev);
> + else
> + pr_warn("%s: %d Could not find disk with uuid: %s", __func__, __LINE__, pretty_uuid(uuid, msg->uuid));
> +}
> +
> static void process_recvd_msg(struct mddev *mddev, struct cluster_msg *msg)
> {
> switch (msg->type) {
> @@ -419,6 +445,15 @@ static void process_recvd_msg(struct mddev *mddev, struct cluster_msg *msg)
> pr_info("%s: %d Received message: NEWDISK from %d\n",
> __func__, __LINE__, msg->slot);
> process_add_new_disk(mddev, msg);
> + break;
> + case REMOVE:
> + pr_info("%s: %d Received REMOVE from %d\n",
> + __func__, __LINE__, msg->slot);
> + process_remove_disk(mddev, msg);
> + break;
> + default:
> + pr_warn("%s:%d Received unknown message from %d\n",
> + __func__, __LINE__, msg->slot);
> };
> }
>
> @@ -854,6 +889,17 @@ static int new_disk_ack(struct mddev *mddev, bool ack)
> return 0;
> }
>
> +static int remove_disk(struct mddev *mddev, struct md_rdev *rdev)
> +{
> + struct cluster_msg cmsg;
> + struct md_cluster_info *cinfo = mddev->cluster_info;
> + struct mdp_superblock_1 *sb = page_address(rdev->sb_page);
> + char *uuid = sb->device_uuid;
> + cmsg.type = REMOVE;
> + memcpy(cmsg.uuid, uuid, 16);
> + return __sendmsg(cinfo, &cmsg);
> +}
> +
> static struct md_cluster_operations cluster_ops = {
> .join = join,
> .leave = leave,
> @@ -868,6 +914,7 @@ static struct md_cluster_operations cluster_ops = {
> .add_new_disk_start = add_new_disk_start,
> .add_new_disk_finish = add_new_disk_finish,
> .new_disk_ack = new_disk_ack,
> + .remove_disk = remove_disk,
> };
>
> static int __init cluster_init(void)
> diff --git a/drivers/md/md-cluster.h b/drivers/md/md-cluster.h
> index 7417133..71e5143 100644
> --- a/drivers/md/md-cluster.h
> +++ b/drivers/md/md-cluster.h
> @@ -22,6 +22,7 @@ struct md_cluster_operations {
> int (*add_new_disk_start)(struct mddev *mddev, struct md_rdev *rdev);
> int (*add_new_disk_finish)(struct mddev *mddev);
> int (*new_disk_ack)(struct mddev *mddev, bool ack);
> + int (*remove_disk)(struct mddev *mddev, struct md_rdev *rdev);
> };
>
> #endif /* _MD_CLUSTER_H */
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index bc11551..0c65e51 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -2291,11 +2291,12 @@ static void export_rdev(struct md_rdev * rdev)
> kobject_put(&rdev->kobj);
> }
>
> -static void kick_rdev_from_array(struct md_rdev * rdev)
> +void md_kick_rdev_from_array(struct md_rdev * rdev)
> {
> unbind_rdev_from_array(rdev);
> export_rdev(rdev);
> }
> +EXPORT_SYMBOL_GPL(md_kick_rdev_from_array);
You didn't create this patch against upstream code did you? The space
between '*' and 'rdev' disappeared in 3.18.
I think I would prefer a separate patch which renames and exports
kick_rdev_from_array, and then a much smaller (easier to review) patch which
adds the 'remove capabilities'.
Thanks,
NeilBrown
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [PATCH] IMSM: Count arrays per orom
From: NeilBrown @ 2015-04-08 23:06 UTC (permalink / raw)
To: Pawel Baldysiak; +Cc: linux-raid, artur.paszkiewicz
In-Reply-To: <20150408094217.10715.28672.stgit@gklab-154-222.intel.com>
[-- Attachment #1: Type: text/plain, Size: 6235 bytes --]
On Wed, 08 Apr 2015 11:42:18 +0200 Pawel Baldysiak
<pawel.baldysiak@intel.com> wrote:
> Active arrays with IMSM metadata are counted per hba so far.
> This is bad due to new functionality of orom shared between multiple
> controllers i.e. more arrays can be created than is supported by orom.
> This patch changes the way of counting arrays, so the result will be
> sum of arrays under every hba supported by specific orom.
>
> Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
> ---
> platform-intel.c | 14 ++++++++-
> platform-intel.h | 1 +
> super-intel.c | 85 +++++++++++++++++++++++++++++++++++++-----------------
> 3 files changed, 71 insertions(+), 29 deletions(-)
>
> diff --git a/platform-intel.c b/platform-intel.c
> index 1e9ddcd..edb8679 100644
> --- a/platform-intel.c
> +++ b/platform-intel.c
> @@ -233,7 +233,7 @@ struct pciExpDataStructFormat {
>
> struct orom_entry *orom_entries;
>
> -const struct imsm_orom *get_orom_by_device_id(__u16 dev_id)
> +const struct orom_entry *get_orom_entry_by_device_id(__u16 dev_id)
> {
> struct orom_entry *entry;
> struct devid_list *devid;
> @@ -241,13 +241,23 @@ const struct imsm_orom *get_orom_by_device_id(__u16 dev_id)
> for (entry = orom_entries; entry; entry = entry->next) {
> for (devid = entry->devid_list; devid; devid = devid->next) {
> if (devid->devid == dev_id)
> - return &entry->orom;
> + return entry;
> }
> }
>
> return NULL;
> }
>
> +const struct imsm_orom *get_orom_by_device_id(__u16 dev_id)
> +{
> + const struct orom_entry *entry = get_orom_entry_by_device_id(dev_id);
> +
> + if (entry)
> + return &entry->orom;
> +
> + return NULL;
> +}
> +
> static struct orom_entry *add_orom(const struct imsm_orom *orom)
> {
> struct orom_entry *list;
> diff --git a/platform-intel.h b/platform-intel.h
> index 631fa76..695d6c6 100644
> --- a/platform-intel.h
> +++ b/platform-intel.h
> @@ -238,5 +238,6 @@ int devt_attached_to_hba(dev_t dev, const char *hba_path);
> char *devt_to_devpath(dev_t dev);
> int path_attached_to_hba(const char *disk_path, const char *hba_path);
> const char *get_sys_dev_type(enum sys_dev_type);
> +const struct orom_entry *get_orom_entry_by_device_id(__u16 dev_id);
> const struct imsm_orom *get_orom_by_device_id(__u16 device_id);
> struct sys_dev *device_by_id(__u16 device_id);
> diff --git a/super-intel.c b/super-intel.c
> index 77df8db..66cc8e5 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -5853,37 +5853,68 @@ count_volumes_list(struct md_list *devlist, char *homehost,
> }
>
> static int
> -count_volumes(char *hba, int dpa, int verbose)
> +count_volumes(struct intel_hba *hba, int dpa, int verbose)
> {
> - struct md_list *devlist = NULL;
> + struct sys_dev *idev, *intel_devices = find_intel_devices();
> int count = 0;
> - int found = 0;;
> + const struct orom_entry *entry;
> + struct devid_list *dv, *devid_list;
>
> - devlist = get_devices(hba);
> - /* if no intel devices return zero volumes */
> - if (devlist == NULL)
> + if (!hba || !hba->path)
> return 0;
>
> - count = active_arrays_by_format("imsm", hba, &devlist, dpa, verbose);
> - dprintf("path: %s active arrays: %d\n", hba, count);
> - if (devlist == NULL)
> + for (idev = intel_devices; idev; idev = idev->next) {
> + if (strstr(idev->path, hba->path))
> + break;
> + }
> +
> + if (!idev || !idev->dev_id)
> return 0;
> - do {
> - found = 0;
> - count += count_volumes_list(devlist,
> - NULL,
> - verbose,
> - &found);
> - dprintf("found %d count: %d\n", found, count);
> - } while (found);
> -
> - dprintf("path: %s total number of volumes: %d\n", hba, count);
> -
> - while(devlist) {
> - struct md_list *dv = devlist;
> - devlist = devlist->next;
> - free(dv->devname);
> - free(dv);
> +
> + entry = get_orom_entry_by_device_id(idev->dev_id);
> +
> + if (!entry || !entry->devid_list)
> + return 0;
> +
> + devid_list = entry->devid_list;
> + for (dv = devid_list; dv; dv = dv->next) {
> +
> + struct md_list *devlist = NULL;
> + struct sys_dev *device = device_by_id(dv->devid);
> + char *hba_path;
> + int found = 0;
> +
> + if (device)
> + hba_path = device->path;
> + else
> + return 0;
> +
> + devlist = get_devices(hba_path);
> + /* if no intel devices return zero volumes */
> + if (devlist == NULL)
> + return 0;
> +
> + count += active_arrays_by_format("imsm", hba_path, &devlist, dpa, verbose);
> + dprintf("path: %s active arrays: %d\n", hba_path, count);
> + if (devlist == NULL)
> + return 0;
> + do {
> + found = 0;
> + count += count_volumes_list(devlist,
> + NULL,
> + verbose,
> + &found);
> + dprintf("found %d count: %d\n", found, count);
> + } while (found);
> +
> + dprintf("path: %s total number of volumes: %d\n", hba_path, count);
> +
> + while (devlist) {
> + struct md_list *dv = devlist;
> + devlist = devlist->next;
> + free(dv->devname);
> + free(dv);
> + }
> }
> return count;
> }
> @@ -6105,7 +6136,7 @@ static int validate_geometry_imsm_volume(struct supertype *st, int level,
> *freesize = maxsize;
>
> if (super->orom) {
> - int count = count_volumes(super->hba->path,
> + int count = count_volumes(super->hba,
> super->orom->dpa, verbose);
> if (super->orom->vphba <= count) {
> pr_vrb(": platform does not support more than %d raid volumes.\n",
> @@ -6261,7 +6292,7 @@ static int validate_geometry_imsm(struct supertype *st, int level, int layout,
> created */
> if (super->orom && freesize) {
> int count;
> - count = count_volumes(super->hba->path,
> + count = count_volumes(super->hba,
> super->orom->dpa, verbose);
> if (super->orom->vphba <= count) {
> pr_vrb(": platform does not support more than %d raid volumes.\n",
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
applied, thanks.
NeilBrown
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2][v2] blk-plug: don't flush nested plug lists
From: Dave Chinner @ 2015-04-08 23:02 UTC (permalink / raw)
To: Jeff Moyer
Cc: Jens Axboe, Ming Lei, Konrad Rzeszutek Wilk, Roger Pau Monn??,
Alasdair Kergon, Mike Snitzer, Neil Brown, Nicholas A. Bellinger,
Alexander Viro, Chris Mason, Josef Bacik, David Sterba,
Theodore Ts'o, Andreas Dilger, Jaegeuk Kim, Changman Lee,
Steven Whitehouse, Mikulas Patocka, Andrew Morton, Rik van Riel,
Johannes Weiner, Mel Gorman, Trond Myklebust
In-Reply-To: <x49wq1nrcoe.fsf_-_@segfault.boston.devel.redhat.com>
On Tue, Apr 07, 2015 at 02:55:13PM -0400, Jeff Moyer wrote:
> The way the on-stack plugging currently works, each nesting level
> flushes its own list of I/Os. This can be less than optimal (read
> awful) for certain workloads. For example, consider an application
> that issues asynchronous O_DIRECT I/Os. It can send down a bunch of
> I/Os together in a single io_submit call, only to have each of them
> dispatched individually down in the bowels of the dirct I/O code.
> The reason is that there are blk_plug-s instantiated both at the upper
> call site in do_io_submit and down in do_direct_IO. The latter will
> submit as little as 1 I/O at a time (if you have a small enough I/O
> size) instead of performing the batching that the plugging
> infrastructure is supposed to provide.
I'm wondering what impact this will have on filesystem metadata IO
that needs to be issued immediately. e.g. we are doing writeback, so
there is a high level plug in place and we need to page in btree
blocks to do extent allocation. We do readahead at this point,
but it looks like this change will prevent the readahead from being
issued by the unplug in xfs_buf_iosubmit().
So while I can see how this can make your single microbenchmark
better (because it's only doing concurrent direct IO to the block
device and hence there are no dependencies between individual IOs),
I have significant reservations that it's actually a win for
filesystem-based workloads where we need direct control of flushing
to minimise IO latency due to IO dependencies...
Patches like this one:
https://lkml.org/lkml/2015/3/20/442
show similar real-world workload improvements to your patchset by
being smarter about using high level plugging to enable cross-file
merging of IO, but it still relies on the lower layers of plugging
to resolve latency bubbles caused by IO dependencies in the
filesystems.
> NOTE TO SUBSYSTEM MAINTAINERS: Before this patch, blk_finish_plug
> would always flush the plug list. After this patch, this is only the
> case for the outer-most plug. If you require the plug list to be
> flushed, you should be calling blk_flush_plug(current). Btrfs and dm
> maintainers should take a close look at this patch and ensure they get
> the right behavior in the end.
IOWs, you are saying we need to change all our current unplugs to
blk_flush_plug(current) to maintain the same behaviour as we
currently have?
If that is the case, shouldn't you actually be trying to fix the
specific plugging problem you've identified (i.e. do_direct_IO() is
flushing far too frequently) rather than making a sweeping
generalisation that the IO stack plugging infrastructure
needs to be fundamentally changed?
Cheers,
Dave.
>
> ---
> Changelog:
> v1->v2: Keep the blk_start_plug interface the same, suggested by Ming Lei.
>
> Test results
> ------------
> Virtio-blk:
>
> unpatched:
>
> job1: (groupid=0, jobs=1): err= 0: pid=8032: Tue Apr 7 13:33:53 2015
> read : io=2736.1MB, bw=280262KB/s, iops=70065, runt= 10000msec
> slat (usec): min=40, max=10472, avg=207.82, stdev=364.02
> clat (usec): min=211, max=35883, avg=14379.83, stdev=2213.95
> lat (usec): min=862, max=36000, avg=14587.72, stdev=2223.80
> clat percentiles (usec):
> | 1.00th=[11328], 5.00th=[12096], 10.00th=[12480], 20.00th=[12992],
> | 30.00th=[13376], 40.00th=[13760], 50.00th=[14144], 60.00th=[14400],
> | 70.00th=[14784], 80.00th=[15168], 90.00th=[15936], 95.00th=[16768],
> | 99.00th=[24448], 99.50th=[25216], 99.90th=[28544], 99.95th=[35072],
> | 99.99th=[36096]
> bw (KB /s): min=265984, max=302720, per=100.00%, avg=280549.84, stdev=10264.36
> lat (usec) : 250=0.01%, 1000=0.01%
> lat (msec) : 2=0.02%, 4=0.02%, 10=0.05%, 20=96.57%, 50=3.34%
> cpu : usr=7.56%, sys=55.57%, ctx=6174, majf=0, minf=523
> IO depths : 1=0.0%, 2=0.0%, 4=0.0%, 8=0.0%, 16=0.1%, 32=0.1%, >=64=100.0%
> submit : 0=0.0%, 4=0.0%, 8=0.0%, 16=100.0%, 32=0.0%, 64=0.0%, >=64=0.0%
> complete : 0=0.0%, 4=0.0%, 8=0.0%, 16=100.0%, 32=0.0%, 64=0.0%, >=64=0.1%
> issued : total=r=700656/w=0/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
> latency : target=0, window=0, percentile=100.00%, depth=1024
>
> Run status group 0 (all jobs):
> READ: io=2736.1MB, aggrb=280262KB/s, minb=280262KB/s, maxb=280262KB/s, mint=10000msec, maxt=10000msec
>
> Disk stats (read/write):
> vdd: ios=695490/0, merge=0/0, ticks=785741/0, in_queue=785442, util=90.69%
>
>
> patched:
> job1: (groupid=0, jobs=1): err= 0: pid=7743: Tue Apr 7 13:19:07 2015
> read : io=8126.6MB, bw=832158KB/s, iops=208039, runt= 10000msec
> slat (usec): min=20, max=14351, avg=55.08, stdev=143.47
> clat (usec): min=283, max=20003, avg=4846.77, stdev=1355.35
> lat (usec): min=609, max=20074, avg=4901.95, stdev=1362.40
> clat percentiles (usec):
> | 1.00th=[ 4016], 5.00th=[ 4048], 10.00th=[ 4080], 20.00th=[ 4128],
> | 30.00th=[ 4192], 40.00th=[ 4192], 50.00th=[ 4256], 60.00th=[ 4512],
> | 70.00th=[ 4896], 80.00th=[ 5664], 90.00th=[ 5920], 95.00th=[ 6752],
> | 99.00th=[11968], 99.50th=[13632], 99.90th=[15552], 99.95th=[17024],
> | 99.99th=[19840]
> bw (KB /s): min=740992, max=896640, per=100.00%, avg=836978.95, stdev=51034.87
> lat (usec) : 500=0.01%, 750=0.01%, 1000=0.01%
> lat (msec) : 4=0.50%, 10=97.79%, 20=1.70%, 50=0.01%
> cpu : usr=20.28%, sys=69.11%, ctx=879, majf=0, minf=522
> IO depths : 1=0.0%, 2=0.0%, 4=0.0%, 8=0.0%, 16=0.1%, 32=0.1%, >=64=100.0%
> submit : 0=0.0%, 4=0.0%, 8=0.0%, 16=100.0%, 32=0.0%, 64=0.0%, >=64=0.0%
> complete : 0=0.0%, 4=0.0%, 8=0.0%, 16=100.0%, 32=0.0%, 64=0.0%, >=64=0.1%
> issued : total=r=2080396/w=0/d=0, short=r=0/w=0/d=0, drop=r=0/w=0/d=0
> latency : target=0, window=0, percentile=100.00%, depth=1024
>
> Run status group 0 (all jobs):
> READ: io=8126.6MB, aggrb=832158KB/s, minb=832158KB/s, maxb=832158KB/s, mint=10000msec, maxt=10000msec
>
> Disk stats (read/write):
> vdd: ios=127877/0, merge=1918166/0, ticks=23118/0, in_queue=23047, util=94.08%
>
> micron p320h:
>
> unpatched:
>
> job1: (groupid=0, jobs=1): err= 0: pid=3244: Tue Apr 7 13:29:14 2015
> read : io=6728.9MB, bw=688968KB/s, iops=172241, runt= 10001msec
> slat (usec): min=43, max=6273, avg=81.79, stdev=125.96
> clat (usec): min=78, max=12485, avg=5852.06, stdev=1154.76
> lat (usec): min=146, max=12572, avg=5933.92, stdev=1163.75
> clat percentiles (usec):
> | 1.00th=[ 4192], 5.00th=[ 4384], 10.00th=[ 4576], 20.00th=[ 5600],
> | 30.00th=[ 5664], 40.00th=[ 5728], 50.00th=[ 5792], 60.00th=[ 5856],
> | 70.00th=[ 6112], 80.00th=[ 6176], 90.00th=[ 6240], 95.00th=[ 6368],
> | 99.00th=[11840], 99.50th=[11968], 99.90th=[12096], 99.95th=[12096],
> | 99.99th=[12224]
> bw (KB /s): min=648328, max=859264, per=98.80%, avg=680711.16, stdev=62016.70
> lat (usec) : 100=0.01%, 250=0.01%, 500=0.01%, 750=0.01%, 1000=0.01%
> lat (msec) : 2=0.01%, 4=0.04%, 10=97.07%, 20=2.87%
> cpu : usr=10.28%, sys=73.61%, ctx=104436, majf=0, minf=6217
> IO depths : 1=0.0%, 2=0.0%, 4=0.0%, 8=0.0%, 16=0.1%, 32=0.1%, >=64=100.0%
> submit : 0=0.0%, 4=0.0%, 8=0.0%, 16=100.0%, 32=0.0%, 64=0.0%, >=64=0.0%
> complete : 0=0.0%, 4=0.0%, 8=0.0%, 16=100.0%, 32=0.0%, 64=0.0%, >=64=0.1%
> issued : total=r=1722592/w=0/d=0, short=r=0/w=0/d=0
> latency : target=0, window=0, percentile=100.00%, depth=1024
>
> Run status group 0 (all jobs):
> READ: io=6728.9MB, aggrb=688967KB/s, minb=688967KB/s, maxb=688967KB/s, mint=10001msec, maxt=10001msec
>
> Disk stats (read/write):
> rssda: ios=1688772/0, merge=0/0, ticks=188820/0, in_queue=188678, util=96.61%
>
> patched:
>
> job1: (groupid=0, jobs=1): err= 0: pid=9531: Tue Apr 7 13:22:28 2015
> read : io=11607MB, bw=1160.6MB/s, iops=297104, runt= 10001msec
> slat (usec): min=21, max=6376, avg=43.05, stdev=81.82
> clat (usec): min=116, max=9844, avg=3393.90, stdev=752.57
> lat (usec): min=167, max=9889, avg=3437.01, stdev=757.02
> clat percentiles (usec):
> | 1.00th=[ 2832], 5.00th=[ 2992], 10.00th=[ 3056], 20.00th=[ 3120],
> | 30.00th=[ 3152], 40.00th=[ 3248], 50.00th=[ 3280], 60.00th=[ 3344],
> | 70.00th=[ 3376], 80.00th=[ 3504], 90.00th=[ 3728], 95.00th=[ 3824],
> | 99.00th=[ 9152], 99.50th=[ 9408], 99.90th=[ 9664], 99.95th=[ 9664],
> | 99.99th=[ 9792]
> bw (MB /s): min= 1139, max= 1183, per=100.00%, avg=1161.07, stdev=10.58
> lat (usec) : 250=0.01%, 500=0.01%, 750=0.01%, 1000=0.01%
> lat (msec) : 2=0.01%, 4=98.31%, 10=1.67%
> cpu : usr=18.59%, sys=66.65%, ctx=55655, majf=0, minf=6218
> IO depths : 1=0.0%, 2=0.0%, 4=0.0%, 8=0.0%, 16=0.1%, 32=0.1%, >=64=100.0%
> submit : 0=0.0%, 4=0.0%, 8=0.0%, 16=100.0%, 32=0.0%, 64=0.0%, >=64=0.0%
> complete : 0=0.0%, 4=0.0%, 8=0.0%, 16=100.0%, 32=0.0%, 64=0.0%, >=64=0.1%
> issued : total=r=2971338/w=0/d=0, short=r=0/w=0/d=0
> latency : target=0, window=0, percentile=100.00%, depth=1024
>
> Run status group 0 (all jobs):
> READ: io=11607MB, aggrb=1160.6MB/s, minb=1160.6MB/s, maxb=1160.6MB/s, mint=10001msec, maxt=10001msec
>
> Disk stats (read/write):
> rssda: ios=183005/0, merge=2745105/0, ticks=31972/0, in_queue=31948, util=97.63%
> ---
> block/blk-core.c | 29 ++++++++++++++++-------------
> block/blk-lib.c | 2 +-
> block/blk-throttle.c | 2 +-
> drivers/block/xen-blkback/blkback.c | 2 +-
> drivers/md/dm-bufio.c | 6 +++---
> drivers/md/dm-crypt.c | 2 +-
> drivers/md/dm-kcopyd.c | 2 +-
> drivers/md/dm-thin.c | 2 +-
> drivers/md/md.c | 2 +-
> drivers/md/raid1.c | 2 +-
> drivers/md/raid10.c | 2 +-
> drivers/md/raid5.c | 4 ++--
> drivers/target/target_core_iblock.c | 2 +-
> fs/aio.c | 2 +-
> fs/block_dev.c | 2 +-
> fs/btrfs/scrub.c | 2 +-
> fs/btrfs/transaction.c | 2 +-
> fs/btrfs/tree-log.c | 12 ++++++------
> fs/btrfs/volumes.c | 6 +++---
> fs/buffer.c | 2 +-
> fs/direct-io.c | 2 +-
> fs/ext4/file.c | 2 +-
> fs/ext4/inode.c | 4 ++--
> fs/f2fs/checkpoint.c | 2 +-
> fs/f2fs/gc.c | 2 +-
> fs/f2fs/node.c | 2 +-
> fs/gfs2/log.c | 2 +-
> fs/hpfs/buffer.c | 2 +-
> fs/jbd/checkpoint.c | 2 +-
> fs/jbd/commit.c | 4 ++--
> fs/jbd2/checkpoint.c | 2 +-
> fs/jbd2/commit.c | 2 +-
> fs/mpage.c | 2 +-
> fs/nfs/blocklayout/blocklayout.c | 4 ++--
> fs/xfs/xfs_buf.c | 4 ++--
> fs/xfs/xfs_dir2_readdir.c | 2 +-
> fs/xfs/xfs_itable.c | 2 +-
> include/linux/blkdev.h | 5 +++--
> mm/madvise.c | 2 +-
> mm/page-writeback.c | 2 +-
> mm/readahead.c | 2 +-
> mm/swap_state.c | 2 +-
> mm/vmscan.c | 2 +-
> 43 files changed, 74 insertions(+), 70 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 794c3e7..fcd9c2f 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -3018,21 +3018,21 @@ void blk_start_plug(struct blk_plug *plug)
> {
> struct task_struct *tsk = current;
>
> + if (tsk->plug) {
> + tsk->plug->depth++;
> + return;
> + }
> +
> + plug->depth = 1;
> INIT_LIST_HEAD(&plug->list);
> INIT_LIST_HEAD(&plug->mq_list);
> INIT_LIST_HEAD(&plug->cb_list);
>
> /*
> - * If this is a nested plug, don't actually assign it. It will be
> - * flushed on its own.
> + * Store ordering should not be needed here, since a potential
> + * preempt will imply a full memory barrier
> */
> - if (!tsk->plug) {
> - /*
> - * Store ordering should not be needed here, since a potential
> - * preempt will imply a full memory barrier
> - */
> - tsk->plug = plug;
> - }
> + tsk->plug = plug;
> }
> EXPORT_SYMBOL(blk_start_plug);
>
> @@ -3177,12 +3177,15 @@ void blk_flush_plug_list(struct blk_plug *plug, bool from_schedule)
> local_irq_restore(flags);
> }
>
> -void blk_finish_plug(struct blk_plug *plug)
> +void blk_finish_plug(void)
> {
> - blk_flush_plug_list(plug, false);
> + struct blk_plug *plug = current->plug;
>
> - if (plug == current->plug)
> - current->plug = NULL;
> + if (--plug->depth > 0)
> + return;
> +
> + blk_flush_plug_list(plug, false);
> + current->plug = NULL;
> }
> EXPORT_SYMBOL(blk_finish_plug);
>
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 7688ee3..ac347d3 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -128,7 +128,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
> */
> cond_resched();
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> /* Wait for bios in-flight */
> if (!atomic_dec_and_test(&bb.done))
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 5b9c6d5..222a77a 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -1281,7 +1281,7 @@ static void blk_throtl_dispatch_work_fn(struct work_struct *work)
> blk_start_plug(&plug);
> while((bio = bio_list_pop(&bio_list_on_stack)))
> generic_make_request(bio);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> }
> }
>
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> index 2a04d34..74bea21 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -1374,7 +1374,7 @@ static int dispatch_rw_block_io(struct xen_blkif *blkif,
> submit_bio(operation, biolist[i]);
>
> /* Let the I/Os go.. */
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> if (operation == READ)
> blkif->st_rd_sect += preq.nr_sects;
> diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> index 86dbbc7..502c63b 100644
> --- a/drivers/md/dm-bufio.c
> +++ b/drivers/md/dm-bufio.c
> @@ -715,7 +715,7 @@ static void __flush_write_list(struct list_head *write_list)
> submit_io(b, WRITE, b->block, write_endio);
> dm_bufio_cond_resched();
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> }
>
> /*
> @@ -1126,7 +1126,7 @@ void dm_bufio_prefetch(struct dm_bufio_client *c,
> &write_list);
> if (unlikely(!list_empty(&write_list))) {
> dm_bufio_unlock(c);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> __flush_write_list(&write_list);
> blk_start_plug(&plug);
> dm_bufio_lock(c);
> @@ -1149,7 +1149,7 @@ void dm_bufio_prefetch(struct dm_bufio_client *c,
> dm_bufio_unlock(c);
>
> flush_plug:
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> }
> EXPORT_SYMBOL_GPL(dm_bufio_prefetch);
>
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 713a962..65d7b72 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -1224,7 +1224,7 @@ pop_from_list:
> rb_erase(&io->rb_node, &write_tree);
> kcryptd_io_write(io);
> } while (!RB_EMPTY_ROOT(&write_tree));
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> }
> return 0;
> }
> diff --git a/drivers/md/dm-kcopyd.c b/drivers/md/dm-kcopyd.c
> index 3a7cade..4a76e42 100644
> --- a/drivers/md/dm-kcopyd.c
> +++ b/drivers/md/dm-kcopyd.c
> @@ -593,7 +593,7 @@ static void do_work(struct work_struct *work)
> process_jobs(&kc->complete_jobs, kc, run_complete_job);
> process_jobs(&kc->pages_jobs, kc, run_pages_job);
> process_jobs(&kc->io_jobs, kc, run_io_job);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> }
>
> /*
> diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
> index 921aafd..be42bf5 100644
> --- a/drivers/md/dm-thin.c
> +++ b/drivers/md/dm-thin.c
> @@ -1824,7 +1824,7 @@ static void process_thin_deferred_bios(struct thin_c *tc)
> dm_pool_issue_prefetches(pool->pmd);
> }
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> }
>
> static int cmp_cells(const void *lhs, const void *rhs)
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 717daad..c4ec179 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -7686,7 +7686,7 @@ void md_do_sync(struct md_thread *thread)
> /*
> * this also signals 'finished resyncing' to md_stop
> */
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> wait_event(mddev->recovery_wait, !atomic_read(&mddev->recovery_active));
>
> /* tell personality that we are finished */
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index d34e238..4f8fad4 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2441,7 +2441,7 @@ static void raid1d(struct md_thread *thread)
> if (mddev->flags & ~(1<<MD_CHANGE_PENDING))
> md_check_recovery(mddev);
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> }
>
> static int init_resync(struct r1conf *conf)
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index a7196c4..92bb5dd 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2835,7 +2835,7 @@ static void raid10d(struct md_thread *thread)
> if (mddev->flags & ~(1<<MD_CHANGE_PENDING))
> md_check_recovery(mddev);
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> }
>
> static int init_resync(struct r10conf *conf)
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index cd2f96b..695bf0f 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -5281,7 +5281,7 @@ static void raid5_do_work(struct work_struct *work)
> pr_debug("%d stripes handled\n", handled);
>
> spin_unlock_irq(&conf->device_lock);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> pr_debug("--- raid5worker inactive\n");
> }
> @@ -5352,7 +5352,7 @@ static void raid5d(struct md_thread *thread)
> spin_unlock_irq(&conf->device_lock);
>
> async_tx_issue_pending_all();
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> pr_debug("--- raid5d inactive\n");
> }
> diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
> index d4a4b0f..17d8730 100644
> --- a/drivers/target/target_core_iblock.c
> +++ b/drivers/target/target_core_iblock.c
> @@ -367,7 +367,7 @@ static void iblock_submit_bios(struct bio_list *list, int rw)
> blk_start_plug(&plug);
> while ((bio = bio_list_pop(list)))
> submit_bio(rw, bio);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> }
>
> static void iblock_end_io_flush(struct bio *bio, int err)
> diff --git a/fs/aio.c b/fs/aio.c
> index f8e52a1..b873698 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1616,7 +1616,7 @@ long do_io_submit(aio_context_t ctx_id, long nr,
> if (ret)
> break;
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> percpu_ref_put(&ctx->users);
> return i ? i : ret;
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 975266b..f5848de 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1609,7 +1609,7 @@ ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
> if (err < 0)
> ret = err;
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> return ret;
> }
> EXPORT_SYMBOL_GPL(blkdev_write_iter);
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index ec57687..f314cfb8 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -3316,7 +3316,7 @@ out:
> scrub_wr_submit(sctx);
> mutex_unlock(&sctx->wr_ctx.wr_lock);
>
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> btrfs_free_path(path);
> btrfs_free_path(ppath);
> return ret < 0 ? ret : 0;
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 8be4278..fee10af 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -983,7 +983,7 @@ static int btrfs_write_and_wait_marked_extents(struct btrfs_root *root,
>
> blk_start_plug(&plug);
> ret = btrfs_write_marked_extents(root, dirty_pages, mark);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> ret2 = btrfs_wait_marked_extents(root, dirty_pages, mark);
>
> if (ret)
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index c5b8ba3..879c7fd 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -2574,7 +2574,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
> blk_start_plug(&plug);
> ret = btrfs_write_marked_extents(log, &log->dirty_log_pages, mark);
> if (ret) {
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> btrfs_abort_transaction(trans, root, ret);
> btrfs_free_logged_extents(log, log_transid);
> btrfs_set_log_full_commit(root->fs_info, trans);
> @@ -2619,7 +2619,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
> if (!list_empty(&root_log_ctx.list))
> list_del_init(&root_log_ctx.list);
>
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> btrfs_set_log_full_commit(root->fs_info, trans);
>
> if (ret != -ENOSPC) {
> @@ -2635,7 +2635,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
> }
>
> if (log_root_tree->log_transid_committed >= root_log_ctx.log_transid) {
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> mutex_unlock(&log_root_tree->log_mutex);
> ret = root_log_ctx.log_ret;
> goto out;
> @@ -2643,7 +2643,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
>
> index2 = root_log_ctx.log_transid % 2;
> if (atomic_read(&log_root_tree->log_commit[index2])) {
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> ret = btrfs_wait_marked_extents(log, &log->dirty_log_pages,
> mark);
> btrfs_wait_logged_extents(trans, log, log_transid);
> @@ -2669,7 +2669,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
> * check the full commit flag again
> */
> if (btrfs_need_log_full_commit(root->fs_info, trans)) {
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> btrfs_wait_marked_extents(log, &log->dirty_log_pages, mark);
> btrfs_free_logged_extents(log, log_transid);
> mutex_unlock(&log_root_tree->log_mutex);
> @@ -2680,7 +2680,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
> ret = btrfs_write_marked_extents(log_root_tree,
> &log_root_tree->dirty_log_pages,
> EXTENT_DIRTY | EXTENT_NEW);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> if (ret) {
> btrfs_set_log_full_commit(root->fs_info, trans);
> btrfs_abort_transaction(trans, root, ret);
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 8222f6f..16db068 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -358,7 +358,7 @@ loop_lock:
> if (pending_bios == &device->pending_sync_bios) {
> sync_pending = 1;
> } else if (sync_pending) {
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> blk_start_plug(&plug);
> sync_pending = 0;
> }
> @@ -415,7 +415,7 @@ loop_lock:
> }
> /* unplug every 64 requests just for good measure */
> if (batch_run % 64 == 0) {
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> blk_start_plug(&plug);
> sync_pending = 0;
> }
> @@ -431,7 +431,7 @@ loop_lock:
> spin_unlock(&device->io_lock);
>
> done:
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> }
>
> static void pending_bios_fn(struct btrfs_work *work)
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 20805db..8181c44 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -758,7 +758,7 @@ static int fsync_buffers_list(spinlock_t *lock, struct list_head *list)
> }
>
> spin_unlock(lock);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> spin_lock(lock);
>
> while (!list_empty(&tmp)) {
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index e181b6b..16f16ed 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -1262,7 +1262,7 @@ do_blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
> if (sdio.bio)
> dio_bio_submit(dio, &sdio);
>
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> /*
> * It is possible that, we return short IO due to end of file.
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 33a09da..3a293eb 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -183,7 +183,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
> ret = err;
> }
> if (o_direct)
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> errout:
> if (aio_mutex)
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 5cb9a21..90ce0cb 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2302,7 +2302,7 @@ static int ext4_writepages(struct address_space *mapping,
>
> blk_start_plug(&plug);
> ret = write_cache_pages(mapping, wbc, __writepage, mapping);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> goto out_writepages;
> }
>
> @@ -2438,7 +2438,7 @@ retry:
> if (ret)
> break;
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> if (!ret && !cycled && wbc->nr_to_write > 0) {
> cycled = 1;
> mpd.last_page = writeback_index - 1;
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 7f794b7..86ba453 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -846,7 +846,7 @@ retry_flush_nodes:
> goto retry_flush_nodes;
> }
> out:
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> return err;
> }
>
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 76adbc3..abeef77 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -678,7 +678,7 @@ static void do_garbage_collect(struct f2fs_sb_info *sbi, unsigned int segno,
> gc_data_segment(sbi, sum->entries, gc_list, segno, gc_type);
> break;
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> stat_inc_seg_count(sbi, GET_SUM_TYPE((&sum->footer)));
> stat_inc_call_count(sbi->stat_info);
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 97bd9d3..c4aa9e2 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1098,7 +1098,7 @@ repeat:
> ra_node_page(sbi, nid);
> }
>
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> lock_page(page);
> if (unlikely(page->mapping != NODE_MAPPING(sbi))) {
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> index 536e7a6..06f25d17 100644
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -159,7 +159,7 @@ restart:
> goto restart;
> }
> spin_unlock(&sdp->sd_ail_lock);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> trace_gfs2_ail_flush(sdp, wbc, 0);
> }
>
> diff --git a/fs/hpfs/buffer.c b/fs/hpfs/buffer.c
> index 8057fe4..138462d 100644
> --- a/fs/hpfs/buffer.c
> +++ b/fs/hpfs/buffer.c
> @@ -35,7 +35,7 @@ void hpfs_prefetch_sectors(struct super_block *s, unsigned secno, int n)
> secno++;
> n--;
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> }
>
> /* Map a sector into a buffer and return pointers to it and to the buffer. */
> diff --git a/fs/jbd/checkpoint.c b/fs/jbd/checkpoint.c
> index 08c0304..cd6b09f 100644
> --- a/fs/jbd/checkpoint.c
> +++ b/fs/jbd/checkpoint.c
> @@ -263,7 +263,7 @@ __flush_batch(journal_t *journal, struct buffer_head **bhs, int *batch_count)
> blk_start_plug(&plug);
> for (i = 0; i < *batch_count; i++)
> write_dirty_buffer(bhs[i], WRITE_SYNC);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> for (i = 0; i < *batch_count; i++) {
> struct buffer_head *bh = bhs[i];
> diff --git a/fs/jbd/commit.c b/fs/jbd/commit.c
> index bb217dc..e1046c3 100644
> --- a/fs/jbd/commit.c
> +++ b/fs/jbd/commit.c
> @@ -447,7 +447,7 @@ void journal_commit_transaction(journal_t *journal)
> blk_start_plug(&plug);
> err = journal_submit_data_buffers(journal, commit_transaction,
> write_op);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> /*
> * Wait for all previously submitted IO to complete.
> @@ -697,7 +697,7 @@ start_journal_io:
> }
> }
>
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> /* Lo and behold: we have just managed to send a transaction to
> the log. Before we can commit it, wait for the IO so far to
> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index 988b32e..6aa0039 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c
> @@ -187,7 +187,7 @@ __flush_batch(journal_t *journal, int *batch_count)
> blk_start_plug(&plug);
> for (i = 0; i < *batch_count; i++)
> write_dirty_buffer(journal->j_chkpt_bhs[i], WRITE_SYNC);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> for (i = 0; i < *batch_count; i++) {
> struct buffer_head *bh = journal->j_chkpt_bhs[i];
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index b73e021..8f532c8 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -805,7 +805,7 @@ start_journal_io:
> __jbd2_journal_abort_hard(journal);
> }
>
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> /* Lo and behold: we have just managed to send a transaction to
> the log. Before we can commit it, wait for the IO so far to
> diff --git a/fs/mpage.c b/fs/mpage.c
> index 3e79220..bf7d6c3 100644
> --- a/fs/mpage.c
> +++ b/fs/mpage.c
> @@ -695,7 +695,7 @@ mpage_writepages(struct address_space *mapping,
> if (mpd.bio)
> mpage_bio_submit(WRITE, mpd.bio);
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> return ret;
> }
> EXPORT_SYMBOL(mpage_writepages);
> diff --git a/fs/nfs/blocklayout/blocklayout.c b/fs/nfs/blocklayout/blocklayout.c
> index 1cac3c1..e93b6a8 100644
> --- a/fs/nfs/blocklayout/blocklayout.c
> +++ b/fs/nfs/blocklayout/blocklayout.c
> @@ -311,7 +311,7 @@ bl_read_pagelist(struct nfs_pgio_header *header)
> }
> out:
> bl_submit_bio(READ, bio);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> put_parallel(par);
> return PNFS_ATTEMPTED;
> }
> @@ -433,7 +433,7 @@ bl_write_pagelist(struct nfs_pgio_header *header, int sync)
> header->res.count = header->args.count;
> out:
> bl_submit_bio(WRITE, bio);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> put_parallel(par);
> return PNFS_ATTEMPTED;
> }
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 1790b00..2f89ca2 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1289,7 +1289,7 @@ _xfs_buf_ioapply(
> if (size <= 0)
> break; /* all done */
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> }
>
> /*
> @@ -1823,7 +1823,7 @@ __xfs_buf_delwri_submit(
>
> xfs_buf_submit(bp);
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> return pinned;
> }
> diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c
> index 098cd78..7e8fa3f 100644
> --- a/fs/xfs/xfs_dir2_readdir.c
> +++ b/fs/xfs/xfs_dir2_readdir.c
> @@ -455,7 +455,7 @@ xfs_dir2_leaf_readbuf(
> }
> }
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> out:
> *bpp = bp;
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 82e3142..c3ac5ec 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -196,7 +196,7 @@ xfs_bulkstat_ichunk_ra(
> &xfs_inode_buf_ops);
> }
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> }
>
> /*
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 7f9a516..188133f 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1091,6 +1091,7 @@ static inline void blk_post_runtime_resume(struct request_queue *q, int err) {}
> * schedule() where blk_schedule_flush_plug() is called.
> */
> struct blk_plug {
> + int depth; /* number of nested plugs */
> struct list_head list; /* requests */
> struct list_head mq_list; /* blk-mq requests */
> struct list_head cb_list; /* md requires an unplug callback */
> @@ -1107,7 +1108,7 @@ struct blk_plug_cb {
> extern struct blk_plug_cb *blk_check_plugged(blk_plug_cb_fn unplug,
> void *data, int size);
> extern void blk_start_plug(struct blk_plug *);
> -extern void blk_finish_plug(struct blk_plug *);
> +extern void blk_finish_plug(void);
> extern void blk_flush_plug_list(struct blk_plug *, bool);
>
> static inline void blk_flush_plug(struct task_struct *tsk)
> @@ -1646,7 +1647,7 @@ static inline void blk_start_plug(struct blk_plug *plug)
> {
> }
>
> -static inline void blk_finish_plug(struct blk_plug *plug)
> +static inline void blk_finish_plug(void)
> {
> }
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index d551475..18a34ee 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -539,7 +539,7 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> vma = find_vma(current->mm, start);
> }
> out:
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> if (write)
> up_write(¤t->mm->mmap_sem);
> else
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 644bcb6..4570f6e 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -2020,7 +2020,7 @@ int generic_writepages(struct address_space *mapping,
>
> blk_start_plug(&plug);
> ret = write_cache_pages(mapping, wbc, __writepage, mapping);
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> return ret;
> }
>
> diff --git a/mm/readahead.c b/mm/readahead.c
> index 9356758..64182a2 100644
> --- a/mm/readahead.c
> +++ b/mm/readahead.c
> @@ -136,7 +136,7 @@ static int read_pages(struct address_space *mapping, struct file *filp,
> ret = 0;
>
> out:
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> return ret;
> }
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 405923f..5721f64 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -478,7 +478,7 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t gfp_mask,
> SetPageReadahead(page);
> page_cache_release(page);
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
>
> lru_add_drain(); /* Push any new pages onto the LRU now */
> skip:
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 5e8eadd..56bb274 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2222,7 +2222,7 @@ static void shrink_lruvec(struct lruvec *lruvec, int swappiness,
>
> scan_adjusted = true;
> }
> - blk_finish_plug(&plug);
> + blk_finish_plug();
> sc->nr_reclaimed += nr_reclaimed;
>
> /*
> --
> 1.8.3.1
>
>
--
Dave Chinner
david@fromorbit.com
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox