From 329e38d4ab5af1a675b4d5651eda983f8a924418 Mon Sep 17 00:00:00 2001 From: Santosh Kumar Pradhan Date: Tue, 17 Dec 2013 08:43:50 +0530 Subject: 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 Reviewed-on: http://review.gluster.org/6523 Reviewed-by: Rajesh Joseph Reviewed-by: Niels de Vos Tested-by: Gluster Build System Reviewed-by: Vijay Bellur --- xlators/nfs/server/src/acl3.c | 35 +++++++++++++++++++++++++---------- xlators/nfs/server/src/acl3.h | 6 ++++++ xlators/nfs/server/src/nfs3-helpers.h | 3 +++ xlators/nfs/server/src/nfs3.h | 4 ++++ 4 files changed, 38 insertions(+), 10 deletions(-) (limited to 'xlators/nfs') diff --git a/xlators/nfs/server/src/acl3.c b/xlators/nfs/server/src/acl3.c index 59c7637e3b4..5286077a891 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 e0e61281a6c..03d626f3e00 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 4de1d5623ee..eada242210d 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 023b394cf5a..e64ef9d1591 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 -- cgit