Re: [PATCH] NFS: Fix mode bits and nlink count for v4 referral dirs

[Chuck Lever]

> On Oct 15, 2020, at 9:59 AM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:
> 
> On Thu, 2020-10-15 at 09:36 -0400, Chuck Lever wrote:
>>> On Oct 15, 2020, at 8:06 AM, Trond Myklebust <
>>> trondmy@xxxxxxxxxxxxxxx> wrote:
>>> 
>>> On Thu, 2020-10-15 at 00:39 +0530, Ashish Sangwan wrote:
>>>> On Wed, Oct 14, 2020 at 11:47 PM Trond Myklebust
>>>> <trondmy@xxxxxxxxxxxxxxx> wrote:
>>>>> On Tue, 2020-10-06 at 08:14 -0700, Ashish Sangwan wrote:
>>>>>> Request for mode bits and nlink count in the
>>>>>> nfs4_get_referral
>>>>>> call
>>>>>> and if server returns them use them instead of hard coded
>>>>>> values.
>>>>>> 
>>>>>> CC: stable@xxxxxxxxxxxxxxx
>>>>>> Signed-off-by: Ashish Sangwan <ashishsangwan2@xxxxxxxxx>
>>>>>> ---
>>>>>> fs/nfs/nfs4proc.c | 20 +++++++++++++++++---
>>>>>> 1 file changed, 17 insertions(+), 3 deletions(-)
>>>>>> 
>>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>>>> index 6e95c85fe395..efec05c5f535 100644
>>>>>> --- a/fs/nfs/nfs4proc.c
>>>>>> +++ b/fs/nfs/nfs4proc.c
>>>>>> @@ -266,7 +266,9 @@ const u32 nfs4_fs_locations_bitmap[3] = {
>>>>>>     | FATTR4_WORD0_FSID
>>>>>>     | FATTR4_WORD0_FILEID
>>>>>>     | FATTR4_WORD0_FS_LOCATIONS,
>>>>>> -     FATTR4_WORD1_OWNER
>>>>>> +     FATTR4_WORD1_MODE
>>>>>> +     | FATTR4_WORD1_NUMLINKS
>>>>>> +     | FATTR4_WORD1_OWNER
>>>>>>     | FATTR4_WORD1_OWNER_GROUP
>>>>>>     | FATTR4_WORD1_RAWDEV
>>>>>>     | FATTR4_WORD1_SPACE_USED
>>>>>> @@ -7594,16 +7596,28 @@ nfs4_listxattr_nfs4_user(struct inode
>>>>>> *inode,
>>>>>> char *list, size_t list_len)
>>>>>> */
>>>>>> static void nfs_fixup_referral_attributes(struct nfs_fattr
>>>>>> *fattr)
>>>>>> {
>>>>>> +     bool fix_mode = true, fix_nlink = true;
>>>>>> +
>>>>>>     if (!(((fattr->valid & NFS_ATTR_FATTR_MOUNTED_ON_FILEID)
>>>>>> ||
>>>>>>            (fattr->valid & NFS_ATTR_FATTR_FILEID)) &&
>>>>>>           (fattr->valid & NFS_ATTR_FATTR_FSID) &&
>>>>>>           (fattr->valid & NFS_ATTR_FATTR_V4_LOCATIONS)))
>>>>>>             return;
>>>>>> 
>>>>>> +     if (fattr->valid & NFS_ATTR_FATTR_MODE)
>>>>>> +             fix_mode = false;
>>>>>> +     if (fattr->valid & NFS_ATTR_FATTR_NLINK)
>>>>>> +             fix_nlink = false;
>>>>>>     fattr->valid |= NFS_ATTR_FATTR_TYPE |
>>>>>> NFS_ATTR_FATTR_MODE |
>>>>>>             NFS_ATTR_FATTR_NLINK |
>>>>>> NFS_ATTR_FATTR_V4_REFERRAL;
>>>>>> -     fattr->mode = S_IFDIR | S_IRUGO | S_IXUGO;
>>>>>> -     fattr->nlink = 2;
>>>>>> +
>>>>>> +     if (fix_mode)
>>>>>> +             fattr->mode = S_IFDIR | S_IRUGO | S_IXUGO;
>>>>>> +     else
>>>>>> +             fattr->mode |= S_IFDIR;
>>>>>> +
>>>>>> +     if (fix_nlink)
>>>>>> +             fattr->nlink = 2;
>>>>>> }
>>>>>> 
>>>>>> static int _nfs4_proc_fs_locations(struct rpc_clnt *client,
>>>>>> struct
>>>>>> inode *dir,
>>>>> 
>>>>> NACK to this patch. The whole point is that if the server has a
>>>>> referral, then it is not going to give us any attributes other
>>>>> than
>>>>> the
>>>>> ones we're already asking for because it may not even have a
>>>>> real
>>>>> directory. The client is required to fake up an inode, hence
>>>>> the
>>>>> existing code.
>>>> 
>>>> Hi Trond, thanks for reviewing the patch!
>>>> Sorry but I didn't understand the reason to NACK it. Could you
>>>> please
>>>> elaborate your concern?
>>>> These are the current attributes we request from the server on a
>>>> referral:
>>>> FATTR4_WORD0_CHANGE
>>>>> FATTR4_WORD0_SIZE
>>>>> FATTR4_WORD0_FSID
>>>>> FATTR4_WORD0_FILEID
>>>>> FATTR4_WORD0_FS_LOCATIONS,
>>>> FATTR4_WORD1_OWNER
>>>>> FATTR4_WORD1_OWNER_GROUP
>>>>> FATTR4_WORD1_RAWDEV
>>>>> FATTR4_WORD1_SPACE_USED
>>>>> FATTR4_WORD1_TIME_ACCESS
>>>>> FATTR4_WORD1_TIME_METADATA
>>>>> FATTR4_WORD1_TIME_MODIFY
>>>>> FATTR4_WORD1_MOUNTED_ON_FILEID,
>>>> 
>>>> So you are suggesting that it's ok to ask for SIZE, OWNER, OWNER
>>>> GROUP, SPACE USED, TIMESTAMPs etc but not ok to ask for mode bits
>>>> and
>>>> numlinks?
>>> 
>>> No. We shouldn't be asking for any of that information for a
>>> referral
>>> because the server isn't supposed to return any values for it.
>>> 
>>> Chuck and Anna, what's the deal with commit c05cefcc7241? That
>>> appears
>>> to have changed the original code to speculatively assume that the
>>> server will violate RFC5661 Section 11.3.1 and/or RFC7530 Section
>>> 8.3.1.
>> 
>> The commit is an attempt to address the many complaints we've had
>> about the ugly appearance of referral anchors. The strange "special"
>> default values made the client appear to be broken, and was confusing
>> to some. I consider this to be a UX issue: the information displayed
>> in this case is not meant to be factual, but rather to prevent the
>> user concluding that something is wrong.
>> 
>> I'm not attached to this particular solution, though. Does it make
>> sense to perform the referral mount before returning "ls" results
>> so that the target server has a chance to supply reasonable
>> attribute values for the mounted-on directory object? Just spit
>> balling here.
>> 
>> 
>>> Specifically, the paragraph that says:
>>> 
>>> "
>>>  Other attributes SHOULD NOT be made available for absent file
>>>  systems, even when it is possible to provide them.  The server
>>> should
>>>  not assume that more information is always better and should
>>> avoid
>>>  gratuitously providing additional information."
>>> 
>>> So why is the client asking for them?
>> 
>> This paragraph (and it's most modern incarnation in RFC 8881 Section
>> 11.4.1) describes server behavior. The current client behavior is
>> spec-compliant because there is no explicit prohibition in the spec
>> language against a client requesting additional attributes in this
>> case.
>> 
>> Either the server can clear those bitmap flags on the GETATTR reply
>> and not supply those attributes, and clients must be prepared for
>> that.
>> 
>> Or, it's also possible to read this paragraph to mean that the
>> server can provide those attributes and the values should not
>> reflect attributes for the absent file system, but rather something
>> else (eg, server-manufactured defaults, or the attributes from the
>> object on the source server).
>> 
>> And since this is a SHOULD NOT rather than a MUST NOT, servers are
>> still free to return information about the absent file system.
>> Clients are not guaranteed this will be the case, however.
>> 
>> I don't think c05cefcc7241 makes any assumption about whether the
>> server is lying about the extra attributes. Perhaps the server has
>> no better values for these attributes than the client's defaults
>> were.
>> 
> 
> SHOULD / SHOULD NOT indicates actions that the server is required to
> take in the absence of a very good reason to do otherwise. In other
> words, the client should expect the majority of servers to behave in a
> certain manner.
> 
> It doesn't matter that the client's behaviour is spec compliant. We're
> asking for information that is not supposed to be divulged by the
> majority of servers, Furthermore, that information is, quite frankly,
> utterly irrelevant to the client and application running on it. Any
> attempt to access that fake object will result in a submount of
> something completely different on top of that object.
> 
> IOW: the only difference here is you're asking that the server provide
> us with a faked up object (which it is not supposed to do), whereas
> previously, we were faking that object up ourselves. What's the big
> deal here?

Right, that boils it down nicely.

The difference has been that by and large the server-provided values
don't look broken to users. Perhaps all we need to do is select better
defaults for these attributes on Linux clients. I haven't followed
Ashish's requirements, so I can't speak to them.

Here is some history.

https://lore.kernel.org/linux-nfs/CAD8zhTAAvTKhp6k0vYRMnhZW5pxjstpBiDKLgoXocfpAXNjKTg@xxxxxxxxxxxxxx/


--
Chuck Lever