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

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

On Wed, Jun 08, 2022 at 10:16:42AM +1000, Dave Chinner wrote:
> On Tue, Jun 07, 2022 at 04:15:12PM +0100, Luís Henriques wrote:
> > CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum
> > size for the full set of an inode's xattrs names+values, which by default
> > is 64K but it can be changed by a cluster admin.
> 
> So given the max attr name length is fixed by the kernel at 255
> bytes (XATTR_NAME_MAX), that means the max value length is somewhere
> around 65000 bytes, not 1024 bytes?

Right, but if the name is smaller (and in this test specifically we're not
using that XATTR_NAME_MAX), then that max value is > 65000.  Or if the
file already has some attributes set (which is the case in this test),
then this maximum will need to be adjusted accordingly.  (See below.)

> Really, we want to stress and exercise max supported sizes - if the
> admin reduces the max size on their test filesystems, that's not
> something we should be trying to work around in the test suite by
> preventing the test code from ever exercising attr values > 1024
> bytes.....

Agreed.  Xiubo also noted that and I also think this test shouldn't care
about other values.  I should drop (or at least rephrase) the reference to
different values in the commit text.

On Wed, Jun 08, 2022 at 04:41:25PM +0800, Xiubo Li wrote:
...
> Why not fixing this by making sure that the total length of 'name' + 'value'
> == 64K instead for ceph case ?

The reason why I didn't do that is because the $testfile *already* has
another attribute set when we set this max value:

user.snrub="fish2\012"

which means that the maximum for this case will be:

 65536 - $max_attrval_namelen - strlen("user.snrub") - strlen("fish2\012")

I'll split the _attr_get_max() function in 2:

 * _attr_get_max() sets max_attrs which is needed in several places in
   generic/020
 * _attr_get_max_size() sets max_attrval_size, and gets called immediately
   before that value is needed so that it can take into account the
   current state.

Does this sound reasonable?

Cheers,
--
Luís