| Message ID | 20200624102136.12495-1-johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: pass checksum type via BTRFS_IOC_FS_INFO ioctl | expand |
On 2020/6/24 下午6:21, Johannes Thumshirn wrote: > With the recent addition of filesystem checksum types other than CRC32c, > it is not anymore hard-coded which checksum type a btrfs filesystem uses. > > Up to now there is no good way to read the filesystem checksum, apart from > reading the filesystem UUID and then query sysfs for the checksum type. > > Add a new csum_type field to the BTRFS_IOC_FS_INFO ioctl command which > usually is used to query filesystem features. Also add a flags member > indicating that the kernel responded with a set csum_type field. > > Fixes: 3951e7f050ac ("btrfs: add xxhash64 to checksumming algorithms") > Fixes: 3831bf0094ab ("btrfs: add sha256 to checksumming algorithm") I don't think the fixes tags are needed. You're just adding a new interface for IOC_FS_INFO to grab the algorithm. Overall looks good. > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > --- > fs/btrfs/ioctl.c | 3 +++ > include/uapi/linux/btrfs.h | 13 ++++++++++++- > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index b3e4c632d80c..16062720f5f3 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -3217,6 +3217,9 @@ static long btrfs_ioctl_fs_info(struct btrfs_fs_info *fs_info, > fi_args->nodesize = fs_info->nodesize; > fi_args->sectorsize = fs_info->sectorsize; > fi_args->clone_alignment = fs_info->sectorsize; > + fi_args->csum_type = > + le16_to_cpu(btrfs_super_csum_type(fs_info->super_copy)); > + fi_args->flags |= BTRFS_FS_INFO_FLAG_CSUM_TYPE; > > if (copy_to_user(arg, fi_args, sizeof(*fi_args))) > ret = -EFAULT; > diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h > index e6b6cb0f8bc6..161d9100c2a6 100644 > --- a/include/uapi/linux/btrfs.h > +++ b/include/uapi/linux/btrfs.h > @@ -250,10 +250,21 @@ struct btrfs_ioctl_fs_info_args { > __u32 nodesize; /* out */ > __u32 sectorsize; /* out */ > __u32 clone_alignment; /* out */ > + __u32 flags; /* out */ The flags looks a little too generic. What about extra_members or things like that? This flag really indicates what extra info the ioctl args can provide, so a better name would be easier to understand. Thanks, Qu > + __u16 csum_type; > + __u16 reserved16; > __u32 reserved32; > - __u64 reserved[122]; /* pad to 1k */ > + __u64 reserved[121]; /* pad to 1k */ > }; > > +/* > + * fs_info ioctl flags > + * > + * Used by: > + * struct btrfs_ioctl_fs_info_args > + */ > +#define BTRFS_FS_INFO_FLAG_CSUM_TYPE (1 << 0) > + > /* > * feature flags > * >
On 24/06/2020 13:03, Qu Wenruo wrote: >> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h >> index e6b6cb0f8bc6..161d9100c2a6 100644 >> --- a/include/uapi/linux/btrfs.h >> +++ b/include/uapi/linux/btrfs.h >> @@ -250,10 +250,21 @@ struct btrfs_ioctl_fs_info_args { >> __u32 nodesize; /* out */ >> __u32 sectorsize; /* out */ >> __u32 clone_alignment; /* out */ >> + __u32 flags; /* out */ > The flags looks a little too generic. > What about extra_members or things like that? > > This flag really indicates what extra info the ioctl args can provide, > so a better name would be easier to understand. Maybe version and for each new member it gets incremented?
On 2020/6/24 下午7:41, Johannes Thumshirn wrote: > On 24/06/2020 13:03, Qu Wenruo wrote: >>> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h >>> index e6b6cb0f8bc6..161d9100c2a6 100644 >>> --- a/include/uapi/linux/btrfs.h >>> +++ b/include/uapi/linux/btrfs.h >>> @@ -250,10 +250,21 @@ struct btrfs_ioctl_fs_info_args { >>> __u32 nodesize; /* out */ >>> __u32 sectorsize; /* out */ >>> __u32 clone_alignment; /* out */ >>> + __u32 flags; /* out */ >> The flags looks a little too generic. >> What about extra_members or things like that? >> >> This flag really indicates what extra info the ioctl args can provide, >> so a better name would be easier to understand. > > Maybe version and for each new member it gets incremented? > That looks even better! Thanks, Qu
On 24.06.20 г. 14:03 ч., Qu Wenruo wrote: > > > On 2020/6/24 下午6:21, Johannes Thumshirn wrote: >> With the recent addition of filesystem checksum types other than CRC32c, >> it is not anymore hard-coded which checksum type a btrfs filesystem uses. >> >> Up to now there is no good way to read the filesystem checksum, apart from >> reading the filesystem UUID and then query sysfs for the checksum type. >> >> Add a new csum_type field to the BTRFS_IOC_FS_INFO ioctl command which >> usually is used to query filesystem features. Also add a flags member >> indicating that the kernel responded with a set csum_type field. >> >> Fixes: 3951e7f050ac ("btrfs: add xxhash64 to checksumming algorithms") >> Fixes: 3831bf0094ab ("btrfs: add sha256 to checksumming algorithm") > > I don't think the fixes tags are needed. > You're just adding a new interface for IOC_FS_INFO to grab the algorithm. True, but ideally this should have gone with the initial submission, so it would make sense to have this interface for every kernel that supports multiple checksums. With this in mind I think the fixes tags are indeed very useful. > > Overall looks good.> >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >> --- >> fs/btrfs/ioctl.c | 3 +++ >> include/uapi/linux/btrfs.h | 13 ++++++++++++- >> 2 files changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >> index b3e4c632d80c..16062720f5f3 100644 >> --- a/fs/btrfs/ioctl.c >> +++ b/fs/btrfs/ioctl.c >> @@ -3217,6 +3217,9 @@ static long btrfs_ioctl_fs_info(struct btrfs_fs_info *fs_info, >> fi_args->nodesize = fs_info->nodesize; >> fi_args->sectorsize = fs_info->sectorsize; >> fi_args->clone_alignment = fs_info->sectorsize; >> + fi_args->csum_type = >> + le16_to_cpu(btrfs_super_csum_type(fs_info->super_copy)); >> + fi_args->flags |= BTRFS_FS_INFO_FLAG_CSUM_TYPE; >> >> if (copy_to_user(arg, fi_args, sizeof(*fi_args))) >> ret = -EFAULT; >> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h >> index e6b6cb0f8bc6..161d9100c2a6 100644 >> --- a/include/uapi/linux/btrfs.h >> +++ b/include/uapi/linux/btrfs.h >> @@ -250,10 +250,21 @@ struct btrfs_ioctl_fs_info_args { >> __u32 nodesize; /* out */ >> __u32 sectorsize; /* out */ >> __u32 clone_alignment; /* out */ >> + __u32 flags; /* out */ > > The flags looks a little too generic. > What about extra_members or things like that? > > This flag really indicates what extra info the ioctl args can provide, > so a better name would be easier to understand. > > Thanks, > Qu > >> + __u16 csum_type; >> + __u16 reserved16; >> __u32 reserved32; >> - __u64 reserved[122]; /* pad to 1k */ >> + __u64 reserved[121]; /* pad to 1k */ >> }; >> >> +/* >> + * fs_info ioctl flags >> + * >> + * Used by: >> + * struct btrfs_ioctl_fs_info_args >> + */ >> +#define BTRFS_FS_INFO_FLAG_CSUM_TYPE (1 << 0) >> + >> /* >> * feature flags >> * >> >
On 6/24/20 1:55 PM, Qu Wenruo wrote: > > > On 2020/6/24 下午7:41, Johannes Thumshirn wrote: >> On 24/06/2020 13:03, Qu Wenruo wrote: >>>> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h >>>> index e6b6cb0f8bc6..161d9100c2a6 100644 >>>> --- a/include/uapi/linux/btrfs.h >>>> +++ b/include/uapi/linux/btrfs.h >>>> @@ -250,10 +250,21 @@ struct btrfs_ioctl_fs_info_args { >>>> __u32 nodesize; /* out */ >>>> __u32 sectorsize; /* out */ >>>> __u32 clone_alignment; /* out */ >>>> + __u32 flags; /* out */ >>> The flags looks a little too generic. >>> What about extra_members or things like that? >>> >>> This flag really indicates what extra info the ioctl args can provide, >>> so a better name would be easier to understand. >> >> Maybe version and for each new member it gets incremented? >> > > That looks even better! Random idea... What if an imaginary me likes building kernels with random features from the future (e.g. on top of 4.19LTS), and the next feature added is to show the value of the new flapsie field. The 'version' will increase, but I decide to not pick the new csum type patches because I don't need them, I only pick the new flapsie feature. Now, how does my userspace tooling know that the u16 flapsie field has a meaning but the csum_type hasn't? ("You shouldn't do that" is also a possible answer...) Hans
Hi, On 6/24/20 12:21 PM, Johannes Thumshirn wrote: > With the recent addition of filesystem checksum types other than CRC32c, > it is not anymore hard-coded which checksum type a btrfs filesystem uses. > > Up to now there is no good way to read the filesystem checksum, apart from > reading the filesystem UUID and then query sysfs for the checksum type. > > Add a new csum_type field to the BTRFS_IOC_FS_INFO ioctl command which > usually is used to query filesystem features. Also add a flags member > indicating that the kernel responded with a set csum_type field. Thanks! It's great to have it in here, since for machine-to-machine communication it feels much better to just be able to bit-test things, than having to parse back human readable strings from sysfs. > Fixes: 3951e7f050ac ("btrfs: add xxhash64 to checksumming algorithms") > Fixes: 3831bf0094ab ("btrfs: add sha256 to checksumming algorithm") > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > --- > fs/btrfs/ioctl.c | 3 +++ > include/uapi/linux/btrfs.h | 13 ++++++++++++- > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index b3e4c632d80c..16062720f5f3 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -3217,6 +3217,9 @@ static long btrfs_ioctl_fs_info(struct btrfs_fs_info *fs_info, > fi_args->nodesize = fs_info->nodesize; > fi_args->sectorsize = fs_info->sectorsize; > fi_args->clone_alignment = fs_info->sectorsize; > + fi_args->csum_type = > + le16_to_cpu(btrfs_super_csum_type(fs_info->super_copy)); > + fi_args->flags |= BTRFS_FS_INFO_FLAG_CSUM_TYPE; > > if (copy_to_user(arg, fi_args, sizeof(*fi_args))) > ret = -EFAULT; > diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h > index e6b6cb0f8bc6..161d9100c2a6 100644 > --- a/include/uapi/linux/btrfs.h > +++ b/include/uapi/linux/btrfs.h > @@ -250,10 +250,21 @@ struct btrfs_ioctl_fs_info_args { > __u32 nodesize; /* out */ > __u32 sectorsize; /* out */ > __u32 clone_alignment; /* out */ > + __u32 flags; /* out */ > + __u16 csum_type; The /* out */ is missing in this line, if you want to keep it consistent. > + __u16 reserved16; > __u32 reserved32; > - __u64 reserved[122]; /* pad to 1k */ > + __u64 reserved[121]; /* pad to 1k */ > }; > > +/* > + * fs_info ioctl flags > + * > + * Used by: > + * struct btrfs_ioctl_fs_info_args > + */ > +#define BTRFS_FS_INFO_FLAG_CSUM_TYPE (1 << 0) > + > /* > * feature flags > * > Thanks, Hans
On 24/06/2020 14:14, Hans van Kranenburg wrote: [...] > Random idea... What if an imaginary me likes building kernels with > random features from the future (e.g. on top of 4.19LTS), and the next > feature added is to show the value of the new flapsie field. > > The 'version' will increase, but I decide to not pick the new csum type > patches because I don't need them, I only pick the new flapsie feature. Then csum_type will be 0, which is crc32c. But this works by accident. > Now, how does my userspace tooling know that the u16 flapsie field has a > meaning but the csum_type hasn't? > > ("You shouldn't do that" is also a possible answer...) While it's a possible answer, I admit your concerns are valid. That can be handled easier by a flags field. Thanks, Johannes
On 24/06/2020 14:16, Hans van Kranenburg wrote: >> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h >> index e6b6cb0f8bc6..161d9100c2a6 100644 >> --- a/include/uapi/linux/btrfs.h >> +++ b/include/uapi/linux/btrfs.h >> @@ -250,10 +250,21 @@ struct btrfs_ioctl_fs_info_args { >> __u32 nodesize; /* out */ >> __u32 sectorsize; /* out */ >> __u32 clone_alignment; /* out */ >> + __u32 flags; /* out */ >> + __u16 csum_type; > The /* out */ is missing in this line, if you want to keep it consistent. > Whoopsie, thanks for spotting.
On 24.06.20 г. 15:17 ч., Johannes Thumshirn wrote: > On 24/06/2020 14:14, Hans van Kranenburg wrote: > [...] >> Random idea... What if an imaginary me likes building kernels with >> random features from the future (e.g. on top of 4.19LTS), and the next >> feature added is to show the value of the new flapsie field. >> >> The 'version' will increase, but I decide to not pick the new csum type >> patches because I don't need them, I only pick the new flapsie feature. > > Then csum_type will be 0, which is crc32c. But this works by accident. > >> Now, how does my userspace tooling know that the u16 flapsie field has a >> meaning but the csum_type hasn't? >> >> ("You shouldn't do that" is also a possible answer...) > > While it's a possible answer, I admit your concerns are valid. That can be > handled easier by a flags field. I agree, basically having a monotonically incrementing counter implies there is a dependency between previous features and currently added one. Whereas with a flag you just get information about what features are supported without any implied dependencies. The flags interface is IMO a better interface which gives the user less freedom to shoot themselves in the foot. OTOH when one starts doing out of tree backports then one is responsible to maintain them. So the answer could go both ways, but this doesn't mean we shouldn't strive to build better/more robust interfaces. > > Thanks, > Johannes > >
On 24/06/2020 14:21, Nikolay Borisov wrote: > So the answer could go both ways, but this doesn't mean we shouldn't > strive to build better/more robust interfaces. By changing the reserved16 field to a version, we could have both. I just don't think there's much value add here. After Hans spoke up, I favor the flags field, as it's more flexible and puts less assumptions on user-space. If the flag is set, field X is valid.
Hi Johannes,
I love your patch! Perhaps something to improve:
[auto build test WARNING on kdave/for-next]
[also build test WARNING on v5.8-rc2 next-20200624]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Johannes-Thumshirn/btrfs-pass-checksum-type-via-BTRFS_IOC_FS_INFO-ioctl/20200624-182429
base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: x86_64-randconfig-s021-20200624 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-dirty
# save the attached .config to linux build tree
make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
fs/btrfs/ioctl.c:1715:17: sparse: sparse: incompatible types in comparison expression (different address spaces):
fs/btrfs/ioctl.c:1715:17: sparse: struct rcu_string [noderef] <asn:4> *
fs/btrfs/ioctl.c:1715:17: sparse: struct rcu_string *
>> fs/btrfs/ioctl.c:3221:25: sparse: sparse: cast to restricted __le16
fs/btrfs/ioctl.c:3260:40: sparse: sparse: incompatible types in comparison expression (different address spaces):
fs/btrfs/ioctl.c:3260:40: sparse: struct rcu_string [noderef] <asn:4> *
fs/btrfs/ioctl.c:3260:40: sparse: struct rcu_string *
vim +3221 fs/btrfs/ioctl.c
3194
3195 static long btrfs_ioctl_fs_info(struct btrfs_fs_info *fs_info,
3196 void __user *arg)
3197 {
3198 struct btrfs_ioctl_fs_info_args *fi_args;
3199 struct btrfs_device *device;
3200 struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
3201 int ret = 0;
3202
3203 fi_args = kzalloc(sizeof(*fi_args), GFP_KERNEL);
3204 if (!fi_args)
3205 return -ENOMEM;
3206
3207 rcu_read_lock();
3208 fi_args->num_devices = fs_devices->num_devices;
3209
3210 list_for_each_entry_rcu(device, &fs_devices->devices, dev_list) {
3211 if (device->devid > fi_args->max_id)
3212 fi_args->max_id = device->devid;
3213 }
3214 rcu_read_unlock();
3215
3216 memcpy(&fi_args->fsid, fs_devices->fsid, sizeof(fi_args->fsid));
3217 fi_args->nodesize = fs_info->nodesize;
3218 fi_args->sectorsize = fs_info->sectorsize;
3219 fi_args->clone_alignment = fs_info->sectorsize;
3220 fi_args->csum_type =
> 3221 le16_to_cpu(btrfs_super_csum_type(fs_info->super_copy));
3222 fi_args->flags |= BTRFS_FS_INFO_FLAG_CSUM_TYPE;
3223
3224 if (copy_to_user(arg, fi_args, sizeof(*fi_args)))
3225 ret = -EFAULT;
3226
3227 kfree(fi_args);
3228 return ret;
3229 }
3230
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed, Jun 24, 2020 at 07:21:36PM +0900, Johannes Thumshirn wrote: > With the recent addition of filesystem checksum types other than CRC32c, > it is not anymore hard-coded which checksum type a btrfs filesystem uses. > > Up to now there is no good way to read the filesystem checksum, apart from > reading the filesystem UUID and then query sysfs for the checksum type. > > Add a new csum_type field to the BTRFS_IOC_FS_INFO ioctl command which > usually is used to query filesystem features. Also add a flags member > indicating that the kernel responded with a set csum_type field. > > Fixes: 3951e7f050ac ("btrfs: add xxhash64 to checksumming algorithms") > Fixes: 3831bf0094ab ("btrfs: add sha256 to checksumming algorithm") > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > --- > fs/btrfs/ioctl.c | 3 +++ > include/uapi/linux/btrfs.h | 13 ++++++++++++- > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index b3e4c632d80c..16062720f5f3 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -3217,6 +3217,9 @@ static long btrfs_ioctl_fs_info(struct btrfs_fs_info *fs_info, > fi_args->nodesize = fs_info->nodesize; > fi_args->sectorsize = fs_info->sectorsize; > fi_args->clone_alignment = fs_info->sectorsize; > + fi_args->csum_type = > + le16_to_cpu(btrfs_super_csum_type(fs_info->super_copy)); > + fi_args->flags |= BTRFS_FS_INFO_FLAG_CSUM_TYPE; > > if (copy_to_user(arg, fi_args, sizeof(*fi_args))) > ret = -EFAULT; > diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h > index e6b6cb0f8bc6..161d9100c2a6 100644 > --- a/include/uapi/linux/btrfs.h > +++ b/include/uapi/linux/btrfs.h > @@ -250,10 +250,21 @@ struct btrfs_ioctl_fs_info_args { > __u32 nodesize; /* out */ > __u32 sectorsize; /* out */ > __u32 clone_alignment; /* out */ > + __u32 flags; /* out */ > + __u16 csum_type; > + __u16 reserved16; > __u32 reserved32; > - __u64 reserved[122]; /* pad to 1k */ > + __u64 reserved[121]; /* pad to 1k */ Maybe we should just switch to u8 for the reserved field.
On Wed, Jun 24, 2020 at 01:01:08PM +0000, Johannes Thumshirn wrote: > On 24/06/2020 14:21, Nikolay Borisov wrote: > > So the answer could go both ways, but this doesn't mean we shouldn't > > strive to build better/more robust interfaces. > > By changing the reserved16 field to a version, we could have both. I > just don't think there's much value add here. > > After Hans spoke up, I favor the flags field, as it's more flexible and > puts less assumptions on user-space. If the flag is set, field X is valid. Yes that's the idea and version does not make sense to me as it's too coarse. The whole problem here is that the default zeroing of the unused members does not help like for other members where zero usually does not make sense at all (like sectorsize, or the latest addition clone_alignment). So we need the bit to say 'this value is actually correct'.
On Wed, Jun 24, 2020 at 10:15:20PM +0800, kernel test robot wrote: > fs/btrfs/ioctl.c:1715:17: sparse: sparse: incompatible types in comparison expression (different address spaces): > fs/btrfs/ioctl.c:1715:17: sparse: struct rcu_string [noderef] <asn:4> * > fs/btrfs/ioctl.c:1715:17: sparse: struct rcu_string * > >> fs/btrfs/ioctl.c:3221:25: sparse: sparse: cast to restricted __le16 > fs/btrfs/ioctl.c:3260:40: sparse: sparse: incompatible types in comparison expression (different address spaces): > fs/btrfs/ioctl.c:3260:40: sparse: struct rcu_string [noderef] <asn:4> * > fs/btrfs/ioctl.c:3260:40: sparse: struct rcu_string * > > 3220 fi_args->csum_type = > > 3221 le16_to_cpu(btrfs_super_csum_type(fs_info->super_copy)); The report is valid, btrfs_super_csum_type returns the data in CPU byte order.
On 6/24/20 5:31 PM, David Sterba wrote: > On Wed, Jun 24, 2020 at 07:21:36PM +0900, Johannes Thumshirn wrote: >> With the recent addition of filesystem checksum types other than CRC32c, >> it is not anymore hard-coded which checksum type a btrfs filesystem uses. >> >> Up to now there is no good way to read the filesystem checksum, apart from >> reading the filesystem UUID and then query sysfs for the checksum type. >> >> Add a new csum_type field to the BTRFS_IOC_FS_INFO ioctl command which >> usually is used to query filesystem features. Also add a flags member >> indicating that the kernel responded with a set csum_type field. >> >> Fixes: 3951e7f050ac ("btrfs: add xxhash64 to checksumming algorithms") >> Fixes: 3831bf0094ab ("btrfs: add sha256 to checksumming algorithm") >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >> --- >> fs/btrfs/ioctl.c | 3 +++ >> include/uapi/linux/btrfs.h | 13 ++++++++++++- >> 2 files changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >> index b3e4c632d80c..16062720f5f3 100644 >> --- a/fs/btrfs/ioctl.c >> +++ b/fs/btrfs/ioctl.c >> @@ -3217,6 +3217,9 @@ static long btrfs_ioctl_fs_info(struct btrfs_fs_info *fs_info, >> fi_args->nodesize = fs_info->nodesize; >> fi_args->sectorsize = fs_info->sectorsize; >> fi_args->clone_alignment = fs_info->sectorsize; >> + fi_args->csum_type = >> + le16_to_cpu(btrfs_super_csum_type(fs_info->super_copy)); >> + fi_args->flags |= BTRFS_FS_INFO_FLAG_CSUM_TYPE; >> >> if (copy_to_user(arg, fi_args, sizeof(*fi_args))) >> ret = -EFAULT; >> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h >> index e6b6cb0f8bc6..161d9100c2a6 100644 >> --- a/include/uapi/linux/btrfs.h >> +++ b/include/uapi/linux/btrfs.h >> @@ -250,10 +250,21 @@ struct btrfs_ioctl_fs_info_args { >> __u32 nodesize; /* out */ >> __u32 sectorsize; /* out */ >> __u32 clone_alignment; /* out */ >> + __u32 flags; /* out */ >> + __u16 csum_type; >> + __u16 reserved16; >> __u32 reserved32; >> - __u64 reserved[122]; /* pad to 1k */ >> + __u64 reserved[121]; /* pad to 1k */ > > Maybe we should just switch to u8 for the reserved field. And then quadruplecheck the result, so that it does not end up like struct btrfs_ioctl_get_dev_stats, 'when adding the flags field' :D K
On 24/06/2020 17:40, David Sterba wrote: > On Wed, Jun 24, 2020 at 10:15:20PM +0800, kernel test robot wrote: >> fs/btrfs/ioctl.c:1715:17: sparse: sparse: incompatible types in comparison expression (different address spaces): >> fs/btrfs/ioctl.c:1715:17: sparse: struct rcu_string [noderef] <asn:4> * >> fs/btrfs/ioctl.c:1715:17: sparse: struct rcu_string * >>>> fs/btrfs/ioctl.c:3221:25: sparse: sparse: cast to restricted __le16 >> fs/btrfs/ioctl.c:3260:40: sparse: sparse: incompatible types in comparison expression (different address spaces): >> fs/btrfs/ioctl.c:3260:40: sparse: struct rcu_string [noderef] <asn:4> * >> fs/btrfs/ioctl.c:3260:40: sparse: struct rcu_string * >> >> 3220 fi_args->csum_type = >>> 3221 le16_to_cpu(btrfs_super_csum_type(fs_info->super_copy)); > > The report is valid, btrfs_super_csum_type returns the data in CPU byte > order. > Yeah, already fixed locally.
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index b3e4c632d80c..16062720f5f3 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -3217,6 +3217,9 @@ static long btrfs_ioctl_fs_info(struct btrfs_fs_info *fs_info, fi_args->nodesize = fs_info->nodesize; fi_args->sectorsize = fs_info->sectorsize; fi_args->clone_alignment = fs_info->sectorsize; + fi_args->csum_type = + le16_to_cpu(btrfs_super_csum_type(fs_info->super_copy)); + fi_args->flags |= BTRFS_FS_INFO_FLAG_CSUM_TYPE; if (copy_to_user(arg, fi_args, sizeof(*fi_args))) ret = -EFAULT; diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h index e6b6cb0f8bc6..161d9100c2a6 100644 --- a/include/uapi/linux/btrfs.h +++ b/include/uapi/linux/btrfs.h @@ -250,10 +250,21 @@ struct btrfs_ioctl_fs_info_args { __u32 nodesize; /* out */ __u32 sectorsize; /* out */ __u32 clone_alignment; /* out */ + __u32 flags; /* out */ + __u16 csum_type; + __u16 reserved16; __u32 reserved32; - __u64 reserved[122]; /* pad to 1k */ + __u64 reserved[121]; /* pad to 1k */ }; +/* + * fs_info ioctl flags + * + * Used by: + * struct btrfs_ioctl_fs_info_args + */ +#define BTRFS_FS_INFO_FLAG_CSUM_TYPE (1 << 0) + /* * feature flags *
With the recent addition of filesystem checksum types other than CRC32c, it is not anymore hard-coded which checksum type a btrfs filesystem uses. Up to now there is no good way to read the filesystem checksum, apart from reading the filesystem UUID and then query sysfs for the checksum type. Add a new csum_type field to the BTRFS_IOC_FS_INFO ioctl command which usually is used to query filesystem features. Also add a flags member indicating that the kernel responded with a set csum_type field. Fixes: 3951e7f050ac ("btrfs: add xxhash64 to checksumming algorithms") Fixes: 3831bf0094ab ("btrfs: add sha256 to checksumming algorithm") Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> --- fs/btrfs/ioctl.c | 3 +++ include/uapi/linux/btrfs.h | 13 ++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-)