Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mfu: copy much more xattrs #594

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KoyamaSohei
Copy link

When copying files using dcp or dsync, extended attributes such as project id are also copied.

Resolves #553.

@KoyamaSohei KoyamaSohei force-pushed the skoyama/xattrs-more branch 3 times, most recently from e2c8147 to 860cda3 Compare October 30, 2024 09:51
Copy link
Collaborator

@ofaaland ofaaland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution @KoyamaSohei, I have one concern.

src/common/mfu_flist_copy.c Outdated Show resolved Hide resolved
src/common/mfu_flist_copy.c Outdated Show resolved Hide resolved
src/common/mfu_flist_copy.c Outdated Show resolved Hide resolved
src/common/mfu_flist_copy.c Outdated Show resolved Hide resolved
@KoyamaSohei
Copy link
Author

@ofaaland Could you please review again?

I fixed to copy only fsx_projid and FS_XFLAG_PROJINHERIT

Copy link
Collaborator

@ofaaland ofaaland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KoyamaSohei Thanks for this improvement.

Please do some testing to and report the performance impact of your code change, preferably on a parallel file system like Lustre which may have high throughput but higher latency than something local like xfs.

Please also modify your commit message to reflect that mfu is copying projectid ; the commit message still makes it sounds like it's copying many xattrs.

src/common/mfu_flist_copy.c Outdated Show resolved Hide resolved
@KoyamaSohei
Copy link
Author

We compared the execution time when copying lustre-release 10 times on Lustre using dcp.

There was a significant decrease in performance ...

without projid with projid
6.730 35.203
6.661 16.111
6.381 12.757
6.189 9.895
6.023 7.868
8.082 11.188
9.612 16.068
10.529 20.939
10.450 15.777
11.078 9.029

When copying files using dcp or dsync, extended attributes such as
fsx_projid and FS_XFLAG_PROJINHERIT are also copied.

Signed-off-by: Sohei Koyama <[email protected]>
@ofaaland
Copy link
Collaborator

ofaaland commented Dec 3, 2024

We compared the execution time when copying lustre-release 10 times on Lustre using dcp.

There was a significant decrease in performance ...
without projid with projid
6.730 35.203
6.661 16.111
6.381 12.757
6.189 9.895
6.023 7.868
8.082 11.188
9.612 16.068
10.529 20.939
10.450 15.777
11.078 9.029

I think we should try and fix that before we merge this. For our site, for example, we don't yet use projectid at all, so this would be a big performance hit for no benefit.

I wonder if this is because we are sort-of opening each file twice, once for getxattr(2) and once for open(2) in preparation for the ioctl(FS_IOC_FSGETXATTR). If that's true, refactoring the code to to call open once and then use that fd for both fgetxattr and ioctl might improve performance.

Before you do that refactoring work, though, maybe you can figure out with some careful experiments whether that's where the time is going, or whether we're spending it elsewhere.

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.

[Request] add support for project id (i.e. lsattr -p)
2 participants