| Message ID | 20220607151513.26347-3-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: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); >
"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,
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.
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 ?
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
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.
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
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);
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(-)