Re: [PATCH 14/14] bcache: move struct cache_sb out of uapi bcache.h

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux ARM Kernel]     [Linux Filesystem Development]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux