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

[Ashish Sangwan]

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