All of lore.kernel.org
 help / color / mirror / Atom feed
From: Coly Li <colyli@suse.de>
To: Hannes Reinecke <hare@suse.de>
Cc: linux-bcache@vger.kernel.org, linux-block@vger.kernel.org
Subject: Re: [PATCH 14/14] bcache: move struct cache_sb out of uapi bcache.h
Date: Mon, 17 Aug 2020 15:10:29 +0800	[thread overview]
Message-ID: <4b5fb2e9-5aae-f156-b680-645de54837e9@suse.de> (raw)
In-Reply-To: <809bef2f-af92-1071-685d-6454fa93b50c@suse.de>

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


  reply	other threads:[~2020-08-17  7:10 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-15  4:10 [PATCH 00/14] bcache: remove multiple caches code framework Coly Li
2020-08-15  4:10 ` [PATCH 01/14] bcache: remove 'int n' from parameter list of bch_bucket_alloc_set() Coly Li
2020-08-17  6:08   ` Hannes Reinecke
2020-08-15  4:10 ` [PATCH 02/14] bcache: explicitly make cache_set only have single cache Coly Li
2020-08-17  6:11   ` Hannes Reinecke
2020-08-15  4:10 ` [PATCH 03/14] bcache: remove for_each_cache() Coly Li
2020-08-17  6:13   ` Hannes Reinecke
2020-08-22 11:40     ` Coly Li
2020-08-15  4:10 ` [PATCH 04/14] bcache: add set_uuid in struct cache_set Coly Li
2020-08-17  6:15   ` Hannes Reinecke
2020-08-15  4:10 ` [PATCH 05/14] bcache: only use block_bytes() on struct cache Coly Li
2020-08-17  6:16   ` Hannes Reinecke
2020-08-15  4:10 ` [PATCH 06/14] bcache: remove useless alloc_bucket_pages() Coly Li
2020-08-17  6:17   ` Hannes Reinecke
2020-08-15  4:10 ` [PATCH 07/14] bcache: remove useless bucket_pages() Coly Li
2020-08-17  6:18   ` Hannes Reinecke
2020-08-15  4:10 ` [PATCH 08/14] bcache: only use bucket_bytes() on struct cache Coly Li
2020-08-17  6:19   ` Hannes Reinecke
2020-08-15  4:10 ` [PATCH 09/14] bcache: avoid data copy between cache_set->sb and cache->sb Coly Li
2020-08-17  6:22   ` Hannes Reinecke
2020-08-17  6:30     ` Coly Li
2020-08-15  4:10 ` [PATCH 10/14] bcache: don't check seq numbers in register_cache_set() Coly Li
2020-08-17  6:23   ` Hannes Reinecke
2020-08-15  4:10 ` [PATCH 11/14] bcache: remove can_attach_cache() Coly Li
2020-08-17  6:24   ` Hannes Reinecke
2020-08-15  4:10 ` [PATCH 12/14] bcache: check and set sync status on cache's in-memory super block Coly Li
2020-08-17  6:25   ` Hannes Reinecke
2020-08-15  4:10 ` [PATCH 13/14] bcache: remove embedded struct cache_sb from struct cache_set Coly Li
2020-08-17  6:26   ` Hannes Reinecke
2020-08-15  4:10 ` [PATCH 14/14] bcache: move struct cache_sb out of uapi bcache.h Coly Li
2020-08-17  6:36   ` Hannes Reinecke
2020-08-17  7:10     ` Coly Li [this message]
2020-08-17 10:28   ` Christoph Hellwig
2020-08-17 11:48     ` Coly Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4b5fb2e9-5aae-f156-b680-645de54837e9@suse.de \
    --to=colyli@suse.de \
    --cc=hare@suse.de \
    --cc=linux-bcache@vger.kernel.org \
    --cc=linux-block@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.