| Message ID | 20230329001308.1275299-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] btrfs: fix fast csum detection | expand |
On Wed, Mar 29, 2023 at 09:13:05AM +0900, Christoph Hellwig wrote: > The BTRFS_FS_CSUM_IMPL_FAST flag is current set whenever a non-generic > crc32c is detected, which is the incorrect check if the file system uses > a different checksumming algorithm. Refactor the code to only checks > this if crc32 is actually used. Note that in an ideal world the > information if an algorithm is hardware accelerated or not should be > provided by the crypto API instead, but that's left for another day. https://lore.kernel.org/linux-crypto/20190514213409.GA115510@gmail.com/ I got pointed to the driver name, the priority that would say if the implementation is accelerated is not exported to the API. This would be cleaner but for a simple 'is/is-not' I think it's sufficient. > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/btrfs/disk-io.c | 18 +++++++++++++++++- > fs/btrfs/super.c | 2 -- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 3f57c41f41bf5f..ec765d6bc53673 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -154,6 +154,21 @@ static bool btrfs_supported_super_csum(u16 csum_type) > } > } > > +/* > + * Check if the CSUM implementation is a fast accelerated one. > + * As-is this is a bit of a hack and should be replaced once the > + * csum implementations provide that information themselves. > + */ > +static bool btrfs_csum_is_fast(u16 csum_type) > +{ > + switch (csum_type) { > + case BTRFS_CSUM_TYPE_CRC32: > + return !strstr(crc32c_impl(), "generic"); This would check the internal shash (lib/libcrc32c.c) not the one we allocate for btrfs in btrfs_init_csum_hash. Though they both should be equivalent as libcrc32c does some tricks to lookup the fastest implementation but theoretically may not find the fast one, while mount could. > + default: > + return false; > + } > +} > + > /* > * Return 0 if the superblock checksum type matches the checksum value of that > * algorithm. Pass the raw disk superblock data. > @@ -3373,7 +3388,8 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device > btrfs_release_disk_super(disk_super); > goto fail_alloc; > } > - > + if (btrfs_csum_is_fast(csum_type)) > + set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags); This ^^^ > fs_info->csum_size = btrfs_super_csum_size(disk_super); > > ret = btrfs_init_csum_hash(fs_info, csum_type); should be moved after the initialization btrfs_init_csum_hash so it would also detect accelerated implementation of other hashes. > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index d8885966e801cd..e94a4cd06607e1 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -1517,8 +1517,6 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type, > shrinker_debugfs_rename(&s->s_shrink, "sb-%s:%s", fs_type->name, > s->s_id); > btrfs_sb(s)->bdev_holder = fs_type; > - if (!strstr(crc32c_impl(), "generic")) > - set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags); > error = btrfs_fill_super(s, fs_devices, data); > } > if (!error) > -- > 2.39.2
On Mon, Apr 03, 2023 at 08:35:26PM +0200, David Sterba wrote: > > a different checksumming algorithm. Refactor the code to only checks > > this if crc32 is actually used. Note that in an ideal world the > > information if an algorithm is hardware accelerated or not should be > > provided by the crypto API instead, but that's left for another day. > > https://lore.kernel.org/linux-crypto/20190514213409.GA115510@gmail.com/ > I got pointed to the driver name, the priority that would say if the > implementation is accelerated is not exported to the API. This would be > cleaner but for a simple 'is/is-not' I think it's sufficient. Except that it diesn't really scale to multiple algorithms very well. I guess the priority might be the logically best thing to do, so I'll try to find some time to look into it. > > +/* > > + * Check if the CSUM implementation is a fast accelerated one. > > + * As-is this is a bit of a hack and should be replaced once the > > + * csum implementations provide that information themselves. > > + */ > > +static bool btrfs_csum_is_fast(u16 csum_type) > > +{ > > + switch (csum_type) { > > + case BTRFS_CSUM_TYPE_CRC32: > > + return !strstr(crc32c_impl(), "generic"); > > This would check the internal shash (lib/libcrc32c.c) not the one we > allocate for btrfs in btrfs_init_csum_hash. Though they both should be > equivalent as libcrc32c does some tricks to lookup the fastest > implementation but theoretically may not find the fast one, while mount > could. Yeah. > > + if (btrfs_csum_is_fast(csum_type)) > > + set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags); > > This ^^^ > > > fs_info->csum_size = btrfs_super_csum_size(disk_super); > > > > ret = btrfs_init_csum_hash(fs_info, csum_type); > > should be moved after the initialization btrfs_init_csum_hash so it > would also detect accelerated implementation of other hashes. Sure. Something like this incremental fix. Do you want to fold it in or should I resend the series? --- diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 7740bb1b152445..eeefa5105c91d5 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -154,21 +154,6 @@ static bool btrfs_supported_super_csum(u16 csum_type) } } -/* - * Check if the CSUM implementation is a fast accelerated one. - * As-is this is a bit of a hack and should be replaced once the - * csum implementations provide that information themselves. - */ -static bool btrfs_csum_is_fast(u16 csum_type) -{ - switch (csum_type) { - case BTRFS_CSUM_TYPE_CRC32: - return !strstr(crc32c_impl(), "generic"); - default: - return false; - } -} - /* * Return 0 if the superblock checksum type matches the checksum value of that * algorithm. Pass the raw disk superblock data. @@ -2260,6 +2245,20 @@ static int btrfs_init_csum_hash(struct btrfs_fs_info *fs_info, u16 csum_type) fs_info->csum_shash = csum_shash; + /* + * Check if the CSUM implementation is a fast accelerated one. + * As-is this is a bit of a hack and should be replaced once the csum + * implementations provide that information themselves. + */ + switch (csum_type) { + case BTRFS_CSUM_TYPE_CRC32: + if (!strstr(crypto_shash_driver_name(csum_shash), "generic")) + set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags); + break; + default: + break; + } + btrfs_info(fs_info, "using %s (%s) checksum algorithm", btrfs_super_csum_name(csum_type), crypto_shash_driver_name(csum_shash)); @@ -3384,8 +3383,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device btrfs_release_disk_super(disk_super); goto fail_alloc; } - if (btrfs_csum_is_fast(csum_type)) - set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags); fs_info->csum_size = btrfs_super_csum_size(disk_super); ret = btrfs_init_csum_hash(fs_info, csum_type);
On Mon, Apr 03, 2023 at 10:06:16PM -0700, Christoph Hellwig wrote: > > > + if (btrfs_csum_is_fast(csum_type)) > > > + set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags); > > > > This ^^^ > > > > > fs_info->csum_size = btrfs_super_csum_size(disk_super); > > > > > > ret = btrfs_init_csum_hash(fs_info, csum_type); > > > > should be moved after the initialization btrfs_init_csum_hash so it > > would also detect accelerated implementation of other hashes. > > Sure. Something like this incremental fix. Do you want to fold it in > or should I resend the series? I ended up with the same diff when reviewing the patch so I can fold it, no need to resend. I'll send a separate patch to add xxhash as a fast implementation, with some numbers.
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 3f57c41f41bf5f..ec765d6bc53673 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -154,6 +154,21 @@ static bool btrfs_supported_super_csum(u16 csum_type) } } +/* + * Check if the CSUM implementation is a fast accelerated one. + * As-is this is a bit of a hack and should be replaced once the + * csum implementations provide that information themselves. + */ +static bool btrfs_csum_is_fast(u16 csum_type) +{ + switch (csum_type) { + case BTRFS_CSUM_TYPE_CRC32: + return !strstr(crc32c_impl(), "generic"); + default: + return false; + } +} + /* * Return 0 if the superblock checksum type matches the checksum value of that * algorithm. Pass the raw disk superblock data. @@ -3373,7 +3388,8 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device btrfs_release_disk_super(disk_super); goto fail_alloc; } - + if (btrfs_csum_is_fast(csum_type)) + set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags); fs_info->csum_size = btrfs_super_csum_size(disk_super); ret = btrfs_init_csum_hash(fs_info, csum_type); diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index d8885966e801cd..e94a4cd06607e1 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -1517,8 +1517,6 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type, shrinker_debugfs_rename(&s->s_shrink, "sb-%s:%s", fs_type->name, s->s_id); btrfs_sb(s)->bdev_holder = fs_type; - if (!strstr(crc32c_impl(), "generic")) - set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags); error = btrfs_fill_super(s, fs_devices, data); } if (!error)
The BTRFS_FS_CSUM_IMPL_FAST flag is current set whenever a non-generic crc32c is detected, which is the incorrect check if the file system uses a different checksumming algorithm. Refactor the code to only checks this if crc32 is actually used. Note that in an ideal world the information if an algorithm is hardware accelerated or not should be provided by the crypto API instead, but that's left for another day. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/disk-io.c | 18 +++++++++++++++++- fs/btrfs/super.c | 2 -- 2 files changed, 17 insertions(+), 3 deletions(-)