On Fri, Jan 19, 2024 at 10:30 PM Miklos Szeredi miklos@szeredi.hu wrote:
On Fri, 19 Jan 2024 at 20:06, Amir Goldstein amir73il@gmail.com wrote:
How about checking xwhiteouts xattrs along with impure and origin xattrs in ovl_get_inode()?
That was not a very good suggestion on my part. ovl_get_inode() only checks impure xattr on upper dir and origin xattr on non-merge non-multi lower dir.
If we change the location of xwhiteout xattr check, it should be in ovl_lookup_single() next to checking opaque xattr, which makes me think - hey, why don't we overload opaque xattr, just like we did with metacopy xattr?
An overlay.opaque xattr with empty string means "may have xwhiteouts" and that is backward compatible, because ovl_is_opaquedir() checks for xattr of length 1.
The only extra getxattr() needed would be for the d->last case...
Then there will be no overhead in readdir and no need for marking the layer root?
Miklos, would that be acceptable?
It's certainly a good idea, but doesn't really address my worry. The minor performance impact is not what bothers me most. It's the fact that in the common case the result of these calls are discarded. That's just plain ugly, IMO.
...so the question boils down to, whether you find it too ugly to always getxattr(opaque) on lookup of the last lower layer and whether you find the overloading of opaque xattr too hacky?
As a precedent, we *always* check metacopy xattr in last lower layer to check for error conditions, even if user did not opt-in to metacopy at all, while we could just as easily have ignored it.
My preferred alternative would be a mount option. Amir, Alex, would you both be okay with that?
I think I had suggested that escaped private xattrs would also require an opt-in mount option, but Alex explained that the users mounting the overlay are not always aware of the fact that the layers were composed this way, but I admit that I do not remember all the exact details.
Alex, do I remember correctly that the overlay instance where xwhiteouts needs to be checked does NOT necessarily have a lowerdata layers? The composefs instance with lowerdata layers is the one exposing the (escaped) xwhiteout entries as xwhiteouts. Is that correct?
Is there even a use case for xwhiteouts NOT inside another lower overlayfs?
If we limit the check for xwhiteouts only to nested overlayfs, then maybe Miklos will care less about an extra getxattr on lookup?
Attached patch implements both xwhiteout and opaque checks during lookup - we can later choose only one of them to keep.
Note that is changes the optimization to per-dentry, not per-layer, so in the common case (no layers have xwhiteouts) xwhiteouts will not be checked, but if xwhiteouts exist in any lower layer in the stack, then xwhiteouts will be checked in all the layers of the merged dir (*).
(*) still need to optimize away lookup of xwhiteouts in upperdir.
Let me know what you think.
Thanks, Amir.