From: Amir Goldstein <amir73il@gmail.com>
To: Vivek Goyal <vgoyal@redhat.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>,
yangerkun <yangerkun@huawei.com>,
overlayfs <linux-unionfs@vger.kernel.org>
Subject: Re: [PATCH 3/3] overlayfs: Initialize OVL_UPPERDATA in ovl_lookup()
Date: Sat, 30 May 2020 14:02:43 +0300 [thread overview]
Message-ID: <CAOQ4uxiv5LqLouM1AJran0Y_EHk6D1uq5368CoCEqxOhA4_waA@mail.gmail.com> (raw)
In-Reply-To: <20200529212952.214175-4-vgoyal@redhat.com>
On Sat, May 30, 2020 at 12:30 AM Vivek Goyal <vgoyal@redhat.com> 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@huawei.com>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 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
>
next prev parent reply other threads:[~2020-05-30 11:02 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-29 21:29 [PATCH 0/3] overlayfs: Do not check metacopy in ovl_get_inode() Vivek Goyal
2020-05-29 21:29 ` [PATCH 1/3] overlayfs: Simplify setting of origin for index lookup Vivek Goyal
2020-05-30 10:37 ` Amir Goldstein
2020-06-01 14:04 ` Vivek Goyal
2020-06-01 15:15 ` Amir Goldstein
2020-06-01 17:51 ` Vivek Goyal
2020-05-29 21:29 ` [PATCH 2/3] overlayfs: ovl_lookup(): Use only uppermetacopy state Vivek Goyal
2020-05-30 11:01 ` Amir Goldstein
2020-06-01 15:22 ` Vivek Goyal
2020-05-29 21:29 ` [PATCH 3/3] overlayfs: Initialize OVL_UPPERDATA in ovl_lookup() Vivek Goyal
2020-05-30 11:02 ` Amir Goldstein [this message]
2020-05-30 0:59 ` [PATCH 0/3] overlayfs: Do not check metacopy in ovl_get_inode() yangerkun
2020-05-30 3:55 ` yangerkun
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOQ4uxiv5LqLouM1AJran0Y_EHk6D1uq5368CoCEqxOhA4_waA@mail.gmail.com \
--to=amir73il@gmail.com \
--cc=linux-unionfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=vgoyal@redhat.com \
--cc=yangerkun@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).