Skip to content

Conversation

@struschev
Copy link

Hello!

I've prepared support of xattr operations measurement into fio. The only thing, I think, is that multiple operations in one IOPS doesn't quite fit the fio philosophy. I would like to know your opinion on this matter.

@vincentkfu
Copy link
Collaborator

The second patch is not needed since there is no change to the ioengine ops struct.

@struschev
Copy link
Author

The second patch is not needed since there is no change to the ioengine ops struct.

Oh, ok, I've understood something wrong, so I reverted commit

@struschev
Copy link
Author

Isn't it normal that CI didn't work for Windows and Macos?

@sitsofe
Copy link
Collaborator

sitsofe commented Nov 1, 2025

Isn't it normal that CI didn't work for Windows and Macos?

We do tend to see the odd failure but this macOS error looks to be a permanent build error:

engines/fileoperations.c:271:61: error: too few arguments to function call, expected 6, have 5
                ret = setxattr(f->file_name, attrname, attrval, o->size, 0);
                      ~~~~~~~~                                            ^
/Applications/Xcode_15.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/sys/xattr.h:65:5: note: 'setxattr' declared here
int setxattr(const char *path, const char *name, const void *value, size_t size, u_int32_t position, int options);
    ^
engines/fileoperations.c:325:42: error: too few arguments to function call, expected 4, have 3
        buflen = listxattr(f->file_name, NULL, 0);
                 ~~~~~~~~~                      ^
/Applications/Xcode_15.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/sys/xattr.h:73:9: note: 'listxattr' declared here
ssize_t listxattr(const char *path, char *namebuff, size_t size, int options);
        ^
engines/fileoperations.c:334:50: error: too few arguments to function call, expected 4, have 3
        buflen = listxattr(f->file_name, attrbuf, buflen);
                 ~~~~~~~~~                              ^
/Applications/Xcode_15.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/sys/xattr.h:73:9: note: 'listxattr' declared here
ssize_t listxattr(const char *path, char *namebuff, size_t size, int options);
        ^
engines/fileoperations.c:342:52: error: too few arguments to function call, expected 6, have 4
                vallen = getxattr(f->file_name, attrname, NULL, 0);
                         ~~~~~~~~                                ^
/Applications/Xcode_15.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/sys/xattr.h:61:9: note: 'getxattr' declared here
ssize_t getxattr(const char *path, const char *name, void *value, size_t size, u_int32_t position, int options);
        ^
engines/fileoperations.c:350:61: error: too few arguments to function call, expected 6, have 4
                        vallen = getxattr(f->file_name, attrname, attrval, vallen);
                                 ~~~~~~~~                                        ^
/Applications/Xcode_15.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/sys/xattr.h:61:9: note: 'getxattr' declared here
ssize_t getxattr(const char *path, const char *name, void *value, size_t size, u_int32_t position, int options);
        ^

(from https://github.com/axboe/fio/actions/runs/18996147708/job/54256207771?pr=2009 ).

Windows also looks like a build failure:

engines/fileoperations.c:13:10: fatal error: sys/xattr.h: No such file or directory
   13 | #include <sys/xattr.h>
      |          ^~~~~~~~~~~~~
compilation terminated.

(from https://github.com/axboe/fio/actions/runs/18996147708/job/54256207773?pr=2009#step:8:183 ).

@sitsofe
Copy link
Collaborator

sitsofe commented Nov 1, 2025

If you're introducing a platform specific engine you will have to add a configure probe to stop it being built on platforms that are not supported. See what e4defrag engine does for an example. For an engine which has parts that may not be available see the #ifdef FIO_HAVE_PWRITEV2 sections in https://github.com/axboe/fio/blob/master/engines/sync.c , https://github.com/axboe/fio/blob/master/options.c and the pwritev probe in https://github.com/axboe/fio/blob/master/configure .

@axboe
Copy link
Owner

axboe commented Nov 1, 2025

What @sitsofe said, but also please sanitize your git history and commit messages too. Your commit message does not follow the required format (signed-off-by, for example), and it should also have an actual description of why this commit exists. All it has is "add xattr ioengines" which is pretty obvious. Explain what the engines are, and what they do. Basically duplicate some of the manual additions you have.

And don't add a commit you don't need (the version bump) and then add a revert of that. I don't want useless commits like that in the git history. Just rebase your branch, dropping those two patches, and then force push a new one for the PR.

Outside of that, I do think the engine makes sense, and it mostly looks good. There are some useless additions like variables that you don't really need. Things like int do_lat = !td->o.disable_lat; - just drop that and use td->o.disable_lat in the code. They don't add anything or optimize anything, they only help to obfuscate what's actually being tested here.

@struschev struschev force-pushed the xattr branch 3 times, most recently from 290d2b1 to 61b483a Compare November 10, 2025 09:30
The patch introduce ioengines that allow to measure extended attributes access: filesetxattr for xattrs value setting, filegetxattr - for lookup.

Signed-off-by: Sergei Truschev <[email protected]>
@struschev
Copy link
Author

Fixed compilation errors, git history and commit message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants