On Wed, May 02, 2018 at 03:09:57PM -0400, Vivek Goyal wrote:
> On Mon, Apr 30, 2018 at 09:08:01PM +0300, Amir Goldstein wrote:
> [..]
> > >> > @@ -836,6 +836,15 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
> > >> > if (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);
> > >> > + }
> > >> > +
> > >>
> > >> There is no reason to ovl_check_metacopy_xattr again here, right?
> > >
> > > I think we need to check metacopy here otherwise it becomes racy. For
> > > example, what if there is a hard link (say, l1 and l2) with metacopy xattr.
> > > ovl_lookup(l1) will think metacopy is on while another thread on another
> > > cpu might have trigged copy up, remove metacopy xattr. And it is possible
> > > that inode got flushed out of cache. So by the time ovl_lookup(l1), calls
> > > iget5_locked(), it will get a new inode and it will initialize inode
> > > with wrong information.
> > >
> > > I had done similar thing for REDIRECT, but once we removed logic to
> > > remove REDIRECT on copy up, I felt I did not have to check redirect
> > > again here.
> > >
> > > In general, I feel that once we have the inode lock, we should check
> > > metacopy and redirect both and then initialize inode. And not rely
> > > on information which was checked outside the lock and might have
> > > been stale by now.
> > >
> >
> > Hmmm... so as you once already said, we have a race with INDEX as well.
>
> I am not sure about INDEX race. I remember talking about INDEX flag race
> in ovl_inode but w.r.t ovl_get_inode(), what's the race you are seeing
> with index. Once ovl_lookup() has found index, can that index go away by
> the time ovl_get_inode() is called? If not, then there should not be
> any race.
May be you are referring to race when ovl_lookup() looks for index
its not there (all lower), and then thread gets blocked, other cpu
triggers a copy up and creates index, flushs ovl_inode and now first
thread continues and does ovl_get_inode() and does not set OVL_INDEX
flag (Despite the fact there is index)?
Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
