On Fri, Jun 16, 2023 at 10:12 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> 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.
Most optimization is a sequence of micro optimizations.
> If you want to optimize xattrs for size, you could have erofs
> compress all trusted.overlay.* names into byte special codes.
This is already supported as per:
https://lists.ozlabs.org/pipermail/linux-erofs/2023-April/008074.html
(Although I have not yet added it to composefs userspace)
I don't see why you would not want to try to minimize both keyname and
value size though.
> 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.
I disagree, but I'll add them.
--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
Alexander Larsson Red Hat, Inc
alexl@xxxxxxxxxx alexander.larsson@xxxxxxxxx
