gNFS: Client cache invalidation with bad fsid

1. Problem:
Couple of issues are seen when NFS-ACL is turned ON. i.e.
i) NFS directory access is too slow, impacting customer workflows
   with ACL
ii)dbench fails with 100 directories.

2. Root cause: Frequent cache invalidation in the client side when ACL
is turned ON with NFS because NFS server getacl() code returns the
wrong fsid to the client.

3. This attr-cache invlaidation triggers the frequent LOOKUP ops for
each file instead of relying on the readdir or readdirp data. As
a result performance gets impacted.

4. In case of dbench workload, the problem is more severe. e.g.

Client side rpcdebug output:
===========================

Dec 16 10:16:53 santosh-3 kernel: NFS:
         nfs_update_inode(0:1b/12061953567282551806 ct=2 info=0x7e7f)
Dec 16 10:16:53 santosh-3 kernel: NFS:
         nfs_fhget(0:1b/12061953567282551806 ct=2)
Dec 16 10:16:53 santosh-3 kernel: <-- nfs_xdev_get_sb() = -116 [splat]
Dec 16 10:16:53 santosh-3 kernel: nfs_do_submount: done
Dec 16 10:16:53 santosh-3 kernel: <-- nfs_do_submount() = ffffffffffffff8c
Dec 16 10:16:53 santosh-3 kernel: <-- nfs_follow_mountpoint() = ffffffffffffff8c
Dec 16 10:16:53 santosh-3 kernel: NFS: dentry_delete(clients/client77, 20008)

As per Jeff Layton, This occurs when the client detects that the fsid on
a filehandle is different from its parent. At that point, it tries to
do a new submount of the new filesystem onto the correct point. It means
client got a superblock reference for the new fs and is now looking to set
up the root of the mount. It calls nfs_get_root to do that, which basically
takes the superblock and a filehandle and returns a dentry.  The problem
here is that the dentry->d_inode you're getting back looks wrong. It's not
a directory as expected -- it's something else. So the client gives up and
tosses back an ESTALE.

Which clearly says that, In getacl() code while it does the stat() call
to get the attrs, it forgets to populate the deviceid or fsid before
going ahead and does getxattr().

FIX:
1. Fill the deviceid in iatt.
2. Do bit more clean up for the confusing part of the code.

NB: Many many thanks to Niels de Vos and Jeff Layton for their
help to debug the issue.

Change-Id: I8d3c2a844c9d1761051a883b5ebaeb84062a11c8
BUG: 1043737
Signed-off-by: Santosh Kumar Pradhan <spradhan@redhat.com>
Reviewed-on: http://review.gluster.org/6523
Reviewed-by: Rajesh Joseph <rjoseph@redhat.com>
Reviewed-by: Niels de Vos <ndevos@redhat.com>
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Vijay Bellur <vbellur@redhat.com>

1 files changed, 25 insertions, 10 deletions

diff --git a/xlators/nfs/server/src/acl3.c b/xlators/nfs/server/src/acl3.c
index 59c7637e3..5286077a8 100644
--- a/xlators/nfs/server/src/acl3.c
+++ b/xlators/nfs/server/src/acl3.c
@@ -66,7 +66,8 @@ nfs3_stat_to_fattr3 (struct iatt *buf);
 #define acl3_validate_gluster_fh(handle, status, errlabel)              \
         do {                                                            \
                 if (!nfs3_fh_validate (handle)) {                       \
-                        status = NFS3ERR_SERVERFAULT;                   \
+                        gf_log (GF_ACL, GF_LOG_ERROR, "Bad Handle");    \
+                        status = NFS3ERR_BADHANDLE;                     \
                         goto errlabel;                                  \
                 }                                                       \
         } while (0)                                                     \
@@ -321,6 +322,7 @@ acl3_stat_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
         getaclreply                     *getaclreply = NULL;
         int                             ret = -1;
         nfs_user_t                      nfu = {0, };
+        uint64_t                        deviceid = 0;
 
         if (!frame->local) {
                 gf_log (GF_ACL, GF_LOG_ERROR, "Invalid argument,"
@@ -336,14 +338,18 @@ acl3_stat_cbk (call_frame_t *frame, void *cookie, xlator_t *this,
                 goto err;
         }
 
-        getaclreply->attr_follows = 1;
+        /* Fill the attrs before xattrs */
+        getaclreply->attr_follows = TRUE;
+        deviceid = nfs3_request_xlator_deviceid (cs->req);
+        nfs3_map_deviceid_to_statdev (buf, deviceid);
         getaclreply->attr = nfs3_stat_to_fattr3 (buf);
-        getaclreply->mask = 0xf;
+        getaclreply->mask = (NFS_ACL|NFS_ACLCNT|NFS_DFACL|NFS_DFACLCNT);
+
         nfs_request_user_init (&nfu, cs->req);
-        ret = nfs_getxattr (cs->nfsx, cs->vol, &nfu, &cs->resolvedloc, NULL, NULL,
-                            acl3_getacl_cbk, cs);
-        if (ret == -1) {
-                stat = nfs3_cbk_errno_status (op_ret, op_errno);
+        ret = nfs_getxattr (cs->nfsx, cs->vol, &nfu, &cs->resolvedloc,
+                            NULL, NULL, acl3_getacl_cbk, cs);
+        if (ret < 0) {
+                stat = nfs3_errno_to_nfsstat3 (-ret);
                 goto err;
         }
         return 0;
@@ -409,6 +415,13 @@ acl3svc_getacl (rpcsvc_request_t *req)
                 rpcsvc_request_seterr (req, GARBAGE_ARGS);
                 goto rpcerr;
         }
+
+        /* Validate ACL mask */
+        if (getaclargs.mask & ~(NFS_ACL|NFS_ACLCNT|NFS_DFACL|NFS_DFACLCNT)) {
+                stat = NFS3ERR_INVAL;
+                goto acl3err;
+        }
+
         fhp = &fh;
         acl3_validate_gluster_fh (&fh, stat, acl3err);
         acl3_map_fh_to_volume (nfs->nfs3state, fhp, req,
@@ -470,11 +483,13 @@ acl3_setacl_resume (void *carg)
         nfs_request_user_init (&nfu, cs->req);
         xattr = dict_new();
         if (cs->aclcount)
-        ret = dict_set_static_bin (xattr, POSIX_ACL_ACCESS_XATTR, cs->aclxattr,
-                                   cs->aclcount * 8 + 4);
+        ret = dict_set_static_bin (xattr, POSIX_ACL_ACCESS_XATTR,
+                                   cs->aclxattr,
+                                   posix_acl_xattr_size (cs->aclcount));
         if (cs->daclcount)
         ret = dict_set_static_bin (xattr, POSIX_ACL_DEFAULT_XATTR,
-                                   cs->daclxattr, cs->daclcount * 8 + 4);
+                                   cs->daclxattr,
+                                   posix_acl_xattr_size (cs->daclcount));
 
         ret = nfs_setxattr (cs->nfsx, cs->vol, &nfu, &cs->resolvedloc, xattr,
                             0, NULL, acl3_setacl_cbk, cs);