Marcel,
Thanks for taking a look at the code.
Frank: Thanks for already responding to some of the comments. :)
A few more responses below, some of which may overlap with Frank's response.
On 06/16/09 00:31, Marcel Telka wrote:
> Hi Piyush,
>
> nfs4_srv_attr.c
> ---------------
>
> 1) val is ulong_t. You cannot compare it with -1:
>
> 1554 ulong_t val;
> 1582 if (val == -1) {
> 1603 if (val == -1) {
>
It does look tricky. Like Frank explained, I am trying to distinguish
between the case when the underlying file system does not support
_PC_FILESIZEBITS versus a general failure of the VOP_PATHCONF call,
which may happen for a variety of reasons. The variable val is set to
-1 (see below) to indicate the failure of the underlying file system to
support _PC_FILESIZEBITS. Please take a look at the comments (copied
below) in fs_pathconf - the default function called for pathconf queries
when a particular query is not supported.
fs_pathconf(...)
{
...
case _PC_FILESIZEBITS:
/*
* If ever we come here it means that underlying file system
* does not recognise the command and therefore this
* configurable limit cannot be determined. We return -1
* and don't change errno.
*/
val = (ulong_t)-1; /* large file support */
break;
...
}
I spent some time staring at the registers in MDB on SPARC as well as
INTEL to make sure that the code is doing what I expect it to do. -1
gets coded as 0xFFFFFFFF, and the right thing happens.
If you have another idea to make that distinction, I will be happy to
consider it.
>
> 2)
>
> 1583 na->maxfilesize = MAXOFF32_T;
> 1604 maxfilesize = MAXOFF32_T;
>
> Why MAXOFF32_T and not MAXOFFSET_T or MAXOFF_T or ...? Could you elaborate,
> please? When _PC_FILESIZEBITS is supported you could return up to UINT64_MAX,
> which is a _bit_ bigger than MAXOFF32_T.
>
I don't have any strong opinion on the default size that is being
returned. Initially, when I started looking at the bug, and saw that the
use case was PCFS (not that it is the only file system where NFS may
have to return a reasonable default max file size limit), I realized
that for FAT32 file system, the maximum filesize is 4g -1, so 2g seemed
reasonable. Then I went and looked at what the NFSv3 code did, and found
the following:
rfs3_fsinfo(..)
{
..
/*
* Large file spec: want maxfilesize based on limit of
* underlying filesystem. We can guess 2^31-1 if need be.
*/
error = VOP_PATHCONF(vp, _PC_FILESIZEBITS, &l, cr, NULL);
if (!error && l != 0 && l <= 64)
resp->resok.maxfilesize = (1LL << (l-1)) - 1;
else
resp->resok.maxfilesize = MAXOFF32_T;
..
}
I did not see a need to make the default max file size inconsistent
between NFSv3 and NFSv4. Of course, the above code may be quite old. If
someone feels strongly that we need to make the default size UINT64_MAX,
then I would suggest that we should change the default across V3 as
well, unless we have a good case not to.
>
> 3) Why there is comment in the NFS4ATTR_GETIT case only? I am missing some
> comment in the NFS4ATTR_VERIT case.
>
I did consider putting that comment in the VERIT case as well. However,
I saw that the first set of comments is just a few lines above it, and I
did not want to duplicate comments that are less than 15 lines or so
apart. I can add comments in nfs4_srv_readdir.c if you think that will help.
> 4) When _PC_FILESIZEBITS is not supported VOP_PATHCONF() will fail and return
> EINVAL (according to the comments section in the CR), so the codepath will
> break at line 1574 and your fix won't be used to salvage the situation:
>
> 1571 error = VOP_PATHCONF(sarg->cs->vp, _PC_FILESIZEBITS,
> &val,
> 1572 sarg->cs->cr, NULL);
> 1573 if (error)
> 1574 break;
> 1575
> 1576 /*
> 1577 * If the underlying file system does not support
> 1578 * _PC_FILESIZEBITS, return a reasonable default. Note
> that
> 1579 * error code on VOP_PATHCONF will be 0, even if the
> underlying
> 1580 * file system does not support _PC_FILESIZEBITS.
> 1581 */
> 1582 if (val == -1) {
> 1583 na->maxfilesize = MAXOFF32_T;
> 1584 } else {
> 1585 if (val >= (sizeof (uint64_t) * 8))
> 1586 na->maxfilesize = UINT64_MAX;
> 1587 else
> 1588 na->maxfilesize = ((1LL << val) - 1);
> 1589 }
>
I believe that Frank has addressed this at length. Please let me know if
they is still any confusion.
Thanks,
Piyush
> On Mon, Jun 15, 2009 at 05:03:26PM -0500, Piyush Shivam wrote:
>
>> Hi All,
>>
>> I would like to request a code review for:
>>
>> 6224897 NFSV4 server and PCFS filesystem don't like to discuss
>> _PC_FILESIZEBITS
>>
>> The CR is at:
>> http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=6224897
>>
>> The webrev is at:
>> http://cr.opensolaris.org/~pshivam/6224897_PC_FILESIZEBITS/
>>
>>
>> Background:
>>
>> 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);
>>
>
>
