Re: [PATCH v2 1/2] generic/020: adjust max_attrval_size for ceph

[Luis Henriques <lhenriques@suse.de>]

Xiubo Li <xiubli@redhat.com> writes:

> On 6/9/22 10:21 PM, David Disseldorp wrote:
>> Hi Luís,
>>
>> On Thu,  9 Jun 2022 11:53:42 +0100, Luís Henriques wrote:
>>
>>> CephFS doesn't have a maximum xattr size.  Instead, it imposes a maximum
>>> size for the full set of xattrs names+values, which by default is 64K.
>>>
>>> This patch fixes the max_attrval_size so that it is slightly < 64K in
>>> order to accommodate any already existing xattrs in the file.
>>>
>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>> ---
>>>   tests/generic/020 | 10 +++++++++-
>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/generic/020 b/tests/generic/020
>>> index d8648e96286e..76f13220fe85 100755
>>> --- a/tests/generic/020
>>> +++ b/tests/generic/020
>>> @@ -128,7 +128,7 @@ _attr_get_max()
>>>   	pvfs2)
>>>   		max_attrval_size=8192
>>>   		;;
>>> -	xfs|udf|9p|ceph)
>>> +	xfs|udf|9p)
>>>   		max_attrval_size=65536
>>>   		;;
>>>   	bcachefs)
>>> @@ -139,6 +139,14 @@ _attr_get_max()
>>>   		# the underlying filesystem, so just use the lowest value above.
>>>   		max_attrval_size=1024
>>>   		;;
>>> +	ceph)
>>> +		# CephFS does not have a maximum value for attributes.  Instead,
>>> +		# it imposes a maximum size for the full set of xattrs
>>> +		# names+values, which by default is 64K.  Set this to a value
>>> +		# that is slightly smaller than 64K so that it can accommodate
>>> +		# already existing xattrs.
>>> +		max_attrval_size=65000
>>> +		;;
>> I take it a more exact calculation would be something like:
>> (64K - $max_attrval_namelen - sizeof(user.snrub="fish2\012"))?
>
> Yeah, something like this looks better to me.

Right, it could be hard-coded.  But we'd need to take into account that
the attribute value may not be ascii.  That's why my initial attempt to
fix this was to decode everything in hex.

> I am afraid without reaching up to the real max size we couldn't test the real
> bugs out from ceph. Such as the bug you fixed in ceph Locker.cc code.

OK, I'll change this to use the exact value.  Thanks, Xiubo.

Cheers,
-- 
Luis

>
>> Perhaps you could calculate this on the fly for CephFS by passing in the
>> filename and subtracting the `getfattr -d $filename` results... That
>> said, it'd probably get a bit ugly, expecially if encoding needs to be
>> taken into account.
>>
>> Reviewed-by: David Disseldorp <ddiss@suse.de>
>>
>> Cheers, David
>>
>