Re: [PATCH 4/7] ovl: add infrastructure for intercepting file ops

[Miklos Szeredi]

On Thu, Nov 24, 2016 at 2:12 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> On Thu, Nov 24, 2016 at 2:03 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>> On Thu, Nov 24, 2016 at 12:52 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>>> On Thu, Nov 24, 2016 at 12:55 PM, Miklos Szeredi <mszeredi@xxxxxxxxxx> wrote:
>>
>>>> +               /*
>>>> +                * These should be intercepted, but they are very unlikely to be
>>>> +                * a problem in practice.  Leave them alone for now.
>>>
>>> It could also be handled in vfs helpers.
>>> Since these ops all start with establishing that src and dest are on
>>> the same sb,
>>> then the cost of copy up of src is the cost of clone_file_range from
>>> lower to upper,
>>> so it is probably worth to copy up src and leave those fops alone.
>>>
>>>> +                */
>>>> +               ofop->fops.copy_file_range = orig->copy_file_range;
>>>> +               ofop->fops.clone_file_range = orig->clone_file_range;
>>>> +               ofop->fops.dedupe_file_range = orig->dedupe_file_range;
>>
>> Not sure I understand.  Why should we copy up src?  Copy up is the
>> problem not the solution.
>>
>
> Maybe the idea is ill conceived, but the reasoning is:
> To avoid the corner case of cloning from a stale lower src,
> call d_real() in vfs helpers to always copy up src before cloning from it
> and pass the correct file onwards.

Which correct file?  src is still the wrong one after calling d_real.
We need to clone-open src, just like we do in ovl_read_iter to get the
correct file.  But then what's the use of copying it up beforehand?

We could move the whole logic into the vfs, but I don't really see the point.

I left these ops alone because there is some confusion in there about
getting the f_op from the source or the destination file.  And while
it doesn't matter normally (all regular files have the same f_op,
regardless of open flags)  it does matter for overlayfs intercept,
because overriding fops in the the dest file would mean additional
complexity and  resources).  That could easily be fixed in the vfs:
calling src->f_ops->foo works equally well, but I simply didn't want
to bother with this.  We can return to it later.

Thanks,
Miklos