On Wed, May 2, 2018 at 10:40 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> 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)?
>
Yes, that's what I figured when you described the potential METACOPY race.
Besides that I am still worried about corners we missed in setup of
metacopy=on,index=off.
I may be confused about whether lower with nlink=1 with index=off
after metacopy up uses upper st_ino and hashed by upper inode?
Need to understand whether the "hashed by" state is consistent with
metacopy=on,index=off in all cases.
Thanks,
Amir.
--
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
