diff mbox series

NFS: Fix mode bits and nlink count for v4 referral dirs

Message ID 20201006151456.20875-1-ashishsangwan2@gmail.com (mailing list archive)
State New, archived
Headers show
Series NFS: Fix mode bits and nlink count for v4 referral dirs | expand

Commit Message

Ashish Sangwan Oct. 6, 2020, 3:14 p.m. UTC
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@vger.kernel.org
Signed-off-by: Ashish Sangwan <ashishsangwan2@gmail.com>
---
 fs/nfs/nfs4proc.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

Comments

Trond Myklebust Oct. 14, 2020, 6:17 p.m. UTC | #1
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@vger.kernel.org
> Signed-off-by: Ashish Sangwan <ashishsangwan2@gmail.com>
> ---
>  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.
Ashish Sangwan Oct. 14, 2020, 7:09 p.m. UTC | #2
On Wed, Oct 14, 2020 at 11:47 PM Trond Myklebust
<trondmy@hammerspace.com> 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@vger.kernel.org
> > Signed-off-by: Ashish Sangwan <ashishsangwan2@gmail.com>
> > ---
> >  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?
Also, isn't the whole point of the server returning attribute map is
to tell the client which attribute is valid? So, in the case where the
server does not have the required information then it will not return
those attributes and we will fall back to the old behavior.
Whether the server has nlink and mode information is entirely up to
the server implementation. For example, the referral's stat
information could be maintained in a distributed database which can be
accessed from any node in the cluster.

Thanks,
Ashish
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>
Trond Myklebust Oct. 15, 2020, 12:06 p.m. UTC | #3
On Thu, 2020-10-15 at 00:39 +0530, Ashish Sangwan wrote:
> On Wed, Oct 14, 2020 at 11:47 PM Trond Myklebust
> <trondmy@hammerspace.com> 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@vger.kernel.org
> > > Signed-off-by: Ashish Sangwan <ashishsangwan2@gmail.com>
> > > ---
> > >  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. 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?
Chuck Lever III Oct. 15, 2020, 1:36 p.m. UTC | #4
> On Oct 15, 2020, at 8:06 AM, Trond Myklebust <trondmy@hammerspace.com> 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@hammerspace.com> 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@vger.kernel.org
>>>> Signed-off-by: Ashish Sangwan <ashishsangwan2@gmail.com>
>>>> ---
>>>> 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.


--
Chuck Lever
Trond Myklebust Oct. 15, 2020, 1:59 p.m. UTC | #5
On Thu, 2020-10-15 at 09:36 -0400, Chuck Lever wrote:
> > On Oct 15, 2020, at 8:06 AM, Trond Myklebust <
> > trondmy@hammerspace.com> 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@hammerspace.com> 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@vger.kernel.org
> > > > > Signed-off-by: Ashish Sangwan <ashishsangwan2@gmail.com>
> > > > > ---
> > > > > 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?
Chuck Lever III Oct. 15, 2020, 2:08 p.m. UTC | #6
> On Oct 15, 2020, at 9:59 AM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Thu, 2020-10-15 at 09:36 -0400, Chuck Lever wrote:
>>> On Oct 15, 2020, at 8:06 AM, Trond Myklebust <
>>> trondmy@hammerspace.com> 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@hammerspace.com> 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@vger.kernel.org
>>>>>> Signed-off-by: Ashish Sangwan <ashishsangwan2@gmail.com>
>>>>>> ---
>>>>>> 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@mail.gmail.com/


--
Chuck Lever
Chuck Lever III Oct. 15, 2020, 2:28 p.m. UTC | #7
> On Oct 15, 2020, at 9:59 AM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Thu, 2020-10-15 at 09:36 -0400, Chuck Lever wrote:
>>> On Oct 15, 2020, at 8:06 AM, Trond Myklebust <
>>> trondmy@hammerspace.com> 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@hammerspace.com> 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@vger.kernel.org
>>>>>> Signed-off-by: Ashish Sangwan <ashishsangwan2@gmail.com>
>>>>>> ---
>>>>>> 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,

We might be reading the spec differently.

I read that SHOULD NOT as saying the server should not hand out
attributes for the absent file system, not that it shouldn't
hand out attributes at all. My experience at that time was that
servers handed out attributes for the referral object that was
present on that server. That seems to be completely allowed by
the spec language.

But you're correct: it's not relevant to application behavior.
As I said, it's merely to prevent users from deciding the Linux
client is somehow not working right.

We're working around the client's behavior: it doesn't follow
the referral until after a user cd's into that directory. Thus
the typical pattern of

$ ls
$ cd

Gives surprising results.


> 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?


--
Chuck Lever
Ashish Sangwan Oct. 15, 2020, 8:15 p.m. UTC | #8
On Thu, Oct 15, 2020 at 7:38 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
>
>
> > On Oct 15, 2020, at 9:59 AM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> >
> > On Thu, 2020-10-15 at 09:36 -0400, Chuck Lever wrote:
> >>> On Oct 15, 2020, at 8:06 AM, Trond Myklebust <
> >>> trondmy@hammerspace.com> 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@hammerspace.com> 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@vger.kernel.org
> >>>>>> Signed-off-by: Ashish Sangwan <ashishsangwan2@gmail.com>
> >>>>>> ---
> >>>>>> 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.
>

The current patch only intended to fix the UX issue, it has no
practical purpose.
If it's breaking the RFC then I agree that the patch should not be included.
Thanks a lot to both of you for explaining the issue in detail!

> Here is some history.
>
> https://lore.kernel.org/linux-nfs/CAD8zhTAAvTKhp6k0vYRMnhZW5pxjstpBiDKLgoXocfpAXNjKTg@mail.gmail.com/
>
>
> --
> Chuck Lever
>
>
>
diff mbox series

Patch

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,