| Message ID | 20220607151513.26347-2-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Two xattrs-related fixes for ceph | expand |
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? 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..... Cheers, Dave.
On 6/7/22 11:15 PM, 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. > > Adjust max_attrval_size so that the test can be executed in this > filesystem. > > Signed-off-by: Luís Henriques <lhenriques@suse.de> > --- > tests/generic/020 | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/tests/generic/020 b/tests/generic/020 > index d8648e96286e..cadfce5f45e3 100755 > --- a/tests/generic/020 > +++ b/tests/generic/020 > @@ -128,15 +128,16 @@ _attr_get_max() > pvfs2) > max_attrval_size=8192 > ;; > - xfs|udf|9p|ceph) > + xfs|udf|9p) > max_attrval_size=65536 > ;; > bcachefs) > max_attrval_size=1024 > ;; > - nfs) > - # NFS doesn't provide a way to find out the max_attrval_size for > - # the underlying filesystem, so just use the lowest value above. > + nfs|ceph) > + # NFS and CephFS don't provide a way to find out the > + # max_attrval_size for the underlying filesystem, so just use > + # the lowest value above. > max_attrval_size=1024 > ;; > *) Why not fixing this by making sure that the total length of 'name' + 'value' == 64K instead for ceph case ? IMO we shouldn't worry about the case that the max could be changeable, we just need to test the framework works well with the default is enough. And then print a warning if the test fails to let users to know that the max size must be as default, which is 64K, or if users didn't change it then it should be a real bug in ceph.
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
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.
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
diff --git a/tests/generic/020 b/tests/generic/020 index d8648e96286e..cadfce5f45e3 100755 --- a/tests/generic/020 +++ b/tests/generic/020 @@ -128,15 +128,16 @@ _attr_get_max() pvfs2) max_attrval_size=8192 ;; - xfs|udf|9p|ceph) + xfs|udf|9p) max_attrval_size=65536 ;; bcachefs) max_attrval_size=1024 ;; - nfs) - # NFS doesn't provide a way to find out the max_attrval_size for - # the underlying filesystem, so just use the lowest value above. + nfs|ceph) + # NFS and CephFS don't provide a way to find out the + # max_attrval_size for the underlying filesystem, so just use + # the lowest value above. max_attrval_size=1024 ;; *)
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. Adjust max_attrval_size so that the test can be executed in this filesystem. Signed-off-by: Luís Henriques <lhenriques@suse.de> --- tests/generic/020 | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)