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.

Sign up for GitHub

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

Jump to bottom

module: zfs: freebsd: vfsops: use setgen for error case #12905

Merged
merged 1 commit into from
Jan 6, 2022
Merged

module: zfs: freebsd: vfsops: use setgen for error case #12905

merged 1 commit into from
Jan 6, 2022

Conversation

nabijaczleweli
Copy link
Contributor

Motivation and Context

https://github.com/nabijaczleweli/zfs/pull/new/lib-ARGSUSED-spinoff-vfsops

Description

Copied from Linux.

How Has This Been Tested?

Not.

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:

@nabijaczleweli nabijaczleweli mentioned this pull request Dec 22, 2021
Closed
13 tasks
@behlendorf behlendorf requested review from amotin and a user December 23, 2021 00:59
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Dec 23, 2021
@behlendorf
Copy link
Contributor

For further context this discrepancy was noticed in #12844 and this PR was opened to get feedback from the FreeBSD developers. Should this code include the same check as the Linux code, or should this unused variable be removed.

amotin
amotin reviewed Dec 23, 2021
View reviewed changes
module/os/freebsd/zfs/zfs_vfsops.c Outdated
@@ -1835,6 +1835,12 @@ zfs_fhtovp(vfs_t *vfsp, fid_t *fidp, int flags, vnode_t **vpp)
return (SET_ERROR(EINVAL));
}

if (fid_gen > 1 || setgen != 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This predictably failed to build since setgen is declared inside the if () scope above. It is indeed not used now and should be always zero, but this check is broken.
Check for fid_gen > 1 looks just wrong to me. I am not sure why it was added here, since it can be 0 for snapshots and must be non-zero for normal dataset files.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was added on the Linux side as an additional sanity check as part of 9b77d1c, which was for a Linux specific issue. I see setgen is similarly unused in the Illumos code. So the question is, is there value in a check along these lines for FreeBSD or would you just rather entirely remove the unused code?

Copy link
Member

Choose a reason for hiding this comment

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

I think checking setgen would be good. It sound potentially dangerous if the same file can be addressed via different file handles.

@nabijaczleweli nabijaczleweli force-pushed the lib-ARGSUSED-spinoff-vfsops branch from 1c455ba to 9edd5f1 Compare December 23, 2021 20:03
@nabijaczleweli
Copy link
Contributor Author

Moved setgen to function scope so it hopefully builds.

@amotin
Copy link
Member

amotin commented Dec 23, 2021

@nabijaczleweli It may build now, but what is about my other comment? And I'd personally move the check in closer to zfsctl_lookup_objset(), where it would check for actual dataste generation, would it be filled there.

@nabijaczleweli
Copy link
Contributor Author

nabijaczleweli commented Dec 23, 2021 •
edited
Loading

This is the first line where both data are available; Linux code fills them in reverse order so it lives closer to zfsctl_snapdir_vget() there (as it needs, since it returns it). And what about it, indeed?

@amotin
Copy link
Member

amotin commented Dec 23, 2021

In zfsctl_snapdir_vget() that condition is inside "if (fidp->fid_len == LONG_FID_LEN)". The way you put it here is plain wrong.

@nabijaczleweli nabijaczleweli force-pushed the lib-ARGSUSED-spinoff-vfsops branch from 9edd5f1 to 05745a4 Compare December 23, 2021 22:25
@nabijaczleweli
Copy link
Contributor Author

Sure, anded with == LONG_FID_LEN

amotin
amotin approved these changes Dec 24, 2021
View reviewed changes
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

I have no more objections.

ghost
ghost reviewed Jan 4, 2022
View reviewed changes
module/os/freebsd/zfs/zfs_vfsops.c Outdated
@@ -1835,6 +1835,12 @@ zfs_fhtovp(vfs_t *vfsp, fid_t *fidp, int flags, vnode_t **vpp)
return (SET_ERROR(EINVAL));
}

if (fidp->fid_len == LONG_FID_LEN && (fid_gen > 1 || setgen != 0)) {
dprintf("snapdir fid: fid_gen (%llu) and setgen (%llu)\n",
fid_gen, setgen);
Copy link

@ghost ghost Jan 4, 2022 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

We've been casting these to u_longlong_t for printing elsewhere in this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed; updated.

@nabijaczleweli nabijaczleweli force-pushed the lib-ARGSUSED-spinoff-vfsops branch from 05745a4 to 35942c3 Compare January 4, 2022 15:18
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jan 5, 2022
@behlendorf behlendorf merged commit 43dbf88 into openzfs:master Jan 6, 2022
mcmilk pushed a commit to mcmilk/zfs that referenced this pull request Jan 7, 2022
@nabijaczleweli @mcmilk
FreeBSD: vfsops: use setgen for error case
6481137 
Fix from openzfs#12844 (comment)

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#12905
ghost pushed a commit to truenas/zfs that referenced this pull request Jan 19, 2022
@nabijaczleweli
FreeBSD: vfsops: use setgen for error case
c3620a3 
Fix from openzfs#12844 (comment)

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#12905
ghost pushed a commit to truenas/zfs that referenced this pull request Feb 2, 2022
@nabijaczleweli
FreeBSD: vfsops: use setgen for error case
c1700fc 
Fix from openzfs#12844 (comment)

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#12905
tonyhutter pushed a commit that referenced this pull request Feb 3, 2022
@nabijaczleweli @tonyhutter
FreeBSD: vfsops: use setgen for error case
36a91d6 
Fix from #12844 (comment)

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes #12905
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
@nabijaczleweli @nicman23
FreeBSD: vfsops: use setgen for error case
335e110 
Fix from openzfs#12844 (comment)

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#12905
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
@nabijaczleweli @nicman23
FreeBSD: vfsops: use setgen for error case
1c777b1 
Fix from openzfs#12844 (comment)

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@ixsystems.com>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#12905
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amotin amotin amotin approved these changes

@behlendorf behlendorf behlendorf approved these changes

Assignees

@tonynguien tonynguien

Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nabijaczleweli @behlendorf @amotin @tonynguien