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: I44d8d2fa3ec7fb33a67dfdd4bbe2c45cdf67db8c
BUG: 1043737
Signed-off-by: Santosh Kumar Pradhan <spradhan@redhat.com>
Reviewed-on: http://review.gluster.org/6526
Reviewed-by: Niels de Vos <ndevos@redhat.com>
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Vijay Bellur <vbellur@redhat.com>

4 files changed, 38 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);
diff --git a/xlators/nfs/server/src/acl3.h b/xlators/nfs/server/src/acl3.h
index e0e61281a..03d626f3e 100644
--- a/xlators/nfs/server/src/acl3.h
+++ b/xlators/nfs/server/src/acl3.h
@@ -16,6 +16,12 @@
 #define GF_ACL3_PORT            38469
 #define GF_ACL                  GF_NFS"-ACL"
 
+/* Flags for the getacl/setacl mode */
+#define NFS_ACL                 0x0001
+#define NFS_ACLCNT              0x0002
+#define NFS_DFACL               0x0004
+#define NFS_DFACLCNT            0x0008
+
 /*
  * NFSv3, identifies the default ACL by NFS_ACL_DEFAULT. Gluster
  * NFS needs to mask it OFF before sending it upto POSIX layer
diff --git a/xlators/nfs/server/src/nfs3-helpers.h b/xlators/nfs/server/src/nfs3-helpers.h
index 4de1d5623..eada24221 100644
--- a/xlators/nfs/server/src/nfs3-helpers.h
+++ b/xlators/nfs/server/src/nfs3-helpers.h
@@ -334,4 +334,7 @@ nfs3_is_parentdir_entry (char *entry);
 uint32_t
 nfs3_request_to_accessbits (int32_t accbits);
 
+void
+nfs3_map_deviceid_to_statdev (struct iatt *ia, uint64_t deviceid);
+
 #endif
diff --git a/xlators/nfs/server/src/nfs3.h b/xlators/nfs/server/src/nfs3.h
index 023b394cf..e64ef9d15 100644
--- a/xlators/nfs/server/src/nfs3.h
+++ b/xlators/nfs/server/src/nfs3.h
@@ -280,4 +280,8 @@ nfs3svc_init (xlator_t *nfsx);
 
 extern int
 nfs3_reconfigure_state (xlator_t *nfsx, dict_t *options);
+
+extern uint64_t
+nfs3_request_xlator_deviceid (rpcsvc_request_t *req);
+
 #endif