On Mon, Apr 30, 2018 at 9:08 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > On Mon, Apr 30, 2018 at 5:02 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: [...] >> >> 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. > In general, I feel it would be better to do a minimal sanity check inside > inode lock for INDEX and METACOPY (maybe REDIRECT) and make > sure they are consistent with the info in ovl_inode_info. > If they are not, return -ESTALE and that may result in re-calling > lookup. > After thinking about it some more, I believe the way we should do it is implement ovl_iget_locked() and call it from ovl_lookup() *before* we lookup index. Then, *after* looking up index, If this turns out to be a broken hardlink, we throw away the inode that we got by lower and get a new inode hashed by upper. Then we can initialize the inode with another helper or directly in ovl_lookup() and unlock_new_inode(). w.r.t METACOPY things will get complicated if we remove METACOPY xattr and the upper does not have ORIGIN. In that case we may have an inode that is already hashed by lower, but for a new lookup that does not see METACOPY xattr it looks up the inode in cache by upper and this is not good. There is one solution that you will surely not like - instead of removing METACOPY xattr set METACOPY=n. That means we still need to lookup lower layers for origin inode, but we do set UPPERDATA flag. There may be other solutions to the problem, but I cannot think of any right now. 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
