Re: linux-next: Tree for Sep 12 (bcachefs)

[Kent Overstreet]

On Wed, Sep 13, 2023 at 06:17:00PM -0700, Kees Cook wrote:
> On Tue, Sep 12, 2023 at 03:26:45PM +1000, Stephen Rothwell wrote:
> > New tree: bcachefs
> 
> Thanks for going through and fixing all the fake flexible array members.
> It looks much nicer. :)
> 
> I have some questions about the remaining "markers", for example:
> 
> $ git grep -A8 '\bkey_start\b' -- fs/bcachefs
> fs/bcachefs/bcachefs_format.h:  __u8            key_start[0];
> ...
> fs/bcachefs/bcachefs_format.h-  __u8            pad[sizeof(struct bkey) - 3];
> --
> fs/bcachefs/bkey.c:     u8 *l = k->key_start;
> 
> Why isn't this just:
> 
> 			u8 *l = k->pad
> 
> and you can drop the marker?

In this case, it's documentation. &k->pad tells us nothing; why is pad
significant? k->key_start documents the intent better.

> And some seem entirely unused, like all of "struct bch_reflink_v".

No, those aren't unused :)

bcachefs does the "list of variable size items" a lot - see vstructs.h.
start[] is the type of the item being stored, _data is what we use for
pointer arithmetic - because we always store sizes in units of u64s, for
alignment.

> 
> And some are going to fail at runtime, since they're still zero-sized
> and being used as an actual array:
> 
> struct bch_sb_field_journal_seq_blacklist {
>         struct bch_sb_field     field;
> 
>         struct journal_seq_blacklist_entry start[0];
>         __u64                   _data[];
> };
> ...
>                 memmove(&bl->start[i],
>                         &bl->start[i + 1],
>                         sizeof(bl->start[0]) * (nr - i));
> 
> It looks like you just want a type union for the flexible array.
> This can be done like this:
> 
> struct bch_sb_field_journal_seq_blacklist {
>         struct bch_sb_field     field;
> 
> 	union {
> 		DECLARE_FLEX_ARRAY(struct journal_seq_blacklist_entry, start);
> 		DECLARE_FLEX_ARRAY(__u64, _data);
> 	};
> };

Eesh, why though?

Honestly, I'm not a fan of the change to get rid of zero size arrays,
this seems to be adding a whole lot of macro layering and indirection
for nothing.

The only thing a zero size array could possibly be is a flexible array
member or a marker, why couldn't we have just kept treating zero size
arrays like flexible array members?