Re: [PATCH 10/10] gfs2: nfs lock support for gfs2

[Marc Eshel <eshel@almaden.ibm.com>]

Here is a rewrite of gdlm_plock_callback(). We still need to add the 
lock cancel.
Marc.

int gdlm_plock_callback(struct plock_op *op)
{
     struct file *file;
     struct file_lock *fl;
     int (*notify)(void *, void *, int) = NULL;
     int rv;

    spin_lock(&ops_lock);
    if (!list_empty(&op->list)) {
        printk(KERN_INFO "plock op on list\n");
        list_del(&op->list);
    }
    spin_unlock(&ops_lock);

    rv = op->info.rv;

    /* check if the following 2 are still valid or make a copy */
    file = op->info.file;
    fl = op->info.fl;
    notify = op->info.callback;

    if (!rv) { /* got fs lock */
        rv = posix_lock_file(file, fl);
        if (rv) { /* did not get posix lock */
            notify(fl, NULL, rv);
            log_error("gdlm_plock: vfs lock error file %p fl %p",
                  file, fl);
            /* XXX: We need to cancel the fs lock here: */
            printk("gfs2 lock posix lock request failed\n");
        }
        else { /* got posix lock */
            if (notify(fl, NULL, 0)) {
                /* XXX: We need to cancel the fs lock here: */
                printk("gfs2 lock granted after lock request failed; 
dangling lock!\n");
            }
        }
    }
    else {  /* did not get fs lock */
        notify(fl, NULL, rv);
    }

    kfree(op);
    return rv;
}


David Teigland wrote:

>On Wed, Dec 06, 2006 at 02:57:22PM -0500, J. Bruce Fields wrote:
>  
>
>>On Wed, Dec 06, 2006 at 09:49:51AM -0600, David Teigland wrote:
>>    
>>
>>>The gfs side looks fine to me.  Did you forget to call fl_notify from
>>>gdlm_plock_callback() or am I missing something?
>>>      
>>>
>>Yes, looks like we missed something, thanks.  This code's an rfc (not
>>even tested), so don't apply it yet!  What we should have there is
>>something like:
>>
>>	rv = op->info.rv;
>>
>>	if (fl_notify(fl, NULL, rv)) {
>>		/* XXX: We need to cancel the lock here: */
>>		printk("gfs2 lock granted after lock request failed; dangling lock!\n");
>>	}
>>
>>	if (!rv) {
>>		/* check if the following are still valid or make a copy */
>>		file = op->info.file;
>>		fl = op->info.fl;
>>		
>>		if (posix_lock_file_wait(file, fl) < 0)
>>			log_error("gdlm_plock: vfs lock error file %p fl %p",
>>				  file, fl);
>>	}
>>
>>Note there's a race condition--that calls fl_notify before actually
>>getting the lock locally.  I don't *think* that's a problem, as long as
>>it's the filesystem and not the local lock list that's authoritative
>>when it comes to who gets a posix lock.
>>    
>>
>
>agree
>
>  
>
>>The more annoying problem is the need to cancel the GFS lock when
>>fl_notify fails; is that something that it's possible for GFS to do?
>>
>>It can fail because lockd has a timeout--it waits a few seconds for the
>>callback, then gives up and returns a failure to the user.  If that
>>happens after your userspace posix lock manager acquires the lock (but
>>before fl_notify is called) then you've got to cancel it somehow.
>>    
>>
>
>I'd think we could just send an unlock for it at that point.  Set up an op
>with GDLM_PLOCK_OP_UNLOCK and the same fields as the lock you're removing
>and call send_op().  We probably need to flag this internal-unlock op so
>that when the result arrives, device_write() delists and frees it itself.
>
>(I wouldn't call this "canceling", I think of cancel as trying to force a
>blocked request to return/fail prematurely.)
>
>Dave
>
>  
>