diff mbox series

[2/5] btrfs: export the device allocation_hint property in sysfs

Message ID aa62c61a0a9858d010d7d3ec67019332bd20d801.1646589622.git.kreijack@inwind.it (mailing list archive)
State New, archived
Headers show
Series btrfs: allocation_hint | expand

Commit Message

Goffredo Baroncelli March 6, 2022, 6:14 p.m. UTC 
From: Goffredo Baroncelli <kreijack@inwind.it>

Export the device allocation_hint property via
/sys/fs/btrfs/<uuid>/devinfo/<devid>/allocation_hint

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
---
 fs/btrfs/sysfs.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

kernel test robot March 8, 2022, 1:25 p.m. UTC | #1 
Hi Goffredo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on v5.17-rc7 next-20220308]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Goffredo-Baroncelli/btrfs-allocation_hint/20220307-145335
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: microblaze-randconfig-s032-20220307 (https://download.01.org/0day-ci/archive/20220308/202203082127.TtxMcqDK-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 11.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/641cd29d0792eb2e702b6c6a226fce5b4a655e20
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Goffredo-Baroncelli/btrfs-allocation_hint/20220307-145335
        git checkout 641cd29d0792eb2e702b6c6a226fce5b4a655e20
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=microblaze SHELL=/bin/bash fs/btrfs/

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/sysfs.c:1583:3: sparse: sparse: symbol 'allocation_hint_name' was not declared. Should it be static?
   fs/btrfs/sysfs.c:630:9: sparse: sparse: context imbalance in 'btrfs_show_u64' - different lock contexts for basic block

vim +/allocation_hint_name +1583 fs/btrfs/sysfs.c

  1578	
  1579	
  1580	struct allocation_hint_name_t {
  1581		const char *name;
  1582		const u64 value;
> 1583	} allocation_hint_name[] = {
  1584		{ "DATA_PREFERRED", BTRFS_DEV_ALLOCATION_HINT_DATA_PREFERRED },
  1585		{ "METADATA_PREFERRED", BTRFS_DEV_ALLOCATION_HINT_METADATA_PREFERRED },
  1586		{ "DATA_ONLY", BTRFS_DEV_ALLOCATION_HINT_DATA_ONLY },
  1587		{ "METADATA_ONLY", BTRFS_DEV_ALLOCATION_HINT_METADATA_ONLY },
  1588	};
  1589	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Josef Bacik March 22, 2022, 6:05 p.m. UTC | #2 
On Sun, Mar 06, 2022 at 07:14:40PM +0100, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli <kreijack@inwind.it>
> 
> Export the device allocation_hint property via
> /sys/fs/btrfs/<uuid>/devinfo/<devid>/allocation_hint
> 
> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> ---
>  fs/btrfs/sysfs.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 17389a42a3ab..59d92a385a96 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -1578,6 +1578,36 @@ static ssize_t btrfs_devinfo_error_stats_show(struct kobject *kobj,
>  }
>  BTRFS_ATTR(devid, error_stats, btrfs_devinfo_error_stats_show);
>  
> +
> +struct allocation_hint_name_t {
> +	const char *name;
> +	const u64 value;
> +} allocation_hint_name[] = {
> +	{ "DATA_PREFERRED", BTRFS_DEV_ALLOCATION_HINT_DATA_PREFERRED },
> +	{ "METADATA_PREFERRED", BTRFS_DEV_ALLOCATION_HINT_METADATA_PREFERRED },
> +	{ "DATA_ONLY", BTRFS_DEV_ALLOCATION_HINT_DATA_ONLY },
> +	{ "METADATA_ONLY", BTRFS_DEV_ALLOCATION_HINT_METADATA_ONLY },
> +};
> +

The ktest robot complained about this, but also we don't need to be this clever,
also it looks better to have the flags first with standard tabbing.

struct allocation_hint_name {
	const u64 val;
	const char *name;
};

static struct allocation_hint_name allocation_hints[] = {
	{ BTRFS_DEV_ALLOCATION_HINT_DATA_PREFERRED,	"DATA_PREFERRED"	},
	{ BTRFS_DEV_ALLOCATION_HINT_METADATA_PREFERRED,	"METADATA_PREFERRED"	},
	...
};


> +static ssize_t btrfs_devinfo_allocation_hint_show(struct kobject *kobj,
> +					struct kobj_attribute *a, char *buf)
> +{
> +	int i;
> +	struct btrfs_device *device = container_of(kobj, struct btrfs_device,
> +						   devid_kobj);
> +
> +	for (i = 0 ; i < ARRAY_SIZE(allocation_hint_name) ; i++) {
> +		if ((device->type & BTRFS_DEV_ALLOCATION_HINT_MASK) !=
> +		    allocation_hint_name[i].value)
> +			continue;
> +
> +		return scnprintf(buf, PAGE_SIZE, "%s\n",
> +			allocation_hint_name[i].name);
> +	}
> +	return scnprintf(buf, PAGE_SIZE, "<UNKNOWN>\n");

Since we have fs'es that won't use this you should just spit out "NONE".
Thanks,

Josef
Goffredo Baroncelli March 22, 2022, 7:02 p.m. UTC | #3 
On 22/03/2022 19.05, Josef Bacik wrote:
> On Sun, Mar 06, 2022 at 07:14:40PM +0100, Goffredo Baroncelli wrote:
>> From: Goffredo Baroncelli <kreijack@inwind.it>
>>
>> Export the device allocation_hint property via
>> /sys/fs/btrfs/<uuid>/devinfo/<devid>/allocation_hint
>>
>> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
>> ---
>>   fs/btrfs/sysfs.c | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>> index 17389a42a3ab..59d92a385a96 100644
>> --- a/fs/btrfs/sysfs.c
>> +++ b/fs/btrfs/sysfs.c
>> @@ -1578,6 +1578,36 @@ static ssize_t btrfs_devinfo_error_stats_show(struct kobject *kobj,
>>   }
>>   BTRFS_ATTR(devid, error_stats, btrfs_devinfo_error_stats_show);
>>   
>> +
>> +struct allocation_hint_name_t {
>> +	const char *name;
>> +	const u64 value;
>> +} allocation_hint_name[] = {
>> +	{ "DATA_PREFERRED", BTRFS_DEV_ALLOCATION_HINT_DATA_PREFERRED },
>> +	{ "METADATA_PREFERRED", BTRFS_DEV_ALLOCATION_HINT_METADATA_PREFERRED },
>> +	{ "DATA_ONLY", BTRFS_DEV_ALLOCATION_HINT_DATA_ONLY },
>> +	{ "METADATA_ONLY", BTRFS_DEV_ALLOCATION_HINT_METADATA_ONLY },
>> +};
>> +
> 
> The ktest robot complained about this, but also we don't need to be this clever,
> also it looks better to have the flags first with standard tabbing.
> 
Yes, ktest is more fine than gcc. I didn't tough about the fact that outside sysfs.c
allocation_hints[] is not used. If in the future it will be used, we can remove "static".

> struct allocation_hint_name {
> 	const u64 val;
> 	const char *name;
> };
> 
> static struct allocation_hint_name allocation_hints[] = {
> 	{ BTRFS_DEV_ALLOCATION_HINT_DATA_PREFERRED,	"DATA_PREFERRED"	},
> 	{ BTRFS_DEV_ALLOCATION_HINT_METADATA_PREFERRED,	"METADATA_PREFERRED"	},
> 	...
> };
> 
Ok

> 
>> +static ssize_t btrfs_devinfo_allocation_hint_show(struct kobject *kobj,
>> +					struct kobj_attribute *a, char *buf)
>> +{
>> +	int i;
>> +	struct btrfs_device *device = container_of(kobj, struct btrfs_device,
>> +						   devid_kobj);
>> +
>> +	for (i = 0 ; i < ARRAY_SIZE(allocation_hint_name) ; i++) {
>> +		if ((device->type & BTRFS_DEV_ALLOCATION_HINT_MASK) !=
>> +		    allocation_hint_name[i].value)
>> +			continue;
>> +
>> +		return scnprintf(buf, PAGE_SIZE, "%s\n",
>> +			allocation_hint_name[i].name);
>> +	}
>> +	return scnprintf(buf, PAGE_SIZE, "<UNKNOWN>\n");
> 
> Since we have fs'es that won't use this you should just spit out "NONE".
> Thanks,
> 
To me NONE means -> I dont' use it
Unknown means -> it is not a understood value

Because BTRFS_DEV_ALLOCATION_HINT_MASK is 2 bits, and we have 4 possible values now it is impossible to get UNKNOWN.
If future if we increase the allocation-hint bits, and we don't map all the possible combination, it is possible that in the disk it is stored an UNKNOWN value. But it may be VALID for a more newer kernel.


> Josef
diff mbox series

Patch

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 17389a42a3ab..59d92a385a96 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1578,6 +1578,36 @@  static ssize_t btrfs_devinfo_error_stats_show(struct kobject *kobj,
 }
 BTRFS_ATTR(devid, error_stats, btrfs_devinfo_error_stats_show);
 
+
+struct allocation_hint_name_t {
+	const char *name;
+	const u64 value;
+} allocation_hint_name[] = {
+	{ "DATA_PREFERRED", BTRFS_DEV_ALLOCATION_HINT_DATA_PREFERRED },
+	{ "METADATA_PREFERRED", BTRFS_DEV_ALLOCATION_HINT_METADATA_PREFERRED },
+	{ "DATA_ONLY", BTRFS_DEV_ALLOCATION_HINT_DATA_ONLY },
+	{ "METADATA_ONLY", BTRFS_DEV_ALLOCATION_HINT_METADATA_ONLY },
+};
+
+static ssize_t btrfs_devinfo_allocation_hint_show(struct kobject *kobj,
+					struct kobj_attribute *a, char *buf)
+{
+	int i;
+	struct btrfs_device *device = container_of(kobj, struct btrfs_device,
+						   devid_kobj);
+
+	for (i = 0 ; i < ARRAY_SIZE(allocation_hint_name) ; i++) {
+		if ((device->type & BTRFS_DEV_ALLOCATION_HINT_MASK) !=
+		    allocation_hint_name[i].value)
+			continue;
+
+		return scnprintf(buf, PAGE_SIZE, "%s\n",
+			allocation_hint_name[i].name);
+	}
+	return scnprintf(buf, PAGE_SIZE, "<UNKNOWN>\n");
+}
+BTRFS_ATTR(devid, allocation_hint, btrfs_devinfo_allocation_hint_show);
+
 /*
  * Information about one device.
  *
@@ -1591,6 +1621,7 @@  static struct attribute *devid_attrs[] = {
 	BTRFS_ATTR_PTR(devid, replace_target),
 	BTRFS_ATTR_PTR(devid, scrub_speed_max),
 	BTRFS_ATTR_PTR(devid, writeable),
+	BTRFS_ATTR_PTR(devid, allocation_hint),
 	NULL
 };
 ATTRIBUTE_GROUPS(devid);