_______________________________________________
LKP mailing list
[email protected]
Hi Ying,
As suggested in the patch, -19 (-ENODEV) happens when debugfs is not configured (see include/linux/debugfs.h). So if debugfs is necessary for the functionality, in Kconfig, we should either declare it as a dependency, or auto select it.
Chengyu
> On Apr 19, 2015, at 11:25 PM, Huang Ying <[email protected]> wrote:
>
> FYI, we noticed the below changes on
>
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> commit e2ac55b6a8e337fac7cc59c6f452caac92ab5ee6 ("ocfs2: incorrect check for debugfs returns")
>
> We found something as below in dmesg.
>
> [ 1.225136] VFS: Disk quotas dquot_6.5.2
> [ 1.225814] VFS: Dquot-cache hash table entries: 1024 (order 0, 4096 bytes)
> [ 1.228207] (swapper/0,1,0):ocfs2_init:1612 ERROR: Unable to create ocfs2 debugfs root.
> [ 1.232445] (swapper/0,1,0):ocfs2_init:1634 ERROR: status = -19
> [ 1.233223] ocfs2: Registered cluster interface o2cb
> [ 1.233994] OCFS2 User DLM kernel interface loaded
> [ 1.234618] (swapper/0,1,0):o2hb_debug_init:1318 ERROR: status = -19
> [ 1.236183] gfs2: GFS2 installed
> [ 1.238123] Key type asymmetric registered
>
> BUG: kernel boot hang
> Elapsed time: 310
> qemu-system-i386 -enable-kvm -cpu Haswell,+smep,+smap -kernel /kernel/i386-randconfig-ib0-04172352/e2ac55b6a8e337fac7cc59c6f452caac92ab5ee6/vmlinuz-4.0.0-02646-ge2ac55b -append 'root=/dev/ram0 user=lkp job=/lkp/scheduled/vm-vp-quantal-i386-23/rand_boot-1-quantal-core-i386.cgz-i386-randconfig-ib0-04172352-e2ac55b6a8e337fac7cc59c6f452caac92ab5ee6-1-20150418-31554-1woqtdb.yaml ARCH=i386 kconfig=i386-randconfig-ib0-04172352 branch=linus/master commit=e2ac55b6a8e337fac7cc59c6f452caac92ab5ee6 BOOT_IMAGE=/kernel/i386-randconfig-ib0-04172352/e2ac55b6a8e337fac7cc59c6f452caac92ab5ee6/vmlinuz-4.0.0-02646-ge2ac55b RESULT_ROOT=/result/vm-vp-quantal-i386/boot/1/quantal-core-i386.cgz/i386-randconfig-ib0-04172352/e2ac55b6a8e337fac7cc59c6f452caac92ab5ee6/0 LKP_SERVER=inn earlyprintk=ttyS0,115200 rd.udev.log-priority=err systemd.log_target=journal systemd.log_level=warning debug apic=debug sysrq_always_enabled rcupdate.rcu_cpu_stall_timeout=100 panic=-1 softlockup_panic=1 nmi_watchdog=panic oops=panic load_ramdisk=2 prompt_ramdisk=0 console=ttyS0,115200 console=tty0 vga=normal rw ip=::::vm-vp-quantal-i386-23::dhcp drbd.minor_count=8' -initrd /fs/sde1/initrd-vm-vp-quantal-i386-23 -m 360 -smp 1 -device e1000,netdev=net0 -netdev user,id=net0 -boot order=nc -no-reboot -watchdog i6300esb -rtc base=localtime -pidfile /dev/shm/kboot/pid-vm-vp-quantal-i386-23 -serial file:/dev/shm/kboot/serial-vm-vp-quantal-i386-23 -daemonize -display none -monitor null
>
> Thanks,
> Ying Huang
>
> <config-4.0.0-02646-ge2ac55b><.dmesg.txt>_______________________________________________
> LKP mailing list
> [email protected]
>
On Sun, 2015-04-19 at 23:45 -0400, Chengyu Song wrote:
> Hi Ying,
>
> As suggested in the patch, -19 (-ENODEV) happens when debugfs is not configured (see include/linux/debugfs.h). So if debugfs is necessary for the functionality, in Kconfig, we should either declare it as a dependency, or auto select it.
I see. Thanks for explanation!
Best Regards,
Huang, Ying
> Chengyu
>
> > On Apr 19, 2015, at 11:25 PM, Huang Ying <[email protected]> wrote:
> >
> > FYI, we noticed the below changes on
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > commit e2ac55b6a8e337fac7cc59c6f452caac92ab5ee6 ("ocfs2: incorrect check for debugfs returns")
> >
> > We found something as below in dmesg.
> >
> > [ 1.225136] VFS: Disk quotas dquot_6.5.2
> > [ 1.225814] VFS: Dquot-cache hash table entries: 1024 (order 0, 4096 bytes)
> > [ 1.228207] (swapper/0,1,0):ocfs2_init:1612 ERROR: Unable to create ocfs2 debugfs root.
> > [ 1.232445] (swapper/0,1,0):ocfs2_init:1634 ERROR: status = -19
> > [ 1.233223] ocfs2: Registered cluster interface o2cb
> > [ 1.233994] OCFS2 User DLM kernel interface loaded
> > [ 1.234618] (swapper/0,1,0):o2hb_debug_init:1318 ERROR: status = -19
> > [ 1.236183] gfs2: GFS2 installed
> > [ 1.238123] Key type asymmetric registered
> >
> > BUG: kernel boot hang
> > Elapsed time: 310
> > qemu-system-i386 -enable-kvm -cpu Haswell,+smep,+smap -kernel /kernel/i386-randconfig-ib0-04172352/e2ac55b6a8e337fac7cc59c6f452caac92ab5ee6/vmlinuz-4.0.0-02646-ge2ac55b -append 'root=/dev/ram0 user=lkp job=/lkp/scheduled/vm-vp-quantal-i386-23/rand_boot-1-quantal-core-i386.cgz-i386-randconfig-ib0-04172352-e2ac55b6a8e337fac7cc59c6f452caac92ab5ee6-1-20150418-31554-1woqtdb.yaml ARCH=i386 kconfig=i386-randconfig-ib0-04172352 branch=linus/master commit=e2ac55b6a8e337fac7cc59c6f452caac92ab5ee6 BOOT_IMAGE=/kernel/i386-randconfig-ib0-04172352/e2ac55b6a8e337fac7cc59c6f452caac92ab5ee6/vmlinuz-4.0.0-02646-ge2ac55b RESULT_ROOT=/result/vm-vp-quantal-i386/boot/1/quantal-core-i386.cgz/i386-randconfig-ib0-04172352/e2ac55b6a8e337fac7cc59c6f452caac92ab5ee6/0 LKP_SERVER=inn earlyprintk=ttyS0,115200 rd.udev.log-priority=err systemd.log_target=journal systemd.log_level=warning debug apic=debug sysrq_always_enabled rcupdate.rcu_cpu_stall_timeout=100 panic=-1 softlockup_panic=1 nmi_watchdog=panic oops=panic load_ramdisk=2 prompt_ramdisk=0 console=ttyS0,115200 console=tty0 vga=normal rw ip=::::vm-vp-quantal-i386-23::dhcp drbd.minor_count=8' -initrd /fs/sde1/initrd-vm-vp-quantal-i386-23 -m 360 -smp 1 -device e1000,netdev=net0 -netdev user,id=net0 -boot order=nc -no-reboot -watchdog i6300esb -rtc base=localtime -pidfile /dev/shm/kboot/pid-vm-vp-quantal-i386-23 -serial file:/dev/shm/kboot/serial-vm-vp-quantal-i386-23 -daemonize -display none -monitor null
> >
> > Thanks,
> > Ying Huang
> >
> > <config-4.0.0-02646-ge2ac55b><.dmesg.txt>_______________________________________________
> > LKP mailing list
> > [email protected]
> >
>
On Sun, Apr 19, 2015 at 8:45 PM, Chengyu Song <[email protected]> wrote:
>
> As suggested in the patch, -19 (-ENODEV) happens when debugfs is not configured (see include/linux/debugfs.h). So if debugfs is necessary for the functionality, in Kconfig, we should either declare it as a dependency, or auto select it.
That makes no sense.
If it used to work before that patch, then this is a regression and
the patch needs to be reverted.
Yes, the old code apparently used to set "o2hb_debug_dir" to an error
pointer when debugfs was compiled out, but since debugfs was compiled
out, that error pointer was probably never actually *used*. So things
presumably worked.
Now, it hangs, according to Huang Ying. If so, that's clearly a
regression. That means that commit e2ac55b6a8e3 ("ocfs2: incorrect
check for debugfs returns") needs to be reverted or fixed.
Andrew?
Linus
On Mon, 20 Apr 2015 13:50:38 -0700 Linus Torvalds <[email protected]> wrote:
> On Sun, Apr 19, 2015 at 8:45 PM, Chengyu Song <[email protected]> wrote:
> >
> > As suggested in the patch, -19 (-ENODEV) happens when debugfs is not configured (see include/linux/debugfs.h). So if debugfs is necessary for the functionality, in Kconfig, we should either declare it as a dependency, or auto select it.
>
> That makes no sense.
>
> If it used to work before that patch, then this is a regression and
> the patch needs to be reverted.
>
> Yes, the old code apparently used to set "o2hb_debug_dir" to an error
> pointer when debugfs was compiled out, but since debugfs was compiled
> out, that error pointer was probably never actually *used*. So things
> presumably worked.
>
> Now, it hangs, according to Huang Ying. If so, that's clearly a
> regression. That means that commit e2ac55b6a8e3 ("ocfs2: incorrect
> check for debugfs returns") needs to be reverted or fixed.
>
> Andrew?
>
Yes, that one snuck through. I think a revert would be best at this
stage, please.
The debugfs interfaces are exceptional, and not very nice. My
understanding of the general idea is:
- debugfs is just for debug and subsystems shouldn't care whether
debugfs is present or not.
- if a debugfs call fails, the subsystem shouldn't care - don't log
it, just ignore it.
The return semantics from things like debugfs_create_dir() are:
NULL: debugfs is available, but something went wrong. We don't
tell you what it was.
-ENODEV: debugfs isn't available
-Exxx: I don't think other errnos are supposed to happen.
So the ofs2 code shouldn't log unless the debugfs calls return NULL.
And really, they shouldn't log at all, due to the general debugfs
philosophy of "errors should be silently ignored".
A problem with functions like o2hb_debug_init() is that when
CONFIG_DEBUG_FS=n they will still generate significant amounts of code.
That's fixable with something like
--- a/fs/ocfs2/cluster/heartbeat.c~a
+++ a/fs/ocfs2/cluster/heartbeat.c
@@ -1312,12 +1312,8 @@ static int o2hb_debug_init(void)
int ret = -ENOMEM;
o2hb_debug_dir = debugfs_create_dir(O2HB_DEBUG_DIR, NULL);
- if (IS_ERR_OR_NULL(o2hb_debug_dir)) {
- ret = o2hb_debug_dir ?
- PTR_ERR(o2hb_debug_dir) : -ENOMEM;
- mlog_errno(ret);
- goto bail;
- }
+ if (IS_ERR(o2hb_debug_dir))
+ return 0;
o2hb_debug_livenodes = o2hb_debug_create(O2HB_DEBUG_LIVENODES,
o2hb_debug_dir,
Here, the compiler shold see that the `return 0' is always taken (due
to the -ENODEV) and the rest of the function will be eliminated.
Or we wrap large pieces of code inside `#ifdef CONFIG_DEBUG_FS'.
Either way, a cleanup here needs some thought and study. And better
testing, plesae.
> On Apr 20, 2015, at 5:10 PM, Andrew Morton <[email protected]> wrote:
>
> On Mon, 20 Apr 2015 13:50:38 -0700 Linus Torvalds <[email protected]> wrote:
>
>> On Sun, Apr 19, 2015 at 8:45 PM, Chengyu Song <[email protected]> wrote:
>>>
>>> As suggested in the patch, -19 (-ENODEV) happens when debugfs is not configured (see include/linux/debugfs.h). So if debugfs is necessary for the functionality, in Kconfig, we should either declare it as a dependency, or auto select it.
>>
>> That makes no sense.
>>
>> If it used to work before that patch, then this is a regression and
>> the patch needs to be reverted.
>>
>> Yes, the old code apparently used to set "o2hb_debug_dir" to an error
>> pointer when debugfs was compiled out, but since debugfs was compiled
>> out, that error pointer was probably never actually *used*. So things
>> presumably worked.
>>
>> Now, it hangs, according to Huang Ying. If so, that's clearly a
>> regression. That means that commit e2ac55b6a8e3 ("ocfs2: incorrect
>> check for debugfs returns") needs to be reverted or fixed.
>>
>> Andrew?
>>
>
> Yes, that one snuck through. I think a revert would be best at this
> stage, please.
>
>
>
> The debugfs interfaces are exceptional, and not very nice. My
> understanding of the general idea is:
>
> - debugfs is just for debug and subsystems shouldn't care whether
> debugfs is present or not.
>
> - if a debugfs call fails, the subsystem shouldn't care - don't log
> it, just ignore it.
>
>
> The return semantics from things like debugfs_create_dir() are:
>
> NULL: debugfs is available, but something went wrong. We don't
> tell you what it was.
>
> -ENODEV: debugfs isn't available
>
> -Exxx: I don't think other errnos are supposed to happen.
>
>
> So the ofs2 code shouldn't log unless the debugfs calls return NULL.
> And really, they shouldn't log at all, due to the general debugfs
> philosophy of "errors should be silently ignored".
>
>
>
> A problem with functions like o2hb_debug_init() is that when
> CONFIG_DEBUG_FS=n they will still generate significant amounts of code.
> That's fixable with something like
>
> --- a/fs/ocfs2/cluster/heartbeat.c~a
> +++ a/fs/ocfs2/cluster/heartbeat.c
> @@ -1312,12 +1312,8 @@ static int o2hb_debug_init(void)
> int ret = -ENOMEM;
>
> o2hb_debug_dir = debugfs_create_dir(O2HB_DEBUG_DIR, NULL);
> - if (IS_ERR_OR_NULL(o2hb_debug_dir)) {
> - ret = o2hb_debug_dir ?
> - PTR_ERR(o2hb_debug_dir) : -ENOMEM;
> - mlog_errno(ret);
> - goto bail;
> - }
> + if (IS_ERR(o2hb_debug_dir))
> + return 0;
>
> o2hb_debug_livenodes = o2hb_debug_create(O2HB_DEBUG_LIVENODES,
> o2hb_debug_dir,
>
> Here, the compiler shold see that the `return 0' is always taken (due
> to the -ENODEV) and the rest of the function will be eliminated.
>
> Or we wrap large pieces of code inside `#ifdef CONFIG_DEBUG_FS'.
>
> Either way, a cleanup here needs some thought and study. And better
> testing, plesae.
First of all, I apologize for this incorrect patch. It was created in concern of potential used of the error pointer, but as there is no such use at all, there’s no need for a patch and should be reverted.
This fake problem was discovered by our static checker that compares different fs implementations. And what we found is that there are three ways how debugfs is handled:
- aware of -ENODEV and explicitly check for this error => ubifs
- warp uses with `#ifdef CONFIG_DEBUG_FS’ => btrfs, certain module of ocfs2
- isolate the code and declare dependency on DEBUG_FS in Kconfig => OCFS2_FS_STATS
Lacking enough testing and understanding of actual dependency of ocfs2 on debugfs, especially it uses two different approaches to avoid -ENODEV, the initial patch was mainly for discussing and I didn’t expect it got merged. But since a little more study would reveal there’s no actual use of these error pointers if debugfs is not configured, it’s my fault and I want to apologize again.
Chengyu-