On Sat, May 30, 2020 at 02:01:28PM +0300, Amir Goldstein wrote:
[..]
> > @@ -982,23 +987,17 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> > }
> > }
> >
> > - if (metacopy) {
> > - /*
> > - * Found a metacopy dentry but did not find corresponding
> > - * data dentry
> > - */
> > - if (d.metacopy) {
> > - err = -EIO;
> > - goto out_put;
> > - }
> > + /* Found a metacopy dentry but did not find corresponding data dentry */
> > + if (d.metacopy) {
>
> I suggested this change and I think it is correct, but it is correct for a bit
> of a subtle reason.
> It is correct because ovl_lookup_layer() (currently) cannot return NULL
> and set d.metacopy to false.
> So I suggest to be a bit more defensive and write this condition as:
>
> if (d.metacopy || (uppermetacopy && !ctr)) {
Ok, will do.
>
> > + err = -EIO;
> > + goto out_put;
> > + }
> >
> > - err = -EPERM;
> > - if (!ofs->config.metacopy) {
> > - pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n",
> > - dentry);
> > - goto out_put;
> > - }
> > - } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
> > + /* For regular non-metacopy upper dentries, there is no lower
> > + * path based lookup, hence ctr will be zero. dentry found using
> > + * ORIGIN xattr on upper, install it in stack.
> > + */
> > + if (!d.is_dir && upperdentry && !ctr && origin_path) {
>
> I don't like this comment style for multi line comment and I don't
> like that you detached this if statement from else if.
> I think it made more sense with the else because this is (as you write)
> the non-metacopy case.
Will do in V2.
Thanks
Vivek
