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

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?