[PATCH] SQUASHME: pnfsd-exofs: Fix exofs_inode_recall_layout() sending 2 recalls

[PATCH] SQUASHME: pnfsd-exofs: Fix exofs_inode_recall_layout() sending 2 recalls

[Boaz Harrosh]

A misunderstanding of the wait_event_interruptible() macro has caused
exofs to always issue two recalls in succession.

Using __wait_event_interruptible() would fix that. But I don't like
how this code is structured so change it around completely to be more
flat and clear.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 fs/exofs/export.c |   67 +++++++++++++++++++---------------------------------
 1 files changed, 25 insertions(+), 42 deletions(-)

diff --git a/fs/exofs/export.c b/fs/exofs/export.c
index 8beebfe..ae3e9fd 100644
--- a/fs/exofs/export.c
+++ b/fs/exofs/export.c
@@ -332,45 +332,15 @@ struct pnfs_export_operations exofs_pnfs_ops = {
 	.get_device_info = exofs_get_device_info,
 };
 
-static bool is_layout_returned(struct exofs_i_info *oi, enum pnfs_iomode iomode,
-			      u64 offset, u64 length, void *cookie, int *status)
+static bool is_layout_returned(struct exofs_i_info *oi)
 {
-	struct inode *inode = &oi->vfs_inode;
-	bool done;
-	int error;
-
-	/* We most probably have finished the recall, unless there was an out-
-	 * of-memory condition when sending the recall. For forward progress
-	 * some recalls where sent and some not, resend these that where lost
-	 * before. If cb_layout_recall returns with -ENOENT we know we are
-	 * done for sure.
-	 */
-
-	error = cb_layout_recall(inode, iomode, offset, length, cookie);
-
-	switch (error) {
-	case 0:
-	case -EAGAIN:
-		done = false;
-		break;
-	case -ENOENT:
-		done = true;
-		break;
-	default:
-		goto err;
-	}
+	bool layout_given;
 
-	*status = 0;
-	return done;
+	spin_lock(&oi->i_layout_lock);
+	layout_given = test_bit(OBJ_LAYOUT_IS_GIVEN, &oi->i_flags);
+	spin_unlock(&oi->i_layout_lock);
 
-err:
-	/* this will cause wait_event_interruptible below to break with
-	 * an -ERESTARTSY return. If nfsd is able to unload we probably
-	 * should break out of any nfs loops
-	 */
-	*status = error;
-	restart_syscall();
-	return false;
+	return !layout_given;
 }
 
 int exofs_inode_recall_layout(struct inode *inode, enum pnfs_iomode iomode,
@@ -388,13 +358,26 @@ int exofs_inode_recall_layout(struct inode *inode, enum pnfs_iomode iomode,
 	if (!layout_given)
 		goto exec;
 
-	EXOFS_DBGMSG("(0x%lx) has_layout issue a recall\n", inode->i_ino);
+	for (;;) {
+		EXOFS_DBGMSG("(0x%lx) has_layout issue a recall\n",
+			     inode->i_ino);
+		error = cb_layout_recall(inode, iomode, 0, NFS4_MAX_UINT64,
+					 &oi->i_wq);
+		switch (error) {
+		case 0:
+		case -EAGAIN:
+			break;
+		case -ENOENT:
+			goto exec;
+		default:
+			goto err;
+		}
 
-	wait_event_interruptible(oi->i_wq,
-			is_layout_returned(oi, iomode, 0, NFS4_MAX_UINT64,
-					   todo, &error));
-	if (error)
-		goto err;
+		error = wait_event_interruptible(oi->i_wq,
+						 is_layout_returned(oi));
+		if (error)
+			goto err;
+	}
 
 exec:
 	error = todo(inode);
-- 
1.6.6.1

Re: [PATCH] SQUASHME: pnfsd-exofs: Fix exofs_inode_recall_layout() sending 2 recalls

[Benny Halevy]

On Jun. 29, 2010, 14:41 +0300, Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
> A misunderstanding of the wait_event_interruptible() macro has caused
> exofs to always issue two recalls in succession.
> 
> Using __wait_event_interruptible() would fix that. But I don't like
> how this code is structured so change it around completely to be more
> flat and clear.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>

Merged at pnfs-all-2.6.35-rc3-2010-07-01

Thanks!

Benny

> ---
>  fs/exofs/export.c |   67 +++++++++++++++++++---------------------------------
>  1 files changed, 25 insertions(+), 42 deletions(-)
> 
> diff --git a/fs/exofs/export.c b/fs/exofs/export.c
> index 8beebfe..ae3e9fd 100644
> --- a/fs/exofs/export.c
> +++ b/fs/exofs/export.c
> @@ -332,45 +332,15 @@ struct pnfs_export_operations exofs_pnfs_ops = {
>  	.get_device_info = exofs_get_device_info,
>  };
>  
> -static bool is_layout_returned(struct exofs_i_info *oi, enum pnfs_iomode iomode,
> -			      u64 offset, u64 length, void *cookie, int *status)
> +static bool is_layout_returned(struct exofs_i_info *oi)
>  {
> -	struct inode *inode = &oi->vfs_inode;
> -	bool done;
> -	int error;
> -
> -	/* We most probably have finished the recall, unless there was an out-
> -	 * of-memory condition when sending the recall. For forward progress
> -	 * some recalls where sent and some not, resend these that where lost
> -	 * before. If cb_layout_recall returns with -ENOENT we know we are
> -	 * done for sure.
> -	 */
> -
> -	error = cb_layout_recall(inode, iomode, offset, length, cookie);
> -
> -	switch (error) {
> -	case 0:
> -	case -EAGAIN:
> -		done = false;
> -		break;
> -	case -ENOENT:
> -		done = true;
> -		break;
> -	default:
> -		goto err;
> -	}
> +	bool layout_given;
>  
> -	*status = 0;
> -	return done;
> +	spin_lock(&oi->i_layout_lock);
> +	layout_given = test_bit(OBJ_LAYOUT_IS_GIVEN, &oi->i_flags);
> +	spin_unlock(&oi->i_layout_lock);
>  
> -err:
> -	/* this will cause wait_event_interruptible below to break with
> -	 * an -ERESTARTSY return. If nfsd is able to unload we probably
> -	 * should break out of any nfs loops
> -	 */
> -	*status = error;
> -	restart_syscall();
> -	return false;
> +	return !layout_given;
>  }
>  
>  int exofs_inode_recall_layout(struct inode *inode, enum pnfs_iomode iomode,
> @@ -388,13 +358,26 @@ int exofs_inode_recall_layout(struct inode *inode, enum pnfs_iomode iomode,
>  	if (!layout_given)
>  		goto exec;
>  
> -	EXOFS_DBGMSG("(0x%lx) has_layout issue a recall\n", inode->i_ino);
> +	for (;;) {
> +		EXOFS_DBGMSG("(0x%lx) has_layout issue a recall\n",
> +			     inode->i_ino);
> +		error = cb_layout_recall(inode, iomode, 0, NFS4_MAX_UINT64,
> +					 &oi->i_wq);
> +		switch (error) {
> +		case 0:
> +		case -EAGAIN:
> +			break;
> +		case -ENOENT:
> +			goto exec;
> +		default:
> +			goto err;
> +		}
>  
> -	wait_event_interruptible(oi->i_wq,
> -			is_layout_returned(oi, iomode, 0, NFS4_MAX_UINT64,
> -					   todo, &error));
> -	if (error)
> -		goto err;
> +		error = wait_event_interruptible(oi->i_wq,
> +						 is_layout_returned(oi));
> +		if (error)
> +			goto err;
> +	}
>  
>  exec:
>  	error = todo(inode);