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

Check for unlinked znodes after igrab() #9602

Merged
merged 1 commit into from
Nov 21, 2019

Conversation

mfoliveira
Copy link
Contributor

Motivation and Context

The changes in commit 41e1aa2a / PR#9583 introduced a regression on
tmpfile_001_pos: fsetxattr() on a O_TMPFILE file descriptor started
to fail with errno ENODATA:

    openat(AT_FDCWD, "/test", O_RDWR|O_TMPFILE, 0666) = 3
    <...>
    fsetxattr(3, "user.test", <...>, 64, 0) = -1 ENODATA (No data available)

Description

The originally proposed change on PR#9583 is not susceptible to it,
so just move the code/if-checks around back in that way, to fix it.

Before that commit:

    $ cat /sys/module/zfs/version
    0.8.0-401_gcc1a1e17

    $ TESTDIR="/test" /usr/share/zfs/zfs-tests/tests/functional/tmpfile/tmpfile_001_pos ; echo $?
    Verify O_TMPFILE is working properly.
    0

After that commit:

    $ cat /sys/module/zfs/version
    0.8.0-402_g41e1aa2a

    $ TESTDIR="/test" /usr/share/zfs/zfs-tests/tests/functional/tmpfile/tmpfile_001_pos ; echo $?
    Verify O_TMPFILE is working properly.
    fsetxattr: No data available
    6

With this commit:

    $ cat /sys/module/zfs/version
    0.8.0-405_g...

    $ TESTDIR="/test" /usr/share/zfs/zfs-tests/tests/functional/tmpfile/tmpfile_001_pos ; echo $?
    Verify O_TMPFILE is working properly.
    0

How Has This Been Tested?

The zfs test-suite (functional) has been run before/after commit 41e1aa2a / PR#9583, and only tmpfile_001_pos had a consistent, repeatable regression.
The patch has been verified to fix it / make this particular test-case pass again.

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

The changes in commit 41e1aa2 / PR#9583 introduced a regression on
tmpfile_001_pos: fsetxattr() on a O_TMPFILE file descriptor started
to fail with errno ENODATA:

    openat(AT_FDCWD, "/test", O_RDWR|O_TMPFILE, 0666) = 3
    <...>
    fsetxattr(3, "user.test", <...>, 64, 0) = -1 ENODATA

The originally proposed change on PR#9583 is not susceptible to it,
so just move the code/if-checks around back in that way, to fix it.

    $ export TESTDIR="/test"  # zfs mountpoint

Before that commit:

    $ cat /sys/module/zfs/version
    0.8.0-401_gcc1a1e17

    $ /usr/share/zfs/zfs-tests/tests/functional/tmpfile/tmpfile_001_pos
    Verify O_TMPFILE is working properly.

    $ echo $?
    0

After that commit:

    $ cat /sys/module/zfs/version
    0.8.0-402_g41e1aa2a

    $ /usr/share/zfs/zfs-tests/tests/functional/tmpfile/tmpfile_001_pos
    Verify O_TMPFILE is working properly.
    fsetxattr: No data available

    $ echo $?
    6

With this commit:

    $ cat /sys/module/zfs/version
    0.8.0-405_g...

    $ /usr/share/zfs/zfs-tests/tests/functional/tmpfile/tmpfile_001_pos
    Verify O_TMPFILE is working properly.

    $ echo $?
    0

Original-patch-by: Heitor Alves de Siqueira <halves@canonical.com>
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Nov 20, 2019
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.

Thank you for the quick fix which is correct. I noticed this myself yesterday but hadn't yet started to investigate. I can verify it does correctly resolve the O_TMPFILE failures.

The root cause here is that new files created with O_TMPFILE are immediately added to the unlinked list and have z_unlinked set to ensure they're not leaked during a crash. Which means we need to perform the igrab() before checking z_unlinked. It's possible this might have also caused an issue when manipulating open but unlinked files, though I didn't see any issue with some quick testing.

