| Message ID | 20200715054612.6349-2-colyli@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bcache: extend bucket size to 32bit width | expand |
On 7/15/20 7:45 AM, Coly Li wrote: > This patch adds comments to mark each member of struct cache_sb_disk, > it is helpful to understand the bcache superblock on-disk layout. > > Signed-off-by: Coly Li <colyli@suse.de> > --- > include/uapi/linux/bcache.h | 39 +++++++++++++++++++------------------ > 1 file changed, 20 insertions(+), 19 deletions(-) > > diff --git a/include/uapi/linux/bcache.h b/include/uapi/linux/bcache.h > index 9a1965c6c3d0..afbd1b56a661 100644 > --- a/include/uapi/linux/bcache.h > +++ b/include/uapi/linux/bcache.h > @@ -158,33 +158,33 @@ static inline struct bkey *bkey_idx(const struct bkey *k, unsigned int nr_keys) > #define BDEV_DATA_START_DEFAULT 16 /* sectors */ > > struct cache_sb_disk { > - __le64 csum; > - __le64 offset; /* sector where this sb was written */ > - __le64 version; > +/*000*/ __le64 csum; > +/*008*/ __le64 offset; /* sector where this sb was written */ > +/*010*/ __le64 version; > > - __u8 magic[16]; > +/*018*/ __u8 magic[16]; > > - __u8 uuid[16]; > +/*028*/ __u8 uuid[16]; > union { > - __u8 set_uuid[16]; > +/*038*/ __u8 set_uuid[16]; > __le64 set_magic; > }; > - __u8 label[SB_LABEL_SIZE]; > +/*048*/ __u8 label[SB_LABEL_SIZE]; > > - __le64 flags; > - __le64 seq; > - __le64 pad[8]; > +/*068*/ __le64 flags; > +/*070*/ __le64 seq; > +/*078*/ __le64 pad[8]; > > union { > struct { > /* Cache devices */ > - __le64 nbuckets; /* device size */ > +/*0b8*/ __le64 nbuckets; /* device size */ > > - __le16 block_size; /* sectors */ > - __le16 bucket_size; /* sectors */ > +/*0c0*/ __le16 block_size; /* sectors */ > +/*0c2*/ __le16 bucket_size; /* sectors */ > > - __le16 nr_in_set; > - __le16 nr_this_dev; > +/*0c4*/ __le16 nr_in_set; > +/*0c6*/ __le16 nr_this_dev; > }; > struct { > /* Backing devices */ > @@ -198,14 +198,15 @@ struct cache_sb_disk { > }; > }; > > - __le32 last_mount; /* time overflow in y2106 */ > +/*0c8*/ __le32 last_mount; /* time overflow in y2106 */ > > - __le16 first_bucket; > +/*0cc*/ __le16 first_bucket; > union { > - __le16 njournal_buckets; > +/*0ce*/ __le16 njournal_buckets; > __le16 keys; > }; > - __le64 d[SB_JOURNAL_BUCKETS]; /* journal buckets */ > +/*0d0*/ __le64 d[SB_JOURNAL_BUCKETS]; /* journal buckets */ > +/*8d0*/ > }; > > struct cache_sb { > Common practice is to place comments at the end; please don't use the start of the line here. Cheers, Hannes
On 2020/7/15 14:02, Hannes Reinecke wrote: > On 7/15/20 7:45 AM, Coly Li wrote: >> This patch adds comments to mark each member of struct cache_sb_disk, >> it is helpful to understand the bcache superblock on-disk layout. >> >> Signed-off-by: Coly Li <colyli@suse.de> >> --- >> include/uapi/linux/bcache.h | 39 +++++++++++++++++++------------------ >> 1 file changed, 20 insertions(+), 19 deletions(-) >> >> diff --git a/include/uapi/linux/bcache.h b/include/uapi/linux/bcache.h >> index 9a1965c6c3d0..afbd1b56a661 100644 >> --- a/include/uapi/linux/bcache.h >> +++ b/include/uapi/linux/bcache.h >> @@ -158,33 +158,33 @@ static inline struct bkey *bkey_idx(const struct >> bkey *k, unsigned int nr_keys) >> #define BDEV_DATA_START_DEFAULT 16 /* sectors */ >> struct cache_sb_disk { >> - __le64 csum; >> - __le64 offset; /* sector where this sb was written */ >> - __le64 version; >> +/*000*/ __le64 csum; >> +/*008*/ __le64 offset; /* sector where this sb was >> written */ >> +/*010*/ __le64 version; >> - __u8 magic[16]; >> +/*018*/ __u8 magic[16]; >> - __u8 uuid[16]; >> +/*028*/ __u8 uuid[16]; >> union { >> - __u8 set_uuid[16]; >> +/*038*/ __u8 set_uuid[16]; >> __le64 set_magic; >> }; >> - __u8 label[SB_LABEL_SIZE]; >> +/*048*/ __u8 label[SB_LABEL_SIZE]; >> - __le64 flags; >> - __le64 seq; >> - __le64 pad[8]; >> +/*068*/ __le64 flags; >> +/*070*/ __le64 seq; >> +/*078*/ __le64 pad[8]; >> union { >> struct { >> /* Cache devices */ >> - __le64 nbuckets; /* device size */ >> +/*0b8*/ __le64 nbuckets; /* device size */ >> - __le16 block_size; /* sectors */ >> - __le16 bucket_size; /* sectors */ >> +/*0c0*/ __le16 block_size; /* sectors */ >> +/*0c2*/ __le16 bucket_size; /* sectors */ >> - __le16 nr_in_set; >> - __le16 nr_this_dev; >> +/*0c4*/ __le16 nr_in_set; >> +/*0c6*/ __le16 nr_this_dev; >> }; >> struct { >> /* Backing devices */ >> @@ -198,14 +198,15 @@ struct cache_sb_disk { >> }; >> }; >> - __le32 last_mount; /* time overflow in y2106 */ >> +/*0c8*/ __le32 last_mount; /* time overflow in y2106 */ >> - __le16 first_bucket; >> +/*0cc*/ __le16 first_bucket; >> union { >> - __le16 njournal_buckets; >> +/*0ce*/ __le16 njournal_buckets; >> __le16 keys; >> }; >> - __le64 d[SB_JOURNAL_BUCKETS]; /* journal buckets */ >> +/*0d0*/ __le64 d[SB_JOURNAL_BUCKETS]; /* journal >> buckets */ >> +/*8d0*/ >> }; >> struct cache_sb { >> > Common practice is to place comments at the end; please don't use the > start of the line here. Hi Hannes, When I try to move the offset comment to the line end, I find it conflicts with normal code comment, e.g. __le64 d[SB_JOURNAL_BUCKETS]; /* journal buckets */ I have to add the offset comment to the line start. I guess this is why ocfs2 code adds the offset comment at the line start. So finally I have to keep the offset comment on line start still... Coly Li
On 15/07/2020 11:03, Coly Li wrote: > On 2020/7/15 14:02, Hannes Reinecke wrote: >> On 7/15/20 7:45 AM, Coly Li wrote: >>> This patch adds comments to mark each member of struct cache_sb_disk, >>> it is helpful to understand the bcache superblock on-disk layout. >>> >>> Signed-off-by: Coly Li <colyli@suse.de> >>> --- >>> include/uapi/linux/bcache.h | 39 +++++++++++++++++++------------------ >>> 1 file changed, 20 insertions(+), 19 deletions(-) >>> >>> diff --git a/include/uapi/linux/bcache.h b/include/uapi/linux/bcache.h >>> index 9a1965c6c3d0..afbd1b56a661 100644 >>> --- a/include/uapi/linux/bcache.h >>> +++ b/include/uapi/linux/bcache.h >>> @@ -158,33 +158,33 @@ static inline struct bkey *bkey_idx(const struct >>> bkey *k, unsigned int nr_keys) >>> #define BDEV_DATA_START_DEFAULT 16 /* sectors */ >>> struct cache_sb_disk { >>> - __le64 csum; >>> - __le64 offset; /* sector where this sb was written */ >>> - __le64 version; >>> +/*000*/ __le64 csum; >>> +/*008*/ __le64 offset; /* sector where this sb was >>> written */ >>> +/*010*/ __le64 version; >>> - __u8 magic[16]; >>> +/*018*/ __u8 magic[16]; >>> - __u8 uuid[16]; >>> +/*028*/ __u8 uuid[16]; >>> union { >>> - __u8 set_uuid[16]; >>> +/*038*/ __u8 set_uuid[16]; >>> __le64 set_magic; >>> }; >>> - __u8 label[SB_LABEL_SIZE]; >>> +/*048*/ __u8 label[SB_LABEL_SIZE]; >>> - __le64 flags; >>> - __le64 seq; >>> - __le64 pad[8]; >>> +/*068*/ __le64 flags; >>> +/*070*/ __le64 seq; >>> +/*078*/ __le64 pad[8]; >>> union { >>> struct { >>> /* Cache devices */ >>> - __le64 nbuckets; /* device size */ >>> +/*0b8*/ __le64 nbuckets; /* device size */ >>> - __le16 block_size; /* sectors */ >>> - __le16 bucket_size; /* sectors */ >>> +/*0c0*/ __le16 block_size; /* sectors */ >>> +/*0c2*/ __le16 bucket_size; /* sectors */ >>> - __le16 nr_in_set; >>> - __le16 nr_this_dev; >>> +/*0c4*/ __le16 nr_in_set; >>> +/*0c6*/ __le16 nr_this_dev; >>> }; >>> struct { >>> /* Backing devices */ >>> @@ -198,14 +198,15 @@ struct cache_sb_disk { >>> }; >>> }; >>> - __le32 last_mount; /* time overflow in y2106 */ >>> +/*0c8*/ __le32 last_mount; /* time overflow in y2106 */ >>> - __le16 first_bucket; >>> +/*0cc*/ __le16 first_bucket; >>> union { >>> - __le16 njournal_buckets; >>> +/*0ce*/ __le16 njournal_buckets; >>> __le16 keys; >>> }; >>> - __le64 d[SB_JOURNAL_BUCKETS]; /* journal buckets */ >>> +/*0d0*/ __le64 d[SB_JOURNAL_BUCKETS]; /* journal >>> buckets */ >>> +/*8d0*/ >>> }; >>> struct cache_sb { >>> >> Common practice is to place comments at the end; please don't use the >> start of the line here. > > Hi Hannes, > > When I try to move the offset comment to the line end, I find it > conflicts with normal code comment, e.g. > __le64 d[SB_JOURNAL_BUCKETS]; /* journal buckets */ > > I have to add the offset comment to the line start. I guess this is why > ocfs2 code adds the offset comment at the line start. > > So finally I have to keep the offset comment on line start still... Why at them at all? pahole -C or crash/gdb will show them for you if you're interested and if you need it in the code you can use offsetof(). I don't really see a good reason to add these comments.
On 2020/7/15 17:08, Johannes Thumshirn wrote: > On 15/07/2020 11:03, Coly Li wrote: >> On 2020/7/15 14:02, Hannes Reinecke wrote: >>> On 7/15/20 7:45 AM, Coly Li wrote: >>>> This patch adds comments to mark each member of struct cache_sb_disk, >>>> it is helpful to understand the bcache superblock on-disk layout. >>>> >>>> Signed-off-by: Coly Li <colyli@suse.de> >>>> --- >>>> include/uapi/linux/bcache.h | 39 +++++++++++++++++++------------------ >>>> 1 file changed, 20 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/include/uapi/linux/bcache.h b/include/uapi/linux/bcache.h >>>> index 9a1965c6c3d0..afbd1b56a661 100644 >>>> --- a/include/uapi/linux/bcache.h >>>> +++ b/include/uapi/linux/bcache.h >>>> @@ -158,33 +158,33 @@ static inline struct bkey *bkey_idx(const struct >>>> bkey *k, unsigned int nr_keys) >>>> #define BDEV_DATA_START_DEFAULT 16 /* sectors */ >>>> struct cache_sb_disk { >>>> - __le64 csum; >>>> - __le64 offset; /* sector where this sb was written */ >>>> - __le64 version; >>>> +/*000*/ __le64 csum; >>>> +/*008*/ __le64 offset; /* sector where this sb was >>>> written */ >>>> +/*010*/ __le64 version; >>>> - __u8 magic[16]; >>>> +/*018*/ __u8 magic[16]; >>>> - __u8 uuid[16]; >>>> +/*028*/ __u8 uuid[16]; >>>> union { >>>> - __u8 set_uuid[16]; >>>> +/*038*/ __u8 set_uuid[16]; >>>> __le64 set_magic; >>>> }; >>>> - __u8 label[SB_LABEL_SIZE]; >>>> +/*048*/ __u8 label[SB_LABEL_SIZE]; >>>> - __le64 flags; >>>> - __le64 seq; >>>> - __le64 pad[8]; >>>> +/*068*/ __le64 flags; >>>> +/*070*/ __le64 seq; >>>> +/*078*/ __le64 pad[8]; >>>> union { >>>> struct { >>>> /* Cache devices */ >>>> - __le64 nbuckets; /* device size */ >>>> +/*0b8*/ __le64 nbuckets; /* device size */ >>>> - __le16 block_size; /* sectors */ >>>> - __le16 bucket_size; /* sectors */ >>>> +/*0c0*/ __le16 block_size; /* sectors */ >>>> +/*0c2*/ __le16 bucket_size; /* sectors */ >>>> - __le16 nr_in_set; >>>> - __le16 nr_this_dev; >>>> +/*0c4*/ __le16 nr_in_set; >>>> +/*0c6*/ __le16 nr_this_dev; >>>> }; >>>> struct { >>>> /* Backing devices */ >>>> @@ -198,14 +198,15 @@ struct cache_sb_disk { >>>> }; >>>> }; >>>> - __le32 last_mount; /* time overflow in y2106 */ >>>> +/*0c8*/ __le32 last_mount; /* time overflow in y2106 */ >>>> - __le16 first_bucket; >>>> +/*0cc*/ __le16 first_bucket; >>>> union { >>>> - __le16 njournal_buckets; >>>> +/*0ce*/ __le16 njournal_buckets; >>>> __le16 keys; >>>> }; >>>> - __le64 d[SB_JOURNAL_BUCKETS]; /* journal buckets */ >>>> +/*0d0*/ __le64 d[SB_JOURNAL_BUCKETS]; /* journal >>>> buckets */ >>>> +/*8d0*/ >>>> }; >>>> struct cache_sb { >>>> >>> Common practice is to place comments at the end; please don't use the >>> start of the line here. >> >> Hi Hannes, >> >> When I try to move the offset comment to the line end, I find it >> conflicts with normal code comment, e.g. >> __le64 d[SB_JOURNAL_BUCKETS]; /* journal buckets */ >> >> I have to add the offset comment to the line start. I guess this is why >> ocfs2 code adds the offset comment at the line start. >> >> So finally I have to keep the offset comment on line start still... > > Why at them at all? pahole -C or crash/gdb will show them for you if you're > interested and if you need it in the code you can use offsetof(). > > I don't really see a good reason to add these comments. > You are right :-) With pahole there is no reason for having this patch. Thanks for the informative hint, this patch will disappear in next version series. Coly Li
diff --git a/include/uapi/linux/bcache.h b/include/uapi/linux/bcache.h index 9a1965c6c3d0..afbd1b56a661 100644 --- a/include/uapi/linux/bcache.h +++ b/include/uapi/linux/bcache.h @@ -158,33 +158,33 @@ static inline struct bkey *bkey_idx(const struct bkey *k, unsigned int nr_keys) #define BDEV_DATA_START_DEFAULT 16 /* sectors */ struct cache_sb_disk { - __le64 csum; - __le64 offset; /* sector where this sb was written */ - __le64 version; +/*000*/ __le64 csum; +/*008*/ __le64 offset; /* sector where this sb was written */ +/*010*/ __le64 version; - __u8 magic[16]; +/*018*/ __u8 magic[16]; - __u8 uuid[16]; +/*028*/ __u8 uuid[16]; union { - __u8 set_uuid[16]; +/*038*/ __u8 set_uuid[16]; __le64 set_magic; }; - __u8 label[SB_LABEL_SIZE]; +/*048*/ __u8 label[SB_LABEL_SIZE]; - __le64 flags; - __le64 seq; - __le64 pad[8]; +/*068*/ __le64 flags; +/*070*/ __le64 seq; +/*078*/ __le64 pad[8]; union { struct { /* Cache devices */ - __le64 nbuckets; /* device size */ +/*0b8*/ __le64 nbuckets; /* device size */ - __le16 block_size; /* sectors */ - __le16 bucket_size; /* sectors */ +/*0c0*/ __le16 block_size; /* sectors */ +/*0c2*/ __le16 bucket_size; /* sectors */ - __le16 nr_in_set; - __le16 nr_this_dev; +/*0c4*/ __le16 nr_in_set; +/*0c6*/ __le16 nr_this_dev; }; struct { /* Backing devices */ @@ -198,14 +198,15 @@ struct cache_sb_disk { }; }; - __le32 last_mount; /* time overflow in y2106 */ +/*0c8*/ __le32 last_mount; /* time overflow in y2106 */ - __le16 first_bucket; +/*0cc*/ __le16 first_bucket; union { - __le16 njournal_buckets; +/*0ce*/ __le16 njournal_buckets; __le16 keys; }; - __le64 d[SB_JOURNAL_BUCKETS]; /* journal buckets */ +/*0d0*/ __le64 d[SB_JOURNAL_BUCKETS]; /* journal buckets */ +/*8d0*/ }; struct cache_sb {
This patch adds comments to mark each member of struct cache_sb_disk, it is helpful to understand the bcache superblock on-disk layout. Signed-off-by: Coly Li <colyli@suse.de> --- include/uapi/linux/bcache.h | 39 +++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 19 deletions(-)