| Message ID | 20200815041043.45116-15-colyli@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bcache: remove multiple caches code framework | expand |
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. 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. 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. Cheers, Hannes
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
On Sat, Aug 15, 2020 at 12:10:43PM +0800, 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. Honestly, nothing in include/uapi/linux/bcache.h is a UAPI. It seems to be a random mix of the on-disk format and internals, but certainly noting that declares a UAPI at all. Which might not invalidate this patch, but while you touch it you should probably add a patch that once only the on-disk format is left the file gets renamed to say drivers/md/bcache/disk_format.h.
On 2020/8/17 18:28, Christoph Hellwig wrote: > On Sat, Aug 15, 2020 at 12:10:43PM +0800, 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. > > Honestly, nothing in include/uapi/linux/bcache.h is a UAPI. It seems > to be a random mix of the on-disk format and internals, but certainly > noting that declares a UAPI at all. > > Which might not invalidate this patch, but while you touch it you > should probably add a patch that once only the on-disk format is > left the file gets renamed to say drivers/md/bcache/disk_format.h. > OK then I keep include/uapi/linux/bcache as it is for this moment. Thanks for the comments. Coly Li
diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h index 1d57f48307e6..b755bf7832ac 100644 --- a/drivers/md/bcache/bcache.h +++ b/drivers/md/bcache/bcache.h @@ -279,6 +279,82 @@ struct bcache_device { unsigned int cmd, unsigned long arg); }; +/* + * This is for in-memory bcache super block. + * NOTE: cache_sb is NOT exactly mapping to cache_sb_disk, the member + * size, ordering and even whole struct size may be different + * from cache_sb_disk. + */ +struct cache_sb { + __u64 offset; /* sector where this sb was written */ + __u64 version; + + __u8 magic[16]; + + __u8 uuid[16]; + union { + __u8 set_uuid[16]; + __u64 set_magic; + }; + __u8 label[SB_LABEL_SIZE]; + + __u64 flags; + __u64 seq; + + __u64 feature_compat; + __u64 feature_incompat; + __u64 feature_ro_compat; + + union { + struct { + /* Cache devices */ + __u64 nbuckets; /* device size */ + + __u16 block_size; /* sectors */ + __u16 nr_in_set; + __u16 nr_this_dev; + __u32 bucket_size; /* sectors */ + }; + struct { + /* Backing devices */ + __u64 data_offset; + + /* + * block_size from the cache device section is still used by + * backing devices, so don't add anything here until we fix + * things to not need it for backing devices anymore + */ + }; + }; + + __u32 last_mount; /* time overflow in y2106 */ + + __u16 first_bucket; + union { + __u16 njournal_buckets; + __u16 keys; + }; + __u64 d[SB_JOURNAL_BUCKETS]; /* journal buckets */ +}; + +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); +#define CACHE_REPLACEMENT_LRU 0U +#define CACHE_REPLACEMENT_FIFO 1U +#define CACHE_REPLACEMENT_RANDOM 2U + +BITMASK(BDEV_CACHE_MODE, struct cache_sb, flags, 0, 4); +#define CACHE_MODE_WRITETHROUGH 0U +#define CACHE_MODE_WRITEBACK 1U +#define CACHE_MODE_WRITEAROUND 2U +#define CACHE_MODE_NONE 3U +BITMASK(BDEV_STATE, struct cache_sb, flags, 61, 2); +#define BDEV_STATE_NONE 0U +#define BDEV_STATE_CLEAN 1U +#define BDEV_STATE_DIRTY 2U +#define BDEV_STATE_STALE 3U + struct io { /* Used to track sequential IO so it can be skipped */ struct hlist_node hash; @@ -840,6 +916,13 @@ static inline bool ptr_available(struct cache_set *c, const struct bkey *k, return (PTR_DEV(k, i) < MAX_CACHES_PER_SET) && PTR_CACHE(c, k, i); } +static inline _Bool SB_IS_BDEV(const struct cache_sb *sb) +{ + return sb->version == BCACHE_SB_VERSION_BDEV + || sb->version == BCACHE_SB_VERSION_BDEV_WITH_OFFSET + || sb->version == BCACHE_SB_VERSION_BDEV_WITH_FEATURES; +} + /* Btree key macros */ /* @@ -958,6 +1041,22 @@ static inline void wait_for_kthread_stop(void) } } +/* generate magic number */ +static inline __u64 jset_magic(struct cache_sb *sb) +{ + return sb->set_magic ^ JSET_MAGIC; +} + +static inline __u64 pset_magic(struct cache_sb *sb) +{ + return sb->set_magic ^ PSET_MAGIC; +} + +static inline __u64 bset_magic(struct cache_sb *sb) +{ + return sb->set_magic ^ BSET_MAGIC; +} + /* Forward declarations */ void bch_count_backing_io_errors(struct cached_dev *dc, struct bio *bio); diff --git a/include/uapi/linux/bcache.h b/include/uapi/linux/bcache.h index 52e8bcb33981..18166a3d8503 100644 --- a/include/uapi/linux/bcache.h +++ b/include/uapi/linux/bcache.h @@ -216,89 +216,6 @@ struct cache_sb_disk { __le16 bucket_size_hi; }; -/* - * This is for in-memory bcache super block. - * NOTE: cache_sb is NOT exactly mapping to cache_sb_disk, the member - * size, ordering and even whole struct size may be different - * from cache_sb_disk. - */ -struct cache_sb { - __u64 offset; /* sector where this sb was written */ - __u64 version; - - __u8 magic[16]; - - __u8 uuid[16]; - union { - __u8 set_uuid[16]; - __u64 set_magic; - }; - __u8 label[SB_LABEL_SIZE]; - - __u64 flags; - __u64 seq; - - __u64 feature_compat; - __u64 feature_incompat; - __u64 feature_ro_compat; - - union { - struct { - /* Cache devices */ - __u64 nbuckets; /* device size */ - - __u16 block_size; /* sectors */ - __u16 nr_in_set; - __u16 nr_this_dev; - __u32 bucket_size; /* sectors */ - }; - struct { - /* Backing devices */ - __u64 data_offset; - - /* - * block_size from the cache device section is still used by - * backing devices, so don't add anything here until we fix - * things to not need it for backing devices anymore - */ - }; - }; - - __u32 last_mount; /* time overflow in y2106 */ - - __u16 first_bucket; - union { - __u16 njournal_buckets; - __u16 keys; - }; - __u64 d[SB_JOURNAL_BUCKETS]; /* journal buckets */ -}; - -static inline _Bool SB_IS_BDEV(const struct cache_sb *sb) -{ - return sb->version == BCACHE_SB_VERSION_BDEV - || sb->version == BCACHE_SB_VERSION_BDEV_WITH_OFFSET - || sb->version == BCACHE_SB_VERSION_BDEV_WITH_FEATURES; -} - -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); -#define CACHE_REPLACEMENT_LRU 0U -#define CACHE_REPLACEMENT_FIFO 1U -#define CACHE_REPLACEMENT_RANDOM 2U - -BITMASK(BDEV_CACHE_MODE, struct cache_sb, flags, 0, 4); -#define CACHE_MODE_WRITETHROUGH 0U -#define CACHE_MODE_WRITEBACK 1U -#define CACHE_MODE_WRITEAROUND 2U -#define CACHE_MODE_NONE 3U -BITMASK(BDEV_STATE, struct cache_sb, flags, 61, 2); -#define BDEV_STATE_NONE 0U -#define BDEV_STATE_CLEAN 1U -#define BDEV_STATE_DIRTY 2U -#define BDEV_STATE_STALE 3U - /* * Magic numbers * @@ -310,21 +227,6 @@ BITMASK(BDEV_STATE, struct cache_sb, flags, 61, 2); #define PSET_MAGIC 0x6750e15f87337f91ULL #define BSET_MAGIC 0x90135c78b99e07f5ULL -static inline __u64 jset_magic(struct cache_sb *sb) -{ - return sb->set_magic ^ JSET_MAGIC; -} - -static inline __u64 pset_magic(struct cache_sb *sb) -{ - return sb->set_magic ^ PSET_MAGIC; -} - -static inline __u64 bset_magic(struct cache_sb *sb) -{ - return sb->set_magic ^ BSET_MAGIC; -} - /* * Journal *
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. Signed-off-by: Coly Li <colyli@suse.de> --- drivers/md/bcache/bcache.h | 99 +++++++++++++++++++++++++++++++++++++ include/uapi/linux/bcache.h | 98 ------------------------------------ 2 files changed, 99 insertions(+), 98 deletions(-)