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

["Luís Henriques" <lhenriques@suse.de>]

David Disseldorp <ddiss@suse.de> writes:

> 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"))?
>
> 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.

In fact, this is *exactly* what I had before Dave suggested to keep it
simple.  After moving the code back into common/attr, where's how the
generic code would look like:

+       ceph)
+		# CephFS does have a limit for the whole set of names+values
+		# attributes in a file.  Thus, it is necessary to get the sizes
+		# of all names and values already existent and subtract them to
+		# the (default) maximum, which is 64k.
+		local len=0
+		while read line; do
+			# skip 1st line
+			[ "$line" != "${line#'#'}" ] && continue
+			n=$(echo $line | awk -F"=0x" '{print $1}')
+			v=$(echo $line | awk -F"=0x" '{print $2}')
+			nlen=${#n}
+			vlen=${#v}
+			# total is the sum of the name len and the value len
+			# divided by 2 because we're dumping them in hex format
+			t=$(($nlen + $vlen / 2))
+			len=$(($len + $t))
+		done <<< $(_getfattr -d -e hex $file 2> /dev/null)
+		echo $((65536 - $max_attrval_namelen - $len))
+		;;

so... yeah, I'm not particularly gifted on shell, it could probably be
done in more clever/cleaner ways.  Anyway, I'm open to revisit this if
this is the preferred solution.

> Reviewed-by: David Disseldorp <ddiss@suse.de>

Thanks David.  (And sorry!  I completely forgot to include you on CC as I
had promised.)

Cheers,
-- 
Luís