| Message ID | 844fa765ab173b8dd24549f145534f41d412d3ea.1684826247.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: metadata_uuid refactors part1 | expand |
On Tue, May 23, 2023 at 06:03:15PM +0800, Anand Jain wrote: > By relocating the bool fsid_change near other bool declarations in the > struct btrfs_fs_devices, approximately 6 bytes is saved. > > before: 512 bytes > after: 496 bytes > > Furthermore, adding comments. I like the better backing. But what looks access to fsid_change and the other bools? For sub-word members atomicy guarantees are very limited, so they'd better all use the same lock.
On Tue, May 23, 2023 at 09:27:44AM -0700, Christoph Hellwig wrote: > On Tue, May 23, 2023 at 06:03:15PM +0800, Anand Jain wrote: > > By relocating the bool fsid_change near other bool declarations in the > > struct btrfs_fs_devices, approximately 6 bytes is saved. > > > > before: 512 bytes > > after: 496 bytes > > > > Furthermore, adding comments. > > I like the better backing. But what looks access to fsid_change > and the other bools? For sub-word members atomicy guarantees are > very limited, so they'd better all use the same lock. Do you have an example where the reordered structure would become problematic? The fs_devices locking is non-standard, the structures are accessed from module context or from filesystem context. There's the uuid_mutex as a big lock for fs_devices, and for access of the individual devices is device_list_mutex. It's possible that there's something wrong still but after a quick look I don't see anything obvious. Because of the complex locking there are no optimizations like unlocked access to bool members.
On Tue, May 23, 2023 at 06:03:15PM +0800, Anand Jain wrote: > By relocating the bool fsid_change near other bool declarations in the > struct btrfs_fs_devices, approximately 6 bytes is saved. > > before: 512 bytes > after: 496 bytes > > Furthermore, adding comments. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/volumes.h | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h > index 5cbbee32748c..a9a86c9220b3 100644 > --- a/fs/btrfs/volumes.h > +++ b/fs/btrfs/volumes.h > @@ -281,7 +281,6 @@ enum btrfs_read_policy { > struct btrfs_fs_devices { > u8 fsid[BTRFS_FSID_SIZE]; /* FS specific uuid */ > u8 metadata_uuid[BTRFS_FSID_SIZE]; > - bool fsid_change; > struct list_head fs_list; > > /* > @@ -337,17 +336,24 @@ struct btrfs_fs_devices { > struct list_head alloc_list; > > struct list_head seed_list; > - bool seeding; > > + /* count fs-devices opened */ > int opened; > > - /* set when we find or add a device that doesn't have the > + /* > + * set when we find or add a device that doesn't have the > * nonrot flag set Please reformat comments so they follow the most up to date style, which is to be a full sentence (with "." at the end) and if it fits on one line then it's the /* text */ otherwise multiline /* ... */.
On 24/5/23 05:31, David Sterba wrote: > On Tue, May 23, 2023 at 06:03:15PM +0800, Anand Jain wrote: >> By relocating the bool fsid_change near other bool declarations in the >> struct btrfs_fs_devices, approximately 6 bytes is saved. >> >> before: 512 bytes >> after: 496 bytes >> >> Furthermore, adding comments. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> fs/btrfs/volumes.h | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h >> index 5cbbee32748c..a9a86c9220b3 100644 >> --- a/fs/btrfs/volumes.h >> +++ b/fs/btrfs/volumes.h >> @@ -281,7 +281,6 @@ enum btrfs_read_policy { >> struct btrfs_fs_devices { >> u8 fsid[BTRFS_FSID_SIZE]; /* FS specific uuid */ >> u8 metadata_uuid[BTRFS_FSID_SIZE]; >> - bool fsid_change; >> struct list_head fs_list; >> >> /* >> @@ -337,17 +336,24 @@ struct btrfs_fs_devices { >> struct list_head alloc_list; >> >> struct list_head seed_list; >> - bool seeding; >> >> + /* count fs-devices opened */ >> int opened; >> >> - /* set when we find or add a device that doesn't have the >> + /* >> + * set when we find or add a device that doesn't have the >> * nonrot flag set > > Please reformat comments so they follow the most up to date style, which > is to be a full sentence (with "." at the end) and if it fits on one > line then it's the /* text */ otherwise multiline /* ... */. Sure. I will fix them. Also, I'll split into two patch adding/fixing comments and moving %fsid_change. Thanks, Anand
On Tue, May 23, 2023 at 10:59:17PM +0200, David Sterba wrote: > On Tue, May 23, 2023 at 09:27:44AM -0700, Christoph Hellwig wrote: > > On Tue, May 23, 2023 at 06:03:15PM +0800, Anand Jain wrote: > > > By relocating the bool fsid_change near other bool declarations in the > > > struct btrfs_fs_devices, approximately 6 bytes is saved. > > > > > > before: 512 bytes > > > after: 496 bytes > > > > > > Furthermore, adding comments. > > > > I like the better backing. But what looks access to fsid_change > > and the other bools? For sub-word members atomicy guarantees are > > very limited, so they'd better all use the same lock. > > Do you have an example where the reordered structure would become > problematic? The fs_devices locking is non-standard, the structures are > accessed from module context or from filesystem context. There's the > uuid_mutex as a big lock for fs_devices, and for access of the > individual devices is device_list_mutex. No, I'm mostly asking for Anand to document the rationale why this is fine. If there is an issue, it's probably pre-existing.
On 24/05/2023 13:56, Christoph Hellwig wrote: > On Tue, May 23, 2023 at 10:59:17PM +0200, David Sterba wrote: >> On Tue, May 23, 2023 at 09:27:44AM -0700, Christoph Hellwig wrote: >>> On Tue, May 23, 2023 at 06:03:15PM +0800, Anand Jain wrote: >>>> By relocating the bool fsid_change near other bool declarations in the >>>> struct btrfs_fs_devices, approximately 6 bytes is saved. >>>> >>>> before: 512 bytes >>>> after: 496 bytes >>>> >>>> Furthermore, adding comments. >>> >>> I like the better backing. But what looks access to fsid_change >>> and the other bools? For sub-word members atomicy guarantees are >>> very limited, so they'd better all use the same lock. >> >> Do you have an example where the reordered structure would become >> problematic? The fs_devices locking is non-standard, the structures are >> accessed from module context or from filesystem context. There's the >> uuid_mutex as a big lock for fs_devices, and for access of the >> individual devices is device_list_mutex. > > No, I'm mostly asking for Anand to document the rationale why this > is fine. If there is an issue, it's probably pre-existing. Expanding on David's comment, these bool variables are not modified after the fs is mounted. However, here is a known bug - %discardable and %rotating aren't updated after device replace/add/remove.
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index 5cbbee32748c..a9a86c9220b3 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -281,7 +281,6 @@ enum btrfs_read_policy { struct btrfs_fs_devices { u8 fsid[BTRFS_FSID_SIZE]; /* FS specific uuid */ u8 metadata_uuid[BTRFS_FSID_SIZE]; - bool fsid_change; struct list_head fs_list; /* @@ -337,17 +336,24 @@ struct btrfs_fs_devices { struct list_head alloc_list; struct list_head seed_list; - bool seeding; + /* count fs-devices opened */ int opened; - /* set when we find or add a device that doesn't have the + /* + * set when we find or add a device that doesn't have the * nonrot flag set */ bool rotating; + /* Devices support TRIM/discard commands */ bool discardable; + bool fsid_change; + + /* fsid is a seed filesystem */ + bool seeding; + struct btrfs_fs_info *fs_info; /* sysfs kobjects */ struct kobject fsid_kobj;
By relocating the bool fsid_change near other bool declarations in the struct btrfs_fs_devices, approximately 6 bytes is saved. before: 512 bytes after: 496 bytes Furthermore, adding comments. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/volumes.h | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)