erofs: Question on unused fields in on-disk structs

erofs: Question on unused fields in on-disk structs

[Richard Weinberger]

Hi!

struct erofs_super_block has "checksum" and "features" fields,
but they are not used in the source.
What is the plan for these?

Same for i_checksum in erofs_inode_v1 and erofs_inode_v2.

At least the "features" field in the super block is something I'd
expect to be used.
...such that you can have new filesystem features in future.

Thanks,
//richard

Re: erofs: Question on unused fields in on-disk structs

[Gao Xiang]

Hi Richard,

On Mon, Aug 19, 2019 at 07:10:33PM +0200, Richard Weinberger wrote:
> Hi!
> 
> struct erofs_super_block has "checksum" and "features" fields,
> but they are not used in the source.
> What is the plan for these?

Yes, both will be used laterly (features is used for compatible
features, we already have some incompatible features in 5.3).

> 
> Same for i_checksum in erofs_inode_v1 and erofs_inode_v2.

checksum field apart from super_block has been reserved again
for linux-next. checksum in the super_block still exists and
will be used sooner.

The reason I discussed with Chao is
since EROFS is a read-only filesystem, we will develop block-based
checksum and integrity check to EROFS in the future version. It's
more effectively than adding such fields to all metadata (some metadata
is too large, we cannot calculate a checksum for the whole metadata
and compare at runtime, but we can do block-based metadata chksum
since EROFS is a read-only fs).

> 
> At least the "features" field in the super block is something I'd
> expect to be used.
> ...such that you can have new filesystem features in future.

Of course, "features" is for compatible features, "requirements" is
for incompatible features. Both will be used in the future.

Thanks,
Gao Xiang

> 
> Thanks,
> //richard

Re: erofs: Question on unused fields in on-disk structs

[Richard Weinberger]

Gao Xiang,

On Mon, Aug 19, 2019 at 10:45 PM Gao Xiang via Linux-erofs
<linux-erofs@lists.ozlabs.org> wrote:
> > struct erofs_super_block has "checksum" and "features" fields,
> > but they are not used in the source.
> > What is the plan for these?
>
> Yes, both will be used laterly (features is used for compatible
> features, we already have some incompatible features in 5.3).

Good. :-)
I suggest to check the fields being 0 right now.
Otherwise you are in danger that they get burned if an mkfs.erofs does not
initialize the fields.

-- 
Thanks,
//richard

Re: erofs: Question on unused fields in on-disk structs

[Gao Xiang]

Hi Richard,

On Wed, Aug 21, 2019 at 11:37:30PM +0200, Richard Weinberger wrote:
> Gao Xiang,
> 
> On Mon, Aug 19, 2019 at 10:45 PM Gao Xiang via Linux-erofs
> <linux-erofs@lists.ozlabs.org> wrote:
> > > struct erofs_super_block has "checksum" and "features" fields,
> > > but they are not used in the source.
> > > What is the plan for these?
> >
> > Yes, both will be used laterly (features is used for compatible
> > features, we already have some incompatible features in 5.3).
> 
> Good. :-)
> I suggest to check the fields being 0 right now.
> Otherwise you are in danger that they get burned if an mkfs.erofs does not
> initialize the fields.

Sorry... I cannot get the point...

super block chksum could be a compatible feature right? which means
new kernel can support it (maybe we can add a warning if such image
doesn't have a chksum then when mounting) but old kernel doesn't
care it.

Or maybe you mean these reserved fields? I have no idea all other
filesystems check these fields to 0 or not... But I think it should
be used with some other flag is set rather than directly use, right?

Thanks,
Gao Xiang

> 
> -- 
> Thanks,
> //richard

Re: erofs: Question on unused fields in on-disk structs

[Richard Weinberger]

On Thu, Aug 22, 2019 at 12:03 AM Gao Xiang <hsiangkao@aol.com> wrote:
>
> Hi Richard,
>
> On Wed, Aug 21, 2019 at 11:37:30PM +0200, Richard Weinberger wrote:
> > Gao Xiang,
> >
> > On Mon, Aug 19, 2019 at 10:45 PM Gao Xiang via Linux-erofs
> > <linux-erofs@lists.ozlabs.org> wrote:
> > > > struct erofs_super_block has "checksum" and "features" fields,
> > > > but they are not used in the source.
> > > > What is the plan for these?
> > >
> > > Yes, both will be used laterly (features is used for compatible
> > > features, we already have some incompatible features in 5.3).
> >
> > Good. :-)
> > I suggest to check the fields being 0 right now.
> > Otherwise you are in danger that they get burned if an mkfs.erofs does not
> > initialize the fields.
>
> Sorry... I cannot get the point...

Sorry for being unclear, let me explain in more detail.

> super block chksum could be a compatible feature right? which means
> new kernel can support it (maybe we can add a warning if such image
> doesn't have a chksum then when mounting) but old kernel doesn't
> care it.

Yes. But you need some why to indicate that the chksum field is now
valid and must be used.

The features field can be used for that, but you don't use it right now.
I recommend to check it for being 0, 0 means then "no features".
If somebody creates in future a erofs with more features this code
can refuse to mount because it does not support these features.

But be very sure that existing erofs filesystems actually have this field
set to 0 or something other which is always the same.
Otherwise you cannot use the field anymore because it could be anything.
A common bug is that the mkfs program keeps such unused fields
uninitialized and then it can be a more or less random value without
notice.

> Or maybe you mean these reserved fields? I have no idea all other
> filesystems check these fields to 0 or not... But I think it should
> be used with some other flag is set rather than directly use, right?

Basically you want a way to know when a field shall be used and when not.
Most filesystems have version/feature fields. Often multiple to denote different
levels of compatibility.

-- 
Thanks,
//richard

Re: erofs: Question on unused fields in on-disk structs

[Gao Xiang]

Hi Richard,

On Thu, Aug 22, 2019 at 10:33:01AM +0200, Richard Weinberger wrote:
> On Thu, Aug 22, 2019 at 12:03 AM Gao Xiang <hsiangkao@aol.com> wrote:
> >
> > Hi Richard,
> >
> > On Wed, Aug 21, 2019 at 11:37:30PM +0200, Richard Weinberger wrote:
> > > Gao Xiang,
> > >
> > > On Mon, Aug 19, 2019 at 10:45 PM Gao Xiang via Linux-erofs
> > > <linux-erofs@lists.ozlabs.org> wrote:
> > > > > struct erofs_super_block has "checksum" and "features" fields,
> > > > > but they are not used in the source.
> > > > > What is the plan for these?
> > > >
> > > > Yes, both will be used laterly (features is used for compatible
> > > > features, we already have some incompatible features in 5.3).
> > >
> > > Good. :-)
> > > I suggest to check the fields being 0 right now.
> > > Otherwise you are in danger that they get burned if an mkfs.erofs does not
> > > initialize the fields.
> >
> > Sorry... I cannot get the point...
> 
> Sorry for being unclear, let me explain in more detail.

Thank you!

