[RFC patch hekafs] Do not return -1 from STACK_WIND targets ever

[Pete Zaitcev]

My hekafs server was misconfigured and some functions failed in places
where they normally would not, and that uncovered a problem: whenever
that happened, the client would hang. This happens because we cannot
return errors like normal people do in the kernel, by throwing an
error code. Functions called through STACK_WIND must return zero and
report the error through the callback.

Not sure what errno to set in this case. Generally I just want to
indicate that "something is busted". In the past I would return EDOM.
The ENOSPC seems ridiculous enough here.

Comments?

---
commit f8d58238c449568e05f71bd29816790e4a38ba4c
Author: Pete Zaitcev <zaitcev(a)yahoo.com&gt;
Date:   Wed Nov 9 16:16:27 2011 -0700

    Prevent a hang in case of errors in fops functions.

diff --git a/xlators/features/uidmap/src/uidmap.c b/xlators/features/uidmap/src/uidmap.c
index 91104fd..191fb57 100644
--- a/xlators/features/uidmap/src/uidmap.c
+++ b/xlators/features/uidmap/src/uidmap.c
@@ -34,6 +34,9 @@
 #include "common-utils.h"
 #include "uidmap.h"
 
+/* ugly #includes below */
+#include <errno.h>
+
 #define HFS_UID_LOW_DEFAULT 10000
 #define HFS_UID_HIGH_DEFAULT 19999
 #define HFS_GID_LOW_DEFAULT 10000
@@ -1350,7 +1353,8 @@ uidmap_entrylk(call_frame_t *frame, xlator_t *this,
 		   ((type == ENTRYLK_RDLCK) ? "ENTRYLK_RDLCK" : "ENTRYLK_WRLCK"));
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(entrylk, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_entrylk_cbk,
@@ -1373,7 +1377,8 @@ uidmap_fentrylk(call_frame_t *frame, xlator_t *this,
 		   ((type == ENTRYLK_RDLCK) ? "ENTRYLK_RDLCK" : "ENTRYLK_WRLCK"));
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(entrylk, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_fentrylk_cbk,
@@ -1389,7 +1394,8 @@ uidmap_inodelk(call_frame_t *frame, xlator_t *this, const char
*volume,
 		  loc_t *loc, int32_t cmd, struct gf_flock *flock)
 {
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(inodelk, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_inodelk_cbk,
@@ -1409,7 +1415,8 @@ uidmap_finodelk(call_frame_t *frame, xlator_t *this, const char
*volume,
 		   frame->root->unique, volume, fd, cmd);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(finodelk, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_finodelk_cbk,
@@ -1429,7 +1436,8 @@ uidmap_xattrop(call_frame_t *frame, xlator_t *this, loc_t *loc,
 		   frame->root->unique, loc->path, loc->inode->ino, flags);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(xattrop, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_xattrop_cbk,
@@ -1450,7 +1458,8 @@ uidmap_fxattrop(call_frame_t *frame, xlator_t *this, fd_t *fd,
 		   frame->root->unique, fd, flags);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(fxattrop, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_fxattrop_cbk,
@@ -1473,7 +1482,9 @@ uidmap_lookup(call_frame_t *frame, xlator_t *this,
 		   loc->inode->ino);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(lookup, frame, -1, ENOSPC,
+					NULL, NULL, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_lookup_cbk,
@@ -1493,7 +1504,8 @@ uidmap_stat(call_frame_t *frame, xlator_t *this, loc_t *loc)
 		   frame->root->unique, loc->path, loc->inode->ino);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(stat, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_stat_cbk,
@@ -1514,7 +1526,8 @@ uidmap_rchecksum(call_frame_t *frame, xlator_t *this, fd_t *fd,
 		   frame->root->unique, fd->inode->ino);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(rchecksum, frame, -1, ENOSPC, 0, 0);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_rchecksum_cbk,
@@ -1535,7 +1548,8 @@ uidmap_getspec(call_frame_t *frame, xlator_t *this, const char
*key,
 		   frame->root->unique, key, flag);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(getspec, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_getspec_cbk,
@@ -1555,7 +1569,8 @@ uidmap_readlink(call_frame_t *frame, xlator_t *this, loc_t *loc,
size_t size)
 		   frame->root->unique, loc->path, loc->inode->ino, size);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(readlink, frame, -1, ENOSPC, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_readlink_cbk,
@@ -1576,7 +1591,9 @@ uidmap_mknod(call_frame_t *frame, xlator_t *this, loc_t *loc,
 		   frame->root->unique, loc->path, loc->inode->ino, mode, dev);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(mknod, frame, -1, ENOSPC, NULL, NULL,
+					NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_mknod_cbk,
@@ -1598,7 +1615,9 @@ uidmap_mkdir(call_frame_t *frame, xlator_t *this, loc_t *loc, mode_t
mode,
 		   ((loc->inode)? loc->inode->ino : 0), mode);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(mkdir, frame, -1, ENOSPC,
+					NULL, NULL, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_mkdir_cbk,
@@ -1617,7 +1636,8 @@ uidmap_unlink(call_frame_t *frame, xlator_t *this, loc_t *loc)
 		   frame->root->unique, loc->path, loc->inode->ino);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(unlink, frame, -1, ENOSPC, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_unlink_cbk,
@@ -1636,7 +1656,8 @@ uidmap_rmdir(call_frame_t *frame, xlator_t *this, loc_t *loc, int
flags)
 		   frame->root->unique, loc->path, loc->inode->ino, flags);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(rmdir, frame, -1, ENOSPC, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_rmdir_cbk,
@@ -1658,7 +1679,9 @@ uidmap_symlink(call_frame_t *frame, xlator_t *this, const char
*linkpath,
 		   ((loc->inode)? loc->inode->ino : 0));
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(symlink, frame, -1, ENOSPC,
+					NULL, NULL, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_symlink_cbk,
@@ -1680,7 +1703,9 @@ uidmap_rename(call_frame_t *frame, xlator_t *this, loc_t *oldloc,
loc_t *newloc)
 		   newloc->path, newloc->ino);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(rename, frame, -1, ENOSPC,
+					NULL, NULL, NULL, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_rename_cbk,
@@ -1702,7 +1727,9 @@ uidmap_link(call_frame_t *frame, xlator_t *this, loc_t *oldloc,
loc_t *newloc)
 		   newloc->path, newloc->inode->ino);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(link, frame, -1, ENOSPC,
+				NULL, NULL, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_link_cbk,
@@ -1727,7 +1754,9 @@ uidmap_setattr(call_frame_t *frame, xlator_t *this, loc_t *loc,
 		scratch_cs.gid = stbuf->ia_gid;
 		if (((*uidmap_map)(frame->root, this->name) == -1) ||
 		    ((*uidmap_map)(&scratch_cs, this->name) == -1)) {
-			return -1;
+			STACK_UNWIND_STRICT(setattr, frame, -1, ENOSPC,
+						NULL, NULL);
+			return 0;
 		}
 		stbuf->ia_uid = scratch_cs.uid;
 		stbuf->ia_gid = scratch_cs.gid;
@@ -1756,7 +1785,9 @@ uidmap_fsetattr(call_frame_t *frame, xlator_t *this, fd_t *fd,
 		scratch_cs.gid = stbuf->ia_gid;
 		if (((*uidmap_map)(frame->root, this->name) == -1) ||
 		    ((*uidmap_map)(&scratch_cs, this->name) == -1)) {
-			return -1;
+			STACK_UNWIND_STRICT(fsetattr, frame, -1, ENOSPC,
+						NULL, NULL);
+			return 0;
 		}
 		stbuf->ia_uid = scratch_cs.uid;
 		stbuf->ia_gid = scratch_cs.gid;
@@ -1780,7 +1811,8 @@ uidmap_truncate(call_frame_t *frame, xlator_t *this, loc_t *loc,
 		   frame->root->unique, loc->path, loc->inode->ino, offset);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(truncate, frame, -1, ENOSPC, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_truncate_cbk,
@@ -1803,7 +1835,8 @@ uidmap_open(call_frame_t *frame, xlator_t *this, loc_t *loc,
 		   fd, wbflags);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(open, frame, -1, ENOSPC, 0);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_open_cbk,
@@ -1823,7 +1856,9 @@ uidmap_create(call_frame_t *frame, xlator_t *this, loc_t *loc,
 		   frame->root->unique, loc->path, loc->inode->ino, flags, mode);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(create, frame, -1, ENOSPC,
+					0, NULL, NULL, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_create_cbk,
@@ -1843,7 +1878,9 @@ uidmap_readv(call_frame_t *frame, xlator_t *this, fd_t *fd,
 		   frame->root->unique, fd, size, offset);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(readv, frame, -1, ENOSPC,
+					NULL, 0, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_readv_cbk,
@@ -1864,7 +1901,8 @@ uidmap_writev(call_frame_t *frame, xlator_t *this, fd_t *fd,
 		   frame->root->unique, fd, vector, count, offset);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(writev, frame, -1, ENOSPC, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_writev_cbk,
@@ -1884,7 +1922,8 @@ uidmap_statfs(call_frame_t *frame, xlator_t *this, loc_t *loc)
 		   ((loc->inode)? loc->inode->ino : 0));
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(statfs, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_statfs_cbk,
@@ -1902,7 +1941,8 @@ uidmap_flush(call_frame_t *frame, xlator_t *this, fd_t *fd)
 		   "%"PRId64": (*fd=%p)", frame->root->unique, fd);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(flush, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_flush_cbk,
@@ -1920,7 +1960,8 @@ uidmap_fsync(call_frame_t *frame, xlator_t *this, fd_t *fd, int32_t
flags)
 		   "%"PRId64": (flags=%d, *fd=%p)", frame->root->unique,
flags, fd);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(fsync, frame, -1, ENOSPC, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_fsync_cbk,
@@ -1941,7 +1982,8 @@ uidmap_setxattr(call_frame_t *frame, xlator_t *this,
 		   ((loc->inode)? loc->inode->ino : 0), dict, flags);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(setxattr, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_setxattr_cbk,
@@ -1962,7 +2004,8 @@ uidmap_getxattr(call_frame_t *frame, xlator_t *this,
 		   ((loc->inode)? loc->inode->ino : 0), name);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(getxattr, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_getxattr_cbk,
@@ -1983,7 +2026,8 @@ uidmap_fsetxattr(call_frame_t *frame, xlator_t *this,
 		   ((fd->inode)? fd->inode->ino : 0), dict, flags);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(fsetxattr, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_fsetxattr_cbk,
@@ -2004,7 +2048,8 @@ uidmap_fgetxattr(call_frame_t *frame, xlator_t *this,
 		   ((fd->inode)? fd->inode->ino : 0), name);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(fgetxattr, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_fgetxattr_cbk,
@@ -2025,7 +2070,8 @@ uidmap_removexattr(call_frame_t *frame, xlator_t *this,
 		   ((loc->inode)? loc->inode->ino : 0), name);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(removexattr, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_removexattr_cbk,
@@ -2045,7 +2091,8 @@ uidmap_opendir(call_frame_t *frame, xlator_t *this, loc_t *loc, fd_t
*fd)
 		   frame->root->unique, loc->path, loc->inode->ino, fd);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(opendir, frame, -1, ENOSPC, 0);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_opendir_cbk,
@@ -2064,7 +2111,8 @@ uidmap_readdirp(call_frame_t *frame, xlator_t *this, fd_t *fd,
size_t size,
 		   frame->root->unique, fd, size, offset);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(readdirp, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_readdirp_cbk,
@@ -2085,7 +2133,8 @@ uidmap_readdir(call_frame_t *frame, xlator_t *this, fd_t *fd,
 		   frame->root->unique, fd, size, offset);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(readdir, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_readdir_cbk,
@@ -2106,7 +2155,8 @@ uidmap_fsyncdir(call_frame_t *frame, xlator_t *this,
 		   frame->root->unique, datasync, fd);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(fsyncdir, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_fsyncdir_cbk,
@@ -2126,7 +2176,8 @@ uidmap_access(call_frame_t *frame, xlator_t *this, loc_t *loc,
int32_t mask)
 		   ((loc->inode)? loc->inode->ino : 0), mask);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(access, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_access_cbk,
@@ -2146,7 +2197,8 @@ uidmap_ftruncate(call_frame_t *frame, xlator_t *this,
 		   frame->root->unique, offset, fd);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(ftruncate, frame, -1, ENOSPC, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_ftruncate_cbk,
@@ -2165,7 +2217,8 @@ uidmap_fstat(call_frame_t *frame, xlator_t *this, fd_t *fd)
 		   "%"PRId64": (*fd=%p)", frame->root->unique, fd);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(fstat, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_fstat_cbk,
@@ -2187,7 +2240,8 @@ uidmap_lk(call_frame_t *frame, xlator_t *this, fd_t *fd,
 		   lock->l_start, lock->l_len, lock->l_pid);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(lk, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_lk_cbk,

[Kaleb S. KEITHLEY]

On 11/09/2011 06:33 PM, Pete Zaitcev wrote:
>  My hekafs server was misconfigured and some functions failed in
> places
>  where they normally would not, and that uncovered a problem: whenever
>  that happened, the client would hang. This happens because we cannot
>  return errors like normal people do in the kernel, by throwing an
>  error code. Functions called through STACK_WIND must return zero and
>  report the error through the callback.
>
>  Not sure what errno to set in this case. Generally I just want to
>  indicate that "something is busted". In the past I would return EDOM.
>  The ENOSPC seems ridiculous enough here.
>
>  Comments? 
I opened BZ 752807 for this.