| Message ID | fec5e2be6fd4c778966edc3c576f7df38cd0ad3d.1684826247.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Tue, May 23, 2023 at 06:03:21PM +0800, Anand Jain wrote: > Refactor the functions find_fsid() and find_fsid_with_metadata_uuid(), as > they currently share a common set of code to compare the fsid and > metadata_uuid. Create a common helper function, > memcmp_fsid_fs_devices(). > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/volumes.c | 40 +++++++++++++++++++++++++--------------- > 1 file changed, 25 insertions(+), 15 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 95b87e9a0a73..8738c8027421 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -427,6 +427,22 @@ void __exit btrfs_cleanup_fs_uuids(void) > } > } > > +static bool memcmp_fsid_fs_devices(struct btrfs_fs_devices *fs_devices, > + const u8 *fsid, const u8 *metadata_fsid) It's confusing when function is named memcmp_* but does not have the same return value semantics as memcmp(). We could use "memeq_" prefix but I don't see it used in kernel so there's not a pattern to follow but for readability, either the helpers should be exactly memcmp() semantics or rename to something better reflecting that it's a equal or not.
On 24/5/23 05:23, David Sterba wrote: > On Tue, May 23, 2023 at 06:03:21PM +0800, Anand Jain wrote: >> Refactor the functions find_fsid() and find_fsid_with_metadata_uuid(), as >> they currently share a common set of code to compare the fsid and >> metadata_uuid. Create a common helper function, >> memcmp_fsid_fs_devices(). >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> fs/btrfs/volumes.c | 40 +++++++++++++++++++++++++--------------- >> 1 file changed, 25 insertions(+), 15 deletions(-) >> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index 95b87e9a0a73..8738c8027421 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -427,6 +427,22 @@ void __exit btrfs_cleanup_fs_uuids(void) >> } >> } >> >> +static bool memcmp_fsid_fs_devices(struct btrfs_fs_devices *fs_devices, >> + const u8 *fsid, const u8 *metadata_fsid) > > It's confusing when function is named memcmp_* but does not have the > same return value semantics as memcmp(). > > We could use "memeq_" prefix but I don't see it used in kernel so > there's not a pattern to follow but for readability, either the helpers > should be exactly memcmp() semantics or rename to something better > reflecting that it's a equal or not. Yeah, calling it "memcmp_" with different return semantic will be confusing. I'll rename. Howabout, "match_"? Thanks, Anand
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 95b87e9a0a73..8738c8027421 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -427,6 +427,22 @@ void __exit btrfs_cleanup_fs_uuids(void) } } +static bool memcmp_fsid_fs_devices(struct btrfs_fs_devices *fs_devices, + const u8 *fsid, const u8 *metadata_fsid) +{ + if (memcmp(fsid, fs_devices->fsid, BTRFS_FSID_SIZE) != 0) + return false; + + if (!metadata_fsid) + return true; + + if (memcmp(metadata_fsid, fs_devices->metadata_uuid, BTRFS_FSID_SIZE) != + 0) + return false; + + return true; +} + static noinline struct btrfs_fs_devices *find_fsid( const u8 *fsid, const u8 *metadata_fsid) { @@ -436,15 +452,8 @@ static noinline struct btrfs_fs_devices *find_fsid( /* Handle non-split brain cases */ list_for_each_entry(fs_devices, &fs_uuids, fs_list) { - if (metadata_fsid) { - if (memcmp(fsid, fs_devices->fsid, BTRFS_FSID_SIZE) == 0 - && memcmp(metadata_fsid, fs_devices->metadata_uuid, - BTRFS_FSID_SIZE) == 0) - return fs_devices; - } else { - if (memcmp(fsid, fs_devices->fsid, BTRFS_FSID_SIZE) == 0) - return fs_devices; - } + if (memcmp_fsid_fs_devices(fs_devices, fsid, metadata_fsid)) + return fs_devices; } return NULL; } @@ -462,14 +471,15 @@ static struct btrfs_fs_devices *find_fsid_with_metadata_uuid( * at all and the CHANGING_FSID_V2 flag set. */ list_for_each_entry(fs_devices, &fs_uuids, fs_list) { - if (fs_devices->fsid_change && - memcmp(disk_super->metadata_uuid, fs_devices->fsid, - BTRFS_FSID_SIZE) == 0 && - memcmp(fs_devices->fsid, fs_devices->metadata_uuid, - BTRFS_FSID_SIZE) == 0) { + if (!fs_devices->fsid_change) + continue; + + if (memcmp_fsid_fs_devices(fs_devices, + disk_super->metadata_uuid, + fs_devices->fsid)) return fs_devices; - } } + /* * Handle scanned device having completed its fsid change but * belonging to a fs_devices that was created by a device that
Refactor the functions find_fsid() and find_fsid_with_metadata_uuid(), as they currently share a common set of code to compare the fsid and metadata_uuid. Create a common helper function, memcmp_fsid_fs_devices(). Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/volumes.c | 40 +++++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 15 deletions(-)