On Thu, Jun 09, 2022 at 07:53:41AM +1000, Dave Chinner wrote:
> 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...
OK, that sounds reasonable. I'll send out v2 shortly. Thanks.
Cheers,
--
Luís
