> 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
