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

[Goffredo Baroncelli <kreijack@libero.it>]

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


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5