On Sat, 2024-01-20 at 12:32 +0200, Amir Goldstein wrote:
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?
No. Strictly speaking regular whiteouts are always preferable to xwhiteouts (as they work for both user and system ovl mounts and are supported by older kernels and existing software, etc). The only place where xwhiteouts are useful is when we need to escape them to put inside an overlayfs mount.
The best way to think about the composefs usecase is:
Suppose you had a pre-existing system image that someone installed a multi layer container image in. This means that somewhere inside this image is a set of lowerdirs, and one of them may have a traditional whiteout. Now we want to create an overlayfs mount that when used, works just like the above image would work, including when you mount the sub-overlayfs mounts from the container image lowerdirs.
Fallout of this:
The composefs overlay lowerdir will need to contain escaped xwhiteouts, so that the mount will have unescaped xwhiteouts. These escaped whiteouts can be anywhere, even in a "single layer" overlayfs. But the unescaped xwhiteouts will never be in a lowermost lowerdir.
In the composefs case we will be using lowerdata for the outer overlayfs, but the actual unescaped xwhiteouts in the container image lowerdirs don't need to have a lowerdata involved at all.
The container mount will be done by pre-existing software (say docker) that isn't aware that we converted the regular whiteouts to xwhiteouts, so having to use a mount option is not ideal (would require docker changes).
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.
Seems like you attached the wrong patch, but I will comment on the other patch you sent to the list.
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.