On Thu, Apr 16, 2020 at 8:47 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> > > > In case of any unexpected errors, we could set a error limit(for example, 10),
> > > > if link error count exceeds the limit then we disable the feature.
> > > >
> > >
> > > As far as I am concerned, you may post the patch without auto disable,
> > > but I object to using an arbitrary number of errors (10) as trigger for auto
> > > disable.
> > >
> > > I do think that it is important to communicate to admin that the whiteout
> > > sharing is not working as expected and it is not acceptable to flood
> > > log with warning on each and every whiteout creation, not even with
> > > pr_warn_ratelimited.
> > >
> > > There are several ways you can go about this, but here is a suggestion:
> > >
> > > + if (err) {
> > > + ofs->whiteout_link_max >>= 1;
> > > + pr_warn("failed to link whiteout (nlink=%u, err = %i)
> > > - lowering whiteout_link_max to %u\n",
> > > + ofs->whiteout->d_inode->i_nlink, err,
> > > ofs->whiteout_link_max);
> > > + should_link = false;
> > > + ovl_cleanup(wdir, ofs->whiteout);
> > > + }
> > >
> >
> > Frankly speaking, I don't like heuristic method to automatically enable/disable feature,
> > so I perfer to disable the feature immediately after hitting any error just like your previous suggestion,
>
> Fine by me.
>
> > and I also hope to add a mount option for this feature to mitigate global effect to different overlayfs instances.
> >
>
> I think that would make sense.
>
> Off topic: from system perspective, the fact that whiteouts take up inodes
> to begin with is a shame. If one had control over the underlying filesystem,
> whiteout should have been implemented as a constant and immutable special
> inode with no nlink count at all and getdents would return DT_WHITEOUT for
> those entries.
Yes, and filesystems are free to implement such a beast. The
operations are given: mknod(S_IFCHR, 0) and RENAME_WHITEOUT. Adding
DT_WHITEOUT handling to overlayfs would be trivial, I guess.
Thanks,
Miklos
