Hi Piyush, Sorry for late response.
Here are some comments: 1. There is extra space in bug synopsis between "NFSv4" and "and" words. It is also visible in the webrev. 2. What will happen when VOP_PATHCONF() will return 0 in l, resp. in val or in pc_val? 3. This code is dangerous and/or confusing (assignment in if without a test): 234 if (error = VOP_PATHCONF(vp, _PC_FILESIZEBITS, &pc_val, cr, 235 NULL)) 236 return (error); I know this is out of the scope of your fix, but I would like to see it fixed. Thanks. On Fri, Jun 26, 2009 at 11:03:10AM -0500, 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);
