Re: Orangefs ABI documentation

[Al Viro <viro@ZenIV.linux.org.uk>]

On Wed, Feb 17, 2016 at 05:40:08PM -0500, Martin Brandenburg wrote:

> In orangefs_clean_up_interrupted_operation
> 
> 	if (op_state_waiting(op)) {
> 		/*
> 		 * upcall hasn't been read; remove op from upcall request
> 		 * list.
> 		 */
> 		spin_unlock(&op->lock);
> 
> 		/* HERE */
> 
> 		spin_lock(&orangefs_request_list_lock);
> 		list_del(&op->list);
> 		spin_unlock(&orangefs_request_list_lock);

Hmm...  We'd already marked it as given up, though.  Before dropping op->lock.

> 		gossip_debug(GOSSIP_WAIT_DEBUG,
> 			     "Interrupted: Removed op %p from request_list\n",
> 			     op);
> 	} else if (op_state_in_progress(op)) {
> 
> and orangefs_devreq_read
> 
> restart:
> 	/* Get next op (if any) from top of list. */
> 	spin_lock(&orangefs_request_list_lock);
> 	list_for_each_entry_safe(op, temp, &orangefs_request_list, list) {
> 		__s32 fsid;
> 		/* This lock is held past the end of the loop when we break. */
> 
> 		/* HERE */
> 
> 		spin_lock(&op->lock);
> 		if (unlikely(op_state_purged(op))) {
> 			spin_unlock(&op->lock);
> 			continue;
> 		}
> 
> I think both processes can end up working on the same
> op.

It can be picked up.  And then we'll run into

        if (unlikely(op_state_given_up(cur_op))) {
                spin_unlock(&cur_op->lock);
                spin_unlock(&htable_ops_in_progress_lock);
                op_release(cur_op);
                goto restart;

Oh, I see...  OK, yes - by the time we get to that check the sucker has
already been through resubmitting it into the list, so the "given up" flag
is lost.

Hrm...  The obvious approach is to at least avoid taking it off the list
if it's given up - i.e. turn
                __s32 fsid;
                /* This lock is held past the end of the loop when we break. */
                spin_lock(&op->lock);
                if (unlikely(op_state_purged(op))) {
into
                __s32 fsid;
                /* This lock is held past the end of the loop when we break. */
                spin_lock(&op->lock);
                if (unlikely(op_state_purged(op) || op_state_given_up(op))) {
a bit before that point.

However, that doesn't prevent all unpleasantness here - giving up just as
it's being copied to userland and going into restart.  Ho-hum...  How about
the following:
	* move increment of op->attempts into the same place where we
set "given up"
	* in addition to the check for "given up" in the request-picking loop
(as above), fetch op->attempts before dropping op->lock
	* after having retaken op->lock (after copy_to_user()) recheck
op->attempts instead of checking for "given up".

IOW, something like this:

diff --git a/fs/orangefs/devorangefs-req.c b/fs/orangefs/devorangefs-req.c
index b27ed1c..1938d55 100644
--- a/fs/orangefs/devorangefs-req.c
+++ b/fs/orangefs/devorangefs-req.c
@@ -109,6 +109,7 @@ static ssize_t orangefs_devreq_read(struct file *file,
 	static __s32 magic = ORANGEFS_DEVREQ_MAGIC;
 	struct orangefs_kernel_op_s *cur_op = NULL;
 	unsigned long ret;
+	int attempts;
 
 	/* We do not support blocking IO. */
 	if (!(file->f_flags & O_NONBLOCK)) {
@@ -133,7 +134,7 @@ restart:
 		__s32 fsid;
 		/* This lock is held past the end of the loop when we break. */
 		spin_lock(&op->lock);
-		if (unlikely(op_state_purged(op))) {
+		if (unlikely(op_state_purged(op) || op_state_given_up(op))) {
 			spin_unlock(&op->lock);
 			continue;
 		}
@@ -207,6 +208,7 @@ restart:
 	list_del_init(&cur_op->list);
 	get_op(op);
 	spin_unlock(&orangefs_request_list_lock);
+	attempts = op->attempts;
 
 	spin_unlock(&cur_op->lock);
 
@@ -227,7 +229,8 @@ restart:
 
 	spin_lock(&htable_ops_in_progress_lock);
 	spin_lock(&cur_op->lock);
-	if (unlikely(op_state_given_up(cur_op))) {
+	if (unlikely(cur_op->attempts != attempts)) {
+		/* given up just as we copied to userland */
 		spin_unlock(&cur_op->lock);
 		spin_unlock(&htable_ops_in_progress_lock);
 		op_release(cur_op);
diff --git a/fs/orangefs/waitqueue.c b/fs/orangefs/waitqueue.c
index d980240..cc43ac8 100644
--- a/fs/orangefs/waitqueue.c
+++ b/fs/orangefs/waitqueue.c
@@ -139,7 +139,6 @@ retry_servicing:
 	op->downcall.status = ret;
 	/* retry if operation has not been serviced and if requested */
 	if (ret == -EAGAIN) {
-		op->attempts++;
 		timeout = op_timeout_secs * HZ;
 		gossip_debug(GOSSIP_WAIT_DEBUG,
 			     "orangefs: tag %llu (%s)"
@@ -208,6 +207,7 @@ static void orangefs_clean_up_interrupted_operation(struct orangefs_kernel_op_s
 	 * Called with op->lock held.
 	 */
 	op->op_state |= OP_VFS_STATE_GIVEN_UP;
+	op->attempts++;
 
 	if (op_state_waiting(op)) {
 		/*