Re: [PATCH] ovl: fix some bug exist in ovl_get_inode

[Vivek Goyal <vgoyal@redhat.com>]

On Wed, May 27, 2020 at 02:16:00PM +0300, Amir Goldstein wrote:

[..]
> > 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.

I think reason behind initializing this attr in ovl_get_inode() was
that this is OVL_UPPERDATA is an inode property. So conceptually
it makes sense to initialize it when inode is being instantiated. And
that too under lock so that there are no races.

I was trying to think if we can trigger a race if we move OVL_UPPERDATA
initialization in ovl_lookup(). But given this is only one way
transition, I could not think of any. So for this speicific flag,
it probably is ok to initialize outside of ovl_get_inode().

Vivek