On Thu, Oct 26, 2017 at 7:11 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> On Thu, Oct 26, 2017 at 5:34 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>> On Thu, Oct 26, 2017 at 05:14:57PM +0300, Amir Goldstein wrote:
>>> On Thu, Oct 26, 2017 at 4:53 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>>> > On Thu, Oct 26, 2017 at 09:04:17AM +0300, Amir Goldstein wrote:
>>> >> On Wed, Oct 25, 2017 at 10:09 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>>> >> > Now we will have the capability to have upper inodes which might be only
>>> >> > metadata copy up and data is still on lower inode. So add a new xattr
>>> >> > OVL_XATTR_METACOPY to distinguish between two cases.
>>> >> >
>>> >> > Presence of OVL_XATTR_METACOPY reflects that file has been copied up metadata
>>> >> > only and and data will be copied up later from lower origin.
>>> >> > So this xattr is set when a metadata copy takes place and cleared when
>>> >> > data copy takes place.
>>> >> >
>>> >> > We also use a bit in ovl_inode->flags to cache OVL_UPPERDATA which reflects
>>> >> > whether ovl inode has data or not (as opposed to metadata only copy up).
>>> >> >
>>> >> > Note, OVL_UPPERDATA is set only on those upper inodes which can possibly
>>> >> > be metacopy only inodes. For example, inode should be a regular file and
>>> >> > there needs to be a lower dentry.
>>> >>
>>> >> Why? is it not simpler and better for readability to set it in all
>>> >> cases except metacopy?
>>> >
>>> > I think it was re-introducing ordering requirement w.r.t oi->__upperdentry
>>> > and that's why I avoided it doing for all type of files.
>>> >
>>> > For example, in file/dir creation path when a new dentry is created, we
>>> > need to make sure OVL_UPPERDATA is visible on other cpus before
>>> > oi->__upperdentry is visible. Otherwise other cpus might try to copy up this
>>> > file for which there might not be any lower and all the bad things can happen.
>>> >
>>> > For example,
>>> >
>>> > CPU1 CPU2
>>> > ovl_instantiate() { ovl_d_real() {
>>> > ovl_dentry_set_upper_alias() ovl_open_maybe_copy_up()
>>> > set OVL_UPPERDATA ovl_dentry_needs_data_copy_up
>>> > smp_wmb(); test OVL_UPPERDATA
>>> > oi->__upperdentry = upperdentry
>>> > }
>>> >
>>> > So if cpu1 is creating a new file and cpu2 does ovl_d_real() and it sees
>>> > upper dentry but not OVL_UPPERDATA, it will try to copy up file (which
>>> > might not be on lower at all).
>>> >
>>> > Similarly, in ovl_d_real() we check OVL_UPPERDATA one more time and if
>>> > its not set, we return lower dentry instead of upper. It can race there
>>> > as well and try to return a lower which does not exist (for a newly
>>> > created file).
>>> >
>>> > So, IIUC, this was bringing back ordering requirement and hence I limited
>>> > this flag.
>>> >
>>>
>>> Fine. but use ovl_should_check_upperdata() in read path (CPU2) anyway
>>> so there is little point in checking whether or not you need to set the flag.
>>> Better just set it unconditionally on copy up (!metacopy) and on
>>> ovl_instantiate().
>>
>> Ok. Why not keep it symmetric in WRITE and READ paths. That way it is
>> easier to read and understand code. Making it asymmetric will only
>> increase confusion. If we are never going to check a flag, why to
>> set it to begin with.
>>
>> Anyway, I don't have strong opinion about it. If you like this better,
>> I am fine.
>>
>
> Please just makes sure the resulting code is simplest.
> Don't add extra if's if setting the flag is not harmful.
>
>>>
>>> > BTW, how did oi->has_upper handle this requirement. IIUC, that can run
>>> > into same issue. It is possible that CPU2 does not see ->has_upper for
>>> > newly created file while it has oi->__upperdentry. That means it can
>>> > try to copy up a non-existent file as well.
>>>
>>> I think you are referring to the "false negative" case in Miklos's comment
>>> in ovl_copy_up_flags(). This is the simplest case because both __upperdentry
>>> and has_alias are checked again under oi->lock in ovl_copy_up_one().
>>
>> But before we take lock, we start doing operations on lower and you might
>> get null pointer dereference. (there will not be any lower for an upper
>> only file).
>>
>> ovl_copy_up_one() {
>> ovl_path_lower(dentry, &ctx.lowerpath);
>> err = vfs_getattr(&ctx.lowerpath, &ctx.stat,
>> STATX_BASIC_STATS, AT_STATX_SYNC_AS_STAT);
>> ovl_do_check_copy_up(ctx.lowerpath.dentry);
>> ovl_copy_up_start(); <--- Now we take lock.
>> }
>>
>
> Ah, I see it now.
>
> I guess there are barriers to make sure that dentry is consistent before
> adding it to dcache and finding it in lookup, but just guessing. Miklos?
>
Vivek,
The false positive case you point out of pure upper entry that will attempt
copy up doesn't require memory barriers to fix. A simple sanity check of
non-NULL lowerpath is enough and return success from ovl_copy_up_one()
If you make this check with WARN_ON() we will know if we had a missing
barrier for has_upper for create upper case, but I don't even know if we care.
Also the check for "should copy up" is starting to grow.
Now it looks like this:
if (!ovl_dentry_upper(dentry) ||
(!ovl_dentry_has_upper_alias(dentry))
and it repeats at least twice in d_real() lockless path.
Now you want to add another condition and my NFS export patches
add another condition (dentry->d_flags & DCACHE_DISCONNECTED)
so it is about time to introduce a helper for the lockless path.
Amir.
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
