Re: [RFC PATCH 2/5] statsfs API: create, add and remove statsfs sources and values

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

On 4/27/20 5:47 PM, Matthew Wilcox wrote:
> On Mon, Apr 27, 2020 at 04:18:13PM +0200, Emanuele Giuseppe Esposito wrote:
>> +static struct statsfs_value *find_value(struct statsfs_value_source *src,
>> +					struct statsfs_value *val)
>> +{
>> +	struct statsfs_value *entry;
>> +
>> +	for (entry = src->values; entry->name; entry++) {
>> +		if (entry == val) {
>> +			WARN_ON(strcmp(entry->name, val->name) != 0);
> 
> Umm.  'entry' and 'val' are pointers.  So if entry is equal to val,
> how could entry->name and val->name not be the same thing?

Good catch, I'll get rid of that check.
> 
> 
> +int statsfs_source_add_values(struct statsfs_source *source,
> +			      struct statsfs_value *stat, void *ptr)
> +{
> +	struct statsfs_value_source *entry, *val_src;
> +	unsigned long index;
> +	int err = -EEXIST;
> +
> +	val_src = create_value_source(ptr);
> +	val_src->values = stat;
> +
> +	xa_lock(&source->values);
> +	xa_for_each(&source->values, index, entry) {
> +		if (entry->base_addr == ptr && entry->values == stat)
> +			goto out;
> +	}
> +
> +	err = __xa_alloc(&source->values, &val_src->id, val_src, xa_limit_32b,
> +			GFP_KERNEL);
> +out:
> +	xa_unlock(&source->values);
> +	if (err)
> +		kfree(val_src);
> +	return err;
> +}
> 
> Using an XArray avoids the occasional latency problems you can see with
> rwsems, as well as being more cache-effective than a linked list.

I didn't know about XArrays, I'll give them a look. I will also fix the 
list initialization with INIT_LIST_HEAD.

Thank you for the above example, but I don't think that each source 
would have more than 2 or 3 value_sources, so using a linked list there 
should be fine.
However, this might be a good point for the subordinates list.

Regarding the locking, the rwsem is also used to protect the other 
lists and dentry of the source, so it wouldn't be removed anyways.

Thank you,

Emanuele