On Wed, May 27, 2020 at 5:46 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > On Wed, May 27, 2020 at 1:16 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > On Wed, May 27, 2020 at 6:45 AM yangerkun <yangerkun@xxxxxxxxxx> wrote: > > > > > > Run generic/461 with ext4 upper/lower layer sometimes may trigger the > > > bug as below(linux 4.19): > > > > > > [ 551.001349] overlayfs: failed to get metacopy (-5) > > > [ 551.003464] overlayfs: failed to get inode (-5) > > > [ 551.004243] overlayfs: cleanup of 'd44/fd51' failed (-5) > > > [ 551.004941] overlayfs: failed to get origin (-5) > > > [ 551.005199] ------------[ cut here ]------------ > > > [ 551.006697] WARNING: CPU: 3 PID: 24674 at fs/inode.c:1528 iput+0x33b/0x400 > > > ... > > > [ 551.027219] Call Trace: > > > [ 551.027623] ovl_create_object+0x13f/0x170 > > > [ 551.028268] ovl_create+0x27/0x30 > > > [ 551.028799] path_openat+0x1a35/0x1ea0 > > > [ 551.029377] do_filp_open+0xad/0x160 > > > [ 551.029944] ? vfs_writev+0xe9/0x170 > > > [ 551.030499] ? page_counter_try_charge+0x77/0x120 > > > [ 551.031245] ? __alloc_fd+0x160/0x2a0 > > > [ 551.031832] ? do_sys_open+0x189/0x340 > > > [ 551.032417] ? get_unused_fd_flags+0x34/0x40 > > > [ 551.033081] do_sys_open+0x189/0x340 > > > [ 551.033632] __x64_sys_creat+0x24/0x30 > > > [ 551.034219] do_syscall_64+0xd5/0x430 > > > [ 551.034800] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > > ... > > > [ 556.107515] BUG: Dentry 000000006bc1d73f{i=4129c,n=fd51} still in use (-1) [unmount of ext4 sdb] > > > [ 556.108946] ------------[ cut here ]------------ > > > [ 556.109686] WARNING: CPU: 1 PID: 24682 at fs/dcache.c:1557 umount_check+0x95/0xc0 > > > [ 556.130343] d_walk+0x10d/0x430 > > > [ 556.130832] do_one_tree+0x30/0x60 > > > [ 556.131365] shrink_dcache_for_umount+0x38/0xe0 > > > [ 556.132063] generic_shutdown_super+0x2e/0x1c0 > > > [ 556.132747] kill_block_super+0x29/0x80 > > > [ 556.133338] deactivate_locked_super+0x7a/0x100 > > > [ 556.134034] deactivate_super+0x9d/0xb0 > > > [ 556.134627] cleanup_mnt+0x67/0x100 > > > [ 556.135173] __cleanup_mnt+0x16/0x20 > > > [ 556.135731] task_work_run+0xdb/0x110 > > > [ 556.136306] exit_to_usermode_loop+0x197/0x1b0 > > > [ 556.136991] do_syscall_64+0x3ce/0x430 > > > [ 556.137571] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > > ... > > > [ 556.378140] VFS: Busy inodes after unmount of sdb. Self-destruct in 5 seconds. Have a nice day... > > > > > > After check the code, there may some bug need to fix: > > > 1. We need to call iput once ovl_check_metacopy_xattr fail. > > > 2. We need to call unlock_new_inode or the above iput(also with iput in > > > ovl_create_object) will trigger the a WARN_ON since the I_NEW still > > > exists. > > > 3. We should move the init for upperdentry to the place below > > > ovl_check_metacopy_xattr. Or the dentry reference will decrease to > > > -1(error path in ovl_create_upper will inc, ovl_destroy_inode too). > > > > > > > OR we don't check metacopy xattr in ovl_get_inode(). > > > > In ovl_lookup() we already checked metacopy xattr. > > No reason to check it again in this subtle context. > > > > In ovl_lookup() can store value of upper metacopy and after we get > > the inode, set the OVL_UPPERDATA inode flag according to > > upperdentry && !uppermetacopy. > > > > That would be consistent with ovl_obtain_alias() which sets the > > OVL_UPPERDATA inode flag after getting the inode. > > I agree that that is a good direction, however for the actual fix I > think the following is sufficient (whitespace damaged, only for > review). > > The reason we can skip the metacopy check for the ->newinode != NULL > case is that that only happens on object creation, which very > obviously won't have metacopy set. > Yes, its subtle but should be sufficient, because lookup shouldn't get this far anyway. However, if we are making that distinction, might as well skip the entire block from ovl_inode_init() to unlock_new_inode(). Doesn't look like any of it is relevant for create. Thanks, Amir.