hpc / mpifileutils

File utilities designed for scalability and performance.
https://hpc.github.io/mpifileutils
BSD 3-Clause "New" or "Revised" License
168 stars 66 forks source link

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

Closed ofaaland closed 2 years ago

ofaaland commented 3 years ago

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 commented 3 years ago

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:

https://github.com/hpc/mpifileutils/issues/324 https://github.com/hpc/mpifileutils/issues/49 https://github.com/hpc/mpifileutils/issues/400

ofaaland commented 3 years ago

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

ofaaland commented 3 years ago

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 commented 3 years ago

@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 commented 2 years ago

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

adammoody commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

ofaaland commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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?

ofaaland commented 2 years ago

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

adammoody commented 2 years ago

Really nice addition, @ofaaland !