The other OpenZFS platforms do not support the O_TMPFILE option so this wouldn't be an issue there.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Nov 21, 2019
@codecov
Copy link

codecov bot commented Nov 21, 2019

Codecov Report

Merging #9602 into master will decrease coverage by 0.18%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9602      +/-   ##
==========================================
- Coverage   79.22%   79.04%   -0.19%     
==========================================
  Files         419      419              
  Lines      123704   123704              
==========================================
- Hits        98009    97785     -224     
- Misses      25695    25919     +224
Flag Coverage Δ
#kernel 79.73% <25%> (-0.19%) ⬇️
#user 66.5% <ø> (-0.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 540312d...010df42. Read the comment docs.

@behlendorf behlendorf merged commit 0c46813 into openzfs:master Nov 21, 2019
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 26, 2019
The changes in commit 41e1aa2 / PR openzfs#9583 introduced a regression on
tmpfile_001_pos: fsetxattr() on a O_TMPFILE file descriptor started
to fail with errno ENODATA:

    openat(AT_FDCWD, "/test", O_RDWR|O_TMPFILE, 0666) = 3
    <...>
    fsetxattr(3, "user.test", <...>, 64, 0) = -1 ENODATA

The originally proposed change on PR openzfs#9583 is not susceptible to it,
so just move the code/if-checks around back in that way, to fix it.

Reviewed-by: Pavel Snajdr <snajpa@snajpa.net>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Original-patch-by: Heitor Alves de Siqueira <halves@canonical.com>
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
Closes openzfs#9602
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 27, 2019
The changes in commit 41e1aa2 / PR openzfs#9583 introduced a regression on
tmpfile_001_pos: fsetxattr() on a O_TMPFILE file descriptor started
to fail with errno ENODATA:

    openat(AT_FDCWD, "/test", O_RDWR|O_TMPFILE, 0666) = 3
    <...>
    fsetxattr(3, "user.test", <...>, 64, 0) = -1 ENODATA

The originally proposed change on PR openzfs#9583 is not susceptible to it,
so just move the code/if-checks around back in that way, to fix it.

Reviewed-by: Pavel Snajdr <snajpa@snajpa.net>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Original-patch-by: Heitor Alves de Siqueira <halves@canonical.com>
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
Closes openzfs#9602
tonyhutter pushed a commit that referenced this pull request Jan 23, 2020
The changes in commit 41e1aa2 / PR #9583 introduced a regression on
tmpfile_001_pos: fsetxattr() on a O_TMPFILE file descriptor started
to fail with errno ENODATA:

    openat(AT_FDCWD, "/test", O_RDWR|O_TMPFILE, 0666) = 3
    <...>
    fsetxattr(3, "user.test", <...>, 64, 0) = -1 ENODATA

The originally proposed change on PR #9583 is not susceptible to it,
so just move the code/if-checks around back in that way, to fix it.

Reviewed-by: Pavel Snajdr <snajpa@snajpa.net>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Original-patch-by: Heitor Alves de Siqueira <halves@canonical.com>
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
Closes #9602
sdimitro pushed a commit to sdimitro/zfs that referenced this pull request Jan 31, 2020
The changes in commit 41e1aa2 / PR openzfs#9583 introduced a regression on
tmpfile_001_pos: fsetxattr() on a O_TMPFILE file descriptor started
to fail with errno ENODATA:

    openat(AT_FDCWD, "/test", O_RDWR|O_TMPFILE, 0666) = 3
    <...>
    fsetxattr(3, "user.test", <...>, 64, 0) = -1 ENODATA

The originally proposed change on PR openzfs#9583 is not susceptible to it,
so just move the code/if-checks around back in that way, to fix it.

Reviewed-by: Pavel Snajdr <snajpa@snajpa.net>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Original-patch-by: Heitor Alves de Siqueira <halves@canonical.com>
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
Closes openzfs#9602
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants