Hi Folks, I would like to request a couple of code reviews for the bug that was originally filed as 6224897: NFSV4 server and PCFS filesystem don't like to discuss _PC_FILESIZEBITS. I have attached the initial set of notes and background that I sent in the first round of code review at the bottom. The summary of changes since the first code review is as follows:
-A new bug-synopsis: NFSv4 and v3 support for _PC_FILESIZEBITS is incorrect and incomplete. The rationale is that PCFS started supporting the _PC_FILESIZEBITS as of snv_15 hence the bug has changed since being originally filed. Moreover, in the process of providing a reasonable default maximum file size returned by the server to the client for NFSv4, we noticed several other defects and change of underlying file system behavior since the bug was originally filed. -The NFSv3 and NFSv4 are inconsistent in the value of maximum file size returned to the client. NFSv4 returns twice the maximum file size as NFSv3. This is so because NFSv3 subtracts 1 from the number of bits returned by VOP_PATHCONF, NFSv4 does not. The number of bits returned by VOP_PATHCONF represent the minimum number of bits required to represent the maximum file size as a signed quantity. For example, for a 32 bit file system, the number of bits should be 33 (32 + 1 sign bit). Hence, NFSv3 is doing the right thing and NFSv4 should also do the same. -The maximum file size that should be returned to the client is (2^63 - 1) even on a 64 bit system. This is because of llseek that takes an offset_t as an argument, which is a signed long long, and the maximum number that can be stored in a signed long long is (2^63 - 1). Thus, the VOP interface as a whole is limited to (2^63 - 1) even if the file system is capable of (2^64 - 1) (or more). Hence, for file systems that return 64 bits or more as a result of VOP_PATHCONF call, the server returns the maximum file size of INT_MAX instead of UNIT_MAX. -The error handling in v3 was incorrect. If the VOP_PATHCONF call returns with an error, the server was returning a default file size; all types errors were being interpreted as lack of support for _PC_FILESIZEBITS. The fact is that if the underlying file system does not support _PC_FILESIZEBITS, there is no error returned, but VOP_PATHCONF call sets the number of bits to -1. Comparison with -1 is the precise way to determine the lack of support for _PC_FILESIZEBITS. If there is an error, it should be bailed up. -UFS currently returns 41 as the number of bits required to represent 2TB. This is incorrect as well, since UFS should return 42 (41 +1 sign bit). A separate bug will be filed to track this issue. The latest webrev is at: http://cr.opensolaris.org/~pshivam/6224897_PC_FILESIZEBITS_latest/ The previous webrev is at: http://cr.opensolaris.org/~pshivam/6224897_PC_FILESIZEBITS Thanks, Piyush Background notes from the earlier mail: The NFSv4 server wants to determine the maximum file size of the underlying file system, e.g., at the client mount time. The server uses VOP_PATHCONF(_PC_FILESIZEBITS) call to do so. The current server implementation does not handle the case where the underlying file system does not support _PC_FILESIZEBITS pathconf query. The problem was first reported against PCFS. However, the problem no longer occurs with PCFS, since it supports _PC_FILESIZEBITS since snv_15. Nevertheless, NFSv4 needs to be more robust to the missing support for maximum file size from the underlying file system, and return a reasonable default. The current implementation seems to be doing the right thing accidentally (as far as I can tell). In the following check, the value of the variable val is -1 when the _PC_FILESIZEBITS is not supported by the underlying file system. However, since val is an unsigned long, the "if" condition evaluates to true, and UINT64_MAX is the default value for maxfilesize. 1573 error = VOP_PATHCONF(sarg->cs->vp, _PC_FILESIZEBITS, &val, 1574 sarg->cs->cr, NULL); 1575 if (error) 1576 break; 1577 if (val >= (sizeof (uint64_t) * 8)) 1578 na->maxfilesize = UINT64_MAX; 1579 else 1580 na->maxfilesize = ((1LL << val) - 1);
