On Fri, Jun 16, 2023 at 11:39 AM Alexander Larsson <alexl@xxxxxxxxxx> wrote:
>
> 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.
Nice.
didn't know that it was already in motion.
>
> > 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.
>
Let's ask for a 3rd opinion.
don't add them for now, unless Miklos says that you should.
Thanks,
Amir.
