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

Remove zpl_revalidate #9549

Merged
merged 1 commit into from
Nov 11, 2019
Merged

Remove zpl_revalidate #9549

merged 1 commit into from
Nov 11, 2019

Conversation

snajpa
Copy link
Contributor

@snajpa snajpa commented Nov 4, 2019

This patch removes the need for zpl_revalidate altogether.

There were 3 main reasons why we used d_revalidate:

  1. periodic automounted snapshots umount deferral
  2. negative dentries created before snapshot rollback
  3. stale inodes referenced by dentry cache after snapshot rollback

Periodic snapshots deferral solution introduces zfs_exit_fs function, which
is called as a part of ZFS_EXIT(zfsvfs_t) macro.

Negative dentries and stale inodes are solved by flushing the dcache for the
particular dataset on zfs_resume_fs call.

This patch also removes now unused HAVE_S_D_OP configure test.

Fixes #8774

Signed-off-by: Pavel Snajdr [email protected]

How Has This Been Tested?

Locally & together with PR #9414

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:

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Nov 6, 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.

Thanks for splitting this out and addressing the review feedback. This looks good to me after a rebase, and ends up being a nice simplification which is great.

#define ZFS_EXIT(zfsvfs) \
do { \
zfs_exit_fs(zfsvfs); \
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Sorry to make you rebase this again, but as part of the ongoing refactoring we've moved these macros in to a platform specific include/os/linux/zfs/sys/zfs_znode_impl.h header. One nice upshot of this is it does provide us more flexibility in adding Linux specific logic to the macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@behlendorf behlendorf requested a review from tcaputi November 6, 2019 19:22
@behlendorf
Copy link
Contributor

@cyphar would you mind also reviewing this part of the larger overlayfs work.

This patch removes the need for zpl_revalidate altogether.

There were 3 main reasons why we used d_revalidate:

1. periodic automounted snapshots umount deferral
2. negative dentries created before snapshot rollback
3. stale inodes referenced by dentry cache after snapshot rollback

Periodic snapshots deferral solution introduces zfs_exit_fs function,
which is called as a part of ZFS_EXIT(zfsvfs_t) macro.

Negative dentries and stale inodes are solved by flushing the dcache
for the particular dataset on zfs_resume_fs call.

This patch also removes now unused HAVE_S_D_OP configure test.

Fixes openzfs#8774

Signed-off-by: Pavel Snajdr <[email protected]>
@codecov
Copy link

codecov bot commented Nov 7, 2019

Codecov Report

Merging #9549 into master will decrease coverage by 0.29%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #9549     +/-   ##
=========================================
- Coverage   79.35%   79.06%   -0.3%     
=========================================
  Files         418      418             
  Lines      123686   123680      -6     
=========================================
- Hits        98157    97788    -369     
- Misses      25529    25892    +363
Flag Coverage Δ
#kernel 79.7% <66.66%> (-0.08%) ⬇️
#user 66.84% <ø> (-0.68%) ⬇️

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 734de7c...286d2eb. Read the comment docs.

@tcaputi
Copy link
Contributor

tcaputi commented Nov 8, 2019

@behlendorf I see you tagged me here for a review. I can take a look if you want, but to be honest I really don't know much about dentries yet. It might be better if we found someone who knew what they were looking at there.

@behlendorf
Copy link
Contributor

@tcaputi no problem. I just wanted to tag you since I thought you might be interested in the changes related to rollback.

@cyphar
Copy link
Contributor

cyphar commented Nov 10, 2019

👍 Removing ->d_revalidate solves one of the problems on the path to overlay-on-ZFS support. Assuming that the new invalidation handling is correct (it seems right to me, at least on a surface-level), this looks good overall. Thanks @snajpa.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Nov 11, 2019
@behlendorf behlendorf merged commit 5a6ac4c into openzfs:master Nov 11, 2019
@ahrens
Copy link
Member

ahrens commented Nov 14, 2019

I think that this introduced a bug: #9587

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.

remove usage of d_revalidate
5 participants