> 
> > super block chksum could be a compatible feature right? which means
> > new kernel can support it (maybe we can add a warning if such image
> > doesn't have a chksum then when mounting) but old kernel doesn't
> > care it.
> 
> Yes. But you need some why to indicate that the chksum field is now
> valid and must be used.

We can add a compat "feature" as my following saying...
(If I missed something, please kindly point out...)

> 
> The features field can be used for that, but you don't use it right now.
> I recommend to check it for being 0, 0 means then "no features".
> If somebody creates in future a erofs with more features this code
> can refuse to mount because it does not support these features.

"requirements" field is for that, it means incompat features as the following code shown:
 69 static bool check_layout_compatibility(struct super_block *sb,
 70                                        struct erofs_super_block *layout)
 71 {
 72         const unsigned int requirements = le32_to_cpu(layout->requirements);
 73
 74         EROFS_SB(sb)->requirements = requirements;
 75
 76         /* check if current kernel meets all mandatory requirements */
 77         if (requirements & (~EROFS_ALL_REQUIREMENTS)) {
 78                 errln("unidentified requirements %x, please upgrade kernel version",
 79                       requirements & ~EROFS_ALL_REQUIREMENTS);
 80                 return false;
 81         }
 82         return true;
 83 }

if some "requirements" don't be recognized by the current kernel,
it will refuse to mount but "features" not.

> 
> But be very sure that existing erofs filesystems actually have this field
> set to 0 or something other which is always the same.
> Otherwise you cannot use the field anymore because it could be anything.
> A common bug is that the mkfs program keeps such unused fields
> uninitialized and then it can be a more or less random value without
> notice.

Why? In my thought, the logic is that
 - v4.3, "features" that kernel can handle is 0, so chksum is unused (DONTCARE field)
   and chksum field could be anything, but the kernel doesn't care.

 - later version, add an extra compat feature to "features" to indicate SB_CHKSUM
    is now valid, such as EROFS_FEATURE_SB_CHKSUM (rather than requirements, it's
    incompat), so the kernel can check the checksum like that:

    if (feature & EROFS_FEATURE_SB_CHKSUM) {	/* chksum is set */
        if (chk crc32c and no match) {
             return -EFSBADCRC;
	}
        go ahead
    } else {
        /* still don't care chksum field but print the following warning to kmsg */
        warnln("You are mounting a image without super_block chksum, please take care!!!!");

        or maybe we can even refuse mount these images, except for some mount option
        such as "force-mount".
    }

 That is also what F2FS did recently, refer the following commit
   commit d440c52d3151("f2fs: support superblock checksum")

> 
> > Or maybe you mean these reserved fields? I have no idea all other
> > filesystems check these fields to 0 or not... But I think it should
> > be used with some other flag is set rather than directly use, right?
> 
> Basically you want a way to know when a field shall be used and when not.
> Most filesystems have version/feature fields. Often multiple to denote different
> levels of compatibility.

On-disk inode has i_advise field, and super_block has
"features" and "requirements" fields. we can use some of them
or any combinations.

Thanks,
Gao Xiang

> 
> -- 
> Thanks,
> //richard

Re: erofs: Question on unused fields in on-disk structs

[Gao Xiang]

On Thu, Aug 22, 2019 at 05:05:41PM +0800, Gao Xiang wrote:

[]

> > 
> > But be very sure that existing erofs filesystems actually have this field
> > set to 0 or something other which is always the same.
> > Otherwise you cannot use the field anymore because it could be anything.
> > A common bug is that the mkfs program keeps such unused fields
> > uninitialized and then it can be a more or less random value without
> > notice.
> 
> Why? In my thought, the logic is that
>  - v4.3, "features" that kernel can handle is 0, so chksum is unused (DONTCARE field)
>    and chksum field could be anything, but the kernel doesn't care.

- sorry, I meant linux <= v5.3. add a word....

Thanks,
Gao Xiang

> 
>  - later version, add an extra compat feature to "features" to indicate SB_CHKSUM
>     is now valid, such as EROFS_FEATURE_SB_CHKSUM (rather than requirements, it's
>     incompat), so the kernel can check the checksum like that:
> 
>     if (feature & EROFS_FEATURE_SB_CHKSUM) {	/* chksum is set */
>         if (chk crc32c and no match) {
>              return -EFSBADCRC;
> 	}
>         go ahead
>     } else {
>         /* still don't care chksum field but print the following warning to kmsg */
>         warnln("You are mounting a image without super_block chksum, please take care!!!!");
> 
>         or maybe we can even refuse mount these images, except for some mount option
>         such as "force-mount".
>     }
> 
>  That is also what F2FS did recently, refer the following commit
>    commit d440c52d3151("f2fs: support superblock checksum")
> 
> > 
> > > Or maybe you mean these reserved fields? I have no idea all other
> > > filesystems check these fields to 0 or not... But I think it should
> > > be used with some other flag is set rather than directly use, right?
> > 
> > Basically you want a way to know when a field shall be used and when not.
> > Most filesystems have version/feature fields. Often multiple to denote different
> > levels of compatibility.
> 
> On-disk inode has i_advise field, and super_block has
> "features" and "requirements" fields. we can use some of them
> or any combinations.
> 
> Thanks,
> Gao Xiang
> 
> > 
> > -- 
> > Thanks,
> > //richard

Re: erofs: Question on unused fields in on-disk structs

[Theodore Y. Ts'o]

On Thu, Aug 22, 2019 at 10:33:01AM +0200, Richard Weinberger wrote:
> > super block chksum could be a compatible feature right? which means
> > new kernel can support it (maybe we can add a warning if such image
> > doesn't have a chksum then when mounting) but old kernel doesn't
> > care it.
> 
> Yes. But you need some why to indicate that the chksum field is now
> valid and must be used.
> 
> The features field can be used for that, but you don't use it right now.
> I recommend to check it for being 0, 0 means then "no features".
> If somebody creates in future a erofs with more features this code
> can refuse to mount because it does not support these features.

The whole point of "compat" features is that the kernel can go ahead
and mount the file system even if there is some new "compat" feature
which it doesn't understand.  So the fact that right now erofs doesn't
have any "compat" features means it's not surprising, and perfectly
OK, if it's not referenced by the kernel.

For ext4, we have some more complex feature bitmasks, "compat",
"ro_compat" (OK to mount read-only if there are features you don't
understand) and "incompat" (if there are any bits you don't
understand, fail the mount).  But since erofs is a read-only file
system, things are much simpler.

It might make life easier for other kernel developers if "features"
was named "compat_features" and "requirements" were named
"incompat_features", just because of the long-standing use of that in
ext2, ext3, ext4, ocfs2, etc.  But that naming scheme really is a
legacy of ext2 and its descendents, and there's no real reason why it
has to be that way on other file systems.

Cheers,

						- Ted

Re: erofs: Question on unused fields in on-disk structs

[Richard Weinberger]

On Thu, Aug 22, 2019 at 4:21 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
> It might make life easier for other kernel developers if "features"
> was named "compat_features" and "requirements" were named
> "incompat_features", just because of the long-standing use of that in
> ext2, ext3, ext4, ocfs2, etc.  But that naming scheme really is a
> legacy of ext2 and its descendents, and there's no real reason why it
> has to be that way on other file systems.

Yes, the naming confused me a little. :-)

-- 
Thanks,
//richard

Re: erofs: Question on unused fields in on-disk structs

[Gao Xiang]

Hi Richard,

On Thu, Aug 22, 2019 at 04:29:44PM +0200, Richard Weinberger wrote:
> On Thu, Aug 22, 2019 at 4:21 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
> > It might make life easier for other kernel developers if "features"
> > was named "compat_features" and "requirements" were named
> > "incompat_features", just because of the long-standing use of that in
> > ext2, ext3, ext4, ocfs2, etc.  But that naming scheme really is a
> > legacy of ext2 and its descendents, and there's no real reason why it
> > has to be that way on other file systems.
> 
> Yes, the naming confused me a little. :-)

Sorry for confusing... And thanks, I'm happy that
you give us those reports. and sorry about my poor
English...

Thanks,
Gao Xiang

> 
> -- 
> Thanks,
> //richard

Re: erofs: Question on unused fields in on-disk structs

[Gao Xiang]

Hi Ted,

On Thu, Aug 22, 2019 at 10:21:42AM -0400, Theodore Y. Ts'o wrote:
> On Thu, Aug 22, 2019 at 10:33:01AM +0200, Richard Weinberger wrote:
> > > super block chksum could be a compatible feature right? which means
> > > new kernel can support it (maybe we can add a warning if such image
> > > doesn't have a chksum then when mounting) but old kernel doesn't
> > > care it.
> > 
> > Yes. But you need some why to indicate that the chksum field is now
> > valid and must be used.
> > 
> > The features field can be used for that, but you don't use it right now.
> > I recommend to check it for being 0, 0 means then "no features".
> > If somebody creates in future a erofs with more features this code
> > can refuse to mount because it does not support these features.
> 
> The whole point of "compat" features is that the kernel can go ahead
> and mount the file system even if there is some new "compat" feature
> which it doesn't understand.  So the fact that right now erofs doesn't
> have any "compat" features means it's not surprising, and perfectly
> OK, if it's not referenced by the kernel.
> 
> For ext4, we have some more complex feature bitmasks, "compat",
> "ro_compat" (OK to mount read-only if there are features you don't
> understand) and "incompat" (if there are any bits you don't
> understand, fail the mount).  But since erofs is a read-only file
> system, things are much simpler.
> 
> It might make life easier for other kernel developers if "features"
> was named "compat_features" and "requirements" were named
> "incompat_features", just because of the long-standing use of that in
> ext2, ext3, ext4, ocfs2, etc.  But that naming scheme really is a
> legacy of ext2 and its descendents, and there's no real reason why it
> has to be that way on other file systems.

Thanks for your detailed explanation, thanks a lot!

Thanks,
Gao Xiang

> 
> Cheers,
> 
> 						- Ted