-
-
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 1) Please use ae instead of ar here. The ae value is what will be encoded, not ar. I know that in this particular case the result will be same, but ae used here should be easier understandable for future readers. This will also avoid a possible future problem when someone will change the source code above and the presence of the FATTR4_RDATTR_ERROR_MASK flag will start to differ in both ar and ae.
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 1) What will happen in a case the fs_locations is requested, rdattr_error is not requested and the file system we are requesting attributes for is moved? My understanding of section 8.3.2 is that the READDIR operation should succeed, but I suspect this code will make the whole READDIR operation to fail with NFS4ERR_MOVED in such a case.
7782 nfs: READDIR for referrals should conform to RFC 7530
Review Request #436 — Created April 21, 2017 and submitted
Information | |
---|---|
vgusev | |
illumos-gate | |
master | |
7782 | |
4e81d4e... | |
Reviewers | |
general | |
7782 nfs: READDIR for referrals should conform to RFC 7530
This patch add returning error for whole READDIR operation if rdattr_error wasn't requested and:
- met referral
- any other error occured during getting attributes (it could be permission denied, for instance).The patch intensionally misses first part of 8.3.2: returning success if fs_locations is set.
For more information look at Section 16.24.4:
"In some cases, the server may encounter an error while obtaining the
attributes for a directory entry..."
With Linux 4.8.5 kernel and vSphere-6.3. Both works fine.
For testing:
- Create nfs referral via nfsref: nfsref add somename 192.168.1.1:/data
- Do ls in directory that contains referral.
Linux client sends READDIR with rdattr_error attribute, but vSphere doesn't.
Before patch for vSphere client:
READDIR reply contains directory entry (that is referral) with empty attributes, i.e. w/o previously requested attributes.After pathch for vSphere client:
READDIR reply with error NFS4ERR_MOVED, i.e. whole READDIR operation fails.ADDITIONAL TESTING BY PYNFS WITH patched nfssrv (Revision/Changeset #10) and original nfssrv. As result: no regression errors. Additionally pynfs fslocations test reported better results (more PASS-ed tests).
PATCHED
./testserver.py 10.42.0.15:/codepool/data/test --rundep readdir
INIT st_setclientid.testValid : PASS
LOOKBLK st_lookup.testBlock : PASS
LOOKCHAR st_lookup.testChar : PASS
LOOKFIFO st_lookup.testFifo : PASS
LOOKFILE st_lookup.testFile : PASS
LOOKLINK st_lookup.testLink : PASS
LOOKSOCK st_lookup.testSocket : PASS
MKDIR st_create.testDir : PASS
MKFILE st_open.testOpen : PASS
MODE st_setattr.testMode : PASS
RDDR1 st_readdir.testEmptyDir : PASS
RDDR2 st_readdir.testFirst : PASS
RDDR3 st_readdir.testAttr : PASS
RDDR4 st_readdir.testSubsequent : PASS
RDDR5a st_readdir.testFhLink : PASS
RDDR5b st_readdir.testFhBlock : PASS
RDDR5c st_readdir.testFhChar : PASS
RDDR5f st_readdir.testFhFifo : PASS
RDDR5r st_readdir.testFhFile : PASS
RDDR5s st_readdir.testFhSocket : PASS
RDDR6 st_readdir.testNoFh : PASS
RDDR7 st_readdir.testMaxcountZero : PASS
RDDR8 st_readdir.testMaxcountSmall : FAILURE
READDIR of empty dir with maxcount=16 should return
NFS4_OK, instead got NFS4ERR_TOOSMALL
RDDR9 st_readdir.testWriteOnlyAttributes : PASS
RDDR10 st_readdir.testReservedCookies : FAILURE
READDIR with reserved cookie=1 should return
NFS4ERR_BAD_COOKIE, instead got NFS4_OK
RDDR11 st_readdir.testUnaccessibleDir : PASS
RDDR12 st_readdir.testUnaccessibleDirAttrs : PASS
Command line asked for 27 of 669 tests
Of those: 0 Skipped, 2 Failed, 0 Warned, 25 Passed
PATCHED
./testserver.py 10.42.0.15:/codepool/data/test --usespecial=/codepool/data/test/dir_00 --rundep fslocations
FSLOC1 st_fslocations.testReference : PASS
FSLOC2 st_fslocations.testReference2 : PASS
FSLOC3 st_fslocations.testReference3 : PASS
FSLOC4a st_fslocations.testAttr1a : FAILURE
GETATTR w/o FSLOC or RDATTR_ERROR should return
NFS4ERR_MOVED, instead got NFS4_OK
FSLOC4b st_fslocations.testAttr1b : PASS
FSLOC5a st_fslocations.testAttr2a : FAILURE
GETATTR w/o FSLOC but with RDATTR_ERROR should return
NFS4ERR_MOVED, instead got NFS4_OK
FSLOC5b st_fslocations.testAttr2b : PASS
FSLOC6a st_fslocations.testAttr3a : PASS
FSLOC6b st_fslocations.testAttr3b : PASS
FSLOC7a st_fslocations.testAttr4a : FAILURE
Expected 3 attrs returned, got 5
FSLOC7b st_fslocations.testAttr4b : PASS
FSLOC8a st_fslocations.testAttr5a : FAILURE
Expected 3 attrs returned, got 4
FSLOC8b st_fslocations.testAttr5b : PASS
GATT8 st_getattr.testFSLocations : PASS
LOOKFILE st_lookup.testFile : PASS
Command line asked for 15 of 669 tests
Of those: 0 Skipped, 4 Failed, 0 Warned, 11 Passed----------------------------------------------------------------------------
ORIGINAL
./testserver.py 10.42.0.15:/codepool/data/test --usespecial=/codepool/data/test/dir_00 --rundep fslocations
FSLOC1 st_fslocations.testReference : PASS
FSLOC2 st_fslocations.testReference2 : PASS
FSLOC3 st_fslocations.testReference3 : PASS
FSLOC4a st_fslocations.testAttr1a : FAILURE
GETATTR w/o FSLOC or RDATTR_ERROR should return
NFS4ERR_MOVED, instead got NFS4_OK
FSLOC4b st_fslocations.testAttr1b : FAILURE
READDIR w/o FSLOC or RDATTR_ERROR should return
NFS4ERR_MOVED, instead got NFS4_OK
FSLOC5a st_fslocations.testAttr2a : FAILURE
GETATTR w/o FSLOC but with RDATTR_ERROR should return
NFS4ERR_MOVED, instead got NFS4_OK
FSLOC5b st_fslocations.testAttr2b : FAILURE
Expected 2 attrs returned for file dir_00, got 1
FSLOC6a st_fslocations.testAttr3a : PASS
FSLOC6b st_fslocations.testAttr3b : FAILURE
Expected 3 attrs returned for file dir_00, got 2
FSLOC7a st_fslocations.testAttr4a : FAILURE
Expected 3 attrs returned, got 5
FSLOC7b st_fslocations.testAttr4b : FAILURE
Expected 3 attrs returned for file dir_00, got 1
FSLOC8a st_fslocations.testAttr5a : FAILURE
Expected 3 attrs returned, got 4
FSLOC8b st_fslocations.testAttr5b : FAILURE
Expected 2 attrs returned for file dir_00, got 0
GATT8 st_getattr.testFSLocations : PASS
LOOKFILE st_lookup.testFile : PASS
Command line asked for 15 of 669 tests
Of those: 0 Skipped, 9 Failed, 0 Warned, 6 Passed
ORIGINAL
./testserver.py 10.42.0.15:/codepool/data/dir_test --rundep readdir
INIT st_setclientid.testValid : PASS
LOOKBLK st_lookup.testBlock : PASS
LOOKCHAR st_lookup.testChar : PASS
LOOKFIFO st_lookup.testFifo : PASS
LOOKFILE st_lookup.testFile : PASS
LOOKLINK st_lookup.testLink : PASS
LOOKSOCK st_lookup.testSocket : PASS
MKDIR st_create.testDir : PASS
MKFILE st_open.testOpen : PASS
MODE st_setattr.testMode : PASS
RDDR1 st_readdir.testEmptyDir : PASS
RDDR2 st_readdir.testFirst : PASS
RDDR3 st_readdir.testAttr : PASS
RDDR4 st_readdir.testSubsequent : PASS
RDDR5a st_readdir.testFhLink : PASS
RDDR5b st_readdir.testFhBlock : PASS
RDDR5c st_readdir.testFhChar : PASS
RDDR5f st_readdir.testFhFifo : PASS
RDDR5r st_readdir.testFhFile : PASS
RDDR5s st_readdir.testFhSocket : PASS
RDDR6 st_readdir.testNoFh : PASS
RDDR7 st_readdir.testMaxcountZero : PASS
RDDR8 st_readdir.testMaxcountSmall : FAILURE
READDIR of empty dir with maxcount=16 should return
NFS4_OK, instead got NFS4ERR_TOOSMALL
RDDR9 st_readdir.testWriteOnlyAttributes : PASS
RDDR10 st_readdir.testReservedCookies : FAILURE
READDIR with reserved cookie=1 should return
NFS4ERR_BAD_COOKIE, instead got NFS4_OK
RDDR11 st_readdir.testUnaccessibleDir : PASS
RDDR12 st_readdir.testUnaccessibleDirAttrs : PASS
Command line asked for 27 of 669 tests
Of those: 0 Skipped, 2 Failed, 0 Warned, 25 Passed
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+15 -3) |
Change Summary:
Added fs_locations attribute handling. This version becomes more complicated than v1, but
this is pay for full conforming to RFC 7530.Current pynfs results are fine for readdir (patched code), failed tests are only for getattr() operations:
FSLOC1 st_fslocations.testReference : PASS
FSLOC2 st_fslocations.testReference2 : PASS
FSLOC3 st_fslocations.testReference3 : PASS
FSLOC4a st_fslocations.testAttr1a : FAILURE
GETATTR w/o FSLOC or RDATTR_ERROR should return
NFS4ERR_MOVED, instead got NFS4_OK
FSLOC4b st_fslocations.testAttr1b : PASS
FSLOC5a st_fslocations.testAttr2a : FAILURE
GETATTR w/o FSLOC but with RDATTR_ERROR should return
NFS4ERR_MOVED, instead got NFS4_OK
FSLOC5b st_fslocations.testAttr2b : PASS
FSLOC6a st_fslocations.testAttr3a : PASS
FSLOC6b st_fslocations.testAttr3b : PASS
FSLOC7a st_fslocations.testAttr4a : FAILURE
Expected 3 attrs returned, got 5
FSLOC7b st_fslocations.testAttr4b : PASS
FSLOC8a st_fslocations.testAttr5a : FAILURE
Expected 3 attrs returned, got 4
FSLOC8b st_fslocations.testAttr5b : PASS
Command line asked for 14 of 668 tests
Of those: 1 Skipped, 4 Failed, 0 Warned, 9 Passed============================
Original (not patched) module has result:
FSLOC1 st_fslocations.testReference : PASS
FSLOC2 st_fslocations.testReference2 : PASS
FSLOC3 st_fslocations.testReference3 : PASS
FSLOC4a st_fslocations.testAttr1a : FAILURE
GETATTR w/o FSLOC or RDATTR_ERROR should return
NFS4ERR_MOVED, instead got NFS4_OK
FSLOC4b st_fslocations.testAttr1b : FAILURE
READDIR w/o FSLOC or RDATTR_ERROR should return
NFS4ERR_MOVED, instead got NFS4_OK
FSLOC5a st_fslocations.testAttr2a : FAILURE
GETATTR w/o FSLOC but with RDATTR_ERROR should return
NFS4ERR_MOVED, instead got NFS4_OK
FSLOC5b st_fslocations.testAttr2b : FAILURE
Expected 2 attrs returned for file dir_00, got 1
FSLOC6a st_fslocations.testAttr3a : PASS
FSLOC6b st_fslocations.testAttr3b : FAILURE
Expected 3 attrs returned for file dir_00, got 2
FSLOC7a st_fslocations.testAttr4a : FAILURE
Expected 3 attrs returned, got 5
FSLOC7b st_fslocations.testAttr4b : FAILURE
Expected 3 attrs returned for file dir_00, got 1
FSLOC8a st_fslocations.testAttr5a : FAILURE
Expected 3 attrs returned, got 4
FSLOC8b st_fslocations.testAttr5b : FAILURE
Expected 2 attrs returned for file dir_00, got 0
GATT8 st_getattr.testFSLocations : PASS
LOOKFILE st_lookup.testFile : PASS
Command line asked for 15 of 668 tests
Of those: 0 Skipped, 9 Failed, 0 Warned, 6 Passed
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+114 -133) |
-
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 3) Please add word "Section" between
7530
and8.3.1
. -
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 3) Section 8.3.1 of the RFC does not list the
rdattr_error
attribute. Also, the section 8.3.1 is for GETATTR and VERIFY/NVERIFY only, not for READDIR.Maybe this is just a typo and the comment above should say
8.3.2
instead of8.3.1
? -
-
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 3) The extra empty line is not needed here.
-
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 3) Here we assume that
*bmval
cannot be zero (the caller of this function is not prepared for that). I suggest to addASSERT(*bmval != 0)
after this line. -
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 3) Why is this code no longer needed here?
-
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 3) No, it wasn't typo. Section 8.3.1 describes GETATTR valid attributes for absent filesystem. This is also true for READDIR attributes with a little addon in Section 8.3.2.
Unfortunately there is no direct pointing from READDIR getting attributes to GETATTR attributes in this RFC, and we are rely on robust logic and linux nfs server sources.
rdattr_error is valid attribute and described below in Section 8.3.2. and in READDIR operation.
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 3) It can be zero. No ASSERT is required.
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 3) It is impossible to mount in softlink.
Referral is link. vn_is_nfs_reparse() quickly returns FALSE for non-link vnode.
Therefore if @vp is mountpoint then this is not referral.
-
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 3) I think you and me are on the same page. However, "restricted to those actually available" information from 8.3.2 is as result of 8.3.1. What it why I mentioned "8.3.1" in comment.
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 3) Caller is aware if returned zero *bmval.
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 3) Yes, I think so. Generally, in Linux community it must be prepared as separate patch.
Change Summary:
Changes:
1. Fix remarks from Marcel
2. Fix original code from changing @ar.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+120 -140) |
-
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 4) Please add more descriptive comment about this. Simple reference to section 8.3 in RFC without any other explanation is not enough, I think. Especially, because particularly section 8.3.1 does not apply here.
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 4) Actually, these parts of 8.3.2 applies here:
o If the attribute set requested includes fs_locations, then the fetching of attributes proceeds normally, and no NFS4ERR_MOVED indication is returned even when the rdattr_error attribute is requested. ... o The unavailability of an attribute because of a file system's absence, even one that is ordinarily REQUIRED, does not result in any error indication. The set of attributes returned for the root directory of the absent file system in that case is simply restricted to those actually available.
IOW, for READDIR that contains an absent directory entry we should provide as much attributes as possible for that missing entry, but this line effectively limits the set of provided attributes to those four only without any explanation why. Unfortunately, this is not aligned with section 8.3 of the RFC (although the comment says so).
Change Summary:
Changed description for ABSENT_FS_ATTRS.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+123 -140) |
-
-
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 5) I suggest to change this to something like:
Supported attributes for READDIR for the root of an absent filesystem.
-
Change Summary:
Updated according with Marcel's comments.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+123 -140) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+124 -140) |
-
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revisions 6 - 7) here we do pointer arithmetics, I think, we may have bug there as ptr is uint32_t *
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revisions 6 - 7) we should not need void * casts for pointer (for second argument); but pointer arithmetics for third argument - wouldn't it better to use uintptr_t to do the arithmetics and then cast final result to void *?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+126 -140) |
-
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 8) Misused
goto
, it used just like a do...while loop:do { from: line 628 . . . to: line 1487 } while (!no_space && nents == 0 && !iseofdir);
-
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 8) Another misused goto, it used just to skip over a section of code:
if (ar != 0) { from: line 715 . . . to: line 763 }
-
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 8) The same macro constants used multiple times in this expression:
2xFATTR4_FILES_AVAIL_MASK
,
2xFATTR4_FILES_FREE_MASK
and
2xFATTR4_FILES_TOTAL_MASK
.It seems like a typo-ed
NFS4_FS_ATTR_MASK
macro from nfs4_attr.h:171.
-
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 8) (2.)
The same macro constants used multiple times in this expression:
2xFATTR4_FILES_AVAIL_MASK
,
2xFATTR4_FILES_FREE_MASK
and
2xFATTR4_FILES_TOTAL_MASK
.It seems like a typo-ed
NFS4_FS_ATTR_MASK
macro from nfs4_attr.h:171.
-
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 8) (3.)
The same macro constants used multiple times in this expression:
2xFATTR4_FILES_AVAIL_MASK
,
2xFATTR4_FILES_FREE_MASK
and
2xFATTR4_FILES_TOTAL_MASK
.It seems like a typo-ed
NFS4_FS_ATTR_MASK
macro from nfs4_attr.h:171.
-
-
usr/src/uts/common/fs/nfs/nfs4_srv_readdir.c (Diff revision 8) The comment is a bit unclear with "can be restricted". How about "can have values masked-out" instead. I.e. "@bmval can have values masked-out due to an absent file system."
Change Summary:
Change comment in compliance with Dan's remark.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+127 -140) |
Change Summary:
Improving are related to the referrals:
- Improve FSID handling (in case of referrals) according to the RFC. The same handling is performed in rfs4_fattr4_fsid() function in nfs4_srv_attr.c
- Move 'migrating' logic from nfs4_readdir_getvp() to uppper level to accomplish consistency when nfs4_readdir_getvp failed.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+131 -139) |
-
Per our IRC chat, make sure the tests reconfirm the newest changes, and bonus points for explaining how changeset 10 came to be.
Change Summary:
Added description about testing.
Addionally note that version #10 is got from final overview and I found several things that had to be fixed:
- FSID conforming to RFC (is described in Section 8.3.1, it is related to GETATTR, but FSID handling is applied to READDIR too)
- Improved nfs4_readdir_getvp() since it can return NULL vp as well as when error occured. Thus consider "migrated" as separete case.
Testing Done: |
|
---|
Change Summary:
Minor change to use conversion, i.e. puterrno4(rddirattr_error) because some places in code (old) can set rddirattr_error as system error.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+131 -139) |
Change Summary:
Minor change in "Testing Done" description for Revision/Changeset 10
Testing Done: |
|
---|