* Re: [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size
2022-06-07 15:15 ` [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size Luís Henriques
@ 2022-06-07 15:33 ` Darrick J. Wong
2022-06-07 16:20 ` Luís Henriques
2022-06-08 0:23 ` [PATCH 2/2] " Dave Chinner
2022-06-08 8:50 ` Xiubo Li
2 siblings, 1 reply; 17+ messages in thread
From: Darrick J. Wong @ 2022-06-07 15:33 UTC (permalink / raw)
To: Luís Henriques; +Cc: fstests, Jeff Layton, Xiubo Li, ceph-devel
On Tue, Jun 07, 2022 at 04:15:13PM +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.
>
> Test generic/486 started to fail after fixing a ceph bug where this limit
> wasn't being imposed. Adjust dynamically the size of the xattr being set
> if the error returned is -ENOSPC.
>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
> src/attr_replace_test.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
> index cca8dcf8ff60..de18e643f469 100644
> --- a/src/attr_replace_test.c
> +++ b/src/attr_replace_test.c
> @@ -62,7 +62,10 @@ int main(int argc, char *argv[])
>
> /* Then, replace it with bigger one, forcing short form to leaf conversion. */
> memset(value, '1', size);
> - ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
> + do {
> + ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
> + size -= 256;
> + } while ((ret < 0) && (errno == ENOSPC) && (size > 0));
Isn't @size a size_t? Which means that it can't be less than zero? I
wouldn't count on st_blksize (or XATTR_SIZE_MAX) always being a multiple
of 256.
--D
> if (ret < 0) die();
> close(fd);
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size
2022-06-07 15:33 ` Darrick J. Wong
@ 2022-06-07 16:20 ` Luís Henriques
2022-06-07 16:51 ` [PATCH v2] " Luís Henriques
0 siblings, 1 reply; 17+ messages in thread
From: Luís Henriques @ 2022-06-07 16:20 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: fstests, Jeff Layton, Xiubo Li, ceph-devel
"Darrick J. Wong" <djwong@kernel.org> writes:
> On Tue, Jun 07, 2022 at 04:15:13PM +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.
>>
>> Test generic/486 started to fail after fixing a ceph bug where this limit
>> wasn't being imposed. Adjust dynamically the size of the xattr being set
>> if the error returned is -ENOSPC.
>>
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> ---
>> src/attr_replace_test.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
>> index cca8dcf8ff60..de18e643f469 100644
>> --- a/src/attr_replace_test.c
>> +++ b/src/attr_replace_test.c
>> @@ -62,7 +62,10 @@ int main(int argc, char *argv[])
>>
>> /* Then, replace it with bigger one, forcing short form to leaf conversion. */
>> memset(value, '1', size);
>> - ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
>> + do {
>> + ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
>> + size -= 256;
>> + } while ((ret < 0) && (errno == ENOSPC) && (size > 0));
>
> Isn't @size a size_t? Which means that it can't be less than zero? I
> wouldn't count on st_blksize (or XATTR_SIZE_MAX) always being a multiple
> of 256.
*sigh*
You're right, of course. Do you think it would be acceptable to do this
instead:
} while ((ret < 0) && (errno == ENOSPC) && (size > 256));
It's still a magic number, but it should do the trick. Although it's
still a bit ugly, I know. My initial idea was to add an arg to this
program that would be then used as the value for 'size'; this way I could
add a ceph-specific value. But not sure that's less ugly...
Cheers,
--
Luís
>
> --D
>
>> if (ret < 0) die();
>> close(fd);
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] src/attr_replace_test: dynamically adjust the max xattr size
2022-06-07 16:20 ` Luís Henriques
@ 2022-06-07 16:51 ` Luís Henriques
2022-06-08 1:17 ` Xiubo Li
0 siblings, 1 reply; 17+ messages in thread
From: Luís Henriques @ 2022-06-07 16:51 UTC (permalink / raw)
To: fstests, Darrick J. Wong
Cc: Jeff Layton, Xiubo Li, ceph-devel, Luís Henriques
CephFS doesn't had a maximum xattr size. Instead, it imposes a maximum size
for the full set of xattrs names+values, which by default is 64K but may be
changed.
Test generic/486 started to fail after fixing a ceph bug where this limit
wasn't being imposed. Adjust dynamically the size of the xattr being set
if the error returned is -ENOSPC.
Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
src/attr_replace_test.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
index cca8dcf8ff60..d1b92703ba2a 100644
--- a/src/attr_replace_test.c
+++ b/src/attr_replace_test.c
@@ -62,7 +62,10 @@ int main(int argc, char *argv[])
/* Then, replace it with bigger one, forcing short form to leaf conversion. */
memset(value, '1', size);
- ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
+ do {
+ ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
+ size -= 256;
+ } while ((ret < 0) && (errno == ENOSPC) && (size > 256));
if (ret < 0) die();
close(fd);
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2] src/attr_replace_test: dynamically adjust the max xattr size
2022-06-07 16:51 ` [PATCH v2] " Luís Henriques
@ 2022-06-08 1:17 ` Xiubo Li
0 siblings, 0 replies; 17+ messages in thread
From: Xiubo Li @ 2022-06-08 1:17 UTC (permalink / raw)
To: Luís Henriques, fstests, Darrick J. Wong; +Cc: Jeff Layton, ceph-devel
On 6/8/22 12:51 AM, Luís Henriques wrote:
> CephFS doesn't had a maximum xattr size. Instead, it imposes a maximum size
> for the full set of xattrs names+values, which by default is 64K but may be
> changed.
>
> Test generic/486 started to fail after fixing a ceph bug where this limit
> wasn't being imposed. Adjust dynamically the size of the xattr being set
> if the error returned is -ENOSPC.
>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
> src/attr_replace_test.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
> index cca8dcf8ff60..d1b92703ba2a 100644
> --- a/src/attr_replace_test.c
> +++ b/src/attr_replace_test.c
> @@ -62,7 +62,10 @@ int main(int argc, char *argv[])
>
> /* Then, replace it with bigger one, forcing short form to leaf conversion. */
> memset(value, '1', size);
> - ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
> + do {
> + ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
> + size -= 256;
I am not sure whether will this break other filesystems tests.
Maybe we should get the filesystem type first from 'st_mode', and then
in ceph case we should minus strlen(name) before replacing the it. And
then if it fails with '-ENOSPC' do the following ?
Or maybe we could get maximum length of xattr from ioctl(fd) in ceph case ?
> + } while ((ret < 0) && (errno == ENOSPC) && (size > 256));
> if (ret < 0) die();
> close(fd);
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size
2022-06-07 15:15 ` [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size Luís Henriques
2022-06-07 15:33 ` Darrick J. Wong
@ 2022-06-08 0:23 ` Dave Chinner
2022-06-08 9:57 ` Luís Henriques
2022-06-08 8:50 ` Xiubo Li
2 siblings, 1 reply; 17+ messages in thread
From: Dave Chinner @ 2022-06-08 0:23 UTC (permalink / raw)
To: Luís Henriques; +Cc: fstests, Jeff Layton, Xiubo Li, ceph-devel
On Tue, Jun 07, 2022 at 04:15:13PM +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.
>
> Test generic/486 started to fail after fixing a ceph bug where this limit
> wasn't being imposed. Adjust dynamically the size of the xattr being set
> if the error returned is -ENOSPC.
Ah, this shouldn't be getting anywhere near the 64kB limit unless
ceph is telling userspace it's block size is > 64kB:
size = sbuf.st_blksize * 3 / 4;
.....
size = MIN(size, XATTR_SIZE_MAX);
Regardless, the correct thing to do here is pass the max supported
xattr size from the command line (because fstests knows what that it
for each filesystem type) rather than hard coding
XATTR_SIZE_MAX in the test.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size
2022-06-08 0:23 ` [PATCH 2/2] " Dave Chinner
@ 2022-06-08 9:57 ` Luís Henriques
2022-06-08 21:59 ` Dave Chinner
0 siblings, 1 reply; 17+ messages in thread
From: Luís Henriques @ 2022-06-08 9:57 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests, Jeff Layton, Xiubo Li, ceph-devel
On Wed, Jun 08, 2022 at 10:23:15AM +1000, Dave Chinner wrote:
> On Tue, Jun 07, 2022 at 04:15:13PM +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.
> >
> > Test generic/486 started to fail after fixing a ceph bug where this limit
> > wasn't being imposed. Adjust dynamically the size of the xattr being set
> > if the error returned is -ENOSPC.
>
> Ah, this shouldn't be getting anywhere near the 64kB limit unless
> ceph is telling userspace it's block size is > 64kB:
>
> size = sbuf.st_blksize * 3 / 4;
> .....
> size = MIN(size, XATTR_SIZE_MAX);
Yep, that's exactly what is happening. The cephfs kernel client reports
here the value that is being used for ceph "object size", which defaults
to 4M. Hence, we'll set size to XATTR_SIZE_MAX.
> Regardless, the correct thing to do here is pass the max supported
> xattr size from the command line (because fstests knows what that it
> for each filesystem type) rather than hard coding
> XATTR_SIZE_MAX in the test.
OK, makes sense. But then, for the ceph case, it becomes messy because we
also need to know the attribute name to compute the maximum size. I guess
we'll need an extra argument for that too.
Cheers,
--
Luís
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size
2022-06-08 9:57 ` Luís Henriques
@ 2022-06-08 21:59 ` Dave Chinner
2022-06-09 10:32 ` Luís Henriques
0 siblings, 1 reply; 17+ messages in thread
From: Dave Chinner @ 2022-06-08 21:59 UTC (permalink / raw)
To: Luís Henriques; +Cc: fstests, Jeff Layton, Xiubo Li, ceph-devel
On Wed, Jun 08, 2022 at 10:57:22AM +0100, Luís Henriques wrote:
> On Wed, Jun 08, 2022 at 10:23:15AM +1000, Dave Chinner wrote:
> > On Tue, Jun 07, 2022 at 04:15:13PM +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.
> > >
> > > Test generic/486 started to fail after fixing a ceph bug where this limit
> > > wasn't being imposed. Adjust dynamically the size of the xattr being set
> > > if the error returned is -ENOSPC.
> >
> > Ah, this shouldn't be getting anywhere near the 64kB limit unless
> > ceph is telling userspace it's block size is > 64kB:
> >
> > size = sbuf.st_blksize * 3 / 4;
> > .....
> > size = MIN(size, XATTR_SIZE_MAX);
>
> Yep, that's exactly what is happening. The cephfs kernel client reports
> here the value that is being used for ceph "object size", which defaults
> to 4M. Hence, we'll set size to XATTR_SIZE_MAX.
Yikes. This is known to break random applications that size buffers
based on a multiple of sbuf.st_blksize and assume that it is going
to be roughly 4kB. e.g. size a buffer at 1024 * sbuf.st_blksize,
expecting to get a ~4MB buffer, and instead it tries to allocate
a 4GB buffer....
> > Regardless, the correct thing to do here is pass the max supported
> > xattr size from the command line (because fstests knows what that it
> > for each filesystem type) rather than hard coding
> > XATTR_SIZE_MAX in the test.
>
> OK, makes sense. But then, for the ceph case, it becomes messy because we
> also need to know the attribute name to compute the maximum size. I guess
> we'll need an extra argument for that too.
Just pass in a size for ceph that has enough spare space for the
attribute names in it, like for g/020. Don't make it more
complex than it needs to be.
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size
2022-06-08 21:59 ` Dave Chinner
@ 2022-06-09 10:32 ` Luís Henriques
0 siblings, 0 replies; 17+ messages in thread
From: Luís Henriques @ 2022-06-09 10:32 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests, Jeff Layton, Xiubo Li, ceph-devel
On Thu, Jun 09, 2022 at 07:59:50AM +1000, Dave Chinner wrote:
> On Wed, Jun 08, 2022 at 10:57:22AM +0100, Luís Henriques wrote:
> > On Wed, Jun 08, 2022 at 10:23:15AM +1000, Dave Chinner wrote:
> > > On Tue, Jun 07, 2022 at 04:15:13PM +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.
> > > >
> > > > Test generic/486 started to fail after fixing a ceph bug where this limit
> > > > wasn't being imposed. Adjust dynamically the size of the xattr being set
> > > > if the error returned is -ENOSPC.
> > >
> > > Ah, this shouldn't be getting anywhere near the 64kB limit unless
> > > ceph is telling userspace it's block size is > 64kB:
> > >
> > > size = sbuf.st_blksize * 3 / 4;
> > > .....
> > > size = MIN(size, XATTR_SIZE_MAX);
> >
> > Yep, that's exactly what is happening. The cephfs kernel client reports
> > here the value that is being used for ceph "object size", which defaults
> > to 4M. Hence, we'll set size to XATTR_SIZE_MAX.
>
> Yikes. This is known to break random applications that size buffers
> based on a multiple of sbuf.st_blksize and assume that it is going
> to be roughly 4kB. e.g. size a buffer at 1024 * sbuf.st_blksize,
> expecting to get a ~4MB buffer, and instead it tries to allocate
> a 4GB buffer....
>
> > > Regardless, the correct thing to do here is pass the max supported
> > > xattr size from the command line (because fstests knows what that it
> > > for each filesystem type) rather than hard coding
> > > XATTR_SIZE_MAX in the test.
> >
> > OK, makes sense. But then, for the ceph case, it becomes messy because we
> > also need to know the attribute name to compute the maximum size. I guess
> > we'll need an extra argument for that too.
>
> Just pass in a size for ceph that has enough spare space for the
> attribute names in it, like for g/020. Don't make it more
> complex than it needs to be.
Well, in that case it's just easier for attr_replace_test.c to simply set
the ceiling to (XATTR_SIZE_MAX - strlen(name+value)). This is because the
fstests don't really know anymore the max xattr size for each filesystem
type; that knowledge is local to generic/020. The other option is to move
that code (back) to common/attr.
Cheers,
--
Luís
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size
2022-06-07 15:15 ` [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size Luís Henriques
2022-06-07 15:33 ` Darrick J. Wong
2022-06-08 0:23 ` [PATCH 2/2] " Dave Chinner
@ 2022-06-08 8:50 ` Xiubo Li
2 siblings, 0 replies; 17+ messages in thread
From: Xiubo Li @ 2022-06-08 8:50 UTC (permalink / raw)
To: Luís Henriques, fstests; +Cc: Jeff Layton, ceph-devel
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.
>
> Test generic/486 started to fail after fixing a ceph bug where this limit
> wasn't being imposed. Adjust dynamically the size of the xattr being set
> if the error returned is -ENOSPC.
>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
> src/attr_replace_test.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
> index cca8dcf8ff60..de18e643f469 100644
> --- a/src/attr_replace_test.c
> +++ b/src/attr_replace_test.c
> @@ -62,7 +62,10 @@ int main(int argc, char *argv[])
>
> /* Then, replace it with bigger one, forcing short form to leaf conversion. */
> memset(value, '1', size);
> - ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
> + do {
> + ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
> + size -= 256;
> + } while ((ret < 0) && (errno == ENOSPC) && (size > 0));
> if (ret < 0) die();
> close(fd);
>
And also here, we shouldn't worry about users will change the default
value, and we can just assume that users should test it with default value.
I am afraid this won't test real bug of the related code then.
Or as I mentioned in another thread, add one ioctl cmd to get the value ?
^ permalink raw reply [flat|nested] 17+ messages in thread