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@xxxxxxxxxxxxx
