On Sun, Sep 01, 2019 at 03:54:11PM +0800, Gao Xiang wrote: > It could be better has a name though, because 1) erofs.mkfs uses this > definition explicitly, and we keep this on-disk definition erofs_fs.h > file up with erofs-utils. > > 2) For kernel use, first we have, > datamode < EROFS_INODE_LAYOUT_MAX; and > !erofs_inode_is_data_compressed, so there are only two mode here, > 1) EROFS_INODE_FLAT_INLINE, > 2) EROFS_INODE_FLAT_PLAIN > if its datamode isn't EROFS_INODE_FLAT_INLINE (tail-end block packing), > it should be EROFS_INODE_FLAT_PLAIN. > > The detailed logic in erofs_read_inode and > erofs_map_blocks_flatmode.... Ok. At least the explicit numbering makes this a little more obvious now. What seems fairly odd is that there are only various places that check for some inode layouts/formats but nothing that does a switch over all of them. > > why are we adding a legacy field to a brand new file system? > > The difference is just EROFS_INODE_FLAT_COMPRESSION_LEGACY doesn't > have z_erofs_map_header, so it only supports default (4k clustersize) > fixed-sized output compression rather than per-file setting, nothing > special at all... It still seems odd to add a legacy field to a brand new file system. > > structures, as that keeps it clear in everyones mind what needs to > > stay persistent and what can be chenged easily. > > All fields in this file are on-disk representation by design > (no logic for in-memory presentation). Ok, make sense. Maybe add a note to the top of the file comment that this is the on-disk format. One little oddity is that erofs_inode_is_data_compressed is here, while is_inode_flat_inline is in internal.h. There are arguments for either place, but I'd suggest to keep the related macros together. _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
