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

dsync and dcp: allow user to control syncing of xattrs #503

Merged
merged 2 commits into from
Dec 9, 2021

Conversation

ofaaland
Copy link
Collaborator

@ofaaland ofaaland commented Oct 9, 2021

Rather than always copying xattrs, introduce a new option
"--xattrs | -e" to allow the user to control how this is done.

Options are
"none" Copy no xattrs
"all" Copy all xattrs (prior default)
"non-lustre" Copy non-lustre xattrs (new default)
"libattr" Copy xattrs not indicated as "skip" in /etc/xattr.conf

Copying xattrs is now independent of --preserve for owner, perms,
etc.

Lustre uses xattrs to record important information such as how the file
data should be distributed among the Lustre servers. As a result,
copying the lustre xattrs prevents certain default values from taking
effect.

If "non-lustre" option is in effect, xattrs named in lustre
source file lustre_idl.h as of this writing are not synced (see
XATTR_NAME_SOM and friends). In addition, xattrs with prefix
"lustre" are not synced.

Signed-off-by: Olaf Faaland faaland1@llnl.gov

@adammoody
Copy link
Member

Thanks for adding this, @ofaaland ! This has been a long requested feature, so users will really appreciate it.

There are some open issues about needing something like that. It's been a while since I reviewed these, but it would be good to see if this would let us close out or make progress on them:

#324
#49
#400

@ofaaland
Copy link
Collaborator Author

@adammoody thanks, I should have looked at the issues. I'll update my patch.

@ofaaland
Copy link
Collaborator Author

Hi @adammoody One thing I need to do after reviewing those tickets is integrate my work into dcp.

Do I also need to integrate into dcp1? I can't seem to find why both exist.

@adammoody
Copy link
Member

adammoody commented Oct 12, 2021

@ofaaland , there are too many issues to track. No problem that you weren't aware. I just wanted to list them here because your work here will help address them.

We can also add these separately in dcp if you want.

We still have dcp1 and dcp because they implement two different algorithms. Depending on the situation, one can be faster than the other. Eventually, it'd be nice to roll both algorithms into one tool, but that's still future work. Don't worry about dcp1.

@ofaaland ofaaland changed the title dsync: allow user to control syncing of xattrs dsync and dcp: allow user to control syncing of xattrs Oct 14, 2021
Copy link
Contributor

@adilger adilger left a comment

Choose a reason for hiding this comment

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

None of my comments are particularly related to the functionality being added here (which looks fine to me, the patch seems rather straight forward). They are mostly generic comments about code style, and trying to minimize/avoid potential future bugs related to the changes being made here.

src/dcp/dcp.c Outdated Show resolved Hide resolved
src/dcp/dcp.c Outdated Show resolved Hide resolved
src/dcp/dcp.c Outdated Show resolved Hide resolved
src/dcp/dcp.c Outdated Show resolved Hide resolved
src/dcp/dcp.c Outdated Show resolved Hide resolved
@ofaaland
Copy link
Collaborator Author

Hi @adammoody when you get a chance, please review, thanks.

man/dcp.1 Outdated Show resolved Hide resolved
src/dcp/dcp.c Outdated Show resolved Hide resolved
src/dcp/dcp.c Outdated Show resolved Hide resolved
src/dsync/dsync.c Outdated Show resolved Hide resolved
@adammoody
Copy link
Member

Thanks again @ofaaland . Sorry to be so tardy on this. Great work. I found a few things to check, but otherwise it looks good.

We use sphinx to generate our man pages from the rst files. I can help you set that up.

@ofaaland ofaaland force-pushed the b-skip-xattrs branch 2 times, most recently from 491606f to 6c6f86a Compare November 18, 2021 23:44
@ofaaland
Copy link
Collaborator Author

Hi @adammoody , I fixed up the issues you found, and also added a test. I ran the test bash file by hand, rather than using nose2 and the python test-runner because I'm not sure how it works - but it should work, I've included a python wrapper copied from the dcp test.

I found doc/README.md and followed the instructions to setup and run Sphinx. It ran, and it updated the man pages to reflect my .rst updates, but it also made a bunch of markup changes that are unrelated to my work. So perhaps the version of Sphinx is different or there's some other Sphinx configuration I haven't done. Here's an example:

 .TP
 .B \-b, \-\-blocksize SIZE
 Set the compression block size, from 1 to 9.
