On 2020/8/17 14:36, Hannes Reinecke wrote: > On 8/15/20 6:10 AM, Coly Li wrote: >> struct cache_sb does not exactly map to cache_sb_disk, it is only for >> in-memory super block and dosn't belong to uapi bcache.h. >> >> This patch moves the struct cache_sb definition and other depending >> macros and inline routines from include/uapi/linux/bcache.h to >> drivers/md/bcache/bcache.h, this is the proper location to have them. >> > And that I'm not sure of. > The 'uapi' directory is there to hold the user-visible kernel API. > So the real question is whether the bcache superblock is or should be > part of the user API or not. Now the superblock structure divided into two formats, - struct cache_sb_disk The exact structure representing on-disk bcache superblock and the format is visible and shared with user space tools. - struct cache_sb This is in-memory super block only used internal bcache code. It is generated and converted from struct cache_sb_disk, but removed some unnecessary part for in-memory usage. This patch moves the in-memory version: struct cache_sb from the uapi header to bcache internal header. > Hence an alternative way would be to detail out the entire superblock in > include/uapi/linux/bcache.h, and remove the definitions from > drivers/md/bcache/bcache.h. > It makes sense to, because the following flags operation indeed is related to on-disk format, BITMASK(CACHE_SYNC, struct cache_sb, flags, 0, 1); BITMASK(CACHE_DISCARD, struct cache_sb, flags, 1, 1); BITMASK(CACHE_REPLACEMENT, struct cache_sb, flags, 2, 3); The suggestion to move struct cache_sb out of the uapi header was from hch, which makes sense too because struct cache_sb is not part of uapi anymore. > There doesn't seem to be a consistent policy here; some things like MD > have their superblock in the uapi directory, others like btrfs only have > the ioctl definitions there. > > I'm somewhat in favour of having the superblock definition as part of > the uapi, as this would make writing external tools like blkid easier. > But then the ultimate decision is yours. Yes, struct cache_sb_disk is still in uapi hearder, which is 100% compatible with the original mixed used struct cache_sb. The "mixed used" means it was used for both on-disk and in-memory, and both for struct cache_set and struct cache, this is not good IMHO. Thanks. Coly Li
