| Message ID | 7aef21820a6bad0e41699f18660038546adbbc9c.1734368270.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: some header cleanups and move things around | expand |
在 2024/12/17 03:47, fdmanana@kernel.org 写道: > From: Filipe Manana <fdmanana@suse.com> > > It's a generic helper not specific to ioctls and used in several places, > so move it out from ioctl.c and into fs.c. While at it change its return > type from int to bool and declare the loop variable in the loop itself. > > This also slightly reduces the module's size. > > Before this change: > > $ size fs/btrfs/btrfs.ko > text data bss dec hex filename > 1781492 161037 16920 1959449 1de619 fs/btrfs/btrfs.ko > > After this change: > > $ size fs/btrfs/btrfs.ko > text data bss dec hex filename > 1781340 161037 16920 1959297 1de581 fs/btrfs/btrfs.ko > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > fs/btrfs/fs.c | 9 +++++++++ > fs/btrfs/fs.h | 2 ++ > fs/btrfs/ioctl.c | 11 ----------- > fs/btrfs/ioctl.h | 1 - > 4 files changed, 11 insertions(+), 12 deletions(-) > > diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c > index 09cfb43580cb..06a863252a85 100644 > --- a/fs/btrfs/fs.c > +++ b/fs/btrfs/fs.c > @@ -55,6 +55,15 @@ size_t __attribute_const__ btrfs_get_num_csums(void) > return ARRAY_SIZE(btrfs_csums); > } > > +bool __pure btrfs_is_empty_uuid(const u8 *uuid) > +{ > + for (int i = 0; i < BTRFS_UUID_SIZE; i++) { > + if (uuid[i] != 0) > + return false; > + } Since we're here, would it be possible to go with mem_is_zero()/memchr_inv() which contains some extra optimization. And if we go calling mem_is_zero()/memchr_inv(), can we change this to an inline? Otherwise looks good to me. Thanks, Qu > + return true; > +} > + > /* > * Start exclusive operation @type, return true on success. > */ > diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h > index b05f2af97140..15c26c6f4d6e 100644 > --- a/fs/btrfs/fs.h > +++ b/fs/btrfs/fs.h > @@ -988,6 +988,8 @@ const char *btrfs_super_csum_name(u16 csum_type); > const char *btrfs_super_csum_driver(u16 csum_type); > size_t __attribute_const__ btrfs_get_num_csums(void); > > +bool __pure btrfs_is_empty_uuid(const u8 *uuid); > + > /* Compatibility and incompatibility defines */ > void __btrfs_set_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag, > const char *name); > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 0ede6a5524c2..7872de140489 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -471,17 +471,6 @@ static noinline int btrfs_ioctl_fitrim(struct btrfs_fs_info *fs_info, > return ret; > } > > -int __pure btrfs_is_empty_uuid(const u8 *uuid) > -{ > - int i; > - > - for (i = 0; i < BTRFS_UUID_SIZE; i++) { > - if (uuid[i]) > - return 0; > - } > - return 1; > -} > - > /* > * Calculate the number of transaction items to reserve for creating a subvolume > * or snapshot, not including the inode, directory entries, or parent directory. > diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h > index 2b760c8778f8..ce915fcda43b 100644 > --- a/fs/btrfs/ioctl.h > +++ b/fs/btrfs/ioctl.h > @@ -19,7 +19,6 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap, > struct dentry *dentry, struct fileattr *fa); > int btrfs_ioctl_get_supported_features(void __user *arg); > void btrfs_sync_inode_flags_to_i_flags(struct inode *inode); > -int __pure btrfs_is_empty_uuid(const u8 *uuid); > void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info, > struct btrfs_ioctl_balance_args *bargs); > int btrfs_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags);
On 17.12.24 01:32, Qu Wenruo wrote: > > > 在 2024/12/17 03:47, fdmanana@kernel.org 写道: >> From: Filipe Manana <fdmanana@suse.com> >> >> It's a generic helper not specific to ioctls and used in several places, >> so move it out from ioctl.c and into fs.c. While at it change its return >> type from int to bool and declare the loop variable in the loop itself. >> >> This also slightly reduces the module's size. >> >> Before this change: >> >> $ size fs/btrfs/btrfs.ko >> text data bss dec hex filename >> 1781492 161037 16920 1959449 1de619 fs/btrfs/btrfs.ko >> >> After this change: >> >> $ size fs/btrfs/btrfs.ko >> text data bss dec hex filename >> 1781340 161037 16920 1959297 1de581 fs/btrfs/btrfs.ko >> >> Signed-off-by: Filipe Manana <fdmanana@suse.com> >> --- >> fs/btrfs/fs.c | 9 +++++++++ >> fs/btrfs/fs.h | 2 ++ >> fs/btrfs/ioctl.c | 11 ----------- >> fs/btrfs/ioctl.h | 1 - >> 4 files changed, 11 insertions(+), 12 deletions(-) >> >> diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c >> index 09cfb43580cb..06a863252a85 100644 >> --- a/fs/btrfs/fs.c >> +++ b/fs/btrfs/fs.c >> @@ -55,6 +55,15 @@ size_t __attribute_const__ btrfs_get_num_csums(void) >> return ARRAY_SIZE(btrfs_csums); >> } >> >> +bool __pure btrfs_is_empty_uuid(const u8 *uuid) >> +{ >> + for (int i = 0; i < BTRFS_UUID_SIZE; i++) { >> + if (uuid[i] != 0) >> + return false; >> + } > > Since we're here, would it be possible to go with > mem_is_zero()/memchr_inv() which contains some extra optimization. > > And if we go calling mem_is_zero()/memchr_inv(), can we change this to > an inline? > > Otherwise looks good to me. The generic uuid code also has a uuid_is_null(): bool __pure btrfs_is_empty_uuid(const u8 *uuid) { return uuid_is_null((const uuid_t *)uuid)); } should work as well, but I've not tested it just eyeballed.
在 2024/12/17 18:23, Johannes Thumshirn 写道: > On 17.12.24 01:32, Qu Wenruo wrote: >> >> >> 在 2024/12/17 03:47, fdmanana@kernel.org 写道: >>> From: Filipe Manana <fdmanana@suse.com> >>> >>> It's a generic helper not specific to ioctls and used in several places, >>> so move it out from ioctl.c and into fs.c. While at it change its return >>> type from int to bool and declare the loop variable in the loop itself. >>> >>> This also slightly reduces the module's size. >>> >>> Before this change: >>> >>> $ size fs/btrfs/btrfs.ko >>> text data bss dec hex filename >>> 1781492 161037 16920 1959449 1de619 fs/btrfs/btrfs.ko >>> >>> After this change: >>> >>> $ size fs/btrfs/btrfs.ko >>> text data bss dec hex filename >>> 1781340 161037 16920 1959297 1de581 fs/btrfs/btrfs.ko >>> >>> Signed-off-by: Filipe Manana <fdmanana@suse.com> >>> --- >>> fs/btrfs/fs.c | 9 +++++++++ >>> fs/btrfs/fs.h | 2 ++ >>> fs/btrfs/ioctl.c | 11 ----------- >>> fs/btrfs/ioctl.h | 1 - >>> 4 files changed, 11 insertions(+), 12 deletions(-) >>> >>> diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c >>> index 09cfb43580cb..06a863252a85 100644 >>> --- a/fs/btrfs/fs.c >>> +++ b/fs/btrfs/fs.c >>> @@ -55,6 +55,15 @@ size_t __attribute_const__ btrfs_get_num_csums(void) >>> return ARRAY_SIZE(btrfs_csums); >>> } >>> >>> +bool __pure btrfs_is_empty_uuid(const u8 *uuid) >>> +{ >>> + for (int i = 0; i < BTRFS_UUID_SIZE; i++) { >>> + if (uuid[i] != 0) >>> + return false; >>> + } >> >> Since we're here, would it be possible to go with >> mem_is_zero()/memchr_inv() which contains some extra optimization. >> >> And if we go calling mem_is_zero()/memchr_inv(), can we change this to >> an inline? >> >> Otherwise looks good to me. > > The generic uuid code also has a uuid_is_null(): > > bool __pure btrfs_is_empty_uuid(const u8 *uuid) > { > return uuid_is_null((const uuid_t *)uuid)); > } > > should work as well, but I've not tested it just eyeballed. Wow, uuid_is_null() is better, because it goes easier to understand memcmp(), which also has extra comparison optimization. Thanks, Qu
On Tue, Dec 17, 2024 at 12:31 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > 在 2024/12/17 03:47, fdmanana@kernel.org 写道: > > From: Filipe Manana <fdmanana@suse.com> > > > > It's a generic helper not specific to ioctls and used in several places, > > so move it out from ioctl.c and into fs.c. While at it change its return > > type from int to bool and declare the loop variable in the loop itself. > > > > This also slightly reduces the module's size. > > > > Before this change: > > > > $ size fs/btrfs/btrfs.ko > > text data bss dec hex filename > > 1781492 161037 16920 1959449 1de619 fs/btrfs/btrfs.ko > > > > After this change: > > > > $ size fs/btrfs/btrfs.ko > > text data bss dec hex filename > > 1781340 161037 16920 1959297 1de581 fs/btrfs/btrfs.ko > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > --- > > fs/btrfs/fs.c | 9 +++++++++ > > fs/btrfs/fs.h | 2 ++ > > fs/btrfs/ioctl.c | 11 ----------- > > fs/btrfs/ioctl.h | 1 - > > 4 files changed, 11 insertions(+), 12 deletions(-) > > > > diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c > > index 09cfb43580cb..06a863252a85 100644 > > --- a/fs/btrfs/fs.c > > +++ b/fs/btrfs/fs.c > > @@ -55,6 +55,15 @@ size_t __attribute_const__ btrfs_get_num_csums(void) > > return ARRAY_SIZE(btrfs_csums); > > } > > > > +bool __pure btrfs_is_empty_uuid(const u8 *uuid) > > +{ > > + for (int i = 0; i < BTRFS_UUID_SIZE; i++) { > > + if (uuid[i] != 0) > > + return false; > > + } > > Since we're here, would it be possible to go with > mem_is_zero()/memchr_inv() which contains some extra optimization. > > And if we go calling mem_is_zero()/memchr_inv(), can we change this to > an inline? I'll send that as a separate change, using uuid_is_null() as Johannes suggested, on top of this. The goal here was just to move code around and not change the implementation while doing it. Thanks. > > Otherwise looks good to me. > > Thanks, > Qu > > + return true; > > +} > > + > > /* > > * Start exclusive operation @type, return true on success. > > */ > > diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h > > index b05f2af97140..15c26c6f4d6e 100644 > > --- a/fs/btrfs/fs.h > > +++ b/fs/btrfs/fs.h > > @@ -988,6 +988,8 @@ const char *btrfs_super_csum_name(u16 csum_type); > > const char *btrfs_super_csum_driver(u16 csum_type); > > size_t __attribute_const__ btrfs_get_num_csums(void); > > > > +bool __pure btrfs_is_empty_uuid(const u8 *uuid); > > + > > /* Compatibility and incompatibility defines */ > > void __btrfs_set_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag, > > const char *name); > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > > index 0ede6a5524c2..7872de140489 100644 > > --- a/fs/btrfs/ioctl.c > > +++ b/fs/btrfs/ioctl.c > > @@ -471,17 +471,6 @@ static noinline int btrfs_ioctl_fitrim(struct btrfs_fs_info *fs_info, > > return ret; > > } > > > > -int __pure btrfs_is_empty_uuid(const u8 *uuid) > > -{ > > - int i; > > - > > - for (i = 0; i < BTRFS_UUID_SIZE; i++) { > > - if (uuid[i]) > > - return 0; > > - } > > - return 1; > > -} > > - > > /* > > * Calculate the number of transaction items to reserve for creating a subvolume > > * or snapshot, not including the inode, directory entries, or parent directory. > > diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h > > index 2b760c8778f8..ce915fcda43b 100644 > > --- a/fs/btrfs/ioctl.h > > +++ b/fs/btrfs/ioctl.h > > @@ -19,7 +19,6 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap, > > struct dentry *dentry, struct fileattr *fa); > > int btrfs_ioctl_get_supported_features(void __user *arg); > > void btrfs_sync_inode_flags_to_i_flags(struct inode *inode); > > -int __pure btrfs_is_empty_uuid(const u8 *uuid); > > void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info, > > struct btrfs_ioctl_balance_args *bargs); > > int btrfs_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags); >
在 2024/12/17 19:27, Filipe Manana 写道: > On Tue, Dec 17, 2024 at 12:31 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: >> >> >> >> 在 2024/12/17 03:47, fdmanana@kernel.org 写道: >>> From: Filipe Manana <fdmanana@suse.com> >>> >>> It's a generic helper not specific to ioctls and used in several places, >>> so move it out from ioctl.c and into fs.c. While at it change its return >>> type from int to bool and declare the loop variable in the loop itself. >>> >>> This also slightly reduces the module's size. >>> >>> Before this change: >>> >>> $ size fs/btrfs/btrfs.ko >>> text data bss dec hex filename >>> 1781492 161037 16920 1959449 1de619 fs/btrfs/btrfs.ko >>> >>> After this change: >>> >>> $ size fs/btrfs/btrfs.ko >>> text data bss dec hex filename >>> 1781340 161037 16920 1959297 1de581 fs/btrfs/btrfs.ko >>> >>> Signed-off-by: Filipe Manana <fdmanana@suse.com> >>> --- >>> fs/btrfs/fs.c | 9 +++++++++ >>> fs/btrfs/fs.h | 2 ++ >>> fs/btrfs/ioctl.c | 11 ----------- >>> fs/btrfs/ioctl.h | 1 - >>> 4 files changed, 11 insertions(+), 12 deletions(-) >>> >>> diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c >>> index 09cfb43580cb..06a863252a85 100644 >>> --- a/fs/btrfs/fs.c >>> +++ b/fs/btrfs/fs.c >>> @@ -55,6 +55,15 @@ size_t __attribute_const__ btrfs_get_num_csums(void) >>> return ARRAY_SIZE(btrfs_csums); >>> } >>> >>> +bool __pure btrfs_is_empty_uuid(const u8 *uuid) >>> +{ >>> + for (int i = 0; i < BTRFS_UUID_SIZE; i++) { >>> + if (uuid[i] != 0) >>> + return false; >>> + } >> >> Since we're here, would it be possible to go with >> mem_is_zero()/memchr_inv() which contains some extra optimization. >> >> And if we go calling mem_is_zero()/memchr_inv(), can we change this to >> an inline? > > I'll send that as a separate change, using uuid_is_null() as Johannes > suggested, on top of this. > The goal here was just to move code around and not change the > implementation while doing it. Got it, feel free to add the reviewed-by tags to the whole series. Thanks, Qu > > Thanks. > >> >> Otherwise looks good to me. >> >> Thanks, >> Qu >>> + return true; >>> +} >>> + >>> /* >>> * Start exclusive operation @type, return true on success. >>> */ >>> diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h >>> index b05f2af97140..15c26c6f4d6e 100644 >>> --- a/fs/btrfs/fs.h >>> +++ b/fs/btrfs/fs.h >>> @@ -988,6 +988,8 @@ const char *btrfs_super_csum_name(u16 csum_type); >>> const char *btrfs_super_csum_driver(u16 csum_type); >>> size_t __attribute_const__ btrfs_get_num_csums(void); >>> >>> +bool __pure btrfs_is_empty_uuid(const u8 *uuid); >>> + >>> /* Compatibility and incompatibility defines */ >>> void __btrfs_set_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag, >>> const char *name); >>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >>> index 0ede6a5524c2..7872de140489 100644 >>> --- a/fs/btrfs/ioctl.c >>> +++ b/fs/btrfs/ioctl.c >>> @@ -471,17 +471,6 @@ static noinline int btrfs_ioctl_fitrim(struct btrfs_fs_info *fs_info, >>> return ret; >>> } >>> >>> -int __pure btrfs_is_empty_uuid(const u8 *uuid) >>> -{ >>> - int i; >>> - >>> - for (i = 0; i < BTRFS_UUID_SIZE; i++) { >>> - if (uuid[i]) >>> - return 0; >>> - } >>> - return 1; >>> -} >>> - >>> /* >>> * Calculate the number of transaction items to reserve for creating a subvolume >>> * or snapshot, not including the inode, directory entries, or parent directory. >>> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h >>> index 2b760c8778f8..ce915fcda43b 100644 >>> --- a/fs/btrfs/ioctl.h >>> +++ b/fs/btrfs/ioctl.h >>> @@ -19,7 +19,6 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap, >>> struct dentry *dentry, struct fileattr *fa); >>> int btrfs_ioctl_get_supported_features(void __user *arg); >>> void btrfs_sync_inode_flags_to_i_flags(struct inode *inode); >>> -int __pure btrfs_is_empty_uuid(const u8 *uuid); >>> void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info, >>> struct btrfs_ioctl_balance_args *bargs); >>> int btrfs_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags); >> >
diff --git a/fs/btrfs/fs.c b/fs/btrfs/fs.c index 09cfb43580cb..06a863252a85 100644 --- a/fs/btrfs/fs.c +++ b/fs/btrfs/fs.c @@ -55,6 +55,15 @@ size_t __attribute_const__ btrfs_get_num_csums(void) return ARRAY_SIZE(btrfs_csums); } +bool __pure btrfs_is_empty_uuid(const u8 *uuid) +{ + for (int i = 0; i < BTRFS_UUID_SIZE; i++) { + if (uuid[i] != 0) + return false; + } + return true; +} + /* * Start exclusive operation @type, return true on success. */ diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h index b05f2af97140..15c26c6f4d6e 100644 --- a/fs/btrfs/fs.h +++ b/fs/btrfs/fs.h @@ -988,6 +988,8 @@ const char *btrfs_super_csum_name(u16 csum_type); const char *btrfs_super_csum_driver(u16 csum_type); size_t __attribute_const__ btrfs_get_num_csums(void); +bool __pure btrfs_is_empty_uuid(const u8 *uuid); + /* Compatibility and incompatibility defines */ void __btrfs_set_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag, const char *name); diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 0ede6a5524c2..7872de140489 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -471,17 +471,6 @@ static noinline int btrfs_ioctl_fitrim(struct btrfs_fs_info *fs_info, return ret; } -int __pure btrfs_is_empty_uuid(const u8 *uuid) -{ - int i; - - for (i = 0; i < BTRFS_UUID_SIZE; i++) { - if (uuid[i]) - return 0; - } - return 1; -} - /* * Calculate the number of transaction items to reserve for creating a subvolume * or snapshot, not including the inode, directory entries, or parent directory. diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h index 2b760c8778f8..ce915fcda43b 100644 --- a/fs/btrfs/ioctl.h +++ b/fs/btrfs/ioctl.h @@ -19,7 +19,6 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry, struct fileattr *fa); int btrfs_ioctl_get_supported_features(void __user *arg); void btrfs_sync_inode_flags_to_i_flags(struct inode *inode); -int __pure btrfs_is_empty_uuid(const u8 *uuid); void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info, struct btrfs_ioctl_balance_args *bargs); int btrfs_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags);