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

[Dave Chinner <david@fromorbit.com>]

On Wed, Jun 08, 2022 at 10:46:40AM +0100, Luís Henriques wrote:
> 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?

It seems like unnecessary additional complexity - keep it simple.
Just set the max size for ceph to ~65000 and add a comment that says
max name+val length for all ceph attrs is 64k and we need enough
space of that space for two attr names...


Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com