Hi Piyush, I like the changes. Push! :-)
Jeff On Jun 26, 2009, at 11:03 AM, 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
