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@xxxxxxx> 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@xxxxxxxxxxxxxxxx> 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