Re: [PATCH 10/10] gfs2: nfs lock support for gfs2
On Wed, Dec 06, 2006 at 10:47:46PM -0800, Marc Eshel wrote:
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 */
If we never request the local lock until after we've gotten the lock
from GFS, then this should never happen. So I think this could just be
a BUG_ON(rv)--except that would mean a failure in the lock manager could
oops the kernel, so maybe it'd be better just to printk.
--b.
-
To unsubscribe from this list: send the line unsubscribe linux-fsdevel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/10] gfs2: nfs lock support for gfs2
Hi,
This looks good to me, and I'm copying in Dave Wendy who have both
done previous work in this area for further comment. Provided we can get
this tested, I'd be happy to accept the patch in its current form.
Steve.
On Wed, 2006-12-06 at 00:34 -0500, J. Bruce Fields wrote:
From: J. Bruce Fields [EMAIL PROTECTED]
From: Marc Eshel [EMAIL PROTECTED]
Add NFS lock support to GFS2. (Untested.)
Signed-off-by: J. Bruce Fields [EMAIL PROTECTED]
---
fs/gfs2/lm.c | 10
fs/gfs2/lm.h |2 +
fs/gfs2/locking/dlm/lock_dlm.h |2 +
fs/gfs2/locking/dlm/mount.c|1 +
fs/gfs2/locking/dlm/plock.c| 95
+++-
fs/gfs2/ops_export.c | 52 ++
include/linux/lm_interface.h |3 +
include/linux/lock_dlm_plock.h |3 +
8 files changed, 166 insertions(+), 2 deletions(-)
diff --git a/fs/gfs2/lm.c b/fs/gfs2/lm.c
index effe4a3..cf7fd52 100644
--- a/fs/gfs2/lm.c
+++ b/fs/gfs2/lm.c
@@ -197,6 +197,16 @@ int gfs2_lm_plock(struct gfs2_sbd *sdp, struct
lm_lockname *name,
return error;
}
+int gfs2_lm_plock_async(struct gfs2_sbd *sdp, struct lm_lockname *name,
+ struct file *file, int cmd, struct file_lock *fl)
+{
+ int error = -EIO;
+ if (likely(!test_bit(SDF_SHUTDOWN, sdp-sd_flags)))
+ error = sdp-sd_lockstruct.ls_ops-lm_plock_async(
+ sdp-sd_lockstruct.ls_lockspace, name, file,
cmd, fl);
+ return error;
+}
+
int gfs2_lm_punlock(struct gfs2_sbd *sdp, struct lm_lockname *name,
struct file *file, struct file_lock *fl)
{
diff --git a/fs/gfs2/lm.h b/fs/gfs2/lm.h
index 21cdc30..1ddd1fd 100644
--- a/fs/gfs2/lm.h
+++ b/fs/gfs2/lm.h
@@ -34,6 +34,8 @@ int gfs2_lm_plock_get(struct gfs2_sbd *sdp, struct
lm_lockname *name,
struct file *file, struct file_lock *fl);
int gfs2_lm_plock(struct gfs2_sbd *sdp, struct lm_lockname *name,
struct file *file, int cmd, struct file_lock *fl);
+int gfs2_lm_plock_async(struct gfs2_sbd *sdp, struct lm_lockname *name,
+ struct file *file, int cmd, struct file_lock *fl);
int gfs2_lm_punlock(struct gfs2_sbd *sdp, struct lm_lockname *name,
struct file *file, struct file_lock *fl);
void gfs2_lm_recovery_done(struct gfs2_sbd *sdp, unsigned int jid,
diff --git a/fs/gfs2/locking/dlm/lock_dlm.h b/fs/gfs2/locking/dlm/lock_dlm.h
index 33af707..82af860 100644
--- a/fs/gfs2/locking/dlm/lock_dlm.h
+++ b/fs/gfs2/locking/dlm/lock_dlm.h
@@ -179,6 +179,8 @@ int gdlm_plock_init(void);
void gdlm_plock_exit(void);
int gdlm_plock(void *, struct lm_lockname *, struct file *, int,
struct file_lock *);
+int gdlm_plock_async(void *, struct lm_lockname *, struct file *, int,
+ struct file_lock *);
int gdlm_plock_get(void *, struct lm_lockname *, struct file *,
struct file_lock *);
int gdlm_punlock(void *, struct lm_lockname *, struct file *,
diff --git a/fs/gfs2/locking/dlm/mount.c b/fs/gfs2/locking/dlm/mount.c
index cdd1694..4339e3f 100644
--- a/fs/gfs2/locking/dlm/mount.c
+++ b/fs/gfs2/locking/dlm/mount.c
@@ -244,6 +244,7 @@ const struct lm_lockops gdlm_ops = {
.lm_lock = gdlm_lock,
.lm_unlock = gdlm_unlock,
.lm_plock = gdlm_plock,
+ .lm_plock_async = gdlm_plock_async,
.lm_punlock = gdlm_punlock,
.lm_plock_get = gdlm_plock_get,
.lm_cancel = gdlm_cancel,
diff --git a/fs/gfs2/locking/dlm/plock.c b/fs/gfs2/locking/dlm/plock.c
index 7365aec..c21e667 100644
--- a/fs/gfs2/locking/dlm/plock.c
+++ b/fs/gfs2/locking/dlm/plock.c
@@ -102,6 +102,93 @@ int gdlm_plock(void *lockspace, struct lm_lockname *name,
return rv;
}
+int gdlm_plock_async(void *lockspace, struct lm_lockname *name,
+struct file *file, int cmd, struct file_lock *fl)
+{
+ struct gdlm_ls *ls = lockspace;
+ struct plock_op *op;
+ int rv;
+
+ op = kzalloc(sizeof(*op), GFP_KERNEL);
+ if (!op)
+ return -ENOMEM;
+
+ op-info.optype = GDLM_PLOCK_OP_LOCK;
+ op-info.pid= fl-fl_pid;
+ op-info.ex = (fl-fl_type == F_WRLCK);
+ op-info.wait = IS_SETLKW(cmd);
+ op-info.fsid = ls-id;
+ op-info.number = name-ln_number;
+ op-info.start = fl-fl_start;
+ op-info.end= fl-fl_end;
+ op-info.owner = (__u64)(long) fl-fl_owner;
+ if (fl-fl_lmops) {
+ op-info.callback = fl-fl_lmops-fl_notify;
+ /* might need to make a copy */
+ op-info.fl = fl;
+ op-info.file = file;
+ } else
+ op-info.callback = NULL;
+
+ send_op(op);
+
+ if (op-info.callback == NULL)
+ wait_event(recv_wq, (op-done != 0));
+ else
+
Re: [PATCH 10/10] gfs2: nfs lock support for gfs2
On Wed, Dec 06, 2006 at 12:34:20AM -0500, J. Bruce Fields wrote:
+int gdlm_plock_callback(struct plock_op *op)
+{
+ struct file *file;
+ struct file_lock *fl;
+ 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;
+
+ 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);
+ }
+
+ kfree(op);
+ return rv;
+}
..
+ if (found) {
+ if (op-info.callback)
+ gdlm_plock_callback(op);
+ else
+ wake_up(recv_wq);
+ }
The gfs side looks fine to me. Did you forget to call fl_notify from
gdlm_plock_callback() or am I missing something?
Dave
-
To unsubscribe from this list: send the line unsubscribe linux-fsdevel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/10] gfs2: nfs lock support for gfs2
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.
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.
--b.
-
To unsubscribe from this list: send the line unsubscribe linux-fsdevel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/10] gfs2: nfs lock support for gfs2
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
-
To unsubscribe from this list: send the line unsubscribe linux-fsdevel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/10] gfs2: nfs lock support for gfs2
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
-
To unsubscribe from this list: send the line unsubscribe linux-fsdevel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
