On Fri, Jun 16, 2023 at 10:50 AM Alexander Larsson <alexl@xxxxxxxxxx> wrote:
>
> On Fri, Jun 16, 2023 at 7:55 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >
> > On Fri, Jun 16, 2023 at 8:24 AM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
> > >
> > > On Fri, Jun 16, 2023 at 08:07:43AM +0300, Amir Goldstein wrote:
> > > > > I would really like to get the overlay.verity xattr support in
> > > > > upstream pretty soon, because without it I can't release a stable
> > > > > version of the composefs userspace code. I don't want to release
> > > > > something that is using an xattr format that is not guaranteed to be
> > > > > stable.
> > > > >
> > > >
> > > > Alex,
> > > >
> > > > Pondering about this last sentence.
> > > >
> > > > The overlay.verity xattr format is already in its 3rd revision since
> > > > the beginning of development.
> > > >
> > > > When it was bare digest, it might have made sense to have no
> > > > header that describes the format.
> > > >
> > > > When the algo byte was added, that was already a very big hint
> > > > that a proper header was in order.
> > > >
> > > > Now that you had to change the meaning of the byte, it is very hard
> > > > to argue that the xattr format is guaranteed to be stable - in the sense
> > > > that it can never change again.
> > > >
> > > > Please add a minimal header to the overlay.verity xattr format,
> > > > similar to ovl_fb, that will allow composefs/overlayfs to be
> > > > maintained as the separate projects that they are.
> > > >
> > > > Something like this?
> > > >
> > > > /* On-disk format for "verity" xattr */
> > > > struct ovl_verity {
> > > > u8 version; /* 0 */
> > > > u8 len; /* size of this header + size of digest */
> > > > u8 flags;
> > > > u8 algo; /* digest algo */
> > > > u8 digest[];
> > > > };
> > > >
> > > > I realize that digest size may be inferred from xattr size,
> > > > but it is better to state the stored digest size explicitly and verify
> > > > that it matches expected xattr size.
> > >
> > > If it was up to me I think I'd keep it even more "minimal" just do:
> > >
> > > struct ovl_verity {
> > > u8 version; /* 0 */
> > > u8 algo; /* digest algo */
> > > u8 digest[];
> > > };
> > >
> > > It's up to you, though.
> >
> > Please keep len and flags.
> > Nothing to be gained from removing them.
>
> Something is gained by removing them: smaller images.
>
> I have a Centos 9 developer rootfs here to test with. The basic
> composefs image size for this with the current format (1 byte for algo)
> is 11M. Adding one more byte (version) adds 88k (0.8%) to it and adding 3
> bytes (version+flags+len) adds 240k (2.1%).
>
I am not impressed.
This is micro optimization IMO.
If you want to optimize xattrs for size, you could have erofs
compress all trusted.overlay.* names into byte special codes.
On local disk fs, this few bytes change in xattr size won't matter one bit.
> These are not huge costs, but they are not really giving any huge advantages
> either. I mean, the digest length can be computed already, both from
> the xattr size
> and the algo type. And flag-like features could be encoded in the
> version byte, or,
> if really needed, a version 2 header could add a flags field.
>
Yes it could at the cost of uglier code.
We will do that if we have to, but not because we failed
to reserve room for flags to begin with.
It would be a rookie on-disk format mistake to do that.
> I don't believe this format will actually need to change much.
> However, I do agree
> with the general requirement for *some* ability to move forward with
> this format,
> so I'm gonna go with a single version byte.
>
I disagree.
If you present a real life use case why that really matters
then I can reconsider.
Thanks,
Amir.