-Where 1=100kB <E2><80><A6> and 9=900kB. Default is 9.
+Where 1=100kB ... and 9=900kB. Default is 9.
 .UNINDENT
 .INDENT 0.0
 .TP

Because I don't know the reason for those markup changes, I've discarded the changes Sphinx made for now. Let me know how to proceed.

Thanks!

@ofaaland
Copy link
Collaborator Author

dsync test output:

Testing dsync
PASSED verify of option none for /mnt/xfs/dest type xfs
PASSED verify of option all for /mnt/xfs/dest type xfs
SKIPPED verify of option non-lustre
PASSED verify of option libattr for /mnt/xfs/dest type xfs

@adammoody
Copy link
Member

Thanks @ofaaland . That's great!

I just gave you a couple of files that I used to install sphinx and then build the man pages. It's been a while since I tried myself, so it could just be due to a newer sphinx version like you say.

We could also merge without the changes to the man files and generate the new man pages in a later commit if you want. I often only update the rst files in my commits and forget all about needing to regenerate the man pages until we stamp a new release.

src/dsync/dsync.c Outdated Show resolved Hide resolved
src/dcp/dcp.c Outdated Show resolved Hide resolved
@ofaaland
Copy link
Collaborator Author

OK, I fixed up those issues, removed a couple of whitespace issues that snuck in, and hand-tested dcp to make sure -X works. I think it's ready to go.

I think it makes sense to update the man pages in a separate commit, in case there are other unrelated man page changes. But I'll take your sphinx files and see if I still get markup changes.

Thanks @adammoody

@ofaaland
Copy link
Collaborator Author

Looks like the markup changes are not due to the process I followed - I get the same results using your scripts. There is at least one other content change (for DAOS) to the .rst files that isn't yet reflected in the man pages, so I think a man-page-only commit makes sense.

@adammoody
Copy link
Member

Thanks, @ofaaland . This is good to go, but we've got a conflict on some rst files due to a change that snuck in. Would you please update to resolve those, and then we can merge?

In dsync and dcp, change the short option letters for those options
so that -X can be used for xattr copying, which is consistent with
rsync.

long            old   new
daos-prefix     -X    -Y
daos-api        -x    -y

Signed-off-by: Olaf Faaland <faaland1@llnl.gov>
Rather than always copying xattrs, introduce option
"--xattrs | -X" to allow the user to control how this is done.

-X is chosen for consistency with rsync.

Options are
  "none"        Copy no xattrs
  "all"         Copy all xattrs (prior default)
  "non-lustre"  Copy non-lustre xattrs (new default)
  "libattr"     Copy xattrs not excluded by /etc/xattr.conf

Copying xattrs is now independent of whether the user used the
--preserve option.

Lustre uses xattrs to record important information such as how the file
data should be distributed among the Lustre servers.  As a result,
copying the lustre xattrs prevents certain default values from taking
effect.

If "non-lustre" option is in effect, xattrs named in lustre
source file lustre_idl.h as of this writing are not copied (see
XATTR_NAME_SOM and friends).  In addition, xattrs with prefix
"lustre" are not copied.

Adds a dsync test that verifies "none", "all", and "libattr" variants.
Tested on an xfs file system.

The format of /etc/xattr.conf seems not to be described in man pages on
RHEL 7 and RHEL 8 machines.  Until that is fixed, see
http://git.savannah.nongnu.org/cgit/attr.git/tree/xattr.conf

Signed-off-by: Olaf Faaland <faaland1@llnl.gov>

Closes hpc#324
Partially Addresses hpc#49
Partially Addresses hpc#400
@ofaaland
Copy link
Collaborator Author

ofaaland commented Dec 7, 2021

Thanks @adammoody. Rebased on master and fixed the conflicts.

@adammoody adammoody merged commit 96a9fe7 into hpc:master Dec 9, 2021
@adammoody
Copy link
Member

Really nice addition, @ofaaland !

@ofaaland ofaaland deleted the b-skip-xattrs branch November 7, 2024 23:33
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