On Sat, May 30, 2020 at 12:30 AM Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > > Currently ovl_get_inode() initializes OVL_UPPERDATA flag and for that it > has to call ovl_check_metacopy_xattr() and check if metacopy xattr is > present or not. > > yangerkun reported sometimes underlying filesystem might return -EIO > and in that case error handling path does not cleanup properly leading > to various warnings. > > 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 > > One solution is to improve error handling and call iget_failed() if error > is encountered. Amir thinks that this path is little intricate and there > is not real need to check and initialize OVL_UPPERDATA in ovl_get_inode(). > Instead caller of ovl_get_inode() can initialize this state. And this > will avoid double checking of metacopy xattr lookup in ovl_lookup() > and ovl_get_inode(). > > OVL_UPPERDATA is inode flag. So I was little concerned that initializing > it outside ovl_get_inode() might have some races. But this is one way > transition. That is once a file has been fully copied up, it can't go > back to metacopy file again. And that seems to help avoid races. So > as of now I can't see any races w.r.t OVL_UPPERDATA being set wrongly. So > move settingof OVL_UPPERDATA inside the callers of ovl_get_inode(). > ovl_obtain_alias() already does it. So only two callers now left > are ovl_lookup() and ovl_instantiate(). > > Reported-by: yangerkun <yangerkun@xxxxxxxxxx> > Suggested-by: Amir Goldstein <amir73il@xxxxxxxxx> > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx> Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx> > --- > fs/overlayfs/dir.c | 2 ++ > fs/overlayfs/inode.c | 11 +---------- > fs/overlayfs/namei.c | 2 ++ > 3 files changed, 5 insertions(+), 10 deletions(-) > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > index 279009dee366..a7cac2ce0fad 100644 > --- a/fs/overlayfs/dir.c > +++ b/fs/overlayfs/dir.c > @@ -262,6 +262,8 @@ static int ovl_instantiate(struct dentry *dentry, struct inode *inode, > inode = ovl_get_inode(dentry->d_sb, &oip); > if (IS_ERR(inode)) > return PTR_ERR(inode); > + if (inode == oip.newinode) > + ovl_set_flag(OVL_UPPERDATA, inode); > } else { > WARN_ON(ovl_inode_real(inode) != d_inode(newdentry)); > dput(newdentry); > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c > index 981f11ec51bc..f2aaf00821c0 100644 > --- a/fs/overlayfs/inode.c > +++ b/fs/overlayfs/inode.c > @@ -957,7 +957,7 @@ struct inode *ovl_get_inode(struct super_block *sb, > bool bylower = ovl_hash_bylower(sb, upperdentry, lowerdentry, > oip->index); > int fsid = bylower ? lowerpath->layer->fsid : 0; > - bool is_dir, metacopy = false; > + bool is_dir; > unsigned long ino = 0; > int err = oip->newinode ? -EEXIST : -ENOMEM; > > @@ -1018,15 +1018,6 @@ struct inode *ovl_get_inode(struct super_block *sb, > if (oip->index) > ovl_set_flag(OVL_INDEX, inode); > > - if (upperdentry) { > - err = ovl_check_metacopy_xattr(upperdentry); > - if (err < 0) > - goto out_err; > - metacopy = err; > - if (!metacopy) > - ovl_set_flag(OVL_UPPERDATA, inode); > - } > - > OVL_I(inode)->redirect = oip->redirect; > > if (bylower) > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c > index a1889a160708..36e2b88a2fd1 100644 > --- a/fs/overlayfs/namei.c > +++ b/fs/overlayfs/namei.c > @@ -1078,6 +1078,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, > err = PTR_ERR(inode); > if (IS_ERR(inode)) > goto out_free_oe; > + if (upperdentry && !uppermetacopy) > + ovl_set_flag(OVL_UPPERDATA, inode); > } > > ovl_dentry_update_reval(dentry, upperdentry, > -- > 2.25.4 >