Hi Rich, Thanks for taking the time to review the code. I will incorporate the UDFS issue in the new CR that I file against UFS.
-Piyush Rich Brown wrote: > Hi Piyush, > > These changes look correct to me. > > One comment on this bullet item: > > > -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. > > UDFS has the same issue. In fact, the comments say that they return the > same value as UFS. Please make sure this is addressed in the CR that will > be filed for this. > > I'll send some pre-RTI comments in a separate e-mail. > > Rich > > On 06/26/09 11:03, Piyush Shivam wrote: >> >> 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); >> _______________________________________________ >> nfs-discuss mailing list >> nfs-discuss at opensolaris.org
