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

linux: zpl: inode: pass through RENAME_NOREPLACE #14084

Closed
wants to merge 1 commit into from

Conversation

nabijaczleweli
Copy link
Contributor

Motivation and Context

Wanted RENAME_NOREPLACE :)

Description

Just don't refuse it.

Linux Documentation/filesystems/vfs.rst:

``rename``
        called by the rename(2) system call to rename the object to have
        the parent and name given by the second inode and dentry.

        The filesystem must return -EINVAL for any unsupported or
        unknown flags.  Currently the following flags are implemented:
        (1) RENAME_NOREPLACE: this flag indicates that if the target of
        the rename exists the rename should fail with -EEXIST instead of
        replacing the target.  The VFS already checks for existence, so
        for local filesystems the RENAME_NOREPLACE implementation is
        equivalent to plain rename.
        (2) RENAME_EXCHANGE: exchange source and target.  Both must
        exist; this is checked by the VFS.  Unlike plain rename, source
        and target may be of different type.

RENAME_EXCHANGE would be non-obvious, insofar as it would require changing more than 1 line and passing the flag to zfs_rename(). This is free.

How Has This Been Tested?

The test passes on tmpfs and currently fails on zfs (EINVAL). If it passes on CI then that exhaustively proves it works.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the OpenZFS code style requirements.
  • I have updated the documentation accordingly.
  • I have read the contributing document.
  • I have added tests to cover my changes.
  • I have run the ZFS Test Suite with this change applied. – CI take my hand
  • All commit messages are properly formatted and contain Signed-off-by.

Fragment cited from Documentation/filesystems/vfs.rst,
the description is unchanged since the original in kernel commit
520c8b16505236fc82daa352e6c5e73cd9870cff ("vfs: add renameat2 syscall")

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

@nabijaczleweli please take a look at #14070 which adds support for all of the RENAME_* flags. Any additional review and testing of that work would be welcome. For example, it'd be nice to be able to include your additional test case there.

As for the unrelated cleanup in this PR I'm all for it, but we'll pull it out in to its own commit.

@behlendorf behlendorf added the Status: Design Review Needed Architecture or design is under discussion label Oct 26, 2022
@nabijaczleweli
Copy link
Contributor Author

nabijaczleweli commented Oct 26, 2022

My only note is that the check in module/os/linux/zfs/zfs_vnops_os.c new line 2900 is always false because that's pre-validated by the VFS (hence why my one-line PR Just Works :)).
The rest of the code is beyond me at present and the only functionality I care about for my use-case is RENAME_NOREPLACE (and this is trivially backportable, #14070 isn't, but).

From what I see of the tests, they're largely equivalent to the ones here (sans the error value validation, which is just pedantry on my part), except that by god please someone teach Ryan to use grep -x.

Don't really see anything else I changed here besides, like, adding the alignment spaces to the Makefile.
In good hands, so closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Design Review Needed Architecture or design is under discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants