From 114c50c1a10d649a8b640627f09fd5872828d4ec Mon Sep 17 00:00:00 2001 From: Poornima G Date: Mon, 21 Nov 2016 19:57:08 +0530 Subject: io-cache: Fix a read hang Issue: ===== In certain cases, there was no unwind of read from read-ahead xlator, thus resulting in hang. RCA: ==== In certain cases, ioc_readv() issues STACK_WIND_TAIL() instead of STACK_WIND(). One such case is when inode_ctx for that file is not present (can happen if readdirp was called, and populates md-cache and serves all the lookups from cache). Consider the following graph: ... io-cache (parent) | readdir-ahead | read-ahead ... Below is the code snippet of ioc_readv calling STACK_WIND_TAIL: ioc_readv() { ... if (!inode_ctx) STACK_WIND_TAIL (frame, FIRST_CHILD (frame->this), FIRST_CHILD (frame->this)->fops->readv, fd, size, offset, flags, xdata); /* Ideally, this stack_wind should wind to readdir-ahead:readv() but it winds to read-ahead:readv(). See below for explaination. */ ... } STACK_WIND_TAIL (frame, obj, fn, ...) { frame->this = obj; /* for the above mentioned graph, frame->this will be readdir-ahead * frame->this = FIRST_CHILD (frame->this) i.e. readdir-ahead, which * is as expected */ ... THIS = obj; /* THIS will be read-ahead instead of readdir-ahead!, as obj expands * to "FIRST_CHILD (frame->this)" and frame->this was pointing * to readdir-ahead in the previous statement. */ ... fn (frame, obj, params); /* fn will call read-ahead:readv() instead of readdir-ahead:readv()! * as fn expands to "FIRST_CHILD (frame->this)->fops->readv" and * frame->this was pointing ro readdir-ahead in the first statement */ ... } Thus, the readdir-ahead's readv() implementation will be skipped, and ra_readv() will be called with frame->this = "readdir-ahead" and this = "read-ahead". This can lead to corruption / hang / other problems. But in this perticular case, when 'frame->this' and 'this' passed to ra_readv() doesn't match, it causes ra_readv() to call ra_readv() again!. Thus the logic of read-ahead readv() falls apart and leads to hang. Solution: ========= Ideally, STACK_WIND_TAIL() should be modified as: STACK_WIND_TAIL (frame, obj, fn, ...) { next_xl = obj /* resolve obj as the variables passed in obj macro can be overwritten in the further instrucions */ next_xl_fn = fn /* resolve fn and store in a tmp variable, before modifying any variables */ frame->this = next_xl; ... THIS = next_xl; ... next_xl_fn (frame, next_xl, params); ... } But for this solution, knowing the type of variable 'next_xl_fn' is a challenge and is not easy. Hence just modifying all the existing callers to pass "FIRST_CHILD (this)" as obj, instead of "FIRST_CHILD (frame->this)". Change-Id: I179ffe3d1f154bc5a1935fd2ee44e912eb0fbb61 BUG: 1388292 Signed-off-by: Poornima G Reviewed-on: http://review.gluster.org/15901 Smoke: Gluster Build System Reviewed-by: Raghavendra G NetBSD-regression: NetBSD Build System CentOS-regression: Gluster Build System --- xlators/performance/io-cache/src/io-cache.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'xlators/performance/io-cache/src') diff --git a/xlators/performance/io-cache/src/io-cache.c b/xlators/performance/io-cache/src/io-cache.c index 98c37746921..df247c2318a 100644 --- a/xlators/performance/io-cache/src/io-cache.c +++ b/xlators/performance/io-cache/src/io-cache.c @@ -1105,16 +1105,16 @@ ioc_readv (call_frame_t *frame, xlator_t *this, fd_t *fd, ioc_inode = (ioc_inode_t *)(long)tmp_ioc_inode; if (!ioc_inode) { /* caching disabled, go ahead with normal readv */ - STACK_WIND_TAIL (frame, FIRST_CHILD (frame->this), - FIRST_CHILD (frame->this)->fops->readv, fd, + STACK_WIND_TAIL (frame, FIRST_CHILD (this), + FIRST_CHILD (this)->fops->readv, fd, size, offset, flags, xdata); return 0; } if (flags & O_DIRECT) { /* disable caching for this fd, if O_DIRECT is used */ - STACK_WIND_TAIL (frame, FIRST_CHILD (frame->this), - FIRST_CHILD (frame->this)->fops->readv, fd, + STACK_WIND_TAIL (frame, FIRST_CHILD (this), + FIRST_CHILD (this)->fops->readv, fd, size, offset, flags, xdata); return 0; } @@ -1149,8 +1149,8 @@ ioc_readv (call_frame_t *frame, xlator_t *this, fd_t *fd, if (!fd_ctx_get (fd, this, NULL)) { /* disable caching for this fd, go ahead with normal readv */ - STACK_WIND_TAIL (frame, FIRST_CHILD (frame->this), - FIRST_CHILD (frame->this)->fops->readv, fd, + STACK_WIND_TAIL (frame, FIRST_CHILD (this), + FIRST_CHILD (this)->fops->readv, fd, size, offset, flags, xdata); return 0; } -- cgit